Conditionally include `stdio.h` and `stdlib.h`

tcharding commented at 11:47 pm on November 2, 2022: contributor

Currently in order to use secp256k1 in WASM builds one must patch into the build system empty header files for stdlib.h and stdio.h [0].

Patch 1: Removes the unconditional include of stdio.h based on the presence of USE_EXTERNAL_DEFAULT_CALLBACKS and VERIFY.
Patch 2: Introduces PREALLOC_INTERFACE_ONLY and ifndef guards any calls to malloc and friends as well as the include of stdio.h.

Works towards fixing: #1095

1095 does not mention string.h but it is a problem as well. I cannot work out how to resolve it so attempting to push this in without it. Happy to extend this further if anyone has any ideas.

Notes on testing

I could not work out how to test this in rust-secp256k1 so the I have not proved that this PR removes the requirement to include empty headers described above.

[0] - https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/depend/secp256k1.c.patch

tcharding renamed this:
~~Conditionally depend on stdio.h~~
Conditionally depend on `stdio.h`
on Nov 2, 2022

real-or-random commented at 4:17 pm on November 3, 2022: contributor

Fix: #1095

Can you remove the magic word fix? I think this PR solves #1095 only partly (stdlib.h).

in src/util.h:19 in d185a6fcc0 outdated

12@@ -13,9 +13,12 @@
13 
14 #include <stdlib.h>
15 #include <stdint.h>
16-#include <stdio.h>
17 #include <limits.h>
18 
19+#ifndef USE_EXTERNAL_DEFAULT_CALLBACKS
20+#include <stdio.h>              /* for fprintf */

real-or-random commented at 4:19 pm on November 3, 2022:

0#include <stdio.h>

style nit: we usually don’t say for what we need includes. (And it’s not only needed for fprintf, it’s also needed for stderr. )

tcharding commented at 10:23 pm on November 3, 2022:

Sure thing, thanks.

real-or-random commented at 4:22 pm on November 3, 2022: contributor

Concept ACK, thanks!

This is a good opportunity to move the definitions of CHECK and TEST_FAILURE (but probably not EXPECT to tests.c, if you’re willing to add a commit. Otherwise we can do it in a follow-up PR.

tcharding commented at 10:26 pm on November 3, 2022: contributor

Fix: #1095

Can you remove the magic word fix? I think this PR solves #1095 only partly (stdlib.h).

bah, sloppy work by me. I’ll try first to fix the stdlib.h issue in this PR as well. Not sure off the top of my head about string.h (referring to rust-secp256k1/ now) because we have a bunch of function declarations in there as well. Will investigate further, sorry for doing half a job.

tcharding commented at 10:27 pm on November 3, 2022: contributor

Concept ACK, thanks!

This is a good opportunity to move the definitions of CHECK and TEST_FAILURE (but probably not EXPECT to tests.c, if you’re willing to add a commit. Otherwise we can do it in a follow-up PR.

Sure thing, happy to look into it.

tcharding marked this as a draft on Nov 4, 2022

tcharding commented at 3:15 am on November 4, 2022: contributor

Concept ACK, thanks!

This is a good opportunity to move the definitions of CHECK and TEST_FAILURE (but probably not EXPECT to tests.c, if you’re willing to add a commit. Otherwise we can do it in a follow-up PR.

EDIT: I realised after posting that all these files are either tests of benches, so I did #1149

I must be missing something, CHECK is used in a whole bunch of files, how can we move the definition to tests.c? Is there another header file that it could go in, is that what you meant?

 0git grep -l ' CHECK\('
 1src/bench.c
 2src/bench_ecmult.c
 3src/bench_internal.c
 4src/ecmult_impl.h
 5src/modules/ecdh/bench_impl.h
 6src/modules/ecdh/tests_impl.h
 7src/modules/extrakeys/tests_exhaustive_impl.h
 8src/modules/extrakeys/tests_impl.h
 9src/modules/recovery/bench_impl.h
10src/modules/recovery/tests_exhaustive_impl.h
11src/modules/recovery/tests_impl.h
12src/modules/schnorrsig/bench_impl.h
13src/modules/schnorrsig/tests_exhaustive_impl.h
14src/modules/schnorrsig/tests_impl.h
15src/tests.c
16src/tests_exhaustive.c
17src/util.h
18src/valgrind_ctime_test.c

tcharding force-pushed on Nov 4, 2022

real-or-random cross-referenced this on Nov 4, 2022 from issue Remove usage of CHECK from non-test file by tcharding

tcharding force-pushed on Nov 8, 2022

tcharding commented at 11:11 pm on November 8, 2022: contributor

This is a good opportunity to move the definitions of CHECK and TEST_FAILURE

I had a play with this @real-or-random but got stuck, leaving for another day. The investigation did lead me however to the observation, now implemented in patch 1, to conditionally include stdio.h based on VERIFY.

tcharding force-pushed on Nov 16, 2022

tcharding renamed this:
~~Conditionally depend on `stdio.h`~~
Conditionally include `stdio.h` and `stdlib.h`
on Nov 16, 2022

tcharding marked this as ready for review on Nov 16, 2022

in src/util.h:19 in 77d97d9ebe outdated

20+#include <stdio.h>
21+#endif
22+
23+#ifdef VERIFY
24+#include <stdio.h>
25+#endif

jonasnick commented at 9:48 pm on November 18, 2022:

maybe

0#if defined(VERIFY) || !defined(USE_EXTERNAL_DEFAULT_CALLBACKS)
1#include <stdio.h>
2#endif

tcharding commented at 10:52 pm on November 20, 2022:

Cheers, rebased and used the suggestion in patch 1.

Can a maintainer please confirm you guys use this process, that this rebasing to keep the PR consisting of only the valid patch series.

jonasnick commented at 4:33 pm on November 21, 2022:

Yes, that’s the process.

tcharding commented at 7:38 pm on November 21, 2022:

Thanks mate.

jonasnick commented at 9:53 pm on November 18, 2022: contributor

ACK mod nit for the first commit

As for the second commit, if I compile with PREALLOC_INTERFACE_ONLY defined, I get a bunch of errors and warnings. I tried to fix them in my branch until I realized that stdlib is still needed for checked_malloc/realloc, i.e., compilation of my branch with ./configure CPPFLAGS="-DPREALLOC_INTERFACE_ONLY -DUSE_EXTERNAL_DEFAULT_CALLBACKS" will error out because of that (if we manage to get this to work nonetheless we should add a CI test for PREALLOC_INTERFACE_ONLY that at least tests successful compilation).

Conditionally include stdio.h

The header `stdio.h` is only needed in code ifdef guarded by
`USE_EXTERNAL_DEFAULT_CALLBACKS` and also (indirectly) by `VERIFY`, we
can therefore guard the include statement in the same manner.

The reason for doing this as that wasm builds downstream have to patch
in an empty `stdio.h` file in order to build because of the
unconditional include.

dca6a583cb

Conditionally include stdlib.h

If we are using the preallocated interface only then stdlib.h is not
required. Currently we unconditionally include it.

Add ifdef guards around any code that uses malloc and friends and then
conditionally include `stdlib.h` based on `PREALLOC_INTERFACE_ONLY`.

The benefit of this patch is that downstream users who wish to build
libsecp256k1 into WASM will not need to patch into their build an empty
`stdlib.h` file.

303a201bea

tcharding force-pushed on Nov 20, 2022

tcharding commented at 7:42 pm on November 21, 2022: contributor

Any clues on the CI fail crew?

./build-aux/test-driver: line 109: 3245 Aborted (core dumped) “$@” > $log_file 2>&1 FAIL: tests

Can I view $log_file in any way?

sipa commented at 7:44 pm on November 21, 2022: contributor

https://api.cirrus-ci.com/v1/task/6006563169632256/logs/cat_tests_log.log

real-or-random commented at 7:51 pm on November 21, 2022: contributor

api.cirrus-ci.com/v1/task/6006563169632256/logs/cat_tests_log.log

Oh that’s a failure that’s expected to occur with a certain low (but still too high to be annoying) probability, see #367 if you’re interested in the details. I’ve restarted the job.

tcharding commented at 10:04 pm on November 21, 2022: contributor

Thanks!

real-or-random commented at 1:08 pm on November 22, 2022: contributor

To be honest, I think we should take a step back and first discuss whether we would like to separate the headers as I suggested in #1095 (comment). That looks like a very fundamental change but it should be backwards compatible. That would be cleaner and it would make this PR here obsolete.

Let me still comment on the PR.

As for the second commit, if I compile with PREALLOC_INTERFACE_ONLY defined, I get a bunch of errors and warnings. I tried to fix them in my branch until I realized that stdlib is still needed for checked_malloc/realloc, i.e., compilation of my branch with ./configure CPPFLAGS="-DPREALLOC_INTERFACE_ONLY -DUSE_EXTERNAL_DEFAULT_CALLBACKS" will error out because of that

The proper way to do this is to just wrap the malloc/realloc calls in checked_malloc/realloc in util.h in #ifndef PREALLOC_INTERFACE_ONLY (instead the context creation function). Then all malloc calls (and only these) will be gone if you set PREALLOC_INTERFACE_ONLY.

Before I came to this realization, I was about to comment that I think we don’t want to change secp256k1_context_preallocated_clone. Cloning a context into preallocated memory should be fine even if the users sets PREALLOC_INTERFACE_ONLY (and the function doesn’t need stdlib.h). But yeah, if we simply wrap inside of checked_malloc/realloc in util.h, then secp256k1_context_preallocated_clone should work.

In general, it’s a little bit inelegant to make secp256k1_context_create simply return NULL when PREALLOC_INTERFACE_ONLY is set. Can we instead call a callback, e.g.,

0            secp256k1_callback_call(&default_illegal_callback, "Use secp256k1_context_preallocated_create instead when compiled with PREALLOC_INTERFACE_ONLY");

Arguably that’s maybe not terribly useful if you also set USE_EXTERNAL_DEFAULT_CALLBACKS and don’t have a proper way to abort, but it’s certainly better than returning just NULL.

(if we manage to get this to work nonetheless we should add a CI test for PREALLOC_INTERFACE_ONLY that at least tests successful compilation).

Well testing is another problem. Many of the tests currently error out when they can’t call secp256k1_context_create…

tcharding commented at 0:06 am on November 23, 2022: contributor

Thanks @real-or-random, I don’t think chopping up headers is a good task for someone new to a project, I’m going to leave this one for now. Will move the PR to draft for the discussion to indicate its not a merge candidate.

tcharding marked this as a draft on Nov 23, 2022

tcharding commented at 7:50 am on March 20, 2023: contributor

No forward path, closing.

tcharding closed this on Mar 20, 2023

real-or-random commented at 9:45 am on March 26, 2023: contributor

No forward path, closing.

Indeed. It’s tracked in #1095, I hope I can come back to this soon. This is a major pain point for users.

Conditionally include stdio.h and stdlib.h #1148

Notes on testing

Conditionally include `stdio.h` and `stdlib.h` #1148