cmake inconsistently overriding `-O3` (sometimes)

fanquake commented at 2:28 pm on December 13, 2024: member

Using master @ d73f37dda221835b5109ede1b84db2dc7c4b74a1.

The following seems incorrect, because

0make -C depends/ NO_QT=1 NO_WALLET=1 NO_ZMQ=1 -j18 CFLAGS="-O3" CXXFLAGS="-O3"
1cmake -B build --toolchain /root/ci_scratch/depends/aarch64-unknown-linux-gnu/toolchain.cmake
2<snip>
3C++ compiler flags .................... -O3 -O2 -g
4Linker flags .......................... -O3 -O2 -g

is overriding the user-provided -O3.

If you configure by providing an explicit -DCMAKE_BUILD_TYPE=RelWithDebInfo, you get:

0cmake -B build --toolchain /root/ci_scratch/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo
1C++ compiler flags .................... -O3
2Linker flags .......................... -O3

No -O2 at all, so the user provided -O3 is used (also missing -g for debug info?).

If you configure with -DCMAKE_BUILD_TYPE=Release:

0cmake -B build --toolchain /root/ci_scratch/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=Release
1C++ compiler flags .................... -O3 -O2
2Linker flags .......................... -O3 -O2

Now we are back to (inconsistently) overriding the user.

fanquake added this to the milestone 29.0 on Dec 13, 2024

hebasto commented at 2:47 pm on December 13, 2024: member

Optimization levels and debugging options are tied to a “build type/configuration” in CMake. If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_<CONFIG> variables. In depends, the $(host)_{release,debug}_CXXFLAGS variables handle this. However, they are not overridable by the user at the moment.

fanquake commented at 3:13 pm on December 13, 2024: member

If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_ variables.

Any user would expect this to be handled by the toolchain file, which is meant to pass everything required through to CMake. Regardless of that, the fact that the behaviour here is inconsistent depending on whether or not you pass -DCMAKE_BUILD_TYPE=RelWithDebug explictly, is confusing.

fanquake commented at 3:21 pm on December 13, 2024: member

If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_ variables.

Does this actually work in this case though? If I do:

0cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
1<snip>
2C++ compiler flags .................... -O2 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/root/ci_scratch/src=. -fmacro-prefix-map=/root/ci_scratch/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 -fstack-clash-protection -mbranch-protection=standard

I still get -O2 ?

theuni commented at 7:34 pm on December 13, 2024: member

I still get -O2 ?

I’m seeing the same. This definitely worked at some point, I assume there’s been some regression since merge?

theuni commented at 7:37 pm on December 13, 2024: member

Ah, I see. It’s: replace_cxx_flag_in_config(Release -O3 -O2)

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

fanquake commented at 12:09 pm on December 16, 2024: member

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

Possibly, but I think the way this has been implented is confusing (also not properly documented), and the goals are unclear. Are we trying to prevent compilation with -O3, or just trying to make things as non-standard (CMake) as possible. Why even do the replacement if you are going to let it be overridden (using the standard flags)? The current code, and that behaviour are just going to trip up everyone that expects ‘Release’ to already be a certain thing.

fanquake commented at 1:15 pm on January 6, 2025: member

@hebasto Given that your suggestion of

> If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_

doesn’t work. What should users do? Are we disallowing -O3 wholesale? If this is the case, it should probably be documented (with the rationale), and likely warned about at configure time (if they have set -O3 via CMAKE_CXX_FLAGS_ or any other way), rather than sliently swapping users flags out from under them.

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

This would be better than the current approach of silently changing user flags, but thinking about this more, I’m also not sure we want to be adding even more special cases, without it being clear what outcome we are trying to acheive.

theuni commented at 6:38 pm on January 6, 2025: member

I’d say this actually isn’t a special case, rather it’s fixing a logical bug in the current impl.

Currently, we essentially do: echo "$CMAKE_CXX_FLAGS_RELEASE" | sed "s/-O3/-O2/".

Instead, I think we only want to do that if we’re not overriding the user. Essentially the same as: https://github.com/bitcoin/bitcoin/blob/28.x/configure.ac#L298

So basically the user will always get -O2, unless they change the optims by setting them in the supported/documented variable: -DCMAKE_CXX_FLAGS_RELEASE=-O3

See here for the fix: https://github.com/theuni/bitcoin/commit/4f8f7de25968dc3ca93798c84781b5413e41afdf

I can PR ^^ if you’re good with it.

theuni commented at 6:51 pm on January 6, 2025: member

Note that this does not address the depends problem. The issue there is that the “-Ox” flags are jumbled in with the rest of the flags.

To fix that, we need to grab the “-O” values out of the passed-in CXXFLAGS and tack the last one onto CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT. I’ll push a commit for that to my branch.

theuni commented at 8:39 pm on January 6, 2025: member

Unfortunately it seems like the behavior around $config_FLAGS is just broken in depends. It uses _INIT values which are always appended-to.

So at the moment, the $config setting always takes precedence over what’s set in depends, even if not overriding C(XX)FLAGS. @hebasto Have you experimented with using regular cache variables as opposed to the _INIT ones?

hebasto added the label Build system on Jan 10, 2025

fanquake commented at 2:43 pm on January 10, 2025: member

Instead, I think we only want to do that if we’re not overriding the user. Essentially the same as: https://github.com/bitcoin/bitcoin/blob/28.x/configure.ac#L298

So should we also be documenting that our -DCMAKE_BUILD_TYPE=Release doesn’t give -O3 (given it’s the expected CMake default)? As users explicitly setting that are expecting -O3, without having to also provide it via CMAKE_CXX_FLAGS_RELEASE, but still wont get it via master or your branch above.

cmake inconsistently overriding -O3 (sometimes) #31491

cmake inconsistently overriding `-O3` (sometimes) #31491