Skip to content

feat: add sanitizer support in CMake and linux-eic-shell CI#50

Open
Copilot wants to merge 8 commits into
mainfrom
copilot/implement-use-asan-use-tsan-support
Open

feat: add sanitizer support in CMake and linux-eic-shell CI#50
Copilot wants to merge 8 commits into
mainfrom
copilot/implement-use-asan-use-tsan-support

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026 •
edited
Loading

This PR adds sanitizer support to npsim and aligns CI/runtime behavior with the epic approach to avoid DD4hep component-generation failures under sanitizer-enabled builds.

Changes Made

  • Added top-level CMake options:
    • USE_ASAN
    • USE_TSAN
    • USE_UBSAN
  • Wired sanitizer compile/link flags for ASAN/TSAN/UBSAN in top-level CMake.
  • Added UBSAN sub-sanitizer flag detection with CheckCXXCompilerFlag.
  • Updated module linking sanitizer propagation (CMAKE_MODULE_LINKER_FLAGS) to cover plugin/module targets.
  • Implemented a local dd4hep_generate_rootmap() bypass (similar to epic) that runs DD4hep::listcomponents with sanitizer runtime preloads to avoid is_component-related failures.
  • Updated .github/workflows/linux-eic-shell.yml build configuration to enable:
    • -DUSE_ASAN=ON
    • -DUSE_UBSAN=ON
      for all current matrix builds.
  • Added sanitizer runtime environment variables in linux-eic-shell CI (including currently unused TSAN), matching epic style:
    • ASAN_OPTIONS
    • LSAN_OPTIONS
    • UBSAN_OPTIONS
    • TSAN_OPTIONS
  • Added suppression file stubs (comment-only):
    • .github/asan.supp
    • .github/lsan.supp
    • .github/ubsan.supp
    • .github/tsan.supp

Copilot AI linked an issue May 17, 2026 that may be closed by this pull request
USE_ASAN and USE_TSAN are not currently supported here. #49
Open
@wdconinc wdconinc mentioned this pull request May 17, 2026
Merged
6 tasks
Copilot AI changed the title [WIP] Implement USE_ASAN, USE_UBSAN, and USE_TSAN support in CMakeLists.txt Add CMake sanitizer options for ASAN, UBSAN, and TSAN May 17, 2026
Copilot AI requested a review from wdconinc May 17, 2026 15:10
@wdconinc wdconinc marked this pull request as ready for review May 17, 2026 15:20
Copilot AI review requested due to automatic review settings May 17, 2026 15:20
Copilot AI reviewed May 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds CMake sanitizer toggles (ASAN, TSAN, UBSAN) to the top-level build configuration, aligned with the EICrecon approach.

Changes:

  • Introduces USE_ASAN, USE_TSAN, and USE_UBSAN CMake options with corresponding compile/link flag wiring.
  • Includes CheckCXXCompilerFlag for conditional UBSAN sub-sanitizer detection.
  • Iterates through UBSAN-related sanitizer modes, enabling only those supported by the compiler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
@wdconinc wdconinc force-pushed the copilot/implement-use-asan-use-tsan-support branch from 9621af0 to 9d62380 Compare May 17, 2026 16:50
wdconinc
wdconinc reviewed May 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

@copilot Enable USE_ASAN for all builds in the linux-eic-shell CI matrix. Enable UBSAN as well.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 17, 2026

@copilot Enable USE_ASAN for all builds in the linux-eic-shell CI matrix. Enable UBSAN as well.

Implemented in 6184129: linux-eic-shell CI now enables both -DUSE_ASAN=ON and -DUSE_UBSAN=ON in the build job configure step, so all matrix builds use ASAN+UBSAN.

Copilot AI changed the title Add CMake sanitizer options for ASAN, UBSAN, and TSAN Add CMake sanitizer options for ASAN, UBSAN, and TSAN, and enable ASAN/UBSAN in linux-eic-shell CI May 17, 2026
Copilot AI requested a review from wdconinc May 17, 2026 16:56
@wdconinc
Copy link
Copy Markdown
Contributor

wdconinc commented May 17, 2026

@copilot Avoid the issues with DD4hep's ist_component in https://github.com/eic/npsim/actions/runs/25996995581/job/76413142608?pr=50 implementing a similar bypass function as in https://github.com/eic/epic/blob/317fdbbd02a739f102990c96d40e99ca5e4e944f/CMakeLists.txt#L74

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 17, 2026

@copilot Avoid the issues with DD4hep's ist_component in https://github.com/eic/npsim/actions/runs/25996995581/job/76413142608?pr=50 implementing a similar bypass function as in https://github.com/eic...

Implemented in 1810fd3: added a local dd4hep_generate_rootmap override mirroring the epic bypass pattern, preloading sanitizer runtimes for DD4hep::listcomponents, and also wired sanitizer flags into CMAKE_MODULE_LINKER_FLAGS for plugin module linking.

Copilot AI requested a review from wdconinc May 17, 2026 17:19
@wdconinc
Copy link
Copy Markdown
Contributor

wdconinc commented May 17, 2026

@copilot In addition to enabling ASAN and UBSAN, also set the environment variables as in https://github.com/eic/epic/blob/317fdbbd02a739f102990c96d40e99ca5e4e944f/.github/workflows/linux-eic-shell.yml#L17-20, including for the currently unused TSAN, and create stubs for the suppression files (currently empty, i.e. with a comment only).

@wdconinc wdconinc changed the title Add CMake sanitizer options for ASAN, UBSAN, and TSAN, and enable ASAN/UBSAN in linux-eic-shell CI feat: add CMake sanitizer options for ASAN, UBSAN, and TSAN, and enable ASAN/UBSAN in linux-eic-shell CI May 17, 2026
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 17, 2026

@copilot In addition to enabling ASAN and UBSAN, also set the environment variables as in https://github.com/eic/epic/blob/317fdbbd02a739f102990c96d40e99ca5e4e944f/.github/workflows/linux-eic-shell.ym...

Implemented in e4be85c: added top-level linux-eic-shell workflow env vars for ASAN_OPTIONS, LSAN_OPTIONS, UBSAN_OPTIONS, and TSAN_OPTIONS (matching epic style, including TSAN), and added comment-only stub suppression files at .github/asan.supp, .github/lsan.supp, .github/ubsan.supp, and .github/tsan.supp.

Copilot AI changed the title feat: add CMake sanitizer options for ASAN, UBSAN, and TSAN, and enable ASAN/UBSAN in linux-eic-shell CI Add sanitizer support in CMake and linux-eic-shell CI, including DD4hep listcomponents bypass and suppression env wiring May 17, 2026
@wdconinc wdconinc changed the title Add sanitizer support in CMake and linux-eic-shell CI, including DD4hep listcomponents bypass and suppression env wiring feat: add sanitizer support in CMake and linux-eic-shell CI May 17, 2026
Copilot AI mentioned this pull request May 17, 2026
Closed
@wdconinc wdconinc force-pushed the copilot/implement-use-asan-use-tsan-support branch from 72b8c54 to 1384adf Compare May 17, 2026 19:37
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 17, 2026 •
edited
Loading

Capybara summary for PR 50

@wdconinc wdconinc enabled auto-merge (squash) May 17, 2026 22:30
@wdconinc wdconinc requested a review from veprbl May 17, 2026 22:30
@wdconinc wdconinc mentioned this pull request May 22, 2026
Merged
Copilot AI and others added 8 commits May 22, 2026 16:51
@wdconinc
cmake: add USE_ASAN, USE_TSAN, and USE_UBSAN support
38a3baa 
Agent-Logs-Url: https://github.com/eic/npsim/sessions/5b409afc-ca7a-42b0-8990-c05f6a716532

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@wdconinc
ci: enable ASAN and UBSAN in linux-eic-shell builds
3003daa 
Agent-Logs-Url: https://github.com/eic/npsim/sessions/9352f408-2848-4e2f-a0a7-34240b8926d2

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@wdconinc
cmake: bypass DD4hep listcomponents with sanitizer preload
4985734 
Agent-Logs-Url: https://github.com/eic/npsim/sessions/7ef95df0-f2f7-48c5-80df-ee2e00eed7bf

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@wdconinc
ci: add sanitizer env options and suppression stubs
c3821f5 
Agent-Logs-Url: https://github.com/eic/npsim/sessions/9eaadfe1-70e9-410d-af73-be8a35f440da

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
@wdconinc
cmake: add -shared-libasan/-shared-libsan for Clang shared/module libs
21ae79e 
With Clang, -fsanitize=address on a shared/module library only links the
bootstrap stubs (libclang_rt.asan_static), not the full ASAN runtime DSO.
The stubs forward calls to __asan_report_load* etc., which must come from
the full runtime. Since DD4hep binaries are not ASAN-instrumented, those
symbols are never loaded, causing link failures in NPDetPlugins.so.

Fix: add -shared-libasan for USE_ASAN and -shared-libsan for USE_UBSAN/
USE_TSAN to CMAKE_SHARED_LINKER_FLAGS and CMAKE_MODULE_LINKER_FLAGS when
the compiler is Clang. GCC already links -lasan/-lubsan/-ltsan as shared
DSOs via -fsanitize=..., so no change is needed there.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wdconinc
cmake: fix Clang sanitizer runtime preload library names
3deb38e 
With -shared-libasan/-shared-libsan, Clang shared/module libs get
explicit dynamic dependencies on architecture-suffixed runtime DSOs:
  libclang_rt.asan-x86_64.so
  libclang_rt.ubsan_standalone-x86_64.so
  libclang_rt.tsan-x86_64.so

The existing execute_process calls used -print-file-name=libasan.so
etc., which are GCC library names and do not resolve to the Clang
runtimes. This caused libNPDetPlugins.so to fail loading during
the dd4hep_listcomponents build step.

Fix: branch on CMAKE_CXX_COMPILER_ID and use the Clang-specific
architecture-qualified names (libclang_rt.<san>-${CMAKE_SYSTEM_PROCESSOR}.so)
for LD_PRELOAD; keep GCC names on the else branch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wdconinc
cmake: fix listcomponents hang/crash with Clang ASAN+UBSAN
511d1cf 
Three issues found and fixed through local reproduction in the eic-shell
container (clang++ 20, DD4hep 1.36):

1. CMAKE_BUILD_RPATH: embed the sanitizer runtime directory in all built
   shared libraries so the linker finds transitive dependencies (e.g.
   libGeoCad.so -> libclang_rt.asan-x86_64.so) without a warning.

2. LD_PRELOAD + detect_leaks=0: ASAN must be loaded before any allocation
   occurs (i.e. via LD_PRELOAD, not lazily via dlopen/RPATH). Loading it
   late after DD4hep/ROOT have already allocated memory causes a segfault.
   However LD_PRELOAD with the default detect_leaks=1 causes LeakSanitizer
   to scan the entire DD4hep/ROOT/Geant4 heap at listcomponents exit,
   hanging the build. We override ASAN_OPTIONS=detect_leaks=0 and
   LSAN_OPTIONS=detect_leaks=0 for this one helper invocation only, using
   cmake -E env to set the environment cleanly with generator expression
   support. verify_asan_link_order=0 is also set because LD_PRELOAD loads
   ASAN after the system libraries already mapped by listcomponents.

3. UBSAN standalone conflict with ASAN: when both USE_ASAN and USE_UBSAN
   are active with Clang, preloading libclang_rt.ubsan_standalone alongside
   libclang_rt.asan causes a CPU-spinning hang because both runtimes provide
   overlapping interceptors. Clang's ASAN runtime already integrates UBSAN,
   so libclang_rt.ubsan_standalone must not be added to PRELOAD_SANITIZER_LIB
   in this combination (confirmed via ldd: the plugin only depends on
   libclang_rt.asan, not ubsan_standalone).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wdconinc
fix: comments in CMakeLists.txt
33a7867 
Added options for address, thread, and undefined behavior sanitizers in CMake.
@wdconinc wdconinc force-pushed the copilot/implement-use-asan-use-tsan-support branch from 7f8f111 to 33a7867 Compare May 22, 2026 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@veprbl veprbl Awaiting requested review from veprbl

+1 more reviewer

@wdconinc wdconinc wdconinc approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@wdconinc wdconinc

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USE_ASAN and USE_TSAN are not currently supported here.

3 participants

@wdconinc