testrand_impl.h
#1743
While reviewing #1734 and suggesting keeping the new test miniframework as a separate translation unit, I noticed a couple of issues with the headers included in testrand_impl.h. In my approach, these lead to compiler warnings and linker errors. This PR fixes them.
These changes are reasonable on their own.
9@@ -10,9 +10,10 @@
10 #include <stdint.h>
11 #include <stdio.h>
12 #include <string.h>
13+#include <time.h>
9@@ -10,9 +10,10 @@
10 #include <stdint.h>
11 #include <stdio.h>
12 #include <string.h>
13+#include <time.h>
14
15 #include "testrand.h"
16-#include "hash.h"
17+#include "hash_impl.h"
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails:
0$ cmake --build build
1[1/20] Building C object src/CMakeFiles/unit_test.dir/unit_test.c.o
2In file included from /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:17,
3 from /home/hebasto/dev/secp256k1/secp256k1/src/unit_test.c:31:
4/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:19:13: warning: ‘secp256k1_sha256_initialize’ used but never defined
5 19 | static void secp256k1_sha256_initialize(secp256k1_sha256 *hash);
6 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
7/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:20:13: warning: ‘secp256k1_sha256_write’ used but never defined
8 20 | static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t size);
9 | ^~~~~~~~~~~~~~~~~~~~~~
10/home/hebasto/dev/secp256k1/secp256k1/src/hash.h:21:13: warning: ‘secp256k1_sha256_finalize’ used but never defined
11 21 | static void secp256k1_sha256_finalize(secp256k1_sha256 *hash, unsigned char *out32);
12 | ^~~~~~~~~~~~~~~~~~~~~~~~~
13[18/20] Linking C executable bin/noverify_tests
14FAILED: bin/noverify_tests
15: && /usr/bin/cc -O2 -g -Wl,--dependency-file=src/CMakeFiles/noverify_tests.dir/link.d src/CMakeFiles/unit_test.dir/unit_test.c.o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o src/CMakeFiles/noverify_tests.dir/tests.c.o -o bin/noverify_tests && :
16/usr/bin/ld: src/CMakeFiles/unit_test.dir/unit_test.c.o: in function `testrand_seed':
17/home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:29:(.text+0x6c2): undefined reference to `secp256k1_sha256_initialize'
18/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:30:(.text+0x6d6): undefined reference to `secp256k1_sha256_write'
19/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:31:(.text+0x6e6): undefined reference to `secp256k1_sha256_write'
20/usr/bin/ld: /home/hebasto/dev/secp256k1/secp256k1/src/testrand_impl.h:32:(.text+0x6f1): undefined reference to `secp256k1_sha256_finalize'
21collect2: error: ld returned 1 exit status
22[19/20] Building C object src/CMakeFiles/tests.dir/tests.c.o
23ninja: build stopped: subcommand failed.
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails
In that case, you should add the *_impl.h include in the binary’s top-level file, not here.
I think it helps to think about the *_impl.h files as if they were .cpp files: you would never include them directly in any file. You would only include the headers with the public functions/structs and then link the object files to get their implementations. But since we have a single translation unit here, we include them only in the top-level binary.
Side note: we could also remove this include from the unit test side if needed. Could just add a pointer to the RNG init function and done.
As I pointed out in the PR description, when building the new test miniframework (from #1734) as a separate translation unit, it fails:
That may be true, but without #1734, including just hash.h is perfectly correct. Exactly because hash_impl.h is included in tests.c. That’s our “unity-build” convention, as correctly described by @furszy.
hash_impl.h in unit_tests.c fixes my branch.