Skip to content

[rocjitsu] Avoid race reports from debugger register reads - #14313

Merged
newling merged 7 commits into
ROCm:developfrom
newling:users/newling/debugger-register-read-observers
Oct 28, 2026
Merged

newling merged 7 commits into
ROCm:developfrom
newling:users/newling/debugger-register-read-observers

Conversation

@newling

@newling newling commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

When ROCgdb captures a stopped wave, rocjitsu copies its scalar and vector registers through Wavefront::debug_read_sgpr and debug_read_vgpr. Those helpers previously called CU accessors that notify execution plugins. If a load into the inspected register is still pending, the race detector treats debugger inspection as a kernel register read and emits a false race report.

This was reproduced through both debugger accessors while reviewing captured-byte race reporting. The helpers were introduced by the ROCgdb support change and already used the observing accessors then. This fix is based directly on develop and is independent of the disassembly change.

The debugger helpers now read register storage without notifying execution plugins and preserve the original returned values and physical bounds. SGPR reads retain the physical-file bounds check in the storage accessor. VGPR reads delegate to a bounded storage accessor in the CU, which checks the actual physical register file and the architecture's stored lane limit. Indices beyond the physical file, and VGPR lanes beyond its stored lane range, still return zero. Normal instruction accesses continue through the observed register API.

The first revision instead added checks against the wave's declared register count and active wave size, returning zero for indices outside those limits. Review raised the alternative of asserting, and a follow-up adopted those assertions plus allocation checks around the restore caller's accesses to s32 and s33.

Further inspection showed that both alternatives changed existing behavior. A wave declaring 16 SGPRs occupies a larger physical storage block; the original accessor can read a nonzero value from the slot corresponding to s32. A wave32 wave backed by 64 stored lanes can also read a lane above 31 through the original CU accessor. Being beyond the declared register count or active wave size does not itself establish C++ undefined behavior when the addressed storage exists. The final choice therefore keeps the existing bounds and values and removes the callbacks. Tightening the allocation contract can be considered separately. The new allocation assertions and their death tests are removed, and debugger restore's existing s32/s33 preservation code is unchanged from develop.

The pending-load regression fails on develop with false SGPR and VGPR reports, with the PC both unchanged and advanced. With the fix, debugger reads return the expected values, produce no callbacks or reports, and leave pending events intact; normal register reads still report the hazards. Compatibility checks cover nonzero physical offsets, reads beyond declared register counts, reads into another allocated block, physical-file bounds, and RDNA4 wave32 reads from 64-lane storage. Nonzero sentinels in the neighboring register verify the lane limit for both 64-lane storage and CDNA5/gfx1250's 32-lane storage. The sentinel tests fail when the debugger helper's lane guard is omitted. Focused native debugger, KFD state-capture and restore, race-detector, and register-observation checks pass with assertions enabled. Changed-file hooks pass. A fresh end-to-end ROCgdb session was not run.

ISSUE ID: #14223 (debugger register-read review finding)

@therock-pr-bot

therock-pr-bot Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 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 Oct 26, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Added a few comments

@kuhar kuhar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@newling
newling force-pushed the users/newling/debugger-register-read-observers branch from 211cc63 to efa3157 Compare October 28, 2026 05:45
@newling
newling force-pushed the users/newling/debugger-register-read-observers branch from efa3157 to 2b3ba80 Compare October 28, 2026 19:00
@newling
newling merged commit 88cd0d9 into ROCm:develop Oct 28, 2026
67 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

rocjitsu post-commit fuzz finding

The 32-worker multiplexer campaigns for commit 88cd0d9e85c7 recorded 1 crash input(s) and 0 hang input(s).

  • ASan+UBSan (6-minute): 625,091 input executions (1,770.05 inputs/s); 0 crash input(s), 0 hang input(s)
  • TSan (6-minute): 558,380 input executions (1,592.01 inputs/s); 1 crash input(s), 0 hang input(s)

A retained input can expose a pre-existing defect; this result does not by itself show that the PR introduced it.

Download the saved inputs and AFL++ statistics (retained for 14 days).

View the workflow run.

@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

2 participants