Define SECP256K1_BUILD in secp256k1.c directly. #928

pull gmaxwell wants to merge 1 commits into bitcoin-core:master from gmaxwell:202105_secpbuild changing 6 files +28 −11

gmaxwell commented at 5:13 PM on May 1, 2021: contributor

This avoids building without it and makes it safer to use a custom building environment. Test harnesses need to #include secp256k1.c first now.

Fixes #927
sipa commented at 5:39 PM on May 1, 2021: contributor

Does this risk accidentally including secp256k1.h somewhere before secp256k1.c, thus getting the wrong interface definitions? If so, perhaps it's a possibility to have .h define a SECP256K1_NO_BUILD (if SECP256K1_BUILD wasn't defined already). .c can then error out if SECP256K1_NO_BUILD was defined.
gmaxwell commented at 5:49 PM on May 1, 2021: contributor

Indeed and the PR fixed a couple tests that were doing exactly that. I didn't consider it much of a concern because the non-null warnings catch it in development... but you're absolutely right that this is easy to avoid and so I added your suggestion.
in include/secp256k1.h:139 in 4849a7b250 outdated
```
 126 | @@ -127,6 +127,10 @@ typedef int (*secp256k1_nonce_function)(
 127 |  #  define SECP256K1_INLINE inline
 128 |  # endif
 129 |  
 130 | +#ifndef SECP256K1_BUILD
 131 | +# define SECP256K1_NO_BUILD
 132 | +#endif
```
real-or-random commented at 6:21 PM on May 1, 2021:

nit: I think this would be a good place to spend one or two sentences to explain the purpose of these macros. (My personal experience: I think I had two ask a few times before I understood the entire concept.)

gmaxwell commented at 7:28 PM on May 1, 2021:

There is a comment below on the nullness macro. But I can add one to the guard about the guard specifically.
real-or-random commented at 6:21 PM on May 1, 2021: contributor

ACK mod nit

Define SECP256K1_BUILD in secp256k1.c directly.

This avoids building without it and makes it safer to use a custom
 building environment.  Test harnesses need to #include secp256k1.c
 first now.

ae9e648526

sipa commented at 7:58 PM on May 1, 2021: contributor

utACK ae9e648526ceaf7cd97ba4dfe3c105db8e226c35
real-or-random approved
real-or-random commented at 9:38 AM on May 2, 2021: contributor

ACK ae9e648526ceaf7cd97ba4dfe3c105db8e226c35
real-or-random merged this on May 2, 2021
real-or-random closed this on May 2, 2021
real-or-random cross-referenced this on May 2, 2021 from issue changed include statements without prefix 'include/' by whb07
real-or-random cross-referenced this on May 2, 2021 from issue Tracking issue for manual (non-autotools) builds by real-or-random
jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick
in src/secp256k1.c:7 in ae9e648526
```
   3 | @@ -4,6 +4,8 @@
   4 |   * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
   5 |   ***********************************************************************/
   6 |  
   7 | +#define SECP256K1_BUILD
```
elichai commented at 12:31 PM on June 30, 2021:

Shouldn't this be wrapped in #ifndef SECP256k1_BUILD?

gmaxwell commented at 6:48 PM on June 30, 2021:

I don't see why-- if it was you wouldn't have spotted that your rust build was incorrectly setting it now that it shouldn't be set that way anymore.

elichai commented at 8:38 PM on June 30, 2021:

I just thought it was common practice to wrap defines in ifndef in the case that the define acts as a flag and doesn't carry any additional value. (I don't think there was any harm in the fact that we kept passing -DSECP256K1_BUILD to the compiler, as we don't really use "headers" in rust)
elichai cross-referenced this on Jun 30, 2021 from issue Fix a C compiler warning because of redefinition of SECP256K1_BUILD by elichai
real-or-random cross-referenced this on Jul 6, 2022 from issue SECP256K1_BUILD sanity checking and USE_FIELD_INV_BUILTIN/USE_SCALAR_INV_BUILTIN simplification. by gmaxwell

Contributors

Linked (view graph)

#800 SECP256K1_BUILD sanity checking and USE_FIELD_INV_BUILTIN/USE_SCALAR_INV_BUILTIN simplification.#925 changed include statements without prefix 'include/' #927 Make it impossible to build without SECP256K1_BUILD defined #929 Tracking issue for manual (non-autotools) builds