cmake: Add `SECP256K1_LATE_CFLAGS` configure option

hebasto commented at 6:52 pm on March 26, 2023: member

This PR enables users to override compiler flags that have been set by the CMake-based build system, such as warning flags.

The Autotools-based build system has the same feature out-of-the-box.

See more details here.

Here are some examples of the new option usage:

0cmake -S . -B build -DSECP256K1_LATE_CFLAGS="-Wno-extra -Wlong-long"

 0cmake -S . -B build -DSECP256K1_BUILD_EXAMPLES=ON -DSECP256K1_LATE_CFLAGS=-O1
 1cmake --build build
 2...
 3In function ‘secp256k1_ecmult_strauss_wnaf’,
 4    inlined from ‘secp256k1_ecmult’ at /home/hebasto/git/secp256k1/src/ecmult_impl.h:353:5:
 5/home/hebasto/git/secp256k1/src/ecmult_impl.h:291:5: warning: ‘aux’ may be used uninitialized [-Wmaybe-uninitialized]
 6  291 |     secp256k1_ge_table_set_globalz(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, state->aux);
 7      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 8In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:29:
 9/home/hebasto/git/secp256k1/src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
10/home/hebasto/git/secp256k1/src/group_impl.h:174:13: note: by argument 3 of type ‘const secp256k1_fe *’ to ‘secp256k1_ge_table_set_globalz’ declared here
11  174 | static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const secp256k1_fe *zr) {
12      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
13In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:30:
14/home/hebasto/git/secp256k1/src/ecmult_impl.h:345:18: note: ‘aux’ declared here
15  345 |     secp256k1_fe aux[ECMULT_TABLE_SIZE(WINDOW_A)];
16      |                  ^~~
17...

Please note that in the last case providing env CFLAGS=-O1 or -DCMAKE_C_FLAGS=-O1 won’t work.

real-or-random commented at 1:23 pm on March 27, 2023: contributor

Concept ACK on giving the user the last word

This seems to be a major pain point of CMake for packaging: https://wiki.archlinux.org/title/CMake_package_guidelines#CMake_can_automatically_override_the_default_compiler_optimization_flag I really don’t understand why they don’t have native ways of prepending flags (instead of appending), or why there’s no commonly accepted practice here.

I’m not convinced that -DSECP256K1_C_FLAGS is the best option. It solves the problem, but it’s hard to discover/non-standard. After having read that piece in the Arch Wiki, maybe we should simply make sure that -DCMAKE_BUILD_TYPE=None does not set potentially undesirable flags that the user may want to override? (Though it’s not clear if we should drop all flags then, e.g., what about warning flags?)

Or perhaps we should make sure that -DCMAKE_BUILD_TYPE=None does not set -O or -g but just warnings (I assume this is the behavior already now?), and still add a SECP256K1_C_FLAGS variable as a last resort (also useful for development, debugging etc.) @theuni What’s your opinion on this?

hebasto commented at 1:53 pm on March 27, 2023: member

Or perhaps we should make sure that -DCMAKE_BUILD_TYPE=None does not set -O or -g but just warnings (I assume this is the behavior already now?)

Yes. That is the current behavior.

real-or-random commented at 3:34 pm on April 25, 2023: contributor

Or perhaps we should make sure that -DCMAKE_BUILD_TYPE=None does not set -O or -g but just warnings (I assume this is the behavior already now?), and still add a SECP256K1_C_FLAGS variable as a last resort (also useful for development, debugging etc.)

I think we should do this, but maybe rename it to SECP256K1_LATE_CFLAGS to emphasize that it’s a special thing. (And I think CFLAGS is more common than C_FLAGS.)

hebasto force-pushed on Apr 25, 2023

hebasto commented at 9:25 pm on April 25, 2023: member

Or perhaps we should make sure that -DCMAKE_BUILD_TYPE=None does not set -O or -g but just warnings (I assume this is the behavior already now?), and still add a SECP256K1_C_FLAGS variable as a last resort (also useful for development, debugging etc.)

I think we should do this, but maybe rename it to SECP256K1_LATE_CFLAGS to emphasize that it’s a special thing. (And I think CFLAGS is more common than C_FLAGS.)

Done.

hebasto renamed this:
~~cmake: Add `SECP256K1_C_FLAGS` configure option~~
cmake: Add `SECP256K1_LATE_CFLAGS` configure option
on Apr 25, 2023

in cmake/AllTargetsCompileOptions.cmake:1 in b177eff962

real-or-random commented at 10:06 pm on April 25, 2023:

Couldn’t we use add_compile_options instead? https://cmake.org/cmake/help/latest/command/add_compile_options.html

hebasto commented at 9:18 am on April 27, 2023:

It

adds options to the COMPILE_OPTIONS directory property.

which means the options won’t be visible in the src subdirectory if added after add_subdirectory(src). Beeing added before add_subdirectory(src) they will be followed by the COMPILE_OPTIONS target properties added by target_compile_options().

in CMakeLists.txt:220 in b177eff962 outdated

216@@ -217,9 +217,14 @@ if(SECP256K1_BUILD_BENCHMARK OR SECP256K1_BUILD_TESTS OR SECP256K1_BUILD_EXHAUST
217   enable_testing()
218 endif()
219 
220+set(SECP256K1_LATE_CFLAGS "" CACHE STRING "Compiler flags that can override flags set by the build system.")

real-or-random commented at 10:09 pm on April 25, 2023:

0set(SECP256K1_LATE_CFLAGS "" CACHE STRING "Compiler flags that are added to the command line after all other flags added by the build system.")

I think something like this is more precise. What the semantics of the flags are (i.e., whether earlier options can actually be overridden) depends on the compiler.

hebasto commented at 10:11 am on April 26, 2023:

Updated.

real-or-random commented at 10:09 pm on April 25, 2023: contributor

Concept ACK

cmake: Add `SECP256K1_LATE_CFLAGS` configure option 42f8c51402

hebasto force-pushed on Apr 26, 2023

theuni commented at 11:33 am on April 27, 2023: contributor

Concept ACK.

hebasto cross-referenced this on Jul 18, 2023 from issue build: Add CMake-based build system (8 of N) by hebasto

real-or-random approved

real-or-random commented at 9:43 am on January 17, 2024: contributor

ACK 42f8c5140227dbdd8ae7eaaecd914e705e1b12d0 @hebasto Is there a specific reason to keep every function in a separate file? We could also just have a single file for all the util functions. I don’t have a strong opinion, it just looks a bit uncommon to me to have separate files. But there’s nothing wrong with it, I guess.

real-or-random added the label build on Jan 17, 2024

real-or-random added the label feature on Jan 17, 2024

hebasto commented at 11:25 am on January 17, 2024: member

@hebasto Is there a specific reason to keep every function in a separate file? … I don’t have a strong opinion, it just looks a bit uncommon to me to have separate files. But there’s nothing wrong with it, I guess.

It is an accepted practice, for example, in https://github.com/llvm/llvm-project/.

We could also just have a single file for all the util functions.

How should that single file be named in such a case?

real-or-random commented at 12:09 pm on January 17, 2024: contributor

@hebasto Is there a specific reason to keep every function in a separate file? … I don’t have a strong opinion, it just looks a bit uncommon to me to have separate files. But there’s nothing wrong with it, I guess.

It is an accepted practice, for example, in llvm/llvm-project.

Ok, I see, then let’s not bother with it. :)

real-or-random merged this on Jan 17, 2024

real-or-random closed this on Jan 17, 2024

hebasto deleted the branch on Jan 17, 2024

cmake: Add SECP256K1_LATE_CFLAGS configure option #1249

cmake: Add `SECP256K1_LATE_CFLAGS` configure option #1249