Skip to content

fix(clr): ihipLaunchKernel catch all errors, except InvalidSymbol - #8501

Merged
litvaOo merged 20 commits into
developfrom
users/jiabaxie/ROCM-27216
Aug 31, 2026
Merged

litvaOo merged 20 commits into
developfrom
users/jiabaxie/ROCM-27216

Conversation

@jiabaxie

@jiabaxie jiabaxie commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

When KFD Doorbell exhausts, we do not return an error and rather assume the symbol pointer to be correct. We should tell the user that the symbol they queried for is invalid because there is a doorbell exhaustion.

Technical Details

ihipLaunchKernel currently catches 3 known errors. If any are these 3, it will propagate the error up and stop. If not, then it assumes the pointer to be valid even if it cannot validate it. This feature I believe is for backwards compatibility. However, it makes more sense to by default catch all errors except hipInvalidSymbol, as other hipErrors are also valid to propagate. hipInvalidSymbol is for when you cannot find the symbol in the dynamic library, which is for the backwards compatibility case. All other errors, including the one described in the ticket hipErrorNoBinaryForGpu for kfd doorbell exhaustion, should indicate real invalid symbols.

JIRA ID

JIRA ID 27216

ROCM-27216

Test Plan

Testing was done on gfx942 machine by attempting to ring doorbell from 300 threads

Test Result

Before patch: segmentation fault on some threads. After: no segmentation fault.

Submission Checklist

@jiabaxie
jiabaxie requested a review from a team as a code owner July 13, 2026 19:35
@therock-pr-bot

therock-pr-bot Bot commented Jul 13, 2026 •

Copy link
Copy Markdown

✅ 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_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?

@therock-pr-bot

therock-pr-bot Bot commented Jul 13, 2026 •

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.

@jiabaxie jiabaxie changed the title ROCM-27216 - ihipLaunchKernel catch all errors, except InvalidSymbol fix(clr): ihipLaunchKernel catch all errors, except InvalidSymbol Jul 13, 2026
@litvaOo

litvaOo commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

The only issue is with the rocgdb-cpu job, overriding

@litvaOo
litvaOo merged commit f82e6dc into develop Aug 31, 2026
153 of 167 checks passed
@litvaOo
litvaOo deleted the users/jiabaxie/ROCM-27216 branch August 31, 2026 15:36
systems-assistant Bot pushed a commit to ROCm/clr that referenced this pull request Aug 31, 2026
 InvalidSymbol (#8501)

## Motivation

<!-- Explain the purpose of this PR and the goals it aims to achieve.
-->
When KFD Doorbell exhausts, we do not return an error and rather assume
the symbol pointer to be correct. We should tell the user that the
symbol they queried for is invalid because there is a doorbell
exhaustion.

## Technical Details

<!-- Explain the changes along with any relevant GitHub links. -->
ihipLaunchKernel currently catches 3 known errors. If any are these 3,
it will propagate the error up and stop. If not, then it assumes the
pointer to be valid even if it cannot validate it. This feature I
believe is for backwards compatibility. However, it makes more sense to
by default catch all errors except hipInvalidSymbol, as other hipErrors
are also valid to propagate. hipInvalidSymbol is for when you cannot
find the symbol in the dynamic library, which is for the backwards
compatibility case. All other errors, including the one described in the
ticket hipErrorNoBinaryForGpu for kfd doorbell exhaustion, should
indicate real invalid symbols.

## JIRA ID
JIRA ID 27216
<!-- If applicable, mention the JIRA ID resolved by this PR (Example:
Resolves SWDEV-12345). -->
<!-- Do not post any JIRA links here. -->
ROCM-27216

## Test Plan

<!-- Explain any relevant testing done to verify this PR. -->
Testing was done on gfx942 machine by attempting to ring doorbell from
300 threads
## Test Result

<!-- Briefly summarize test outcomes. -->
Before patch: segmentation fault on some threads. After: no segmentation
fault.

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
[rocm-systems] ROCm/rocm-systems#8501 (commit f82e6dc)
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

LakshmiKumar23 added a commit that referenced this pull request Sep 30, 2026
…ple relink (#12476)

## Summary

On Windows, the exported `rocdecode` CMake target published absolute
build-tree paths to the VA import libraries (`rocm_sysdeps_va*.lib`) in
its public `INTERFACE_LINK_LIBRARIES`. When a consumer relinks against
the installed target — specifically the ctest `--build-and-test`
samples, which rebuild on a different machine than the one that produced
the artifacts — it inherits those paths, which do not exist there, and
the link fails:

```
ninja: error: '.../rocm_sysdeps/lib/rocm_sysdeps_va.lib', needed by 'videodecoderaw.exe', missing and no known rule to make it
```

`rocdecode` is a SHARED library exposing only a C API, so its link
dependencies (HIP, the VA import libs, `d3d12`, `dxgi`) are
implementation details. This change links them `PRIVATE` on Windows so
they stay out of the exported interface. The consumer's own objects
reference none of these symbols directly (the sample includes only the
public `rocdecode.h` / `rocparser.h` / `roc_bitstream_reader.h` and
`hip_runtime.h`, and `find_package`s HIP itself), so nothing needed came
through transitive propagation.

The change is gated `WIN32`-only; Linux keeps its existing plain
(transitive) linking behavior unchanged.

## Impact

- **Windows:** VA import-lib paths no longer leak into the exported
target; the sample relink on the CI test runner resolves its
dependencies independently.
- **Linux:** no behavior change.

## Issue Tracking

JIRA ID : AICV-285

This is a pre-requisite for **ROCm/TheRock#8501** (Windows rocDecode
enablement + CI tests) to pass. TheRock's rocm-systems submodule pointer
must be bumped to include this commit before #8501's Windows `Test
rocdecode` job can succeed.
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