Introduce CMake toolchain #76

wusatosi · 2024-11-14T20:31:45Z

This PR adopts @bretbrownjr 's suggestion in #44 (review).

You can use variables like CMAKE_CXX_FLAGS_Debug_INIT to tune what a Debug or Release build entails, for what it's worth. It's maybe a little nicer to use those variables instead of CMAKE_CXX_FLAGS. But not a huge deal.

https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG_INIT.html

This PR introduces toolchain files for supported platforms:

gcc
clang
msvc

Updated CI and preset to use the new toolchain files.

Race with #82

cmake/toolchain.cmake

steve-downey

"We need to verify CMAKE_CXX_COMPILER_ID for g++ on macos is AppleClang."

Confirm what the compiler identification is for the default false g++ on Darwin is.

Marking "Request changes" so this doesn't get landed prematurely.

cmake/toolchain.cmake

steve-downey · 2024-11-15T02:50:23Z

So, yes:
The CXX compiler identification is AppleClang 16.0.0.16000026

steve-downey · 2024-11-15T03:00:39Z

Not sure if picking the toolchain for Darwin better, or making the generic toolchain smarter would work better, but either should work?

ClausKlein · 2024-11-15T03:23:23Z

Wait, that can't work!

iMac:exemplar clausklein$ cmake --preset gcc-debug  --trace-expand --trace-source=toolchain.cmake
Put cmake in trace mode, but with variables expanded.
Put cmake in trace mode, but output only lines of a specified file. Multiple options are allowed.
Preset CMake variables:

  BEMAN_BUILDSYS_SANITIZER="ASan"
  CMAKE_BUILD_TYPE="Debug"
  CMAKE_CXX_COMPILER="g++"
  CMAKE_CXX_STANDARD="20"
  CMAKE_TOOLCHAIN_FILE="cmake/toolchain.cmake"

/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(3):  set(CMAKE_C_FLAGS_RELEASE_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(4):  set(CMAKE_CXX_FLAGS_RELEASE_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(6):  set(CMAKE_C_FLAGS_RELWITHDEBINFO_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(7):  set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(16):  if(DEFINED BEMAN_BUILDSYS_SANITIZER )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(17):  if(BEMAN_BUILDSYS_SANITIZER STREQUAL ASan )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(19):  set(SANITIZER_FLAGS -fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(24):  if(NOT CMAKE_CXX_COMPILER_ID )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(25):  message(WARNING toolchain is used before CMAKE_CXX_COMPILER_ID was set! )
CMake Warning at cmake/toolchain.cmake:25 (message):
  toolchain is used before CMAKE_CXX_COMPILER_ID was set!
Call Stack (most recent call first):
  build/gcc-debug/CMakeFiles/3.31.0-dirty/CMakeSystem.cmake:6 (include)
  CMakeLists.txt:5 (project)


/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(27):  if(APPLE )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(28):  message(STATUS Using GCC on macOS; excluding -fsanitize=leak )
-- Using GCC on macOS; excluding -fsanitize=leak
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(44):  list(APPEND CMAKE_C_FLAGS_DEBUG_INIT -fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(45):  list(APPEND CMAKE_CXX_FLAGS_DEBUG_INIT -fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined )
-- The CXX compiler identification is AppleClang 16.0.0.16000026
-- Detecting CXX compiler ABI info
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(3):  set(CMAKE_C_FLAGS_RELEASE_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(4):  set(CMAKE_CXX_FLAGS_RELEASE_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(6):  set(CMAKE_C_FLAGS_RELWITHDEBINFO_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(7):  set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT -O3 )
/Users/clausklein/Workspace/cpp/beman-project/exemplar/cmake/toolchain.cmake(16):  if(DEFINED BEMAN_BUILDSYS_SANITIZER )
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
Examples to be built: identity_direct_usage;identity_as_default_projection
-- Configuring done (2.4s)

wusatosi · 2024-11-15T03:35:40Z

Not sure if picking the toolchain for Darwin better, or making the generic toolchain smarter would work better, but either should work?

I am leaning on having separate tool chain files and having like a central tool chain dispatch logic based on compiler and platform.

But then I realized there's not that much variance in building across platforms and compilers, at least in exemplar, to warrant separate files.

ClausKlein · 2024-11-15T03:48:34Z

But then I realized there's not that much variance in building across platforms and compilers, at least in exemplar, to warrant separate files.

That is why often I use the project_options
see i.e.: https://github.com/aminya/project_options/blob/main/src/Sanitizers.cmake

wusatosi · 2024-11-15T04:08:43Z

That is why often I use the project_options see i.e.: https://github.com/aminya/project_options/blob/main/src/Sanitizers.cmake

That's project looks fantastic!

I can bring this up in weekly sync and see if we want to use this.
Clang-tidy support, coverage, doxygen (and potentially vcpkg) are improvement features we would love to have, it would be fantastic if we can get all these done with this dependency.

wusatosi · 2024-11-15T04:17:21Z

Ah I think tool chain file is executed before project(), so CMAKE_CXX_COMPILER_ID maybe unset?

wusatosi · 2024-12-11T22:55:27Z

This PR proposes to add complexity to the top-level CMakeLists.txt and platform-specific flag selection. What I am proposing involves no changes to the top-level CMakeLists.txt file and no platform-specific flag selection. All complexity is moved to CI and the toolchains it uses. I disagree that my proposal goes against the minimal CMake approach.

Okay I can try to go back to implement toolchain file

wusatosi · 2024-12-12T00:25:51Z

@camio I implemented this using toolchain files, is this more what you are looking for?

cmake/gnu-toolchain.cmake

steve-downey · 2024-12-13T23:05:13Z

cmake/gnu-toolchain.cmake

+set(CMAKE_C_COMPILER gcc)
+set(CMAKE_CXX_COMPILER g++)
+
+if(BEMAN_BUILDSYS_SANITIZER STREQUAL "ASan")


Modeling sanitizers as a build type works better, either that or an entirely distinct toolchain so Thread and Memory can be uniformly applied to all packages. If everything in an address space aren't using msan or tsan the reports they provide are broken, so you have to rebuild and relink the whole world consistently.
UB sanitizer and address, don't suffer the same problems.

So something like (not tested!):

set(CMAKE_CXX_FLAGS_ASAN "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -fsanitize=address,undefined,leak" CACHE STRING "C++ ASAN Flags" FORCE )

Also at -O0 there's often no undefined behavior emitted for the sanitizer to see, for the same reasons that debug builds seg fault less often.

(optional26 doesn't use the _INIT variables because it's copied from ancient sources before that rule was clarified. Above should be using the *_INIT vars)

Thanks for the write-up.

Separate ASAN as build targets seems a bit like overkill for the exemplars use case.

The design goal here isn't to have a full fledged instrumentation based analysis build system but just a quick hand for "enable all flags for sanitizers".

Given there's no dependency for exemplar, and the current recommendation for dependency management is to build with dependency's source code instead of including the dependent library at link time. I don't think there's value in complications here.

Remember, though, exemplar doesn't do anything. It's entire purpose is to serve as a starting point and reference point for further work. Everything we've done is entirely overkill for providing ... checks notes ... std::identity.

Recommending building as part of the dependers source tree is a huge overstatement. We're making that possible, but it's still a terrible idea and does not scale to large systems. Getting to the point where we play well with package systems with public visibility is still on the todo list. (I haven't made it work with my internal one, but I know exactly how to.)

Ah I see where u r coming from, I didn't think about exemplar as a dependency for other libraries (being a dependent) and were only commenting on its use of dependency and a standalone development library / CI test target.

I think you are right, there should be an ASAN target to produce an ASAN enabled library so someone could link us as a dependency to use. I get what you are talking about. But I think this is more of a package/ export issue, outside of scope for this PR for now and to be honest outside of my skill tree for now.

Again again again, the main motivation here is just to simply CI/ workflow.

Honestly I am tentatively waiting for someone to implement package export, do a quick write up, evaluate it and yonk it over (just like code coverage).

Could we delegate this suggestion to another PR? Let me know if I should add something/ structure this tool chain in anticipation of this feature.

Modeling sanitizers as a build type works better, either that or an entirely distinct toolchain so Thread and Memory can be uniformly applied to all packages.

How so? This isn't obvious for me.

Tools like IDEs usually treat selecting the build type as a first class UI operation, where setting custom options is much more buried, so if running sanitizers is a build type, it's much easier to run them reliably that way.

The alternative of using a distinct toolchain where e.g. memory sanitizer is part of the default build types makes it easier to apply the same options across many packages. That's particularly useful if you're using a package manager and getting pre-built libraries from them if they're available. Mixing thread or memory sanitizer libraries with ones without produces far too many false positives as the sanitizer can't do the tracking for memory initialization or lock acquisition/release in the libraries that aren't instrumented.

Address and undefined behavior sanitizers don't have that tracing/tracking problem, so they're reasonably accurate if just the code under test is instrumented.

steve-downey · 2024-12-17T15:36:20Z

Figuring out better ergonomics for handling sanitizers (and fuzzers, coverage, and the rest of the laundry list) can be ongoing work. Getting sanitizers in CI is an immediate improvement.

I would base the sanitizers on the release or relwithdebinfo profile in CI, as debug tends to not exercise any of the runtime problems that the sanitizers detect.

camio · 2024-12-19T16:28:31Z

cmake/llvm-toolchain.cmake

@@ -0,0 +1,21 @@
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+include_guard(GLOBAL)


Did you consider omitting this include guard?

I will verify this afterwards.

Cmake will process the toolchain file a random number of times if it isn't guarded. If you are careful and all the string operations are idempotent this is harmless, but it's a common source of bugs, as well as overhead in configuration.

@steve-downey I think I've seen what you've described in a previous iteration, where the toolchain gets executed multiple times adding duplicated flags. If we assign string in this style: set(CMAKE_CXX_FLAGS_DEBUG_INIT "${CMAKE_CXX_FLAGS_DEBUG_INIT} ${OTHER_FLAGS}"), we may have trouble with the toolchain getting executed multiple times.

This doesn't happen on this PR as the previous values of XXX_INIT doesn't get carried over.

Looks like this is not needed. I have omitted include guard for current toolchain file, we can include this in the future if needed.

That it doesn't produce incorrect results this time doesn't mean the template should drop it. It still adds overhead on reconfigure and can produce confusing bugs if it's not idempotent.
Begin as you mean to proceed in the example we are providing as the basis for a project.

camio · 2024-12-19T17:04:35Z

cmake/llvm-toolchain.cmake

+set(CMAKE_C_COMPILER clang)
+set(CMAKE_CXX_COMPILER clang++)
+
+if(BEMAN_BUILDSYS_SANITIZER STREQUAL "ASan")


ASan already has a well-known meaning in the community: address sanitizer. Using the same word to mean "all sanitizers" is confusing.

What about "MaxSan" (maximum sanitizers) which would mean getting as many sanitizers enabled as possible.

👌 good suggestion

This is my fault, to the extent this is coming from optional26. Address composes with many others, and I didn't try to find a better name for the collection. MaxSan is at least not wrong, and I don't have a better idea.

camio · 2024-12-19T17:06:52Z

cmake/llvm-toolchain.cmake

+    set(CMAKE_CXX_FLAGS_DEBUG_INIT
+        "-fsanitize=address -fsanitize=leak -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined"
+    )
+    set(CMAKE_C_FLAGS_DEBUG_INIT
+        "-fsanitize=address -fsanitize=leak -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined"
+    )


Would this not also apply to release builds?

Addressed, nice catch

camio · 2024-12-19T17:08:31Z

cmake/msvc-toolchain.cmake

+    set(CMAKE_CXX_FLAGS_DEBUG_INIT "/fsanitize=address /Zi")
+    set(CMAKE_C_FLAGS_DEBUG_INIT "/fsanitize=address /Zi")


Is /Zi is related to ASan? Should it be enabled in all build modes?

Yes.

Do I need to include documentation for this?

Omitting this flag will cause warning C5072.

MSVC side documentation:
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-c5072?view=msvc-170

If they fix it, we could remove the flag? "Why" comments are useful.

cmake/llvm-toolchain.cmake

wusatosi · 2024-12-25T02:46:07Z

@camio this PR is ready for another round of review.

camio

I left a couple minor naming suggestions, but otherwise this looks ready to ship.

camio · 2025-01-06T22:45:20Z

cmake/appleclang-toolchain.cmake

+set(CMAKE_C_FLAGS_DEBUG_INIT "${SANITIZER_FLAGS}")
+set(CMAKE_CXX_FLAGS_DEBUG_INIT "${SANITIZER_FLAGS}")
+
+set(RELEASE_FLAG "-O3 ${SANITIZER_FLAGS}")


I think this variable should be prefixed with BEMAN_BUILDSYS_ to avoid conflicts with project variables.

camio · 2025-01-06T22:45:52Z

cmake/appleclang-toolchain.cmake

+set(CMAKE_C_FLAGS_DEBUG_INIT "${SANITIZER_FLAGS}")
+set(CMAKE_CXX_FLAGS_DEBUG_INIT "${SANITIZER_FLAGS}")
+
+set(RELEASE_FLAG "-O3 ${SANITIZER_FLAGS}")


FLAG -> FLAGS to match the existing CMake conventions.

wusatosi requested review from bretbrownjr, camio, dietmarkuehl, neatudarius and steve-downey as code owners November 14, 2024 20:31

wusatosi added 5 commits November 14, 2024 15:32

add toolchain

60c0bd5

enable santizier

054c877

fix toolchain

8a59d2e

update action script

d65aeaf

use toolchain in CI

aba4789

wusatosi force-pushed the toolchain branch from 4aad024 to aba4789 Compare November 14, 2024 20:32

wusatosi commented Nov 14, 2024

View reviewed changes

cmake/toolchain.cmake Outdated Show resolved Hide resolved

steve-downey requested changes Nov 14, 2024

View reviewed changes

wusatosi commented Nov 14, 2024

View reviewed changes

cmake/toolchain.cmake Outdated Show resolved Hide resolved

Update toolchain.cmake

730028a

This comment was marked as resolved.

Sign in to view

wusatosi force-pushed the toolchain branch 2 times, most recently from 5f20499 to 117fcd9 Compare November 15, 2024 04:30

run gcc-debug on macos

f4932e9

wusatosi force-pushed the toolchain branch 2 times, most recently from a3f18b3 to b000f40 Compare November 15, 2024 04:49

give up using toolchain

cc37b1a

wusatosi added 7 commits December 11, 2024 18:16

Merge branch 'main' into toolchain

ac9408b

use toolchain

8057e45

move generator out

6822493

remove generator at preset

b99f9ad

remove cmake/apply_santizers.cmake

f309be8

add msvc toolchain

b93b070

ensure portable

0c6f92b

wusatosi force-pushed the toolchain branch from 4d42cf4 to 0c6f92b Compare December 11, 2024 23:49

msvc does not support TSan

9127cc2

wusatosi requested a review from camio December 12, 2024 00:25

Merge branch 'main' into toolchain

ab682c6

steve-downey reviewed Dec 13, 2024

View reviewed changes

wusatosi added 2 commits December 16, 2024 13:11

apply license

3bba8f1

adopt include guard

a0f8cd2

camio reviewed Dec 19, 2024

View reviewed changes

wusatosi added 5 commits December 20, 2024 15:50

apply sanitizer to all targets

f7e1c96

update all release targets

f1f3f69

asan -> maxsan

66d8fd1

add documentation

fa2f1b9

remove unneeded include guard

1dee249

wusatosi requested a review from camio December 25, 2024 02:27

wusatosi added 2 commits December 24, 2024 21:35

Merge branch 'main' into toolchain

c0910c4

update use of toolchain for appleclang and msvc

e97cec1

wusatosi changed the title ~~Specify sanitizer parameters in CMake~~ Introduce CMake toolchain Dec 25, 2024

wusatosi requested a review from steve-downey January 6, 2025 18:51

camio approved these changes Jan 6, 2025

View reviewed changes

		@@ -0,0 +1,21 @@
		# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

		include_guard(GLOBAL)

		set(CMAKE_CXX_FLAGS_DEBUG_INIT "/fsanitize=address /Zi")
		set(CMAKE_C_FLAGS_DEBUG_INIT "/fsanitize=address /Zi")

Introduce CMake toolchain #76

Are you sure you want to change the base?

Introduce CMake toolchain #76

Conversation

wusatosi commented Nov 14, 2024 • edited Loading

steve-downey left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

steve-downey commented Nov 15, 2024

steve-downey commented Nov 15, 2024

ClausKlein commented Nov 15, 2024

wusatosi commented Nov 15, 2024

ClausKlein commented Nov 15, 2024

wusatosi commented Nov 15, 2024

wusatosi commented Nov 15, 2024

wusatosi commented Dec 11, 2024

wusatosi commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-downey commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi commented Dec 25, 2024

camio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi commented Nov 14, 2024 •

edited

Loading

steve-downey left a comment •

edited

Loading

wusatosi Dec 20, 2024 •

edited

Loading

wusatosi Dec 20, 2024 •

edited

Loading