-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: master
Are you sure you want to change the base?
Don't #include standard library headers unconditionally #1461
Conversation
Epic, this is awesome. I can confirm that I was able to build the code on this branch in |
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." |
There was a problem hiding this comment.
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.)
@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... |
I went to try [0] but the [0] (I'm know basically nothing about wasm) |
There was a problem hiding this 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
Will address all comments.
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.
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. |
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? |
e8ed16f
to
7a1f700
Compare
7a1f700
to
b4ec838
Compare
Done.
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 Of course, we could add |
CI disagrees. Never mind. And this is, in fact, an argument for adding flags like Marking as draft for now. I'll need to look at this again with a fresh mind. |
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:
<stdio.h>
is not available, disablefprintf(stderr, ...)
in callbacks and emit a note during compilation (#pragma message
)<stdlib.h>
is not available and external default callbacks are not used, then we don't haveabort
. Raise an error in compilation and tell the user to use external callbacks<stdlib.h>
is not available, we don't havemalloc
(andfree
). Disable all API functions that need it.When building against our API:
<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
andmemcpy
from<string.h>
(but notmemcmp
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:
<stdlib.h>
for bothabort
andmalloc
and is rather crude. I'm sure there are embedded platforms out there which havemalloc
and noabort
, 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>
withmalloc
but withoutabort
, you can get away with providing external callbacks. If you have a nonstandard<stdlib.h>
withabort
but withoutmalloc
, you can get away with overriding the detection of<stdlib.h>
to "no" while at the same time providing external callbacks (that may still callabort
then.) For a different, much more flexible approach, see for example Mbed TLS, which has a focus on embedded system.@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.