Patch out any instances of printf in upstream #663
Conversation
This patch adds a declaration of the `ecdsa_parse_compact` function to util.h. This function isn't called from within libsecp proper; it is called in lax_der_parse.c (which we patch separately with a declaration) and in example code (which we don't compile at all).
| @@ -20,11 +20,16 @@ fn main() { | |||
| .include("depend/secp256k1/include") | |||
| .include("depend/secp256k1/src") | |||
| .flag_if_supported("-Wno-unused-function") // some ecmult stuff is defined but not used upstream | |||
| .flag_if_supported("-Wno-unused-parameter") // patching out printf causes this warning | |||
There was a problem hiding this comment.
I think this can be solved more locally but I haven't used C for a while so I don't remember how.
There was a problem hiding this comment.
There's no standard C way to do it. Any specific compiler would have annotations but they wouldn't work for others. (Of course, flag_if_supported also not universal. But cc makes an effort to translate it for the common compilers.)
And in any case, if I wanted a more local solution, I'd just patch out the offending methods entirely :).
There was a problem hiding this comment.
I mean something like using (void)x works IIRC which can be used in the define. But I don't know how to apply it to multiple arguments.
|
@cryptoquick are you able to test this PR (or just its last commit) and see if it works for you? |
|
When I add these crates: secp256k1 = { git = "https://github.com/apoelstra/rust-secp256k1", branch = "2023-11--define-not-patch" }
secp256k1-sys = { git = "https://github.com/apoelstra/rust-secp256k1", branch = "2023-11--define-not-patch" }I get this error: Is this the proper way to test this PR? |
|
Thanks! Yes, it looks like you're testing it correctly. Very strange that your compiler is getting upset at the use of I will add to this PR to replace |
|
Ok, I got rid of all warnings on my machine by using: // WASM headers and size/align defines.
if env::var("CARGO_CFG_TARGET_ARCH").unwrap() == "wasm32" {
base_config.include("wasm/wasm-sysroot")
// upstream sometimes introduces calls to printf, which we cannot compile
// with WASM due to its lack of libc. printf is never necessary and we can
// just #define it away.
.define("printf(...)", Some(""))
.flag("-Wno-unused-parameter") // patching out printf causes this warning
.flag("-Wno-unused-function") // some ecmult stuff is defined but not used upstream
.flag("-Wno-implicit-function-declaration")
.file("wasm/wasm.c");
}I believe its ok to use |
| // upstream sometimes introduces calls to printf, which we cannot compile | ||
| // with WASM due to its lack of libc. printf is never necessary and we can | ||
| // just #define it away. | ||
| .define("printf(...)", Some("")); |
There was a problem hiding this comment.
I think we should only use this if the target is wasm ie., put it in:
// WASM headers and size/align defines.
if env::var("CARGO_CFG_TARGET_ARCH").unwrap() == "wasm32" {There was a problem hiding this comment.
Also the -Wno-unused-paramater
There was a problem hiding this comment.
Because there is a bug in cc::Build::flag_if_supported so we are getting loads of warnings. Well on my machine anyways.
There was a problem hiding this comment.
Why?
Did you mean why do I think we should put the .define on the wasm-only cc::Build or why the "Also the -Wno-unused-parameter? Ignore the second one, and for the first, I thought this because the code comment specifically mentions that this is a wasm problem so it seems logical to put it in the wasm-only place.
There was a problem hiding this comment.
I don't think it's wasm-only. It would be any linker that tries to resolve libc symbols before pruning unused code.
Ok, but what about every other target we support? Do we know that they will use gcc or clang? |
|
I'm not sure gcc figures into wasm compilation. I'm using clang. Here are the details on my system: |
|
Here is the solution to the wasm/flag_if_supported thing: rust-lang/cc-rs#839 |
|
@tcharding so looks like @cryptoquick just needs to run |
|
@Kixunil Tried that, but I'm already running [email protected]. I noticed there was an update to libc v0.2.150, from 149, but that did not help. Just realized, the fix that was merged has not yet been released. Will have to keep an eye on that. |
Oh, didn't notice. Perhaps test with their master branch using |
Oops, just did this now. Was not as simple as using a #define since the function is actually defined, and trying to stub it out with the preprocessor messes up the definition. |
| @@ -145,11 +145,9 @@ static const rustsecp256k1_v0_9_0_callback default_error_callback = { | |||
| #endif | |||
|
|
|||
| static SECP256K1_INLINE void *checked_malloc(const rustsecp256k1_v0_9_0_callback* cb, size_t size) { | |||
There was a problem hiding this comment.
Maybe delete it completely to ensure it's not called by accident?
There was a problem hiding this comment.
Then I would need to delete all the functions that call it, and so on, which is much more invasive.
There was a problem hiding this comment.
Sucks. Also I wonder how does it interact with the system library. Will anything break if it has different code?
There was a problem hiding this comment.
No. It's not an exposed function, it's only used internally.
|
AFAICT this is right to go, ACK 7a0c60e |
|
Heh, github warned me that I'm bypassing the "one ack from a maintainer" rule. But I think that something I authored, with 2 acks from rust-bitcoin maintainers, and the original bug reporter saying that it helped, is ok to merge. |
|
Just tried the master branch with wasm-pack, it works great, thanks! |
Rather than using a new patchfile, just
#defineit away. Also includes a commit which removes one of the existing patchfiles, which I discovered was out of date while auditing the others to see if they could be replaced by#defines. (No, they cannot.)Fixes #660