Skip to content

Add support for sanitizers in cmake script and in CI #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jbcoe wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add support for sanitizers in cmake script and in CI #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8063112
Enable sanitizers
jbcoe Mar 31, 2026
07b4d93
Merge branch 'main' into jbcoe/sanitizers
jbcoe Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions .github/workflows/sanitizers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
name: Sanitizers

on:
schedule:
- cron: "0 1 * * *"
push:
branches: [main]
paths:
- ".github/workflows/sanitizers.yml"
- "**/*.cc"
- "**/*.h"
- "**/CMakeLists.txt"
- "CMakePresets.json"
- "**/*.cmake"
- "scripts/cmake.py"
pull_request:
branches: [main]
paths:
- ".github/workflows/sanitizers.yml"
- "**/*.cc"
- "**/*.h"
- "**/CMakeLists.txt"
- "CMakePresets.json"
- "**/*.cmake"
- "scripts/cmake.py"

env:
UV_FROZEN: 1

jobs:
sanitized-tests:
name: ${{ matrix.sanitizer }} (Clang-${{ matrix.clang_version }})
runs-on: ubuntu-24.04
strategy:
fail-fast: false
matrix:
sanitizer: ["asan", "tsan", "msan"]
clang_version: ["19"]
steps:
- uses: actions/checkout@v4
- uses: seanmiddleditch/gha-setup-ninja@master
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This workflow references seanmiddleditch/gha-setup-ninja@master, which is a mutable ref. For supply-chain safety and reproducibility, pin third-party actions to a version tag or (preferably) a commit SHA.

Suggested change
- uses: seanmiddleditch/gha-setup-ninja@master
- uses: seanmiddleditch/gha-setup-ninja@v0.3

Copilot uses AI. Check for mistakes.
- name: Install Clang
uses: egor-tensin/setup-clang@v1
with:
version: ${{ matrix.clang_version }}
platform: x64
- name: Install uv
uses: astral-sh/setup-uv@v5
- name: Set up Python
run: uv sync
- name: Run CMake (Default VTable)
run: |
UBSAN_FLAG=""
if [ "${{ matrix.sanitizer }}" = "asan" ]; then
UBSAN_FLAG="--ubsan"
fi
./scripts/cmake.sh -B build_${{ matrix.sanitizer }}_default --debug --${{ matrix.sanitizer }} ${UBSAN_FLAG}
env:
CC: clang-${{ matrix.clang_version }}
CXX: clang++-${{ matrix.clang_version }}
- name: Run CMake (Manual VTable)
run: |
UBSAN_FLAG=""
if [ "${{ matrix.sanitizer }}" = "asan" ]; then
UBSAN_FLAG="--ubsan"
fi
./scripts/cmake.sh -B build_${{ matrix.sanitizer }}_manual --debug --manual-vtable --${{ matrix.sanitizer }} ${UBSAN_FLAG}
env:
CC: clang-${{ matrix.clang_version }}
CXX: clang++-${{ matrix.clang_version }}
9 changes: 9 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ option(ENABLE_SANITIZERS
"Enable Address Sanitizer and Undefined Behaviour Sanitizer if available"
OFF)

option(ENABLE_ASAN "Enable Address Sanitizer" OFF)
option(ENABLE_UBSAN "Enable Undefined Behaviour Sanitizer" OFF)
option(ENABLE_TSAN "Enable Thread Sanitizer" OFF)
option(ENABLE_MSAN "Enable Memory Sanitizer" OFF)

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENABLE_SANITIZERS is still documented as enabling ASAN+UBSAN, but after this change it no longer actually enables any sanitizer unless one of ENABLE_{A,U,T,M}SAN is also set. If a user/config sets only -DENABLE_SANITIZERS=ON (the existing public option), sanitizers.cmake will run but xyz_add_test() won’t link any sanitizer interface libs because ENABLE_ASAN/ENABLE_UBSAN remain OFF.

To avoid a silent behavior regression, either (a) keep the old semantics by treating ENABLE_SANITIZERS=ON as enabling ASAN+UBSAN (and update xyz_add_test() generator expressions accordingly), or (b) deprecate/remove ENABLE_SANITIZERS and update its help string to reflect that it’s an internal aggregate flag/alias only.

Suggested change
# Preserve documented behavior: ENABLE_SANITIZERS enables ASAN and UBSAN.
if(ENABLE_SANITIZERS)
set(ENABLE_ASAN ON)
set(ENABLE_UBSAN ON)
endif()
# Aggregate flag: turn ENABLE_SANITIZERS on if any specific sanitizer is enabled.

Copilot uses AI. Check for mistakes.
if(ENABLE_ASAN OR ENABLE_UBSAN OR ENABLE_TSAN OR ENABLE_MSAN)
set(ENABLE_SANITIZERS ON)
Comment on lines +32 to +33
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENABLE_SANITIZERS is still a public option documented as enabling ASAN+UBSAN, but with the new per-sanitizer options it no longer implies any specific sanitizer is enabled. If a user configures with -DENABLE_SANITIZERS=ON, tests won’t actually get sanitized unless ENABLE_ASAN/ENABLE_UBSAN are also set. Consider making ENABLE_SANITIZERS=ON default to ASAN+UBSAN when no specific ENABLE_*SAN flags are provided (or explicitly deprecate/remove the old option).

Suggested change
if(ENABLE_ASAN OR ENABLE_UBSAN OR ENABLE_TSAN OR ENABLE_MSAN)
set(ENABLE_SANITIZERS ON)
if(ENABLE_ASAN OR ENABLE_UBSAN OR ENABLE_TSAN OR ENABLE_MSAN)
# If any individual sanitizer is enabled, mark sanitizers as enabled globally.
set(ENABLE_SANITIZERS ON)
elseif(ENABLE_SANITIZERS)
# Legacy behavior: ENABLE_SANITIZERS implies ASAN + UBSAN by default when
# no specific sanitizers have been selected.
set(ENABLE_ASAN ON)
set(ENABLE_UBSAN ON)

Copilot uses AI. Check for mistakes.
endif()

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no CMake-side validation preventing incompatible sanitizer combinations (e.g., ASAN+TSAN, ASAN+MSAN, TSAN+MSAN). Since these options are now user-facing, it’s worth failing fast at configure time with a clear message(FATAL_ERROR ...) when an invalid combination is selected (while still allowing ASAN+UBSAN if desired).

Suggested change
# Fail fast on incompatible sanitizer combinations. ASAN+UBSAN is allowed.
if(ENABLE_ASAN AND ENABLE_TSAN)
message(
FATAL_ERROR
"Invalid sanitizer configuration: ASAN (AddressSanitizer) and TSAN (ThreadSanitizer) "
"cannot be enabled at the same time.")
endif()
if(ENABLE_ASAN AND ENABLE_MSAN)
message(
FATAL_ERROR
"Invalid sanitizer configuration: ASAN (AddressSanitizer) and MSAN (MemorySanitizer) "
"cannot be enabled at the same time.")
endif()
if(ENABLE_TSAN AND ENABLE_MSAN)
message(
FATAL_ERROR
"Invalid sanitizer configuration: TSAN (ThreadSanitizer) and MSAN (MemorySanitizer) "
"cannot be enabled at the same time.")
endif()

Copilot uses AI. Check for mistakes.
# Set up Python discovery
set(Python3_FIND_VIRTUALENV FIRST)
if(DEFINED ENV{UV_PROJECT_ENVIRONMENT})
Expand Down
14 changes: 13 additions & 1 deletion GEMINI.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,19 @@ general defaults for this repository.

- **Tooling:** Always use `uv` for Python dependency management (`uv run ...`).
- **Build & Test:** Use `scripts/cmake.sh` for all build and test operations.
- **Verification:** All changes must be verified against both the default (virtual dispatch) and manual vtable configurations. The `scripts/cmake.sh` script must be run twice: once without any flags, and a second time with the `--manual-vtable` flag to build and test the alternative implementation.
The `scripts/cmake.sh` entrypoint supports `--debug`, `--release`,
`--manual-vtable`, `--asan`, `--ubsan`, `--tsan`, and `--msan`.
- **Compiler Preferences:** Prefer Clang 19+ for sanitizer-based verification
and CI, as it provides superior support for MSAN and TSAN compared to
older GCC versions.
- **Verification:** All changes must be verified against both the default
(virtual dispatch) and manual vtable configurations. The `scripts/cmake.sh`
script must be run twice: once without any flags, and a second time with
the `--manual-vtable` flag to build and test the alternative implementation.
- **Sanitizer Verification:** When modifying memory-sensitive or concurrent
code, verify changes locally using at least one sanitizer (e.g.,
`./scripts/cmake.sh --asan` or `--tsan`). Note that ASAN, TSAN, and MSAN
are mutually exclusive.
Comment on lines +36 to +37
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section states “ASAN, TSAN, and MSAN are mutually exclusive”, but the build entrypoint currently allows combining flags (e.g. --asan --tsan) and CMake doesn’t reject it. Either enforce mutual exclusivity in scripts/cmake.py/CMake configure (preferred), or adjust this guidance to match the actual supported combinations.

Suggested change
`./scripts/cmake.sh --asan` or `--tsan`). Note that ASAN, TSAN, and MSAN
are mutually exclusive.
`./scripts/cmake.sh --asan` or `--tsan`). While the build entrypoint
currently allows multiple sanitizer flags, only one of `--asan`, `--tsan`,
or `--msan` should be used per invocation; combinations are not a supported
configuration.

Copilot uses AI. Check for mistakes.
- **Post-Change Checks:** Tests and pre-commit checks MUST be run after any
modifications to the codebase.

Expand Down
44 changes: 41 additions & 3 deletions cmake/sanitizers.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,36 @@
include_guard(GLOBAL)

if(ENABLE_SANITIZERS)
set(SANITIZER_FLAGS_ASAN "-fsanitize=address -fno-omit-frame-pointer")
set(SANITIZER_FLAGS_ASAN "-fsanitize=address" "-fno-omit-frame-pointer")
set(SANITIZER_FLAGS_UBSAN "-fsanitize=undefined")
set(SANITIZER_FLAGS_TSAN "-fsanitize=thread")
set(SANITIZER_FLAGS_MSAN "-fsanitize=memory" "-fsanitize-memory-track-origins")

include(CheckCXXCompilerFlag)
check_cxx_compiler_flag("${SANITIZER_FLAGS_ASAN}" COMPILER_SUPPORTS_ASAN)
check_cxx_compiler_flag("${SANITIZER_FLAGS_UBSAN}" COMPILER_SUPPORTS_UBSAN)

# Check ASAN
set(CMAKE_REQUIRED_FLAGS "-fsanitize=address -fno-omit-frame-pointer")
set(CMAKE_REQUIRED_LINK_OPTIONS "-fsanitize=address")
check_cxx_compiler_flag("-fsanitize=address" COMPILER_SUPPORTS_ASAN)

# Check UBSAN
set(CMAKE_REQUIRED_FLAGS "-fsanitize=undefined")
set(CMAKE_REQUIRED_LINK_OPTIONS "-fsanitize=undefined")
check_cxx_compiler_flag("-fsanitize=undefined" COMPILER_SUPPORTS_UBSAN)

# Check TSAN
set(CMAKE_REQUIRED_FLAGS "-fsanitize=thread")
set(CMAKE_REQUIRED_LINK_OPTIONS "-fsanitize=thread")
check_cxx_compiler_flag("-fsanitize=thread" COMPILER_SUPPORTS_TSAN)

# Check MSAN
set(CMAKE_REQUIRED_FLAGS "-fsanitize=memory -fsanitize-memory-track-origins")
set(CMAKE_REQUIRED_LINK_OPTIONS "-fsanitize=memory")
check_cxx_compiler_flag("-fsanitize=memory" COMPILER_SUPPORTS_MSAN)

# Reset required flags
unset(CMAKE_REQUIRED_FLAGS)
unset(CMAKE_REQUIRED_LINK_OPTIONS)
Comment on lines +11 to +33
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module unconditionally unset()s CMAKE_REQUIRED_FLAGS and CMAKE_REQUIRED_LINK_OPTIONS at the end. If either variable was set by the caller (or another check module) before including sanitizers.cmake, that state will be lost.

Prefer cmake_push_check_state() / cmake_pop_check_state() (or save/restore the previous values) around the check_cxx_compiler_flag() calls so you don’t clobber the global check state.

Copilot uses AI. Check for mistakes.

if(COMPILER_SUPPORTS_ASAN)
add_library(asan INTERFACE IMPORTED)
Expand All @@ -21,4 +45,18 @@ if(ENABLE_SANITIZERS)
ubsan PROPERTIES INTERFACE_COMPILE_OPTIONS "${SANITIZER_FLAGS_UBSAN}"
INTERFACE_LINK_OPTIONS "${SANITIZER_FLAGS_UBSAN}")
endif(COMPILER_SUPPORTS_UBSAN)

if(COMPILER_SUPPORTS_TSAN)
add_library(tsan INTERFACE IMPORTED)
set_target_properties(
tsan PROPERTIES INTERFACE_COMPILE_OPTIONS "${SANITIZER_FLAGS_TSAN}"
INTERFACE_LINK_OPTIONS "${SANITIZER_FLAGS_TSAN}")
endif(COMPILER_SUPPORTS_TSAN)

if(COMPILER_SUPPORTS_MSAN)
add_library(msan INTERFACE IMPORTED)
set_target_properties(
msan PROPERTIES INTERFACE_COMPILE_OPTIONS "${SANITIZER_FLAGS_MSAN}"
INTERFACE_LINK_OPTIONS "${SANITIZER_FLAGS_MSAN}")
endif(COMPILER_SUPPORTS_MSAN)
endif(ENABLE_SANITIZERS)
6 changes: 4 additions & 2 deletions cmake/xyz_add_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@ function(xyz_add_test)
target_link_libraries(
${XYZ_NAME}
PRIVATE ${XYZ_LINK_LIBRARIES} GTest::gtest_main common_compiler_settings
$<$<BOOL:${COMPILER_SUPPORTS_ASAN}>:asan>
$<$<BOOL:${COMPILER_SUPPORTS_UBSAN}>:ubsan>)
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_ASAN}>,$<BOOL:${ENABLE_ASAN}>>:asan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_UBSAN}>,$<BOOL:${ENABLE_UBSAN}>>:ubsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_TSAN}>,$<BOOL:${ENABLE_TSAN}>>:tsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_MSAN}>,$<BOOL:${ENABLE_MSAN}>>:msan>)
Comment on lines +80 to +83
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking sanitizer interface libs is now gated on ENABLE_*SAN, which means turning on the existing ENABLE_SANITIZERS option alone will no longer apply any sanitizer flags to tests (regression vs the previous behavior). Consider either (1) keying ASAN/UBSAN on ENABLE_SANITIZERS as a fallback, or (2) making ENABLE_SANITIZERS=ON set ENABLE_ASAN/ENABLE_UBSAN by default.

Suggested change
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_ASAN}>,$<BOOL:${ENABLE_ASAN}>>:asan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_UBSAN}>,$<BOOL:${ENABLE_UBSAN}>>:ubsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_TSAN}>,$<BOOL:${ENABLE_TSAN}>>:tsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_MSAN}>,$<BOOL:${ENABLE_MSAN}>>:msan>)
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_ASAN}>,$<OR:$<BOOL:${ENABLE_ASAN}>,$<BOOL:${ENABLE_SANITIZERS}>>>:asan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_UBSAN}>,$<OR:$<BOOL:${ENABLE_UBSAN}>,$<BOOL:${ENABLE_SANITIZERS}>>>:ubsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_TSAN}>,$<OR:$<BOOL:${ENABLE_TSAN}>,$<BOOL:${ENABLE_SANITIZERS}>>>:tsan>
$<$<AND:$<BOOL:${COMPILER_SUPPORTS_MSAN}>,$<OR:$<BOOL:${ENABLE_MSAN}>,$<BOOL:${ENABLE_SANITIZERS}>>>:msan>)

Copilot uses AI. Check for mistakes.

set_target_properties(
${XYZ_NAME}
Expand Down
10 changes: 10 additions & 0 deletions scripts/cmake.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ def main():
action="store_true",
help="Set XYZ_PROTOCOL_GENERATE_MANUAL_VTABLE=ON",
)
parser.add_argument("--asan", action="store_true", help="Enable Address Sanitizer")
parser.add_argument(
"--ubsan", action="store_true", help="Enable Undefined Behaviour Sanitizer"
)
parser.add_argument("--tsan", action="store_true", help="Enable Thread Sanitizer")
parser.add_argument("--msan", action="store_true", help="Enable Memory Sanitizer")
Comment on lines +35 to +40
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new sanitizer CLI flags can be combined arbitrarily (e.g. --asan --tsan), but ASAN/TSAN/MSAN are incompatible in practice (and GEMINI.md now states they’re mutually exclusive). Consider enforcing this at the CLI layer (argparse mutually-exclusive group for --asan/--tsan/--msan, while still allowing --ubsan to combine with --asan), and emit a clear error if an invalid combination is requested.

Suggested change
parser.add_argument("--asan", action="store_true", help="Enable Address Sanitizer")
parser.add_argument(
"--ubsan", action="store_true", help="Enable Undefined Behaviour Sanitizer"
)
parser.add_argument("--tsan", action="store_true", help="Enable Thread Sanitizer")
parser.add_argument("--msan", action="store_true", help="Enable Memory Sanitizer")
# Sanitizers: ASAN/TSAN/MSAN are mutually exclusive, but UBSAN can be combined
san_group = parser.add_mutually_exclusive_group()
san_group.add_argument(
"--asan", action="store_true", help="Enable Address Sanitizer"
)
san_group.add_argument(
"--tsan", action="store_true", help="Enable Thread Sanitizer"
)
san_group.add_argument(
"--msan", action="store_true", help="Enable Memory Sanitizer"
)
parser.add_argument(
"--ubsan", action="store_true", help="Enable Undefined Behaviour Sanitizer"
)

Copilot uses AI. Check for mistakes.
parser.add_argument("-B", "--build-dir", help="Build directory")
parser.add_argument(
"--clean", action="store_true", help="Fresh configuration and clean-first build"
Expand Down Expand Up @@ -66,6 +72,10 @@ def log(msg):
"--preset",
preset,
f"-DXYZ_PROTOCOL_GENERATE_MANUAL_VTABLE={'ON' if args.manual_vtable else 'OFF'}",
f"-DENABLE_ASAN={'ON' if args.asan else 'OFF'}",
f"-DENABLE_UBSAN={'ON' if args.ubsan else 'OFF'}",
f"-DENABLE_TSAN={'ON' if args.tsan else 'OFF'}",
f"-DENABLE_MSAN={'ON' if args.msan else 'OFF'}",
]
if args.build_dir:
configure_args.extend(["-B", args.build_dir])
Expand Down
Loading