cmake: Respect user-provided configuration-specific flags

hebasto commented at 11:46 am on April 26, 2025: member

This PR addresses this comment:

I suppose that should only happen if the -O3 isn’t coming from an explicitly set CMAKE_CXX_FLAGS_RELEASE.

With this PR:

0$ cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
1<snip>
2C++ compiler flags .................... -O3 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
3Linker flags .......................... -O3 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie

and

0$ cmake -B build -DCMAKE_BUILD_TYPE=Release
1<snip>
2C++ compiler flags .................... -O2 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
3Linker flags .......................... -O2 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie

When calling cmake repeatedly using the same build directory, each newly provided CMAKE_CXX_FLAGS_RELEASE value will be accommodated. In such a scenario, if the user wishes to revert to the build system defaults, they should unset the CMAKE_CXX_FLAGS_RELEASE variable by passing -UCMAKE_CXX_FLAGS_RELEASE to cmake.

This PR does not aim to resolve all issues mentioned in #31491.

hebasto added the label Bug on Apr 26, 2025

hebasto added the label Build system on Apr 26, 2025

DrahtBot commented at 11:46 am on April 26, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32356.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

l0rinc commented at 12:44 pm on April 26, 2025: contributor

Concept ACK

Nit: the title is a bit ambiguous, not sure what “replacing them” mean in this context

hebasto commented at 12:53 pm on April 26, 2025: member

Concept ACK

Nit: the title is a bit ambiguous, not sure what “replacing them” mean in this context

I meant “we try to replace -O3 with -O2 but only when CMAKE_CXX_FLAGS_RELEASE is not specified by the user explicitly”. I’ll be happy to adjust wording for a better suggestion.

l0rinc commented at 1:35 pm on April 26, 2025: contributor

How about:

0cmake: Respect user-provided compiler optimization level

cmake: Respect user-provided configuration-specific flags 2f68531c53

hebasto force-pushed on Apr 26, 2025

hebasto commented at 6:20 pm on April 26, 2025: member

The commit message has been amended.

hebasto renamed this:
~~cmake: Do not override flags set by the user when replacing them~~
cmake: Respect user-provided configuration-specific flags
on Apr 26, 2025

in cmake/module/ProcessConfigurations.cmake:110 in 2f68531c53

112-    CACHE STRING
113-    "Flags used by the CXX compiler during ${config_uppercase} builds."
114-    FORCE
115-  )
116+  set(var_name CMAKE_CXX_FLAGS_${config_uppercase})
117+  if("${var_name}" IN_LIST precious_variables)

janb84 commented at 8:40 am on April 28, 2025:

NIT: precious_variables is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?

in cmake/module/ProcessConfigurations.cmake:113 in 2f68531c53

115-  )
116+  set(var_name CMAKE_CXX_FLAGS_${config_uppercase})
117+  if("${var_name}" IN_LIST precious_variables)
118+    return()
119+  endif()
120+  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" ${var_name} "${${var_name}}")

janb84 commented at 8:50 am on April 28, 2025:

0  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" "${var_name}" "${${var_name}}")

NIT: add quotes to ensure proper handling of variable name

in cmake/module/ProcessConfigurations.cmake:115 in 2f68531c53

117+  if("${var_name}" IN_LIST precious_variables)
118+    return()
119+  endif()
120+  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" ${var_name} "${${var_name}}")
121+  set(${var_name} "${${var_name}}" PARENT_SCOPE)
122+  set_property(CACHE ${var_name} PROPERTY VALUE "${${var_name}}")

janb84 commented at 8:52 am on April 28, 2025:

0  set_property(CACHE "${var_name}" PROPERTY VALUE "${${var_name}}")

NIT: add quotes to ensure proper handling of variable name

janb84 commented at 8:55 am on April 28, 2025: contributor

Few suggestions, although the changes do work as described (tested)

Master: cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"

0C++ compiler flags .................... -O2 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -mbranch-protection=bti
1Linker flags .......................... -O2 -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -fstack-protector-all -Wl,-fixup_chains -fPIE -Xlinker -pie

This PR: cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"

0C++ compiler flags .................... -O3 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -mbranch-protection=bti
1Linker flags .......................... -O3 -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -fstack-protector-all -Wl,-fixup_chains -fPIE -Xlinker -pie

(as described calling cmake again without -03 and without deleting the dir will still result in -03)

purpleKarrot commented at 11:04 am on May 5, 2025: contributor

The best approach to respect user-provided flags is to treat all CMAKE_... variables as read-only. The only place where those variables may be set, is outside the project (ie: env vars, toolchain files, initial cache, cli arguments).

Consider you have a function in C. Inside that function, you have to perform a division by one of the arguments. Since you cannot divide by zero, you treat the value 0 as if the user provided the value 2 instead. Would you consider that a valid approach? Every C programmer should agree that it would be better to have a narrow contract that excludes the value 0 as input with assert(arg != 0); for example.

The same applies in CMake. A project should not modify compile flags, irrespective of where the flags are defined. What a project may do, is provide a defensive error/warning when problematic flags are detected:

0if(CMAKE_CXX_FLAGS MATCHES "-O3")
1   message(FATAL_ERROR "Building this project with -O3 is not supported.")
2endif()

hebasto commented at 11:59 am on May 5, 2025: member

The same applies in CMake. A project should not modify compile flags, irrespective of where the flags are defined. What a project may do, is provide a defensive error/warning when problematic flags are detected:
0if(CMAKE_CXX_FLAGS MATCHES "-O3")
1   message(FATAL_ERROR "Building this project with -O3 is not supported.")
2endif()

This results in behaviour where every configuration step using -DCMAKE_BUILD_TYPE=Release with the default flags ends in an error, which is a terrible UX.

cmake: Respect user-provided configuration-specific flags #32356

Code Coverage & Benchmarks

Reviews