Skip to content

fix(rocr): load thunk libraries from the runtime directory#8912

Open
fcui-amd wants to merge 1 commit into
developfrom
users/fcui/dxg_ld_library_path
Open

fix(rocr): load thunk libraries from the runtime directory#8912
fcui-amd wants to merge 1 commit into
developfrom
users/fcui/dxg_ld_library_path

Conversation

@fcui-amd

Copy link
Copy Markdown
Contributor

Motivation

rocminfo

WSL environment detected.
Cannot load librocdxg.so, failed:librocdxg.so: cannot open shared object file: No such file or directory
GetExportAddress failed: /opt/venv/lib/python3.14/site-packages/_rocm_sdk_devel/bin/../lib/libhsa-runtime64.so.1: undefined symbol: hsaKmtOpenKFD
hsa_init Failed, possibly no supported GPU devices

Technical Details

Add a fallback path for loading thunk libraries when the normal platform library search fails.
The fallback locates the directory containing the currently loaded ROCR runtime library and attempts to load the thunk library from the same directory.

Issue Tracking

Test Plan

rocminfo pass without specified LD_LIBRARY_PATH setting

Test Result

rocminfo pass

Submission Checklist

@therock-pr-bot

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
🌿 Branch Name ✅ Pass
📝 PR Title/Description ✅ Pass
Forbidden Files ✅ Pass
🧪 Unit Test ❌ Fail Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocr-runtime/runtime/hsa-runtime/core/runtime/thunk_loader.cpp, projects/rocr-runtime/runtime/hsa-runtime/core/util/lnx/os_linux.cpp, projects/rocr-runtime/runtime/hsa-runtime/core/util/os.h, projects/rocr-runtime/runtime/hsa-runtime/core/util/win/os_win.cpp; no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ Unit Test

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@kentrussell kentrussell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thunk part is fine. Will let David/Chris give the approval for the ROCr side

@cfreeamd cfreeamd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI-assisted pre-review (for @cfreeamd)

The adjacent-path loader fallback in thunk_loader.cpp and the new os::GetAdjacentLibraryPath implementations (Linux dladdr, Windows GetModuleHandleExA/GetModuleFileNameA) look correct — self-locating via the runtime's own address, with sane truncation handling on Windows and a null-guard on DlError(). Two inline notes below (one Question on the bundled .ver exports, one Nit on the success log). Noting the PR is labeled "Not ready to Review," so this is early feedback.

hsaKmtGetRuntimeCapabilities;
hsaKmtDebugTrapIoctl;
hsaKmtGetCoreRuntimeInfo;
hsaKmtGetCoreDeviceInfo;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI-assisted pre-review (for @cfreeamd)

[Question] These three exports (hsaKmtGetKernelQueueId, hsaKmtGetCoreRuntimeInfo, hsaKmtGetCoreDeviceInfo, mirrored in librocdxg.ver) come from the separate "export missing libhsakmt symbols" commit and are unrelated to the adjacent-path loader change. Two things worth confirming: (a) are all three actually defined in libhsakmt? A .ver entry for an undefined symbol is a silent no-op. (b) Is widening the public export surface intended as part of this PR, or would it be cleaner to split it out? The ABI change and the loader change have independent risk/review profiles.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this commit belongs to another PR and was included here by mistake.

}
}
if (thunk_handle != nullptr) {
debug_print("Load %s successully!\n", library_name.c_str());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI-assisted pre-review (for @cfreeamd)

[Nit] When the library is loaded via the adjacent-path fallback, this success message still prints library_name (the original name), so the log won't reveal that the fallback path was actually used. Consider logging the path that succeeded to aid diagnosis. (Also a pre-existing typo here while the line is nearby: "successully" → "successfully".)

Signed-off-by: Flora Cui <flora.cui@amd.com>
@fcui-amd
fcui-amd force-pushed the users/fcui/dxg_ld_library_path branch from 5495cc3 to ed7768a Compare July 22, 2026 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants