fix(build-test-workflow): make ASan-enabled Python tests work under debug-*-cpu presets#1061
fix(build-test-workflow): make ASan-enabled Python tests work under debug-*-cpu presets#1061IvanaGyro wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request documents and implements preloading rules for ASan-instrumented Python-target test runs on CPU presets to prevent crashes from C++ exceptions and false-positive leak reports. The reviewer provided valuable feedback regarding the robustness of hardcoding gcc to locate libasan.so and libstdc++.so, highlighting potential issues with compiler mismatches or multiple installed GCC versions, and suggested a more robust approach by extracting the compiler path from CMakeCache.txt.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
gemini-code-assist flagged (high priority, PR #1061) that hardcoding `gcc` to locate libasan.so/libstdc++.so for the debug-*-cpu Python-test LD_PRELOAD shim is fragile: it preloads whatever `gcc` resolves to on PATH regardless of which compiler actually built cytnx.so, so a Clang-built preset or a non-default GCC version would preload a mismatched ASan runtime, and -print-file-name silently returns its bare input filename when it can't resolve a library, yielding an unusable relative LD_PRELOAD path. Read CMAKE_CXX_COMPILER from the configured build's own CMakeCache.txt instead, fall back to `gcc` only if the cache entry is unavailable, confirm the resolved compiler identifies as GCC (Clang's --version has no "Free Software Foundation" line), and require both resolved library paths to be absolute before preloading anything -- otherwise the shim is skipped rather than exporting a broken LD_PRELOAD. Verified: build/debug-openblas-cpu/CMakeCache.txt's CMAKE_CXX_COMPILER:FILEPATH=/usr/bin/c++ resolves to GCC 13.3.0 and absolute library paths; re-ran pytests/binding_exception_test.py (the same file used to verify the original fix) with the new lookup in place: 4 passed, 1 warning -- same outcome as the gcc-hardcoded version, confirming no regression on the working case. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1061 +/- ##
===========================================
+ Coverage 60.49% 72.67% +12.18%
===========================================
Files 229 226 -3
Lines 31873 28177 -3696
Branches 71 71
===========================================
+ Hits 19282 20479 +1197
+ Misses 12570 7677 -4893
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
IvanaGyro
left a comment
IvanaGyro
left a comment
There was a problem hiding this comment.
Skill.md should not explain the reason of adding env var. The reason can be put into script command.
For easy to test, if gtest binary can contain ASan, why not also let pycynx contain ASan for debug* preset? Is there any drawback?
…te once Two review findings from IvanaGyro on PR #1060: 1. Hardcoding `gcc` to locate libasan.so/libstdc++.so has the same compiler-mismatch gap gemini-code-assist flagged in PR #1061's copy of this exact pattern: it preloads whatever `gcc` resolves to on PATH regardless of which compiler actually built cytnx.so, and -print-file-name silently returns its bare input filename (a relative path) when it can't resolve a library, yielding an unusable relative LD_PRELOAD path. Read CMAKE_CXX_COMPILER from the configured build's own CMakeCache.txt instead, confirm it identifies as GCC (Clang's --version has no "Free Software Foundation" line), and require both resolved library paths to be absolute. Unlike the skill script's graceful degradation (silently skip the shim), this CI job's toolchain is always GCC by construction (the `compilers` conda package installed earlier in this job) -- an unexpected result here means the environment changed unexpectedly, so fail loudly with `::error::` rather than silently skip the shim and hit a confusing crash two steps later. 2. Compute the resolved LD_PRELOAD value once and reuse it, instead of recomputing `gcc -print-file-name` twice (once per Python-hosted step). Added a dedicated "Resolve ASan runtime paths" step that writes the resolved value to $GITHUB_OUTPUT, referenced by both "Sanity-check the install" and "Run pytest with Python coverage" via their own step-scoped `env:`. Deliberately NOT hoisted to $GITHUB_ENV (job-wide): that would leak ASAN_OPTIONS=detect_leaks=0 into "Run gtest via ctest" too, undoing the existing design that keeps leak detection on for the one place a genuine Cytnx C++ leak would actually be caught. Verified locally: build/debug-openblas-cpu/CMakeCache.txt's CMAKE_CXX_COMPILER:FILEPATH=/usr/bin/c++ resolves via the new lookup to GCC 13.3.0 and absolute paths for both libraries; re-ran the resulting LD_PRELOAD/ASAN_OPTIONS values against `python -c "import cytnx"` end-to-end -- succeeds, matching the prior gcc-hardcoded behavior. Co-Authored-By: Claude <noreply@anthropic.com>
|
Replying to your review (pullrequestreview-4703567269) — no inline thread to reply on since it was a top-level review comment. SKILL.md wording: agreed, fixed in 25a8f74. Trimmed the ASan + Python rules bullet down to stating the behavior, matching the terser style the adjacent GPU rules bullet already uses; the full reasoning stays in "Why not build ASan into pycytnx?": checked before assuming — it already is. The actual constraint is different: running I don't see a way to avoid the shim without shipping a custom ASan-linked Python interpreter, which seems like a much larger cost than the current one-time-computed Also fixed the compiler-mismatch issue you flagged on #1060 (line 152) here as well, since this PR's Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3007b3e40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…er debug-*-cpu presets A plain `python` process under debug-openblas-cpu/debug-mkl-cpu's ASan-instrumented cytnx.so has two problems the CUDA branch's ASAN_OPTIONS export doesn't cover: ASan's __cxa_throw interceptor CHECK-fails because python never links libstdc++ (the first C++ exception surfaced through cytnx_error_msg as cytnx.CytnxError kills the process instantly, silently under pytest's captured fds), and LeakSanitizer reports hundreds of false positives from CPython's own interpreter-shutdown non-cleanup. Preload both libasan.so and libstdc++.so and set ASAN_OPTIONS=detect_leaks=0 for a Python-target --test run on debug-*-cpu presets (Linux only; the shim is skipped -- not force-fitted -- whenever the resolved compiler isn't GCC or a library path can't be resolved to an absolute path. debug-openblas-apple's dylib-based ASan is out of scope). Scoped to the pytest invocation only, not the build or the test_main/gpu_test_main ctest path, which stays untouched and keeps leak detection on. The compiler used for the preload is read from the configured build's own CMakeCache.txt CMAKE_CXX_COMPILER entry, not a bare `gcc` guess on PATH: preloading one GCC's libasan.so into a binary built by a different GCC (or by Clang, if one happens to be on PATH too) is an ASan runtime/ABI mismatch, and `-print-file-name` silently returns its bare input filename (a relative path) when it can't resolve a library, which LD_PRELOAD then fails to find. The cache entry's type is normally FILEPATH (auto-detected) but STRING when the build was configured with an explicit -DCMAKE_CXX_COMPILER=..., so the lookup matches either type; under this script's `set -euo pipefail`, a grep with no match at all would otherwise abort the whole script on this line before the `gcc` fallback runs, breaking Python test runs for an otherwise valid GCC ASan build -- `|| true` keeps that case falling through instead. Falls back to a bare `gcc` on PATH only if CMakeCache.txt has no entry, and confirms whichever compiler was resolved actually identifies as GCC (Clang's --version has no "Free Software Foundation" line) before trusting its `-print-file-name` output. Verified: build/debug-openblas-cpu/CMakeCache.txt's CMAKE_CXX_COMPILER:FILEPATH=/usr/bin/c++ resolves to GCC 13.3.0 and absolute library paths; ran pytests/binding_exception_test.py against the resulting LD_PRELOAD/ASAN_OPTIONS -- 4 passed. Confirmed the STRING-type cache entry and the CMakeCache.txt-missing paths both fall through to the `gcc` fallback without aborting the script, by exercising the grep-and-cut lookup standalone against synthetic CMakeCache.txt fixtures under `set -euo pipefail`. Co-Authored-By: Claude <noreply@anthropic.com>
f3007b3 to
c99797d
Compare
…ing test runs HPTT opens an OpenMP team of Device.Ncpus threads on every permute, even for 50-element tensors, and idle workers busy-spin between the microsecond-spaced parallel regions the test suites trigger. Under build_preset.sh's parallel ctest (--parallel nproc) the spinners from concurrent test processes oversubscribe the CPU and inflate test wall time several-fold; the Lanczos/Arnoldi suites are hit hardest (issue #1058). Export OMP_WAIT_POLICY=passive (governs every OpenMP runtime in the process, including an openmp-variant OpenBLAS) and OPENBLAS_THREAD_TIMEOUT=4 (a pthreads-variant OpenBLAS pool ignores OMP_WAIT_POLICY) for the test paths. Both are set because either OpenBLAS variant may be the one linked. benchmarks_main is exempt so measured numbers keep default threading behavior. Measured (4-core runner, debug-openblas-cpu, ctest -j4 -R '^Lanczos_Ut\.'): 23.9 s wall without the exports, 0.9 s with them. Co-Authored-By: Claude <noreply@anthropic.com>
…ositive A conda-forge openmp-variant OpenBLAS carries NEEDED libgomp.so.1 plus an $ORIGIN RPATH, and in a conda env with _openmp_mutex=llvm that soname is a symlink to libomp.so, so LLVM's OpenMP runtime loads into every ASan test process regardless of LD_LIBRARY_PATH. Its startup allocation (__kmp_allocate_align) is reported by LeakSanitizer as a leak and fails every ctest test under the debug-*-cpu presets. Document the recovery in SKILL.md's ASan section: repoint the libgomp.so.1 soname at GNU libgomp (symlink it, or LD_PRELOAD libasan.so followed by the system libgomp) instead of disabling leak detection. Co-Authored-By: Claude <noreply@anthropic.com>
…rks too Export OMP_WAIT_POLICY=passive and OPENBLAS_THREAD_TIMEOUT=4 for benchmarks_main runs as well, not only ctest/pytest, so every timed run through build_preset.sh -- tests and benchmarks alike -- executes under one and the same threading environment and benchmark numbers stay comparable with test-suite behavior. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Problem
.claude/skills/build-test-workflow/scripts/build_preset.shis the repo's single required entry point for building/testing Cytnx (perCLAUDE.md). It already has special-casedASAN_OPTIONShandling fordebug-*-cudapresets, but no equivalent handling existed fordebug-*-cpupresets (debug-openblas-cpu,debug-mkl-cpu) when running Python-target tests (pytest against an ASan-instrumentedcytnx.so).Running Python tests against these presets hit two independent problems, neither fixed by
LD_PRELOAD=libasan.soalone:__cxa_throwinterceptor CHECK-fail. ASan's__cxa_throwinterceptor needs to resolve the real__cxa_throw, which lives inlibstdc++— but a plainpythonprocess never linkslibstdc++. The instant any C++ code insidecytnx.sothrows (routine:cytnx_error_msgthrowscytnx::error, surfaced in Python ascytnx.CytnxError; ordinary error-path tests hit this constantly), the interceptor CHECK-fails and the process dies instantly. Under pytest's output capture this looks like a bareexit=1with no traceback — the ASan report goes to a captured fd pytest never shows.Neither problem is specific to any one test; both would silently break every Python-target
--testrun ofdebug-openblas-cpu/debug-mkl-cpuunder this script.Fix
In
build_preset.sh, right before the script invokespytest(not before the build — only the pytest invocation needs this), fordebug-*-cpupresets on Linux (gated oncommand -v gcc, since the mechanism is gcc/.so-specific;debug-openblas-appleis a different, dylib-based ASan mechanism and is explicitly out of scope/unverified here):This mirrors the existing
debug-*-cudabranch's convention of unconditionally exportingASAN_OPTIONS(rather than deferring to a caller-supplied value), for consistency with that existing code.Scope notes:
test_main/gpu_test_main(ctest) path — that's a natively-linked ASan executable with a clean process exit, needs no shim, and is the one place a genuine Cytnx C++ leak would actually be caught, so leak detection stays on there.debug-openblas-apple— thedebug-*-cpuglob pattern already excludes it (preset name ends in-apple, not-cpu).pip install/cmake/ninjainvocations, which don't expect an ASan-preloaded environment.Also updated
SKILL.md's "GPU rules" section with a new "ASan + Python rules" subsection documenting this, mirroring the existingdebug-*-cudabullet.Testing
Verified directly on this branch (GCC 13 / Ubuntu,
debug-openblas-cpu):build_preset.sh debug-openblas-cpu --target pycytnx --test pytests/binding_exception_test.py, a file that exercisescytnx_error_msg→cytnx.CytnxErrorviapytest.raises). Result: pytest starts ("test session starts"), then dies with zero further output andEXIT_CODE=1— no test results printed at all, matching the described silent-kill signature.4 passed, 1 warning in 8.26s,EXIT_CODE=0.pre-commit run --files .claude/skills/build-test-workflow/SKILL.md .claude/skills/build-test-workflow/scripts/build_preset.sh— clean.Still in progress / not yet confirmed on this branch (posting this draft now per reviewer request, rather than blocking on the full matrix — this PR only touches repo tooling under
.claude/skills/, which has no CI coverage, so a human/bot reviewer reading the diff can proceed in parallel):pytest pytests/run underdebug-openblas-cpuwith the fix (only a single test file has been run so far, not the whole suite).debug-mkl-cpu(onlydebug-openblas-cpuverified so far; the fix branches ondebug-*-cpusodebug-mkl-cpushould behave identically, but this hasn't been separately run).build_preset.sh debug-openblas-cuda, no--test, no GPU needed) confirming the neighboringdebug-*-cudaASAN_OPTIONSbranch is unaffected — running now, not yet complete.build_preset.sh debug-openblas-cpu --target test_main --test) confirming this change doesn't affect the ctest path — not yet run (expected to pass except for the pre-existing, unrelatedBlockUniTensorTest.CombineBondForceDoesNotRewriteUserHeldBondfailure tracked in ASan memcpy-param-overlap in BlockUniTensor::permute_ (via combineBond), CombineBondForceDoesNotRewriteUserHeldBond #1052, out of scope for this PR).Related to #1052 (part of the same ASan-hardening effort; does not fix the underlying bug directly).