MDEV-39086 MSAN/UBSAN/ASAN builds run without basic optimization#4816
MDEV-39086 MSAN/UBSAN/ASAN builds run without basic optimization#4816dr-m merged 1 commit intoMariaDB:10.6from
Conversation
MSAN/UBSAN/ASAN/TSAN builds are preforming addition runtime checks requiring additional CPU to do so. To help the test systems of these run with a bit faster, but still be debuggable, we should add -Og where another C/CXX optimization flag isn't specified. Gcc/Clang environment that support these range of sanitizers use -O followed by 0-3, g, s, z, and apparently "fast". These are the flags matched by the regex.
grooverdan
added
the
MariaDB Foundation
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
I see that there should be a way to generate an unoptimized build by setting -O0 in CMAKE_C_FLAGS or CMAKE_CXX_FLAGS. Therefore, this should be a safe change to apply.
| AND NOT CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE} MATCHES "-O[0-9sgzf]") | ||
| MY_CHECK_AND_SET_COMPILER_FLAG("-Og") | ||
| ENDIF() | ||
|
|
There was a problem hiding this comment.
CMAKE_C_FLAGS_${CMAKE_BUILD_TYPE} does not work unless you uppercase the build type.
You do not want that to run for MSVC, because options check is brittle, assume GCCisms
There was a problem hiding this comment.
Given they where under the sanitizers I assumed MSVC wasn't an option.
Ack on upper case build type. I'll work out a fix.
There was a problem hiding this comment.
MSVC is an option, for ASAN, not anything else yet.
There was a problem hiding this comment.
Actually, it is also an option because of both cl and clang-cl (which accepts MSVC command line). Thus please add AND NOT MSVC to the check
There was a problem hiding this comment.
I think @grooverdan is already on it, and it is totally doable by the original committer :) 2 points -
- AND NOT MSVC
- uppercase CMAKE_BUILD_TYPE when looking for flags.
There was a problem hiding this comment.
Overall though, I think -Og should be something set in the CI, rather than in CMakeLists.txt, my opinion.
There was a problem hiding this comment.
Having seen a few years of developers asking for X flag and and Y flag out into the CI environment, I'm pretty firmly of the opinion that the build configuration should be standard, available to everyone external, usable by users in a default configuration, and go through a review process of developer peers rather than back channel.
There was a problem hiding this comment.
Having seen a bunch of CMake in a few years, I developed a certain dislike when people sneak other options that what was asked for. I have not seen it anywhere else, and I have seen a bunch of CMake code.
If I want ASAN it is just ASAN, and not "ASAN and lets make your debugging a little bit more complicated". If I want optimized ASAN, it will be -DCMAKE_BUILD_TYPE=RelWithDebInfo. If I want something special , CFLAGS and CXXFLAGS environment variables. Rather than forced -Og
There are times when something makes sense, such as disabling SAFEMALLOC with sanitizer, because it is layer upon another layer, and sanitizer is doing better job.
But here I do not remember anyone complained about slow ASAN. It is absolutely solely for speeding up CI, and this is how it was in the MDEV, "to help the CI systems .. run with a bit faster"
3 participants
MSAN/UBSAN/ASAN/TSAN builds are preforming addition runtime checks requiring additional CPU to do so. To help the test systems of these run with a bit faster, but still be debuggable, we should add -Og where another C/CXX optimization flag isn't specified.
Gcc/Clang environment that support these range of sanitizers use -O followed by 0-3, g, s, z, and apparently "fast". These are the flags matched by the regex.