Document running DOLFINx under sanitizers - #4447
Draft
jhale wants to merge 28 commits into
Draft
Conversation
Introduce a DeveloperDebug CMake build type alongside Developer: -Og plus sanitizers and hardening, prioritising bug-finding over performance (#4395). Warning flags stay in DOLFINX_CXX_DEVELOPER_DEBUG_FLAGS, scoped per-target as before; -Og/-g3/sanitizers are set via the standard CMAKE_{C,CXX}_FLAGS_DEVELOPERDEBUG and linker-flag variables so they reach every target and link line, including FFCx-generated C kernels, without touching every consumer CMakeLists.txt. Also add dolfinx_validate_build_type(), rejecting an unrecognised CMAKE_BUILD_TYPE instead of silently producing an unflagged build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add the DeveloperDebug compile-options/definitions genex alongside Developer's in every consumer, and call dolfinx_validate_build_type() from the C++ and Python top-level builds (not from cpp/test/, cpp/demo/, or the generated demo template, which downstream projects may build with their own build types). Regenerate the nine demo CMakeLists.txt from the updated template. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Seed ASan/LSan/UBSan suppressions for known non-DOLFINx allocation and leak noise (MPI, PETSc, HDF5), installed alongside DolfinxDeveloperCompilerFlags.cmake so downstream builds can reference them via DOLFINX_DIR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Point AGENTS.md, cpp/demo/README.md and python/README.md at the new build type and the sanitizer runbook (cpp/cmake/sanitizers/README.md), which covers running ctest and pytest under it, including the LD_PRELOAD/ DYLD_INSERT_LIBRARIES recipe the Python extension module needs and the macOS SIP caveat for venvs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nt arrays &coeffs(idx, 0[, 0]) dereferences through mdspan::operator() to form a reference before taking its address. When a form has no Function coefficients (only Constants -- e.g. the Poisson bilinear form), coeffs has a zero-length second dimension and a null data pointer, so this is UB regardless of the index value. UBSan under the new DeveloperDebug build type (see the preceding commits) caught this on a completely mainstream fem::assemble_matrix call, aborting every C++ and Python test that assembles a form without Function coefficients. Replace with md::submdspan(coeffs, idx, md::full_extent).data_handle(), which computes the same address via pointer arithmetic only, across all 11 call sites in assemble_matrix_impl.h, assemble_vector_impl.h, assemble_scalar_impl.h and assemble_expression_impl.h. Confirmed against a DeveloperDebug + ASan/UBSan build: the C++ suite (cpp/test/, all 76 Catch2 test cases at np=1/2/3) and the Python suite (3486 tests) both now pass cleanly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…inter submdspan(coeffs, idx, md::full_extent).data_handle() is UB-free but not free: at -O2 its generic offset computation compiles to a few extra instructions (a compare and conditional select) per call versus the original &coeffs(idx, 0), inside a loop that runs once per cell/facet per assembly. Replace it with plain pointer arithmetic on a base pointer and stride hoisted out of the loop once: coeffs.data_handle() + idx * coeffs.extent(1) (and the corresponding two-term offset for interior-facet integrals, where coeffs is indexed (f, side, cstride)). This is well-defined even when coeffs is empty, since every offset then collapses to nullptr + 0 (a zero-offset addition on a null pointer is explicitly well-defined by the standard, unlike a nonzero one), and it compiles to strictly less code than the original &coeffs(idx, 0) -- the per-iteration multiply is also gone, since the stride no longer needs recomputing from an mdspan each time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eloper-debug-mode
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop dolfinx_validate_build_type() and its call sites: an unrecognised CMAKE_BUILD_TYPE now behaves like any other CMake project (silently produces no extra flags), rather than a hard error. Simplify the DeveloperDebug flag construction: drop per-flag check_cxx_compiler_flag probing for -Og/-g3/-fno-omit-frame-pointer, the per-sanitizer link-probe loop with a WARNING fallback, and the Linux+Clang -shared-libasan detection -- these flags are supported unconditionally by the GCC/Clang versions DOLFINx already requires elsewhere in this module, so probing them added complexity without changing the outcome. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nanobind_add_stub's nanobind_sanitizer_preload_env scans a target's
COMPILE_OPTIONS/LINK_OPTIONS properties for -fsanitize= to decide whether
to preload a sanitizer runtime for its stub-generation subprocess (which
imports the freshly built extension module directly). It cannot see flags
injected via the CMAKE_*_FLAGS_<CONFIG> variables that
DolfinxDeveloperCompilerFlags.cmake otherwise uses to reach every target
without per-target edits, so building the Python bindings under
DeveloperDebug with any sanitizer enabled crashed during the build itself
with "Interceptors are not working" -- before a single test could run.
Publish the flag lists computed for CMAKE_CXX_FLAGS_DEVELOPERDEBUG and
CMAKE_*_LINKER_FLAGS_DEVELOPERDEBUG as DOLFINX_CXX_DEVELOPER_DEBUG_
{SANITIZE_FLAGS,LINK_OPTIONS}, and attach them explicitly to the cpp
target in python/CMakeLists.txt so nanobind's existing detection can see
them. Guarded by CMAKE_BUILD_TYPE rather than a $<CONFIG:...> genex: a
genex-guarded entry's text is still present in COMPILE_OPTIONS even when
inactive, and nanobind's regex-based scan cannot tell the difference.
Confirmed: a from-scratch DeveloperDebug + ASan + UBSan Python build now
completes, including the stub-generation step, and pytest passes cleanly
(3486 passed, 80 skipped, 30 xfailed) once the process is launched
correctly for the sanitizer runtime to intercept from the start (see the
sanitizer runbook).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The SIP-stripping explanation in the original runbook was untested and wrong for this failure mode. The real cause, confirmed empirically: a framework Python build's venv bin/python3.x is a second launch layer over Resources/Python.app/Contents/MacOS/Python, so dyld loads the sanitizer runtime twice -- once via DYLD_INSERT_LIBRARIES at the outer launch, again when dolfinx.cpp's own @rpath-linked copy is dlopened at import time -- and the too-late second initialisation aborts with "Interceptors are not working". Launching the framework binary directly (as nanobind's own darwin-python-path.py helper already does for the build-time stub generator) avoids the double load. Also update "Known findings" to record that both suites pass cleanly as of the preceding two fixes, rather than implying an open, unquantified list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
LLM rubbish, will revise. |
Cut from ~130 to ~75 lines: lead with commands, cut rationale down to what's needed to follow the steps correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Clang's ASan runtime is named per-arch (libclang_rt.asan-<arch>.so); use uname -m instead of a hardcoded x86_64 so the command works on arm64 too. Replace the macOS recipe's fragile nested-quote one-liner with a plain heredoc script plus a flat command, both directly copy-pastable, and derive the venv's site-packages from $VIRTUAL_ENV rather than shelling out again -- matches how virtually everyone actually runs this (venv active). Verified end to end on this machine (Apple Silicon, Homebrew Python framework build): the framework-binary launch avoids the double ASan load, and site.addsitedir via $VIRTUAL_ENV picks up editable installs (ufl, ffcx) correctly, which plain PYTHONPATH does not. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The runbook jumped straight to running tests, assuming the reader already had a DeveloperDebug C++ install and Python bindings built against it. Add explicit "Build the C++ library" and "Build the Python bindings" steps, and rename the run sections for parallel structure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A dedicated build type turned out not to be needed: -Og, sanitizer flags,
and hardening are exactly what CMAKE_{CXX,C}_FLAGS(_INIT) exist for, and
apply uniformly to every target (library, tests, demos) with no DOLFINx-side
wiring at all -- there's already precedent for this in this repo's own
build-wheels.yml. Revert cpp/CMakeLists.txt, DolfinxDeveloperCompilerFlags.cmake,
generate-cmakefiles.py, the 9 demo CMakeLists.txt, cpp/dolfinx/CMakeLists.txt
and cpp/test/CMakeLists.txt to their state on main.
The one piece of this that isn't replaceable by a documented cmake flag is
kept and simplified in the next commit: nanobind_add_stub's own sanitizer
detection needs a small, genuinely necessary fix in python/CMakeLists.txt.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… one nanobind_add_stub's own sanitizer auto-detection scans a target's COMPILE_OPTIONS/LINK_OPTIONS for -fsanitize=; it can't see flags injected via CMAKE_CXX_FLAGS(_INIT), so building the Python bindings with a sanitizer enabled that way crashes during the build's own stub-generation step with "Interceptors are not working" -- before a single test runs. The previous fix gated this on CMAKE_BUILD_TYPE STREQUAL "DeveloperDebug", which no longer exists. Replace it with a plain regex match against CMAKE_CXX_FLAGS: it reacts to any -fsanitize= however it got there, needs no DOLFINx-specific cache variable, and has zero footprint when no sanitizer is active. Verified end to end: pip install with -Ccmake.define.CMAKE_CXX_FLAGS containing -fsanitize=address,undefined completes (including stub generation), the resulting cpp.so links the sanitized C++ library first in its rpath, and a real ASan-instrumented pytest run passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace every -DCMAKE_BUILD_TYPE=DeveloperDebug reference with the
CMAKE_{CXX,C}_FLAGS(_INIT)-based recipe from the preceding commits, and
trim prose throughout: lead with commands, keep only the handful of notes
that are load-bearing (CMAKE_BUILD_TYPE=Debug and *_FLAGS_DEBUG="" being
required to avoid flags getting silently overridden; the macOS
DYLD_INSERT_LIBRARIES double-load abort and the exact fix for it; the
Open MPI + DYLD_INSERT_LIBRARIES incompatibility on macOS). Drop path
placeholders a developer building DOLFINx already knows how to fill in
(CMAKE_PREFIX_PATH, --prefix) and the two linker-flag variables that are
dead for the Python build (nanobind_add_module only produces a MODULE
target; no EXE or plain SHARED target exists to apply them to).
Update AGENTS.md, cpp/demo/README.md and python/README.md to point at the
runbook without naming a build type.
Every command in the runbook has been run on this machine end to end:
cpp/test (single-process and MPI, np=1..3), the Python build (including
the nanobind stub-generation step), and a real ASan+UBSan pytest run,
using the exact commands as written.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jhale
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a runbook (
cpp/cmake/sanitizers/README.md) for building and running DOLFINx under ASan/UBSan:-Ogplus sanitizer and hardening flags via plainCMAKE_{CXX,C}_FLAGS(_INIT), no dedicated CMake build type needed — DOLFINx's targets pick these up with zero wiring, the same waybuild-wheels.ymlalready injectsCMAKE_CXX_FLAGS_INITfor its own purposes.The one piece of CMake logic that's actually necessary:
nanobind_add_stub's own sanitizer auto-detection scans a target'sCOMPILE_OPTIONS/LINK_OPTIONS, notCMAKE_CXX_FLAGS, so building the Python bindings with a sanitizer enabled this way crashes during the build's own stub-generation step with "Interceptors are not working" unless fed explicitly. Fixed with a small, generic, build-type-agnostic block inpython/CMakeLists.txt(reacts to any-fsanitize=inCMAKE_CXX_FLAGS, zero footprint otherwise).An earlier version of this PR added a dedicated
DeveloperDebugCMake build type instead. That machinery has been reverted — the doc-only approach covers the same ground with far less surface area (12 files changed here vs. 26 previously, mostly the runbook and suppressions files, no changes tocpp/CMakeLists.txt, the demo generator, or any of the 9 demoCMakeLists.txt).Every command in the runbook has been run end to end on this machine:
cpp/test(single-process and MPI, np=1..3), the Python build (including the stub-generation step), and a real ASan+UBSanpytestrun — confirmed viaotool/dyldtracing that the sanitized library is actually what gets linked and loaded, not a stray non-sanitized one from elsewhere on the search path.Also includes the
&coeffs(idx, 0[, 0])UB fix from #4446 (merged in so this could be exercise-tested under real sanitizers); that diff will disappear from here once #4446 merges and this branch rebases.