ci(cmake_tests): run the main test gate under ASan (debug-openblas-cpu)#1060
Open
IvanaGyro wants to merge 2 commits into
Open
ci(cmake_tests): run the main test gate under ASan (debug-openblas-cpu)#1060IvanaGyro wants to merge 2 commits into
IvanaGyro wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates CLAUDE.md to document that the ci-cmake_tests CI gate now builds under the debug-openblas-cpu preset with USE_DEBUG=ON and AddressSanitizer enabled. It advises developers to reproduce issues locally using this preset rather than a release-mode CPU build to catch memory-safety issues. There are no review comments, so I have no feedback to provide.
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.
IvanaGyro
commented
Jul 15, 2026
IvanaGyro
added a commit
that referenced
this pull request
Jul 15, 2026
…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>
IvanaGyro
added a commit
that referenced
this pull request
Jul 15, 2026
IvanaGyro's review on PR #1060: the added ci-cmake_tests bullet detail is redundant with what the build-test-workflow skill's own docs already cover for reproducing this gate locally under debug-openblas-cpu. Reverts to the original one-line bullet. 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 #1060 +/- ##
===========================================
+ Coverage 60.49% 72.94% +12.44%
===========================================
Files 229 226 -3
Lines 31873 28225 -3648
Branches 71 71
===========================================
+ Hits 19282 20588 +1306
+ Misses 12570 7616 -4954
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:
|
Member
Author
|
The failed test is fixed by #1054. |
IvanaGyro
marked this pull request as ready for review
July 15, 2026 13:51
![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: 940f286400
ℹ️ 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".
The "Editable install" step built with CMAKE_BUILD_TYPE=Debug and RUN_TESTS=ON via individual cmake.define overrides, but left USE_DEBUG at the preset default OFF, so -fsanitize=address was never applied to the cytnx library. That let a real overlapping-memcpy UB bug (BlockUniTensor::combineBonds, #1052) go undetected in CI for a long time -- it only surfaced when a developer happened to build locally with a debug-* preset. Switch the step to configure via --preset=debug-openblas-cpu instead of individual cmake.define flags; the preset already sets CMAKE_BUILD_TYPE=Debug, RUN_TESTS=ON, and USE_DEBUG=ON, so this is now the main CI gate running under ASan. ASan-instrumented cytnx.so loaded into the uninstrumented `python` binary needs libasan.so and libstdc++.so preloaded via LD_PRELOAD, or every native (C++-side) exception crashes the process: ASan's __cxa_throw interceptor CHECK-fails because plain `python` doesn't link libstdc++, so it can't resolve the real symbol at init. Add that preload, plus ASAN_OPTIONS=detect_leaks=0 (LeakSanitizer's exit-time scan otherwise reports CPython's own deliberately-not-cleaned-up interpreter state as leaks), to the two Python-hosted steps: "Sanity-check the install" and "Run pytest with Python coverage". The gtest steps are left untouched -- test_main links against ASan natively and needs no shim, and this keeps leak detection on for the one place a genuine Cytnx C++ leak would actually be caught. 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 compiler/GCC version is an ASan runtime/ABI mismatch, and -print-file-name silently returns its bare input filename (a relative path) when it can't resolve the library, yielding an unusable relative LD_PRELOAD path. This job's toolchain is always GCC (the `compilers` conda package installed earlier in the job), so failing to resolve a GCC compiler here means the environment changed unexpectedly -- fail loudly with `::error::` rather than silently continue and hit a confusing crash two steps later. The lookup runs once in a dedicated "Resolve ASan runtime paths" step that writes the resolved LD_PRELOAD value to $GITHUB_OUTPUT, reused by both Python-hosted steps via their own step-scoped `env:` instead of recomputing `gcc -print-file-name` twice. Deliberately not hoisted to $GITHUB_ENV (job-wide): that would leak ASAN_OPTIONS=detect_leaks=0 into "Run gtest via ctest" too, undoing the deliberate leak-detection-on behavior for that step. Verified locally: build/debug-openblas-cpu/CMakeCache.txt's CMAKE_CXX_COMPILER:FILEPATH=/usr/bin/c++ resolves via this lookup to GCC 13.3.0 and absolute paths for both libraries; ran the resulting LD_PRELOAD/ASAN_OPTIONS values against `python -c "import cytnx"` end-to-end -- succeeds. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
force-pushed
the
claude/issue-1052-ci-asan-preset
branch
from
July 15, 2026 19:02
940f286 to
a965368
Compare
… the ASan run The ASan gate runs the full ctest suite serially. HPTT's OpenMP worker threads and OpenBLAS's thread pool busy-spin between the tiny, microsecond-spaced parallel regions the iterative eigensolver suites (Lanczos/Arnoldi, issue #1058) trigger; on the runner's few cores those spinners oversubscribe the CPU and inflate ctest wall time several-fold. Pin libopenblas=*=*openmp* so OpenBLAS shares the single llvm OpenMP runtime HPTT and cytnx's -fopenmp code already use -- conda-forge's openmp OpenBLAS hard-requires _openmp_mutex=*_llvm, which also makes the previously explicit mutex pin redundant. Set OMP_WAIT_POLICY=passive so idle worker threads sleep instead of spinning. One knob then governs all OpenMP threading in the job. Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Problem
The
ci-cmake_testsjob builds the "Editable install" step withCMAKE_BUILD_TYPE=Debug+RUN_TESTS=ON, but explicitly leavesUSE_DEBUG=OFF(the preset default), so-fsanitize=addressis never applied. AddressSanitizer therefore never runs in the main CI gate. This let a real overlapping-memcpyUB bug (BlockUniTensor::combineBonds, #1052) sit undetected for a long time — it only surfaces locally when a developer happens to build with adebug-*preset.Fix
--config-settings=cmake.args=--preset=debug-openblas-cpuinstead of individualcmake.defineoverrides. The preset already setsCMAKE_BUILD_TYPE=Debug,RUN_TESTS=ON, andUSE_DEBUG=ON, so this job now runs the whole suite (ctest + pytest) under ASan.build-dir=buildis kept so ctest/gcovr still find a stable path.USE_DEBUGstayed off; that rationale is now inverted.LD_PRELOAD/ASAN_OPTIONSshim to the two Python-hosted steps only ("Sanity-check the install" and "Run pytest with Python coverage"): ASan-instrumentedcytnx.soloaded into the uninstrumentedpythonbinary needslibasan.sopreloaded, or every native (C++-side) exception crashes the process — ASan's__cxa_throwinterceptor CHECK-fails because plainpythondoesn't linklibstdc++, so it can't resolve the real symbol at init.libstdc++.somust be preloaded alongsidelibasan.sofor the same reason.ASAN_OPTIONS=detect_leaks=0is also needed for these two steps — LeakSanitizer's exit-time scan otherwise reports CPython's own deliberately-not-cleaned-up interpreter state (interned strings, static type objects, allocator arenas) as leaks, unrelated to whether Cytnx itself leaks.test_mainlinks against ASan natively and needs no shim, and this keeps leak detection on for the one place a genuine Cytnx C++ leak would actually be caught.CLAUDE.md's CI-gates section to note this gate now runs underdebug-openblas-cpuwith ASan, in a separate commit from the workflow change. CheckedGEMINI.mdfor overlapping review guidance — it does not mentionci-cmake_testsor ASan status anywhere, so it was left unchanged.This PR does not fix #1052 itself (that's a separate PR).
ctestis expected to show exactly one known failure,BlockUniTensorTest.CombineBondForceDoesNotRewriteUserHeldBond, until the separate #1052 fix PR merges. Related to #1052 (not closing it).Testing
debug-openblas-cpubuild +ctest(via this repo'sbuild-test-workflowskill): 1784 tests run, 1783 passed, 1 failed — the failure is exactlyBlockUniTensorTest.CombineBondForceDoesNotRewriteUserHeldBond, matching the known pre-existing ASan memcpy-param-overlap in BlockUniTensor::permute_ (via combineBond), CombineBondForceDoesNotRewriteUserHeldBond #1052 bug, and nothing else.LD_PRELOAD/ASAN_OPTIONSshim is still in progress (the Python-bindings build for this preset takes a while on this machine) — it will be confirmed as a follow-up comment/push here.ci-cmake_tests.yml, the same-repo CI run on this PR branch exercises the actual modified workflow (preset switch +LD_PRELOADshim) and is the most direct verification of whether the ASan setup works end-to-end. Watching CI once it starts and will fix anything it catches.