fix(clr) - elegantly return err when kernels are unsupported by agunashe · Pull Request #10150 · ROCm/rocm-systems

agunashe · 2026-08-13T22:15:28Z

Motivation

kernels with Kpack and hip runtime built with ROCM_KPACK_ENABLED=OFF. ideally hip runtime should be built with ROCM_KPACK_ENABLED=ON as per Rock, but we should not crash the app if its not built with it

Technical Details

kernels/apps built with ROCM_KPACK_ENABLED=ON and when executed on hip runtime with ROCM_KPACK_ENABLED=OFF crashes. this PR gracefully handles the incompatible kernels and returns an error

Issue Tracking

JIRA ID: ROCM-29380

Test Plan

hip-tests should continue to build/test fine.

Test Result

hip-tests should continue to build/test fine.

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

therock-pr-bot · 2026-08-13T22:16:16Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	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/clr/hipamd/src/hip_code_object.cpp`, `projects/clr/hipamd/src/hip_platform.cpp`; no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

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

🙋 Wish to Override Policy?

Copilot

Pull request overview

This PR aims to prevent crashes when running kernels built with ROCM_KPACK_ENABLED=ON on a HIP runtime built with ROCM_KPACK_ENABLED=OFF, by propagating “unsupported/invalid code object” errors back to the caller instead of proceeding into execution paths that assume compatibility.

Changes:

Update ihipLaunchKernel to return lookup errors from StatCO::GetFunc rather than always falling back to treating the input as a hipFunction_t.
Make several lazy-load call sites propagate DigestFatBinary(...) failures (instead of ignoring them) so unsupported binaries fail fast with an error.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
projects/clr/hipamd/src/hip_platform.cpp	Adjusts kernel launch function-resolution behavior and error propagation.
projects/clr/hipamd/src/hip_code_object.cpp	Propagates fatbinary digest failures during lazy-load for attributes/globals/managed vars.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

kernels with Kpack and hip runtime built with ROCM_KPACK_ENABLED=OFF. ideally hip runtime should be built with ROCM_KPACK_ENABLED=ON as per Rock, but we should not crash the app if its not built with it

(#10150) ## Motivation kernels/apps with Kpack and hip runtime built with ROCM_KPACK_ENABLED=OFF crashes. Ideally hip runtime should be built with ROCM_KPACK_ENABLED=ON as per Rock, but we should not crash the app if its not built with it ## Technical Details kernels/apps built with ROCM_KPACK_ENABLED=ON and when executed on hip runtime with ROCM_KPACK_ENABLED=OFF crashes. this PR gracefully handles the incompatible kernels and returns an error ## Issue Tracking JIRA ID: ROCM-29380 ## Test Plan hip-tests should continue to build/test fine. ## Test Result hip-tests should continue to build/test fine. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. [rocm-systems] ROCm/rocm-systems#10150 (commit e14bd06)

…11580) ## Motivation `ihipLaunchKernel` could be made to dereference an arbitrary host pointer as an `amd::Kernel*`, killing the process with `SIGSEGV` inside `hipLaunchKernel` instead of returning an error. `StatCO::GetFunc()` returns `hipErrorInvalidSymbol` for any pointer that is not in the registered function table. `ihipLaunchKernel` took that as a cue to `reinterpret_cast` the pointer to `hipFunction_t`, and `hip::asKernel()` is a raw `reinterpret_cast<amd::Kernel*>`, so `ihipLaunchKernel_validate()` goes on to read the caller's pointer as a C++ object. The `if (!kernel)` check there does not help, since non-null garbage passes it. The same cast has crashed before through a different error code: kpack fat binaries with `ROCM_KPACK_ENABLED=OFF` made `GetFunc()` fail with `hipErrorNotSupported`, which was fixed by propagating that error in e14bd06 (#10150). `hipErrorInvalidSymbol` was the last code still reaching the cast. ## Technical Details The cast cannot simply be dropped. A caller may legitimately pass a handle from `hipModuleGetFunction` or `hipGetFuncBySymbol` straight into `hipLaunchKernel`, and that only works because of it — both confirmed against an unpatched build. So the cast stays, but is taken only for a pointer the runtime actually produced; anything else returns `hipErrorInvalidDeviceFunction`. Recognising a kernel handle needed a registry. `PlatformState` now keeps one in a set, populated in `Function::BuildKernel()` and cleared in `~Function()`. `BuildKernel()` is the only place an `amd::Kernel` is constructed, so the set covers every handle the runtime hands out, from statically registered fat binaries and dynamically loaded modules alike. This replaces `IsValidDynFunc()`, which scanned every function of every loaded module on each call and only ever matched dynamic modules. The new lookup is O(1), and is renamed `IsValidFuncHandle()` because it now answers for static handles too. `DynCO::isValidDynFunc()` and `Function::IsValidDynFunc()` become unused and are removed. The set takes its own leaf mutex rather than `PlatformState::lock_`. Kernels are built with `sclock_`, `dclock_` or `FatBinaryLock` already held, and other paths run `lock_ -> sclock_`, so reusing `lock_` here would invert that order. Two incidental improvements fall out: - `Function::IsValidDynFunc()` compared only against `dFunc_[ihipGetDevice()]`, while `ihipLaunchKernel` resolves against the stream's device, so a valid handle could be rejected when the two differed. A flat set has no such mismatch. - A handle used after `hipModuleUnload` is now rejected instead of dereferencing the released kernel. ## Issue Tracking GitHub issue: #11563 This covers that issue's request that "HIP validates this condition and returns an error rather than dereferencing invalid state in `hipLaunchKernel`". It does **not** explain the reporter's gfx950 VF `hipErrorInvalidValue` / `hipErrorOutOfMemory`: their reproducer passes on gfx942 with ROCm 10.1, including with `TMPDIR` on a full `/dev/shm`, and neither HIP nor ROCr reads `TMPDIR`. Those errors look like device state specific to that VF and want separate investigation. ## Test Plan gfx942 (MI300X). Every case was run against a baseline library built from the same tree, so this change is the only difference: 1. `hipLaunchKernel` with an unregistered host function address, with a zeroed heap buffer, and with an unmapped address. 2. A `hipModuleGetFunction` handle passed into `hipLaunchKernel`. 3. A `hipGetFuncBySymbol` handle passed into `hipLaunchKernel`. 4. `hipLaunchKernel` with a module's handle after `hipModuleUnload`. ## Test Result | case | before | after | | --- | --- | --- | | unregistered host function address | SIGSEGV | `hipErrorInvalidDeviceFunction` | | zeroed heap buffer | SIGSEGV | `hipErrorInvalidDeviceFunction` | | unmapped address | SIGSEGV | `hipErrorInvalidDeviceFunction` | | `hipModuleGetFunction` handle | launches, correct result | unchanged | | `hipGetFuncBySymbol` handle | launches, correct result | unchanged | | after `hipModuleUnload` | `hipErrorInvalidDeviceOrdinal`, released kernel dereferenced | `hipErrorInvalidDeviceFunction` | For reference, the crash signature this removes, with the host stub address reaching `getDeviceKernel` as `this`: ``` #0 amd::Symbol::getDeviceKernel hashtable.h:1954 <-- deviceKernels_.find() faults #1 amd::Kernel::getDeviceKernel rocclr/platform/kernel.cpp:35 #2 hip::ihipLaunchKernel_validate hipamd/src/hip_module.cpp:356 #3 hip::ihipModuleLaunchKernel hipamd/src/hip_module.cpp:511 #4 hip::ihipLaunchKernel hipamd/src/hip_platform.cpp #5 hip::hipLaunchKernel_common hipamd/src/hip_module.cpp:903 #6 hipLaunchKernel ``` ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

…OCm#11580) ## Motivation `ihipLaunchKernel` could be made to dereference an arbitrary host pointer as an `amd::Kernel*`, killing the process with `SIGSEGV` inside `hipLaunchKernel` instead of returning an error. `StatCO::GetFunc()` returns `hipErrorInvalidSymbol` for any pointer that is not in the registered function table. `ihipLaunchKernel` took that as a cue to `reinterpret_cast` the pointer to `hipFunction_t`, and `hip::asKernel()` is a raw `reinterpret_cast<amd::Kernel*>`, so `ihipLaunchKernel_validate()` goes on to read the caller's pointer as a C++ object. The `if (!kernel)` check there does not help, since non-null garbage passes it. The same cast has crashed before through a different error code: kpack fat binaries with `ROCM_KPACK_ENABLED=OFF` made `GetFunc()` fail with `hipErrorNotSupported`, which was fixed by propagating that error in e14bd06 (ROCm#10150). `hipErrorInvalidSymbol` was the last code still reaching the cast. ## Technical Details The cast cannot simply be dropped. A caller may legitimately pass a handle from `hipModuleGetFunction` or `hipGetFuncBySymbol` straight into `hipLaunchKernel`, and that only works because of it — both confirmed against an unpatched build. So the cast stays, but is taken only for a pointer the runtime actually produced; anything else returns `hipErrorInvalidDeviceFunction`. Recognising a kernel handle needed a registry. `PlatformState` now keeps one in a set, populated in `Function::BuildKernel()` and cleared in `~Function()`. `BuildKernel()` is the only place an `amd::Kernel` is constructed, so the set covers every handle the runtime hands out, from statically registered fat binaries and dynamically loaded modules alike. This replaces `IsValidDynFunc()`, which scanned every function of every loaded module on each call and only ever matched dynamic modules. The new lookup is O(1), and is renamed `IsValidFuncHandle()` because it now answers for static handles too. `DynCO::isValidDynFunc()` and `Function::IsValidDynFunc()` become unused and are removed. The set takes its own leaf mutex rather than `PlatformState::lock_`. Kernels are built with `sclock_`, `dclock_` or `FatBinaryLock` already held, and other paths run `lock_ -> sclock_`, so reusing `lock_` here would invert that order. Two incidental improvements fall out: - `Function::IsValidDynFunc()` compared only against `dFunc_[ihipGetDevice()]`, while `ihipLaunchKernel` resolves against the stream's device, so a valid handle could be rejected when the two differed. A flat set has no such mismatch. - A handle used after `hipModuleUnload` is now rejected instead of dereferencing the released kernel. ## Issue Tracking GitHub issue: ROCm#11563 This covers that issue's request that "HIP validates this condition and returns an error rather than dereferencing invalid state in `hipLaunchKernel`". It does **not** explain the reporter's gfx950 VF `hipErrorInvalidValue` / `hipErrorOutOfMemory`: their reproducer passes on gfx942 with ROCm 10.1, including with `TMPDIR` on a full `/dev/shm`, and neither HIP nor ROCr reads `TMPDIR`. Those errors look like device state specific to that VF and want separate investigation. ## Test Plan gfx942 (MI300X). Every case was run against a baseline library built from the same tree, so this change is the only difference: 1. `hipLaunchKernel` with an unregistered host function address, with a zeroed heap buffer, and with an unmapped address. 2. A `hipModuleGetFunction` handle passed into `hipLaunchKernel`. 3. A `hipGetFuncBySymbol` handle passed into `hipLaunchKernel`. 4. `hipLaunchKernel` with a module's handle after `hipModuleUnload`. ## Test Result | case | before | after | | --- | --- | --- | | unregistered host function address | SIGSEGV | `hipErrorInvalidDeviceFunction` | | zeroed heap buffer | SIGSEGV | `hipErrorInvalidDeviceFunction` | | unmapped address | SIGSEGV | `hipErrorInvalidDeviceFunction` | | `hipModuleGetFunction` handle | launches, correct result | unchanged | | `hipGetFuncBySymbol` handle | launches, correct result | unchanged | | after `hipModuleUnload` | `hipErrorInvalidDeviceOrdinal`, released kernel dereferenced | `hipErrorInvalidDeviceFunction` | For reference, the crash signature this removes, with the host stub address reaching `getDeviceKernel` as `this`: ``` #0 amd::Symbol::getDeviceKernel hashtable.h:1954 <-- deviceKernels_.find() faults #1 amd::Kernel::getDeviceKernel rocclr/platform/kernel.cpp:35 #2 hip::ihipLaunchKernel_validate hipamd/src/hip_module.cpp:356 #3 hip::ihipModuleLaunchKernel hipamd/src/hip_module.cpp:511 #4 hip::ihipLaunchKernel hipamd/src/hip_platform.cpp #5 hip::hipLaunchKernel_common hipamd/src/hip_module.cpp:903 #6 hipLaunchKernel ``` ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

agunashe requested a review from a team as a code owner August 13, 2026 22:15

Copilot AI lite review requested due to automatic review settings August 13, 2026 22:15

github-actions Bot added the project: clr label Aug 13, 2026

Copilot started reviewing on behalf of agunashe August 13, 2026 22:16

View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hip_platform.cpp Resolved

systems-assistant Bot added the organization: ROCm label Aug 13, 2026

agunashe changed the title ~~fix(clr) - elegently return err when kernels are unsupported~~ fix(clr) - elegantly return err when kernels are unsupported Aug 14, 2026

chrispaquot approved these changes Aug 14, 2026

View reviewed changes

agunashe and others added 4 commits August 14, 2026 14:46

fix(clr) - elegently return err when kernels are unsupported

4ae913e

kernels with Kpack and hip runtime built with ROCM_KPACK_ENABLED=OFF. ideally hip runtime should be built with ROCM_KPACK_ENABLED=ON as per Rock, but we should not crash the app if its not built with it

check hipErrorNotSupported specifically

58bcfe0

formatting errors

8530cc0

fix missing braces

22ff79c

agunashe force-pushed the users/agunashe/rocm_kpack_off_hip_err branch from 71f90a2 to 22ff79c Compare August 14, 2026 21:46

agunashe merged commit e14bd06 into develop Aug 17, 2026
42 of 44 checks passed

agunashe deleted the users/agunashe/rocm_kpack_off_hip_err branch August 17, 2026 16:33

saleelk mentioned this pull request Sep 14, 2026

fix(clr): validate kernel handles before casting in ihipLaunchKernel #11580

Merged

1 task done

Navigation Menu

Repository navigation

fix(clr) - elegantly return err when kernels are unsupported - #10150

fix(clr) - elegantly return err when kernels are unsupported#10150
agunashe merged 4 commits into
developfrom
users/agunashe/rocm_kpack_off_hip_err

agunashe commented Aug 13, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Aug 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

agunashe commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Aug 13, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

agunashe commented Aug 13, 2026 •

edited

Loading