Repository navigation
ci: add address, undefined behavior and thread sanitizers - #41
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved active finding remains, flagging an inaccurate CMakeLists.txt comment that misstates where the sanitizer runtime options are configured.
1 open issue
What changed in this PR
This PR adds AddressSanitizer (ASan), UndefinedBehaviorSanitizer (UBSan), and ThreadSanitizer (TSan) support to the algorithms framework, following the EICrecon conventions. It introduces opt-in CMake flags, a dedicated CI job that exercises them, LeakSanitizer suppressions needed because ROOT keeps DD4hep-related allocations alive until exit, and developer-facing documentation. It builds on the stacked PR #40, which defines the workflow-level CMAKE_CXX_STANDARD: 20 env used by the new job. Because sanitizers is a separate job, the status-check names required by the main ruleset are unchanged.
Changes:
- Add
USE_ASAN,USE_UBSAN,USE_TSANCMake options (ASan+UBSan combinable; TSan+ASan is a configure error), with UBSan flags guarded bycheck_cxx_compiler_flagso Clang-only checks are skipped elsewhere. - Add a
sanitizersCI job (clang++, Debug) withasan-ubsanandtsanmatrix configs and runtime options (halt_on_error=1for UBSan/TSan) via jobenv:. - Add
.github/lsan.suppLeakSanitizer suppressions forlibCore/libRIO, plus a README section on running tests and sanitizers locally.
| File | Description |
|---|---|
CMakeLists.txt |
Adds the three sanitizer options, flag detection, and the TSan/ASan mutual-exclusion guard. |
.github/workflows/linux-eic-shell.yml |
Adds the sanitizers matrix job building and testing under ASan+UBSan and TSan in eic-shell. |
.github/lsan.supp |
New LeakSanitizer suppressions for ROOT allocations retained until exit. |
README.md |
Documents running tests and enabling/configuring sanitizers locally. |
Notes: The changes are well-scoped and consistent with the existing build job. I left one minor documentation comment on the CMake comment wording (runtime options live in the workflow, not in .supp files). The CI portion itself can only be fully validated by an actual sanitizer run, which is still pending in the test plan.
Review effort: Balanced
Add USE_ASAN, USE_UBSAN and USE_TSAN options, following EICrecon, and a sanitizers CI job that builds with clang++ and runs the tests with ASan and UBSan combined, and with TSan. UBSan and TSan halt on the first error so findings fail CI. ROOT keeps allocations from DD4hep geometry loading alive until exit; these are suppressed for LeakSanitizer in .github/lsan.supp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bce588f to
282b727
Compare

Stacked on #40.
Summary
USE_ASAN,USE_UBSANandUSE_TSAN, following EICrecon. ASan and UBSan can be combined; TSan cannot be combined with ASan (configure error).sanitizers(clang++, Debug) with two configurations:asan-ubsanandtsan. It is a separate job, so the job names required by themainruleset are unchanged. UBSan and TSan run withhalt_on_error=1, so findings fail CI instead of only being printed..github/lsan.supp: ROOT keeps allocations from DD4hep geometry loading (libCore,libRIO) alive until exit, which made the three GeoSvc tests fail under ASan. No other suppressions are needed.The new job uses
eic/run-cvmfs-osg-eic-shell@mainlike the existing build job. Pinning that action (CodeQL alert) is left as a repo-wide decision: itsmainis 9 commits ahead of the latest tag,v1.5.0, including changes toaction.yml.Test plan
eic_xl:nightlywith clang++ Debug: 85/85 tests pass with ASan (with the LSan suppressions), UBSan, and TSan🤖 Generated with Claude Code