Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
dileepr1
added a commit
to ROCm/rocm-systems
that referenced
this pull request
Sep 9, 2026
Validate ROCm/TheRock#7780 on top of current main (which already carries ROCm/TheRock#7602 and ROCm/TheRock#7600) without the sanity-check fixes still in review as ROCm/TheRock#8100. Co-authored-by: Cursor <cursoragent@cursor.com>
dileepr1
force-pushed
the
users/dileepr1/asan-sanity-fixes
branch
from
September 10, 2026 20:01
ab67b82 to
170ade3
Compare
run_command pipes stdout and stderr but only logs them on success, so when a probe fails the command's own diagnostics are discarded and the job shows a bare CalledProcessError traceback. Nightly ASAN runs failed this way for days with no indication of why amd-smi was returning non-zero. Print the captured output before propagating. This has no CI or sanitizer coupling; it applies to any failing probe on any system. Co-authored-by: Cursor <cursoragent@cursor.com>
hip_check is compiled without -fsanitize=address on purpose, so running it against instrumented ROCm libraries aborts with "ASan runtime does not come first in initial library list". Instrumenting it instead would pull in the device-side instrumentation that the host-asan variant exists to avoid. Preload the runtime for that one subprocess, reusing ASAN_RUNTIME_PATH which CI already resolves. LD_PRELOAD is deliberately not exported globally, to keep third-party libraries from producing spurious leak reports. Ported from #7311. Co-authored-by: Cursor <cursoragent@cursor.com>
dileepr1
force-pushed
the
users/dileepr1/asan-sanity-fixes
branch
from
September 10, 2026 21:06
170ade3 to
d719e3c
Compare
dileepr1
added a commit
that referenced
this pull request
Sep 11, 2026
Rebuilds the validation branch on top of current main, which already carries #8123's ASAN environment setup, so only #7780 remains on top. #8100 is left out while it is parked. The host-asan gate moved and was widened to startswith() on main since #7780 branched, so the presubmit escape is applied to that structure rather than the original elif. Content matches #7780. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Sep 16, 2026
dileepr1
added a commit
that referenced
this pull request
Sep 22, 2026
## Motivation ### In Short: #8268 fixes two independent bugs that together made every host-ASan hip-tests shard fail without running a single test. First, is_asan() matched BUILD_VARIANT exactly against "asan", so it was false for host-asan and everything gated on it silently did nothing Second, configure_asan_env.py pinned the runtime filename to libclang_rt.asan.so, which #8077 renamed to libclang_rt.asan-<arch>.so, so the ASan runtime hadn't resolved at all since that landed. ## Details GitHub issue: #7202 Found while looking at the first host-asan presubmit run that got as far as the component test matrix (ROCm/rocm-systems#10088, [run 35004915528](https://github.com/ROCm/rocm-systems/actions/runs/35004915528)). All four `hip-tests` shards failed **without running a single test**, and with zero AddressSanitizer findings: ``` catch_tests/RTC: error while loading shared libraries: libclang_rt.asan-x86_64.so: cannot open shared object file CMake Error at CatchAddTests.cmake:82: Error listing tests from executable '.../RTC': Result: 127 ctest ... returned non-zero exit status 8 ``` ctest runs `RTC --list-tests` to enumerate Catch2 tests; it cannot load the instrumented binary, CMake aborts the listing, and the shard reports failure. Two separate defects combine to produce this. ### 1. `is_asan()` excluded host-asan ```python return BUILD_VARIANT == "asan" ``` An exact match, so `host-asan` (and `asan-debug`, `host-asan-debug`) evaluated false and everything gated on it silently did nothing: | Caller | What was skipped | | --- | --- | | `test_hiptests.py` | `LD_PRELOAD` of the runtime, `HSA_XNACK`, extended timeout | | `test_rocprofiler_sdk.py` | clang resource dir on the loader path, preload, ASAN test exclusions | | `test_hipfile.py` | preload | | `test_rocblas.py`, `test_hipblas.py`, `test_hipblaslt.py` | `HSA_XNACK` | | `tests/test_rocm_sanity.py` | the #3312 rocminfo skip | The `test_hiptests.py` preload is the one that breaks discovery. Notably the machinery was already correct — `get_asan_lib_path()` there asks clang for `libclang_rt.asan-<arch>.so`, exactly the name `RTC` needs — it just never ran. The rest of the codebase already treats these as a family: `fetch_test_configurations.py` uses `"asan" in build_variant` with a comment listing all four variants, `configure_multi_arch_ci.py` uses `suffix in ("asan", "host-asan")`, and `publish_rocm_to_release_buckets.py` has `_is_asan_variant()`. `is_asan()` was the odd one out. ### 2. `configure_asan_env.py` pinned one runtime filename `ASAN_RUNTIME_LIB = "libclang_rt.asan.so"` is the per-target-runtime-dir name. #8077 (`cd8ed0c3f`) set `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF`, moving the runtime to `lib/linux/libclang_rt.asan-<arch>.so`. Resolution has failed ever since, and every ASAN test job has been logging `##[warning]ASAN runtime not found`. The regression is visible across runs: | Run | TheRock base | Result | | --- | --- | --- | | [34549006044](https://github.com/ROCm/rocm-systems/actions/runs/34549006044) | `b85dc2119` (pre-#8077) | resolved `.../x86_64-unknown-linux-gnu/libclang_rt.asan.so` | | [34613837319](https://github.com/ROCm/rocm-systems/actions/runs/34613837319) | `24cc5c7d6` | not found | | [35004915528](https://github.com/ROCm/rocm-systems/actions/runs/35004915528) | `24cc5c7d6` | not found | ## Technical Details `is_asan()` now matches any ASAN-family variant via `"asan" in BUILD_VARIANT`, mirroring `fetch_test_configurations.py`. `_resolve_asan_runtime()` tries both filenames in turn, per-target first then the legacy arch-suffixed one, relying on the fact that clang echoes the requested name back unchanged when it cannot locate the file. The warning now names both candidates. TheRock has flipped between these layouts, so pinning either one is fragile. ## Test Plan - `python -m pytest build_tools/ tests/ -q --continue-on-collection-errors` (against baseline) - `pre-commit run --files build_tools/github_actions/amdgpu_family_matrix.py build_tools/github_actions/configure_asan_env.py build_tools/github_actions/tests/configure_asan_env_test.py` ## Test Result 2432 passed / 16 failed / 6 collection errors, against a baseline of 2431 passed / 16 failed / 6 collection errors on `origin/main`. The failures and collection errors are identical on both sides and are pre-existing local dependency gaps (`zlib_ng`, `pkginfo`, `_rocm_sdk_core_DEFAULT`); the delta is the one test added here. Targeted pre-commit passed. ## Risk Assessment Risk 3. Widening `is_asan()` activates ASAN handling for host-asan across several test scripts that have never had it, which is the intent but does change their behavior. The specific known side effect is that the TheRock#3312 rocminfo skip now applies to host-asan. That `skipif` is parametrized over three search patterns, so three sanity tests that were passing under host-asan are now skipped, taking the sanity tally from `1 failed, 4 passed, 3 skipped` to `1 failed, 1 passed, 6 skipped`: ``` test_rocm_output[rocminfo - GPU Device Type Search] SKIPPED test_rocm_output[rocminfo - GFX Name Search] SKIPPED test_rocm_output[rocminfo - AMD Vendor Name Search] SKIPPED ``` That coverage loss is accepted here in exchange for the variants behaving consistently. Worth a follow-up: #3312 was filed against full ASAN, and these tests demonstrably pass under host-asan, so narrowing that `skipif` to the full-ASAN variant may be the better long-term answer. The runtime-name change is additive: it only adds a second candidate after the existing one fails. ## Related - Work tracking: #7202 - Caused the rename: #8077 - Depends on nothing; independent of #7780 and #8100 ## Device / Architecture Coverage Validated on gfx942 via the rocm-systems host-asan presubmit. No device code changes. ## Testing Summary Added `test_resolves_the_legacy_arch_suffixed_runtime`, which stubs a clang that resolves only the arch-suffixed name and echoes the request back otherwise, matching real clang behavior. Existing coverage for the per-target name, missing clang, missing runtime, missing symbolizer and the GITHUB_ENV format is unchanged. ## Testing Checklist - [x] Full unit suite vs baseline - Status: Passed, no regressions - [x] Targeted pre-commit - Status: Passed - [x] host-asan presubmit against ROCm/rocm-systems#10088 - [run 35140846158](https://github.com/ROCm/rocm-systems/actions/runs/35140846158) - Status: Passed. hip-tests went from 0 tests discovered to ~4,900 run across 4 shards (~2,050 passing). Every remaining failure is the pre-existing `hsa_amd_memory_pool_free` bad-free. The runtime now resolves to `.../lib/clang/24/lib/linux/libclang_rt.asan-x86_64.so`, confirming the #8077 layout. - [ ] PR CI - Status: Pending ## Flags / Guardrails None. ## Adjacent Tests Considered `rocprofiler_sdk_dashboard_test.py` patches `is_asan` directly rather than driving it through `BUILD_VARIANT`, so it is unaffected by the widening and needed no change. ## Risk Acceptance / Waivers None. ## Technical Changes - `is_asan()` matches any ASAN-family `BUILD_VARIANT` instead of exactly `"asan"`. - `configure_asan_env.py` resolves the ASAN runtime under either the per-target or the legacy arch-suffixed filename. - Add a unit test for the legacy name. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
This branch has not been deployed
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
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.
Depends on #8123, which sets up the ASAN environment for test jobs:
configure_asan_env.pyresolvesASAN_RUNTIME_PATHandASAN_SYMBOLIZER_PATHfrom the artifact tree, and the workflow scopes
LD_PRELOADand the leaksuppression to the sanity step.
This PR is the remainder: the two script-side changes needed to get past the
sanity check and read what ASAN reports once it runs. It touches no workflow
files.
Earlier revisions of this PR also carried the workflow reorder, a
BUILD_VARIANTbranch insideprint_driver_gpu_info.py, and theASAN_SYMBOLIZER_PATHfix. All three now live in #8123 — the first two afterreview feedback on #7311 asking that generic utilities not learn about CI build
variants, the third as part of moving that logic out of inline bash.
Commits
print a failing command's output in the driver sanity script —
run_commandpipes stdout and stderr but only logs them on success, so afailing probe produced a bare
CalledProcessErrortraceback. Nightly ASANruns failed this way for days with no indication of why
amd-smireturnednon-zero. No CI or sanitizer coupling: it applies to any failing probe on
any system.
preload the ASAN runtime for hip_check in the sanity test —
hip_checkis deliberately built without
-fsanitize=address, so it aborts with "ASanruntime does not come first" when it loads instrumented ROCm libraries.
Instrumenting it instead would pull in the device-side instrumentation that
the host-asan variant exists to avoid, so the runtime is preloaded for that
one subprocess, reusing the
ASAN_RUNTIME_PATHthat fix(ci): preload ASAN runtime for GPU sanity check in host-asan builds #8123 exports.Kept in the project test rather than exported job-wide. A job-wide
LD_PRELOADputs the non-instrumented Python driving pytest underLeakSanitizer, which then reports CPython's own end-of-process allocations
and fails test steps: measured at 531244 bytes in 499 allocations, all
_PyMem_RawMalloc/_PyBytes_Resize/rpds, in run34492719442.
Validation
Exercised against ROCm/rocm-systems#10088 on a branch carrying #7780 and #8123
plus these changes, added progressively:
amd-smi staticexit 1 after 0.19s, reason not loggedhip_checkaborts, now visiblebad-freeinlibamdhip64.so.7, unsymbolizedThe sanity check passes and the remaining failure is a genuine defect:
A 4 MiB region allocated through the HSA pool allocator is released through a
path the allocator does not recognise, during static destructor teardown. That
is tracked separately; it is not addressed here.
ISSUE ID: #7202