Conditionally include stdio.h and stdlib.h

[tcharding]

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

[real-or-random]

Fix: #1095

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

[real-or-random]

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]

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]

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]

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?

git grep -l ' CHECK\('
src/bench.c
src/bench_ecmult.c
src/bench_internal.c
src/ecmult_impl.h
src/modules/ecdh/bench_impl.h
src/modules/ecdh/tests_impl.h
src/modules/extrakeys/tests_exhaustive_impl.h
src/modules/extrakeys/tests_impl.h
src/modules/recovery/bench_impl.h
src/modules/recovery/tests_exhaustive_impl.h
src/modules/recovery/tests_impl.h
src/modules/schnorrsig/bench_impl.h
src/modules/schnorrsig/tests_exhaustive_impl.h
src/modules/schnorrsig/tests_impl.h
src/tests.c
src/tests_exhaustive.c
src/util.h
src/valgrind_ctime_test.c

[tcharding]

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.

[jonasnick]

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).

[jonasnick]

maybe

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

[tcharding]

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]

Yes, that's the process.

[tcharding]

Thanks mate.

[tcharding]

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]

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

[real-or-random]

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]

Thanks!

[real-or-random]

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.,

            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]

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]

No forward path, closing.

[real-or-random]

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.