[hipDNN] Fix runtime backend discovery under ASan - #12009
Draft
BrianHarrisonAMD wants to merge 13 commits into
Draft
[hipDNN] Fix runtime backend discovery under ASan#12009BrianHarrisonAMD wants to merge 13 commits into
BrianHarrisonAMD wants to merge 13 commits into
Conversation
The runtime-load frontend opened the bare soname libhipdnn_backend.so and left the location to the dynamic linker. Under the shared or preloaded ASan runtime the intercepted dlopen is issued from inside libclang_rt.asan.so, so glibc resolves the soname against that object's RUNPATH rather than the caller's, and the backend is never found. The asan-debug artifact tier fails 3 of 4 tests in hipdnn_frontend_dynamic_load_tests for exactly this reason, while the public dynamic-load suite hides the same failure behind a skip. Resolution now computes an absolute path: an explicit override, the calling module's own directory, its sibling lib/ or lib64/, the directory the HIP runtime was loaded from, and only then the bare name for the loader. Self-relative outranks the HIP anchor so a development build cannot pick up a stale installed backend. A candidate that is absent, or present but unloadable, is recorded and skipped rather than denying the backend outright, and one stderr report lists everything tried. The override is HIPDNN_BACKEND_LIBRARY_PATH, a directory joined with the platform's backend filename so the variable can never name an arbitrary object, plus a per-shared-object setBackendLibraryPath(). Both are read through getSecureEnv()/isSecureExecution(), which consults AT_SECURE directly rather than secure_getenv() so the header stays buildable in a consumer's translation unit against glibc, musl or bionic. A secure-execution process keeps only the programmatic override and the loader's own hardened search: a directory this code computes is not subject to the restrictions the loader places on $ORIGIN expansion for such a process. The plugin search-path variables move to the same accessor, being the older and broader instance of the same exposure. The ASan configuration now has a test that fails without the fix: hipdnn_frontend_dynamic_load_tests_asan_preload runs the suite with the ASan runtime preloaded onto an ordinary build, and the suite asserts the resolved path rather than a non-null handle, so a pass through the loader's own search is distinguishable from self-relative resolution. The entry reaches the install tree through the ctest include snippet mechanism, which carries its environment where a plain test property is dropped. The public dynamic-load suite now fails instead of skipping when the build produced a backend it cannot load. Fixes: #11964
✅ 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #12009 +/- ##
===========================================
+ Coverage 70.55% 70.57% +0.02%
===========================================
Files 2810 2812 +2
Lines 462535 463147 +612
Branches 68105 68182 +77
===========================================
+ Hits 326312 326821 +509
- Misses 112680 112757 +77
- Partials 23543 23569 +26
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
BrianHarrisonAMD
changed the title
Remediates eleven deduplicated review findings against the runtime-load backend resolution change. Production fixes: - Add utilities::detail::pathForDiagnostic() and route the three resolver diagnostic sites and the Windows openLibrary error through it. A C++20 consumer previously failed to compile, because path::u8string() returns std::u8string there and the shipped headers concatenated it with narrow literals. The helper keeps C++17's movable string and bridges char8_t bytes explicitly rather than converting through the active code page. - Absorb path-encoding failures in that helper and return a fixed ASCII marker. An unpaired UTF-16 surrogate in a candidate path previously threw out of the diagnostic, past the per-candidate try, and cached a null handle -- abandoning a self-relative or loader-search backend that was still usable. Allocation failure still propagates; only conversion errors are absorbed. The LoadLibraryW error code is still captured before any formatting can fail. - Reject relative loader names in getLoadedLibraryDirectoryForAddress() before canonicalizing, matching getLoadedLibraryOrigin(). A consumer DSO loaded as plugins/frontend.so, in a process that later changed directory, otherwise resolved 'plugins' beneath the new working directory and could load an unrelated backend ahead of the real module's loader search. The normal-main empty-l_name path is unchanged; an unknown origin now skips that tier rather than redirecting it. - Grow the buffer in getLoadedLibraryOrigin() when GetModuleFileNameW reports truncation. A HIP runtime at an extended-length path previously looked like an unavailable origin, removing the HIP-directory tier. Actual API failures still fail, and the three unrelated MAX_PATH helpers are left alone. Test and CI fixes: - Exclude BUILD_THREAD_SANITIZER and THEROCK_SANITIZER=TSAN from the Linux ASan-preload registration, which would otherwise inject the ASan runtime into a TSan-instrumented binary. A missing runtime remains a run-time failure rather than a configure-time omission. - Emit bare cmake in installed CTest snippets, matching plugin_sdk, so the installed tests do not depend on the configure host's absolute path. - Apply category labels once per test name. The two-name call did not survive the shared helper's argument handling, so neither the native override nor its cleanup was labelled and both vanished from tier-filtered runs. Adds shared/ctest to the Windows sparse checkout and PyYAML to the CI requirements, without which the parser silently labels nothing. - Allocate the resolver fixture root through ScopedDirectory with exclusive creation, so concurrent runs sharing a temporary directory cannot delete each other's libraries mid-test. Nothing is removed before it is acquired. - Stage an isolated fixture for the self-relative regression instead of imposing a sibling lib/lib64 layout on every install. The previous assertion failed a correctly loading CMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu build; ordinary cases now assert real backend use without constraining layout, and the exact expectation moved to a staged fresh-process run with real copied artifacts. - Run the one-shot setter lifecycle in a fresh process per iteration. The test necessarily failed under --gtest_repeat, since resolution is a per-process one-shot that survives fixture teardown. - Always exercise the Windows native-path case, and gate the ANSI non-representability requirement behind a test-only flag set on the CI validation step. The fatal ANSI assertion previously failed ordinary ctest on a UTF-8 active code page, which Windows supports. Two build fixes fall out of the above. EXPECT_EXIT expands to a switch with no default label, attributed to the expansion site rather than the GoogleTest header, so -Wswitch-default with -Werror rejects it; the suppression is one statement wide. Adding utilities::detail also made unqualified detail:: in TestLoggingUtils.cpp ambiguous against logging::detail, so those uses are now qualified.
The new compile-only C++20 target exists to prove the shipped runtime-load headers still compile when a consumer selects C++20. A superbuild sets CMAKE_CXX_CLANG_TIDY globally, which also pointed clang-tidy at that translation unit, and several modernize checks are gated on the language standard: modernize-use-ranges and modernize-use-constraints stay silent everywhere else and fire only here, demanding std::ranges and requires in a header that must keep compiling as C++17. Satisfying them would raise the product's minimum standard, which is the opposite of what this target defends. Clear CXX_CLANG_TIDY on that target alone. Every other target continues to lint the same header at the standard it actually ships as, and the target keeps the full warning set, so the C++20 compile gate itself is unchanged. Also return a braced initializer list from pathForDiagnostic's C++20 branch. That finding was genuine but only reachable here, since the branch does not exist under C++17.
Name the death-test suite TestBackendLibraryResolutionDeath. The project's test-name validator reserves "Test" for the leading keyword and rejects it inside a feature name, so the GoogleTest-conventional *DeathTest spelling failed validation. GoogleTest matches that suffix to order death suites first, which this gives up; the protection that matters here is the threadsafe death-test style the test already sets, which re-execs instead of forking a threaded parent. Forward Python3_EXECUTABLE into the nested configure that shared_ctest_optional_args_scope launches. It already receives ROCM_LIBRARIES_ROOT, but not the interpreter, so it ran its own find_package(Python3) and could select a different one than the build was given. Where that interpreter cannot import yaml, the category parser fails and the test reports missing labels rather than the missing dependency. Checking out shared/ctest so hipDNN's labels apply on Windows is what first exposed this. A yaml-less interpreter still fails the test, as it should -- this corrects which interpreter is used, not whether the dependency is detected.
The validation added around these fixes outgrew the fixes. Three bespoke Windows workflow steps, a sparse-checkout entry, a CI dependency and a staged-fixture runner are all reverted, leaving the code changes and tests that cover them in the existing test structure. The workflow steps were mostly redundant: the native-override test is registered unconditionally, so the existing "Run tests" step already runs it, and only the strict ANSI mode needed a step of its own. Checking out shared/ctest to prove the category-label fix also pulled that component's own self-tests into this lane, which is a surface hipDNN's superbuild CI should not own. Neither lane checked out shared/ctest before, so labels were never applied there and the per-name fix stays correct but remains unobservable in CI. The staged fixture proved the self-relative tier by copying real artifacts into a generated lib/lib64 tree under a process lock. It required three CTest entries, a test-only path variable and a disabled child case. What it defended is already addressed: the assertion that failed a valid CMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu build is gone, replaced by one that holds in any layout -- the resolved path names the file the loader actually mapped, and that backend answers real calls. Likewise the strict ANSI mode and its variable are removed; the Windows test now exercises the wide path on every code page and records which one ran. Retained: the diagnostic formatter and its C++20 compile gate, the origin rejection, the Windows buffer growth, and three one-line registration fixes for the sanitizer pairing, the installed CMake command and the label calls.
The guard reads as a defensive habit without it, and deleting it would reintroduce the defect this test was changed to fix: an unconditional comparison rejects a correctly loading build whose backend came from the loader search, where the resolver has no directory of its own to report.
The fixture root is now owned by ScopedDirectory, whose destructor removes the tree with the throwing overload. A throwing destructor terminates, so a test that failed before releasing its backend would take the rest of the binary with it -- on Windows a mapped library cannot be unlinked, which is exactly the state a failed resolver test leaves behind. The previous error-swallowing teardown could not do this. Report the failure and keep the remaining results.
The ASan-preload CTest entry simulated an intercepted dlopen in builds that cannot exhibit the failure. A real sanitizer build needs no simulation: the test binary links the runtime, so ASan owns dlopen and the ordinary dynamic-load entry covers it. Confirmed by building the pre-fix content in TheRock asan-debug, where that ordinary entry fails 3 of 4 with the original report signature. Both entries also wrote a .cmake fragment next to the installed tests and pulled it in with include(). The artifact descriptor packages only CTestTestfile.cmake, so the include dangled and ctest -N failed to enumerate any test in the component -- discovery for everything, lost to an auxiliary file that never shipped. Removing the fragments removes that whole class of failure rather than teaching the packaging about one more path. The Windows entry needed a fresh process only because it drove the one-shot global. Reading the environment is not one-shot, so its unique property -- that an override arrives as UTF-16 and is not narrowed through the active code page -- moves to ordinary unit tests over the production input path, comparing native() so a narrowed spelling cannot pass. An absent variable is covered too: it must not become an engaged empty override. Not carried over: loading a real backend from a Unicode directory end to end, which the fresh process existed to allow. No DISABLED_ test or --gtest_also_run_disabled_tests remains in these suites.
GetEnvironmentVariable is sized by one call and read by a second. Anything in the process can replace the value in between -- the cuDNN logging bridge does exactly that at runtime -- and when the new value no longer fits, the read writes nothing and returns the size it now needs. The buffer was already full of NULs, so the caller received a string of them, which became a filesystem path and then the highest-priority backend candidate. Retry with the size the read asks for. An unset variable still returns the default. The backend override is read through a new getSecureEnvW rather than getEnvW. The two are the same function on Windows today, because secure execution is always false there, but the narrow path already goes through the secure accessor and an asymmetry that only matters once someone implements Windows detection is one nobody will notice then. Name the wide variable next to the narrow one. Renaming either alone compiles, passes every Linux test, and stops the override working on Windows. The docs claimed the secure-execution guarantee without saying it is Linux only. Windows has no equivalent mode, so those variables are always honored there, and a Windows reader was entitled to conclude otherwise. expandUser reads HOME without the secure accessor; it feeds a cache directory today and that is fine, but the contract now says so rather than leaving the next caller to find out. Also record why the runtime-load tests are a separate executable, and add that executable to the coverage objects: it carries 17 covered lines of resolution code that nothing was measuring.
Every test of HIPDNN_BACKEND_LIBRARY_PATH sat inside an ifdef for Windows, and every other resolver test injects BackendResolutionInputs directly, so the read itself measured zero on Linux -- a documented knob, with docs changed in this same branch, that could stop working without a test noticing. Read it on Linux too: once with the variable set, asserting the source is the environment rather than a programmatic override, and once with it unset, asserting no override rather than an engaged empty one. These call backendResolutionInputs(), which latches resolutionStarted. That bool is written in one place and read in one, by setBackendLibraryPath, which this binary calls only inside the death test -- and threadsafe style re-execs, so the child gets a fresh static. No existing test can observe the latch. Also correct the coverage comment added with the runtime-load binary. The extra missed lines are resolveSymbol's null-handle return, not the never-run static-check translation unit: its inline instantiations are folded onto counters the executed tests already increment, and every record it contributes has a nonzero count.
… one The retry loop added for the growth race broke a variable that is set to an empty string. The sizing call counts the terminator and so returns one; the fetch copies nothing and returns zero, which is a successful read of an empty value. Breaking out of the loop on a zero-length fetch turned that into the caller-supplied default, and TestPlatformUtils.GetEnvReturnsEmptyStringValue says so. A fetch that fits always reports less than the buffer it was given, including zero, so that comparison alone decides success. Removing the extra check restores the previous behaviour on every path -- unset yields the default, empty yields empty, a value removed between the two calls yields empty as it always did -- and keeps the growth retry. Only the Windows reader has this shape, so no Linux test covers it.
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.
Summary
Fix runtime backend discovery when a sanitizer runtime intercepts
dlopenand the loader's implicit search no longer finds the backend. The frontend now resolves explicit candidate paths before falling back to the bare soname.Fixes #11964.
Risk Assessment
3/5 — Changes host-side library selection and adds a per-module path override; header-only consumers must rebuild. Candidate order, fallthrough, one-shot caching, handle ownership and secure-execution gates are preserved. No new public API, search tier, build flag or configuration option.
ASIC Coverage
No kernel, device-dispatch or supported-ASIC changes. Resolution is entirely host-side —
dladdr1/dlinfo,std::filesystem, anddlopenon an absolute path — and the failure mode is a property of the sanitizer runtime and the loader, not of the GPU target. Standard Linux/Windows CI is sufficient; no multi-ASIC sweep is warranted. Local verification ran on gfx942.Testing Summary
Before/after on the tier the issue came from. The pre-fix content and this branch were built in the same scoped TheRock
linux-release-asan-debugtree — ASAN-instrumented HIP and HSA runtimes,gfx942:xnack+— and run through TheRock's owntest_hipdnn.py:a3eb50b9ea8)hipdnn_frontend_dynamic_load_testslibhipdnn_backend.so: cannot open shared object filehipdnn_public_frontend_dynamic_load_testsLD_DEBUGfind library=libhipdnn_backend"3 of 4" matches the original report. The 7 skips are notable on their own: the public suite was skipping rather than failing, so a second suite stayed green while hitting the same defect.
unit-check8/8, and the dynamic-loading entries in both the build tree and a relocated install prefix.Testing Checklist
ninjaandninja install— passed.ninja unit-check— 8/8 passed.asan-debugbefore/after — pre-fix content reproduces the original 3/4 failure in the same tree; this branch passes.pre-commit run --files— passed.src/path changed) but adds Windows test code that cannot be compiled outside Windows CI.Install build tools, then a GitHub 504 fetchingnlohmann/jsonduring configure.git diff <base> HEAD -- .githubis empty.asan-debugartifact replay — not run; deliberately substituted. Scheduling runs in that environment is not available, so the gate is unreachable rather than merely unperformed. The substitute is the before/after above: the same tier, a genuinely ASAN-instrumented stack, TheRock's own test path, and — importantly — a harness observed failing on known-bad input rather than one only ever seen to pass.Technical Changes
Resolution. Candidates are tried in order: explicit override, the calling module's directory, sibling
libandlib64, the HIP runtime directory, then the bare soname. Each is attempted independently, so a stale or corrupt library in an early directory cannot deny the backend. Secure-execution processes use the loader search only, and honour the programmatic setter but not environment or computed locations.Diagnostics. Path text for messages goes through one internal helper.
path::u8string()returnsstd::u8stringunder C++20, so the previous concatenations made the shipped headers ill-formed for any C++20 consumer — the helper bridges those bytes explicitly rather than narrowing through the active code page. It also absorbs conversion failures: an unpaired UTF-16 surrogate in a candidate path previously threw out of the diagnostic, past the per-candidate handler, and cached a null handle — abandoning a backend that was still usable.Untrusted origins. A module loaded under a relative name no longer contributes a search directory. Canonicalising it against a working directory the process may since have changed can name an unrelated tree, so that tier is skipped instead.
Windows paths.
GetModuleFileNameWsignals truncation by filling the buffer; the origin lookup now grows and retries, so a runtime at an extended-length path is no longer treated as having no origin.Tests. Resolution is covered by unit tests over synthetic layouts — tier precedence, absent and unloadable candidates, duplicate directories, override rejection, secure-mode short-circuit — plus a compile-only translation unit that includes the shipped headers as C++20, which is the only way to catch a regression that breaks consumers at include time.
Notes
setBackendLibraryPath()is per-shared-object and takes effect only before first resolution. Process-wide injection isHIPDNN_BACKEND_LIBRARY_PATH, which must be a non-empty absolute directory.