Repository navigation
fix(ci): preload ASAN runtime for GPU sanity check in host-asan builds - #8123
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. |
Move the sanitizer-specific test flags setup before the GPU sanity check and resolve the ASAN runtime library path. Set LD_PRELOAD only for the sanity check step so that non-instrumented Python can load ASAN-instrumented libraries (e.g., libamdhip64). LD_PRELOAD is not set globally to avoid false leak reports from third-party libraries during tests.
12f44c4 to
080f831
Compare
| LD_PRELOAD: ${{ env.ASAN_RUNTIME_PATH }} | ||
| # Disable leak detection for sanity check only - Python itself is not | ||
| # ASAN-instrumented and reports false positive leaks | ||
| ASAN_OPTIONS: detect_leaks=0 |
There was a problem hiding this comment.
As written, the step-level ASAN_OPTIONS: detect_leaks=0 replaces rather than extends, so the sanity check loses the detect_odr_violation=0 and quarantine_size_mb=600 that the preceding step just exported — and amd-smi loads libamdhip64, which is why detect_odr_violation=0 is there.
Change it to ASAN_OPTIONS: ${{ env.ASAN_OPTIONS }}:detect_leaks=0, which works because that step already reads ${{ env.ASAN_RUNTIME_PATH }} set the same way.
| - name: Enable sanitizer-specific test flags | ||
| if: ${{ contains(inputs.artifact_group, 'asan') }} | ||
| run: | | ||
| echo "ASAN_OPTIONS=detect_odr_violation=0:quarantine_size_mb=600" >> $GITHUB_ENV |
There was a problem hiding this comment.
Is it better to have a small python script for setting up ASAN runtime env? Our coding guidelines mentions not to have logics in yml files
The inline shell that resolved the ASAN runtime and symbolizer paths used nested conditionals, command substitution and path manipulation, which the GitHub Actions style guide calls out as signs that logic belongs in Python rather than a workflow file. configure_asan_env.py resolves both paths from the artifact tree and exports them, along with the static ASAN_OPTIONS and HSA_XNACK values, to GITHUB_ENV. Missing binaries warn as before, now as ::warning:: annotations so they are visible outside the raw log. The step drops from ~20 lines of bash to one invocation, and the resolution is unit tested. Two fixes come with the move: ASAN_SYMBOLIZER_PATH is now absolute. OUTPUT_ARTIFACTS_DIR is relative, so the exported path only resolved for processes running from the workspace root; hip_check runs from build/bin, where ASAN reported "invalid path to external symbolizer" and fell back to raw hex offsets. The sanity step's ASAN_OPTIONS now extends the exported value instead of replacing it. As written the step set ASAN_OPTIONS: detect_leaks=0 outright, which dropped detect_odr_violation=0 and quarantine_size_mb=600 for probes that load instrumented ROCm libraries. Leak suppression stays scoped to that step, so component tests keep leak detection enabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Testing branch only. Matches PR #8123's approach - TheRock's LLVM build apparently names the runtime without an arch suffix, not libclang_rt.asan-<arch>.so like test_hiptests.py's get_asan_lib_path() assumes. Also guards on the resolved path actually existing before setting LD_PRELOAD, since clang --print-file-name silently echoes back the bare filename when it can't locate the file.
Testing branch only. amd-smi now runs and reports full, correct GPU data (ASan preload + LD_LIBRARY_PATH fixes both confirmed working) but still exits non-zero because LeakSanitizer flags false-positive leaks - Python itself isn't ASAN-instrumented, same issue PR #8123 already identified and worked around. Set detect_leaks=0 for this step only.
| # are not actionable from a test job. | ||
| # quarantine_size_mb=600 : bounds ASAN's quarantine for freed memory. | ||
| STATIC_ASAN_ENV = { | ||
| "ASAN_OPTIONS": "detect_odr_violation=0:quarantine_size_mb=600", |
There was a problem hiding this comment.
Add this option as well. ASAN_OPTIONS=verify_asan_link_order=0
it might help in some of the test failures like amdclang++ execution
There was a problem hiding this comment.
Added in 48f3746.
ASAN_OPTIONS for every ASAN test job is now:
detect_odr_violation=0:quarantine_size_mb=600:verify_asan_link_order=0
Confirmed against the runtime that this is a real flag rather than a silently-ignored one — ASAN_OPTIONS=help=1 lists it as "Check position of ASan runtime in library list (needs to be disabled when other library has to be preloaded system-wide)", default true.
Worth recording why both this and ASAN_RUNTIME_PATH are here, since they address the same abort from opposite directions. Preloading the runtime makes the ordering genuinely correct, so interception is complete; this flag only stops the runtime from checking. Where we launch the process ourselves (the sanity step, hip_check) preload stays the fix. For tools a test invokes indirectly — amdclang++ being the case you hit — there is nowhere to set LD_PRELOAD, and this is what keeps them running.
nunnikri
left a comment
There was a problem hiding this comment.
Overall looks good to me
Uninstrumented binaries abort with "ASan runtime does not come first in initial library list" when they load instrumented ROCm libraries. Preloading the runtime fixes that where we control the process, which is why ASAN_RUNTIME_PATH is exported, but tests also invoke tools they do not spawn directly -- amdclang++, for example -- and there is no place to set LD_PRELOAD for those. verify_asan_link_order=0 keeps them running. It suppresses the guard rather than correcting the ordering, so preloading remains preferred wherever the process is ours to launch. Co-authored-by: Cursor <cursoragent@cursor.com>
dileepr1
left a comment
There was a problem hiding this comment.
Approving since the requested change is added.
Resolves the conflict in test_component.yml. #8123 landed the same move of "Enable sanitizer-specific test flags" above the driver / GPU sanity check and replaced the inline echoes with configure_asan_env.py. Took main's side and kept only the LD_LIBRARY_PATH export, which configure_asan_env.py does not set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
Takes main's versions of the ASAN env helpers, which #8721 refactored (get_asan_runtime_path, and _resolve_library_path now appends the clang resource directory), and restores main's test_components gate: #8123 fixed the sanity LeakSanitizer failure, so the TEMPORARY !failure() removal is no longer needed and the run reproduces production behaviour.
## Summary Fixes #6331. `rocm-sdk test` aborts on every Linux ASAN run with `ASan runtime does not come first in initial library list`, so the wheel test jobs fail without executing a single test. A process's library load order is fixed at `exec`, so nothing inside the test can insert the ASAN runtime ahead of the interpreter; the only lever is the environment before Python starts. [#8123](#8123) added that preloading but wired it into `test_component.yml` only, and `test_rocm_wheels.yml` had no ASAN handling at all. ## What changed - **`test_rocm_wheels.yml`** — new optional `artifact_group` input, gated on `contains(inputs.artifact_group, 'asan')` the same way `test_component.yml` is. Resolves the ASAN environment after each install, because the runtime ships inside the wheels and `rocm[profiler]` installs into its own venv, then sets `LD_PRELOAD` on the `rocm-sdk test` steps. - **`multi_arch_ci_linux.yml`** — passes `artifact_group` through. - **`configure_asan_env.py`** — locates the compiler on `PATH` as well as in an artifact tree, since these jobs `pip install` ROCm and have no artifact tree. Both runtime spellings are handled (#8077). Exports `LSAN_OPTIONS` pointing at the new suppressions file. `get_asan_runtime_path()` from #8721 keeps working for standalone launchers, now with unit tests. - **`test_tools/lsan_suppressions.txt`** — new. Preloading also puts the uninstrumented CPython and `/usr/bin/perl` under LeakSanitizer, which reports their interpreter internals as leaks. These are scoped suppressions rather than `detect_leaks=0`, so leaks in the ROCm libraries under test still fail the job. - **`test_component.yml`** — puts the extracted `llvm/bin` on `PATH` for the above. ## Validation **CI:** [run 40887799669](https://github.com/ROCm/TheRock/actions/runs/40887799669) — `test_rocm_wheels.yml` on `gfx94X-dcgpu` against ASAN wheels, green. All three `rocm-sdk test` steps execute and pass: | Step | Result | | --- | --- | | `Test rocm[libraries]` | `Ran 19 tests` — OK | | `Test rocm[libraries,devel]` | `Ran 27 tests` — OK | | `Test rocm[profiler]` | `Ran 21 tests` — OK | (Validated with workflow run since [#8978](#8978) removes the ASAN GPU runner fields unless the event is a pull_request in ROCm/rocm-systems. An empty runner list skips Test Python rather than failing.) Leak detection stays on rather than being switched off: LeakSanitizer prints its suppression accounting at each exit and finds nothing outside the suppressed templates. ``` Suppressions used: count bytes template 74 83083 libpython ``` **Counterfactual:** [run 40888153220](https://github.com/ROCm/TheRock/actions/runs/40888153220) is the same wheels with `artifact_group` left empty, so the preload is off. All three steps abort with `ASan runtime does not come first in initial library list` and execute zero tests. With the input empty every gate in this PR is off, which is `main`'s behaviour today — so that run is #6331 reproduced against current wheels. **Unit:** `build_tools/github_actions/tests/` — 17 new tests, all passing, and no pre-existing test changed state. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Move the sanitizer-specific test flags setup before the GPU sanity check
and resolve the ASAN runtime library path. Set LD_PRELOAD only for the
sanity check step so that non-instrumented Python can load ASAN-instrumented
libraries (e.g., libamdhip64).
LD_PRELOAD is not set globally to avoid false leak reports from third-party
libraries during tests.
Leak detection is also disabled for the sanity check
since Python itself is not ASAN-instrumented and triggers false positives.
ISSUE ID: #7202