Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't #include standard library headers unconditionally #1461

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

real-or-random
Copy link
Contributor

This is supposed to resolve #1095 and help with restricted settings such as embedded systems or WASM.

The approach is to detect the availability of <stdio.h> and <stdio.h>, both when building the library or building against the API. The effects depend on the case:

When building the library:

  • If <stdio.h> is not available, disable fprintf(stderr, ...) in callbacks and emit a note during compilation (#pragma message)
  • If <stdlib.h> is not available and external default callbacks are not used, then we don't have abort. Raise an error in compilation and tell the user to use external callbacks
  • If <stdlib.h> is not available, we don't have malloc (and free). Disable all API functions that need it.

When building against our API:

  • If <stdlib.h> is not available, make an educated guess that it wasn't available when the library was built, and tell the user that certain API functions are not available.

We use different methods of detecting the presence of the headers, see the code. The user can always override our detection mechanism as a last resort.

The other kind of standard library functionality we use is memset and memcpy from <string.h> (but not memcmp because it's a policy to use `secp256k1_memcmp_var). There's not much we can do for these two, unless we want to provide our own implementations, but in practice these functions are pretty common. C23 has adopted a proposal to make these mandatory even for freestanding implementations (i.e., embedded systems in standard-speak).

A good way to test this is to change the filename in _has_include to simulate the case when headers are unavailable.


I went through a lot of back and forth when working on this. Here are some alternatives and why I didn't choose them for now:

  • Detecting <stdlib.h> for both abort and malloc and is rather crude. I'm sure there are embedded platforms out there which have malloc and no abort, or the other way around. However, dealing with this will probably need more machinery (e.g., special non-standard headers), and compilers don't have automatic mechanism to detect the availability of functions. Moreover, I don't want to overshoot in the dark with my limited experience in embedded system and given the fact that noone has complained so far. After this PR, if you have a nonstandard <stdlib.h> with malloc but without abort, you can get away with providing external callbacks. If you have a nonstandard <stdlib.h> with abort but without malloc, you can get away with overriding the detection of <stdlib.h> to "no" while at the same time providing external callbacks (that may still call abort then.) For a different, much more flexible approach, see for example Mbed TLS, which has a focus on embedded system.
  • I'm not entirely convinced that it's a good idea to warn the user when building against our API. We could be wrong and headers were available when building the library and just not now. But in that case, the user can still override, so I think it's better to loud here.
  • I'm also not sure that it's a good idea to let the compiler issue a "note" message when we disable features when compiling the library, also taking into account that it's apparently difficult to disable this kind of messages in GCC. But again, I felt it's a bit better to be loud here.

@tcharding This is supposed to improve the situation for rust-secp256k1 wasm significantly. Perhaps except for the aforementioned string.h, but if you're using wasi, then I think you should just use the wabi sysroot and not your own sysroot then. Searching the web also tells me that clang may need some compiler option. I don't know, I haven't tried this.

@tcharding
Copy link
Contributor

Epic, this is awesome. I can confirm that I was able to build the code on this branch in rust-secp256k1 after using a modified version of the vendor script in rust-secp256k1/secp256k1-sys (that removed all the source file patching). I was also able to remove the printf stuff from build.rs and delete the stdio.h and stdlib.h files from rust-secp256k1/secp256k1-sys/wasm/wasm-sysroot. Massive win, thanks man.

src/util.h Outdated
#if SECP256K1_HAVE_STDIO_H
# include <stdio.h>
#else
# pragma message "<stdio.h> not available, disabling debugging output for fatal library errors."
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should only be displayed when the default callbacks are active.

(Or I change my mind and just get rid of it entirely... I'm not sure if these notes are too annoying/intrusive if it's too hard to silence them.)

@real-or-random
Copy link
Contributor Author

@tcharding If I understand correctly, this means that the custom string.h would be the only hack left on the rust-secp256k1 side? Have you tried pointing it to the wasi sysroot?

If that's not desirable for whatever reason, we could also consider "fixing" the issue here: We could simply avoid including the header and then put the required functions declarations in secp256k1 directly. I just would want to put this behind some #ifdef because I think it shouldn't be enabled by default.

If you ask me, I think it's philosophically the user's responsibility to configure their sysroot correctly, and if they have memcpy, to provide a header that declares it. But pragmatically, a workaround will add 5 lines to our code, so why not...

@tcharding
Copy link
Contributor

tcharding commented Dec 14, 2023

Have you tried pointing it to the wasi sysroot?

I went to try [0] but the wasm32-wasi target is not is not supported by wasm-pack which we use in CI. I didn't get any further than that.

[0] (I'm know basically nothing about wasm)

ref: rustwasm/wasm-pack#654

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. I played around with it a bit and it works as expected.

Should we test in CI that SECP256K1_HAVE_STDIO_H=0 works and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

I'm not entirely convinced that it's a good idea to warn the user when building against our API. We could be wrong and headers were available when building the library and just not now. But in that case, the user can still override, so I think it's better to loud here.

Agreed

include/secp256k1.h Outdated Show resolved Hide resolved
src/modinv32_impl.h Show resolved Hide resolved
@real-or-random
Copy link
Contributor Author

Will address all comments.


If you ask me, I think it's philosophically the user's responsibility to configure their sysroot correctly, and if they have memcpy, to provide a header that declares it. But pragmatically, a workaround will add 5 lines to our code, so why not...

I said this above, but I think I changed my mind. If we do it, we'll have to support it basically forever. If you can keep the workaround for that one header, you could remove it once wasm support in Rust is better.


  • I'm also not sure that it's a good idea to let the compiler issue a "note" message when we disable features when compiling the library, also taking into account that it's apparently difficult to disable this kind of messages in GCC. But again, I felt it's a bit better to be loud here.

We could also add a simple _QUIET macro, but not sure if this is overkill. I lean towards removing the messages. Silent builds are a nice thing, and we don't want to be annoying.

@sipa
Copy link
Contributor

sipa commented Dec 18, 2023

Concept ACK. I agree with @jonasnick's review comments. Do the warning messages add anything, as there are already deprecated markers on the unavailable functions?

@real-or-random real-or-random force-pushed the 202312-only-prealloc branch 2 times, most recently from e8ed16f to 7a1f700 Compare December 20, 2023 12:48
@real-or-random
Copy link
Contributor Author

Should we test in CI that SECP256K1_HAVE_STDIO_H=0 works

Done.

and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

Done, we don't need to disable the tests and benchmarks, they ignore the external callbacks setting anyway.

I also moved the pragma message for building the library to secp256k1.h. This seems wrong because we could print it in the actual code instead of the public header. But this approach avoids having two separate macros for what the user wants and what we have autodetected. (Only in the preprocessor block in secp256k1.h., we still have three values: undefined, true and false, where undefined means "auto". After the block, we'll reduce this to just true and false...)

@sipa
In contrast do what we said on IRC, what I haven't done now is to add autotools/CMake options. I'm a bit unsure. So the concept of this PR is to use the preprocessor for detection, and I think that's the way to go in the long run, also for other config options if possible, because the preprocessor is build system agnostic and works even without a build system.

Of course, we could add --with-stdio=[yes|no|auto] (default: auto) and the like, but I don't think this adds a lot to the this simple approach of this PR: If you have a modern and not too exotic compiler, the result of our auto detection will just be correct, and there's really no need to override it. In other words: If the compiler says it doesn't have stdlib.h for inclusion, then just overriding this and still issuing an #include won't magically make the compiler work. (If you still have a malloc somewhere, this PR anyway won't help you.) And even if you have a strange compiler, it could very well happen that the build systems will fail in other ways and you'll anyway need to invoke the compiler manually. The only reason to override our detection would be to disable functionality that is actually available, but then you'll probably just want the external callbacks... What do you think?

@real-or-random
Copy link
Contributor Author

and/or that --disable-exhaustive-tests --disable-tests --disable-benchmark --enable-external-default-callbacks CFLAGS=-DSECP256K1_HAVE_STDLIB_H=0 builds?

Done, we don't need to disable the tests and benchmarks, they ignore the external callbacks setting anyway.

CI disagrees. Never mind.

And this is, in fact, an argument for adding flags like--with-stdlib to build system... (If stdlib is not is available, then don't build tests, benchmarks etc...)

Marking as draft for now. I'll need to look at this again with a fresh mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't #include standard library headers unconditionally
4 participants