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

Using master @ d73f37dda221835b5109ede1b84db2dc7c4b74a1.

The following seems incorrect, because

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

is overriding the user-provided -O3.

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

cmake -B build --toolchain /root/ci_scratch/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo
C++ compiler flags .................... -O3
Linker 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:

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

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

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.

If a user wants to override any of these, they should use the CMAKE_CXX_FLAGS_<CONFIG> 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.

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:

cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
<snip>
C++ 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 ?

I still get -O2 ?

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

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.

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.

@hebasto Given that your suggestion of

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

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.

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.

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.

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?

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.

Summarizing:

Ignoring the current impl details, the main question to be answered here is: Do we consider -O3 to be harmful/dangerous?

A ~decade ago it was commonly accepted that compiler bugs/UB were more likely to be exposed at that level, but I'm not sure how relevant that is these days.

Historically we've defaulted everything to -O2, but if a user set their CXXFLAGS to -O3 then that's what they'd get. It would've been intentional.

It's a bit different with CMake because the default setting for the Release build type is -O3. Our default is RelWithDebInfo which sets -O2, but many guides recommend using Release, and it's safe to assume that many packagers will be doing so as well.

Currently we intercept flags for the Release type and downgrade to -O2. This would be pretty unexpected behavior and unnecessary unless we believe that -O3 is potentially harmful/dangerous.

Ignoring the current impl details, the main question to be answered here is: Do we consider -O3 to be harmful/dangerous?

Is it the case that the primary danger for us would be the compiler optimising out undefined behaviour? If so, then our compiler warnings and native_asan ci test which has the undefined sanitizer enabled should (in theory) catch said instances of UB?

Currently we intercept flags for the Release type and downgrade to -O2. This would be pretty unexpected behavior and unnecessary unless we believe that -O3 is potentially harmful/dangerous.

I think I might have the opposite view here, in that for our software specifically; that we should actively downgrade to "known good" configuration when users who are self-compiling will likely by-default choose Release, as you mention.

As far as I know, we can't claim we fully understand the implications of -O3 on our codebase in terms of optimizations like:

• Loop vectorization
• More aggressive loop optimizations
• Memory aliasing assumptions
• etc.

In particular we will of course be most worried about -O3 "optimizing" away cryptographic security checks, or inadvertently creating new side-channel attacks, etc.

For someone who knows what they are doing and really wants (to test?) -O3, it doesn't seem a lot of work to use our almost currently-documented workaround:

cmake -B build -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="-O3 -g"

<snip>
C++ compiler flags .................... -Wthread-safety -Wthread-safety-analysis -O3 -g -Wall -Wextra -std=c++20 -fPIC -fdebug-prefix-map=/home/will/src/core/bitcoin/src=. -fmacro-prefix-map=/home/will/src/core/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 -fcf-protection=full -fstack-clash-protection
Linker flags .......................... -Wthread-safety -Wthread-safety-analysis -O3 -g -Wall -Wextra -fuse-ld=mold -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie

I also benchmarked O2 vs O3 using benchcoin (where the O3 build also included an O3 build of libsecp), which found a partial IBD (block 800k -> 855k) using -O3 to be at most 4% faster when running with a 32GB dbcache:

  O2 (base)
  Time (mean ± σ):     885.754 s ±  4.830 s    [User: 761.173 s, System: 94.122 s]
  Range (min … max):   881.841 s … 891.152 s    3 runs

  O3 (head)
  Time (mean ± σ):     852.058 s ±  8.978 s    [User: 731.521 s, System: 93.994 s]
  Range (min … max):   843.264 s … 861.210 s    3 runs

  Summary
  head (bafb9b1e75c74a4a003a5e35ead50ea0b8a76018) ran
    1.04 ± 0.01 times faster than base (57616211dc0aa0b3cc79cfb38340e1ebf3018a37)

And with a default dbcache of 450MB only a meagre 2% faster.

  O2 (base)
  Time (mean ± σ):     1333.866 s ± 47.501 s    [User: 1026.961 s, System: 177.274 s]
  Range (min … max):   1285.064 s … 1379.948 s    3 runs

  O3 (head)
  Time (mean ± σ):     1306.266 s ± 11.358 s    [User: 944.099 s, System: 170.093 s]
  Range (min … max):   1294.300 s … 1316.897 s    3 runs

  Summary
  head (bafb9b1e75c74a4a003a5e35ead50ea0b8a76018) ran
    1.02 ± 0.04 times faster than base (57616211dc0aa0b3cc79cfb38340e1ebf3018a37)

In summary, for such a small potential maximum benefit, accepting the unknowns don't personally feel worth it to me. I think if the automatic downgrade and workaround can be documented, and a fix provided for the current -DCMAKE_CXX_FLAGS_RELEASE bug, we would be in a nice conservative position, where advanced users can still build Release with -O3 too, if they really want to, but otherwise we will override the flag for Release.

PS. The binaries for the benchcoin run are available here in case anyone would like to run their own tests with them.

PPS. It seems that Linus as recently as 2022 still does not want to entertain the option of an -O3 Kernel: https://lore.kernel.org/lkml/CA+55aFz2sNBbZyg-_i8_Ldr2e8o9dfvdSfHHuRzVtP2VMAUWPg@mail.gmail.com/ outside of manually setting special build flags (much like our current "workaround" described above).

@sipa I think you were going to follow up here?

I think the goal should be that there is as little "unexpected inconsistency" between builds. Our release (guix) binaries are created with -O2, and most of our testing is with -O2 I believe, so I think the goal should be that people don't get an -O3 build unless they specifically want that. If cmake "Release" builds create -O3 binaries by default, and there are developers out there who set "Release" by default to build binaries that are used in production, that's perhaps undesirable if we don't put a significant amount of testing specifically for -O3, and that's probably not worth doing unless evidence of significant performance gain. I don't think there is a problem with supporting developers explicitly overriding compiler flags for whatever reason, though.

We do see compiler bugs still; we've even reported ones to GCC, including one from 2019 that took several years to get fixed: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348. It wasn't -O3 specific, and only affected tests, but it still feels like it's worth trying to avoid unnecessary risks around it.

Should we just add a release note about this issue? IIUC it's a regression in our move to cmake, but I suspect that anyone who actually wants to use -O3 can use master (once it's fixed) or will be happy to review a backport.

I see a note in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft#build-system, so will remove this from the 29 milestone. Lmk if this seems wrong.

See also #32337.

@hebasto can you followup with all the questions asked in this thread:

#31491 (comment)

#31491 (comment)

It'd be good to get some clarification on which behaviour is meant to work.

I guess it could make sense to append -O2 in the case of -DCMAKE_BUILD_TYPE=RelWithDebInfo reported in OP as well, to avoid the unexpected -O3 there? (The output reading a bit odd seems like a style issue that can be ignored for now)

@hebasto can you followup with all the questions asked in this thread:

#31491 (comment)

It'd be good to get some clarification on which behaviour is meant to work. @theuni

@hebasto Have you experimented with using regular cache variables as opposed to the _INIT ones?

Cache variables are designed to be set by the user. With the suggested approach, there is no way to distinguish whether the variable is set on the command line by the user or in a toolchain file.

@theuni

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

@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.

Fixed in #32356.

Using master @ d73f37d.

The following seems incorrect, because

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

is overriding the user-provided -O3.

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

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

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

I cannot reproduce the output for the second cmake invocation.

Anything left to be done here? It looks like the initial reports were about some depends-specific issues. However, the discussion then went into a general discussion about O3 and release notes? The -DCMAKE_CXX_FLAGS_type thing should be fixed by now.

It could make sense to file a new issue, or at least change the title to clarify that this is a depends-related report. Assuming that this isn't already covered by #31920 or https://github.com/bitcoin/bitcoin/issues/30813

Given the amount of discussion here, we can re-open new issues.