Skip to content

gdb.rocm: select precise-memory fault insn by XNACK mode - #332

Merged
spatrang merged 1 commit into
amd-stagingfrom
users/spatrang/precise-memory-xnack
Sep 17, 2026
Merged

spatrang merged 1 commit into
amd-stagingfrom
users/spatrang/precise-memory-xnack

Conversation

@spatrang

Copy link
Copy Markdown
Contributor

Summary

gdb.rocm/precise-memory.exp failed on MI325X while passing on MI300X, even though both are gfx942. The divergence is driven by XNACK mode, not architecture: with XNACK enabled the faulting s_load_dword is replayable, so the wave halts on that instruction; with XNACK disabled the PC advances to the following s_nop.

Change

  • Gate the expected instruction on the effective XNACK mode instead of hard-coding s_nop.
  • Add a hip_device_xnack_on helper to lib/rocm.exp that reads hipDeviceProp_t::gcnArchName, whose feature suffix (e.g. gfx942:sramecc+:xnack+) reflects the runtime mode set by HSA_XNACK and device config.

Distinct per-mode expectations are kept (rather than accepting both) to preserve the test's ability to catch a fault-PC regression.

Test

  • MI300X (xnack- default): PASS
  • MI300X (HSA_XNACK=1): PASS
  • MI325X (xnack+ default): PASS

Made with Cursor

@spatrang
spatrang requested a review from a team as a code owner September 10, 2026 16:08
@spatrang spatrang self-assigned this Sep 10, 2026
@spatrang
spatrang requested review from lumachad and removed request for a team September 10, 2026 16:12
@spatrang spatrang assigned lumachad and unassigned spatrang Sep 10, 2026

@lancesix lancesix left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the device supports it, should we try to test both the HSA_XNACK modes?

Comment thread gdb/testsuite/gdb.rocm/precise-memory.exp
@lumachad

Copy link
Copy Markdown
Collaborator

If the device supports it, should we try to test both the HSA_XNACK modes?

If we support both modes on a machine and we can easily configure it, I think we should do it.

@lumachad lumachad assigned lancesix and unassigned lumachad Sep 11, 2026
@lumachad

Copy link
Copy Markdown
Collaborator

@spatrang Given we need to confirm where the fix should land, could you please prototype a dbgapi-side fix? If the fix is in dbgapi, technically we wouldn't need to deal with this situation in rocgdb, and then we could drop this PR.

@spatrang

Copy link
Copy Markdown
Contributor Author

@spatrang Given we need to confirm where the fix should land, could you please prototype a dbgapi-side fix? If the fix is in dbgapi, technically we wouldn't need to deal with this situation in rocgdb, and then we could drop this PR.

Sure, let me give a try from dbgapi side.

@lancesix

Copy link
Copy Markdown
Collaborator

I am not sure it would be trivial to apply, as dbgapi does not know how a particular process is configured regarding xnack-replay. My best guess for now is that we should update the documentation specifying that with precise memory, the PC might be at the faulty insn or just after it depending on the xnack-replay configuration (this is what we have today anyway).

Having dbgapi auto-correct would probably imply needing to have knowledge of the xnack configuration (so needs to be advertised to us by the driver), and we need to ensure we do not double fixup (a scenario like: we get a memviol, show it in the debugger, create a coredump with gcore, and re-open the core dump later - the later opening of the core dump should not re do any fixup).

@lumachad

Copy link
Copy Markdown
Collaborator

Let's go with the rocgdb + updated documentation strategy.

@lumachad lumachad assigned spatrang and unassigned lancesix Sep 11, 2026
@spatrang
spatrang force-pushed the users/spatrang/precise-memory-xnack branch 3 times, most recently from 4ba8bb7 to 03e6f3d Compare September 15, 2026 09:31
@spatrang

Copy link
Copy Markdown
Contributor Author

Rebased onto amd-staging and resolved the rocm.exp conflict with the new generic_target_for proc by keeping both procs.

@spatrang spatrang assigned lancesix and lumachad and unassigned spatrang Sep 16, 2026
@lumachad lumachad assigned spatrang and unassigned lancesix and lumachad Sep 16, 2026
@lumachad

Copy link
Copy Markdown
Collaborator

Looks good to me as-is, but Lancelot suggested we check if both modes are supported and proceed to try them both to make sure they do the expected thing.

@lumachad

Copy link
Copy Markdown
Collaborator

This is an automated-assisted review.

gdb/testsuite/gdb.rocm/precise-memory.cpp:30-31: The inline comment is now inaccurate after this patch. It says "With precise-memory, the exception should be reported on the s_nop", but that is only true under XNACK disabled; under XNACK enabled it stops on s_load_dword. Suggested replacement:

/* Simple kernel which loads from address 0 to trigger a pagefault.
   With precise-memory disabled the fault is reported after the
   s_nop.  With precise-memory enabled, the location depends on the
   XNACK mode: on the s_load_dword itself (XNACK enabled) or on the
   following s_nop (XNACK disabled).  */

gdb/doc/gdb.texinfo (@item on section): "immediately after it" is ambiguous -- "it" could refer to the instruction or to the event. Suggest replacing:

the program location is left either at the instruction that caused the event or immediately after it

with:

the program location is left either at the faulting instruction or immediately after it

gdb/testsuite/gdb.rocm/precise-memory.exp:97 (nit): The pattern "s_load_dword\[^\r\n\]*" also matches s_load_dwordx2, s_load_dword_d16, etc. Since the kernel hard-codes s_load_dword s12, [s10, s11], a tighter pattern such as "s_load_dword\[ \t\]" would catch a future ISA change more visibly. Not blocking.

@spatrang
spatrang force-pushed the users/spatrang/precise-memory-xnack branch from 03e6f3d to 3b2789b Compare September 16, 2026 13:04
@spatrang

Copy link
Copy Markdown
Contributor Author

Updated the PR based on the review feedback.

  • The test now exercises both XNACK modes instead of only the device default. A new hip_device_xnack_modes helper in lib/rocm.exp reports which modes the device supports, and the test forces HSA_XNACK for each one and checks the expected instruction (s_nop for off, s_load_dword for on).
  • Fixed the stale kernel comment and reworded the precise-memory doc entry to match the XNACK-dependent location.
  • Tightened the s_load_dword pattern so it no longer matches wider variants like s_load_dwordx2, while still consuming the operands to end of line so the match anchors correctly.

Validated on both machines, both modes pass:

  • MI300X (default xnack-): off and on PASS
  • MI325X (default xnack+): off and on PASS

Both boxes can be forced to either mode, so both iterations run and pass on each.

@spatrang spatrang assigned lumachad and unassigned spatrang Sep 16, 2026
@lumachad

Copy link
Copy Markdown
Collaborator

This is an automated-assisted review.

lib/rocm.exp:hip_device_xnack_modes -- bool vs list

The helper returns a list of supported modes ({off}, {off on}) to handle the theoretical case where HSA_XNACK=0 is ignored by the runtime and a device is stuck in XNACK-on. If that case cannot happen in practice, a simpler bool (hip_device_xnack_supported) would express the intent more directly, and the XNACK-off iteration could run unconditionally:

if {[hip_device_xnack_supported]} {
    with_test_prefix "xnack on"  { do_test 1 {s_load_dword[ \t][^\r\n]*} }
}
with_test_prefix "xnack off" { do_test 0 {s_nop[ \t]+0} }

Worth confirming with the driver/hardware team whether a device can ever be locked to XNACK-on such that HSA_XNACK=0 has no effect. If not, the list is unnecessary complexity.

precise-memory.exp:107 -- foreach flat-list iteration (nit)

The triple-variable foreach over a flat list is valid Tcl but uncommon enough that a brief comment would help readers who expect single-variable iteration:

# Iterate {mode request expected_insn} triples.
foreach { mode request expected_insn } { ... }

@lumachad lumachad assigned spatrang and unassigned lumachad Sep 17, 2026
The precise-memory fault location depends on the GPU XNACK (memory
retry) mode.  With XNACK enabled the faulting s_load_dword is
replayable, so the wave stops on it.  With XNACK disabled the wave
stops on the following s_nop.  The test previously expected the s_nop
unconditionally, which failed on machines running with XNACK enabled.

Add a hip_device_xnack_supported helper to lib/rocm.exp that probes
whether the device can enable XNACK.  Every device can run with it off,
so run the test with XNACK off unconditionally and add the XNACK on run
only when supported, forcing the mode with HSA_XNACK and checking the
expected instruction.  Also update the precise-memory documentation to
describe the XNACK-dependent location.

The behavior is left as is in amd-dbgapi, which already permits it.  A
fix there would still require handling the same case for core files in
rocgdb.
@spatrang
spatrang force-pushed the users/spatrang/precise-memory-xnack branch from 3b2789b to 7335375 Compare September 17, 2026 10:44
@spatrang

Copy link
Copy Markdown
Contributor Author

Simplified per the review. Replaced the list-returning helper with a bool hip_device_xnack_supported that only probes whether XNACK can be turned on. The test now runs the XNACK off path unconditionally (every device can run with it off) and adds the XNACK on path only when supported. Also updated the commit message to match.

Re-validated on both machines, both paths pass:

  • MI300X (default xnack-): off and on PASS
  • MI325X (default xnack+): off and on PASS

@spatrang spatrang assigned lumachad and unassigned spatrang Sep 17, 2026

@lumachad lumachad left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK if CI is clean.

@spatrang
spatrang merged commit 2c2f6e0 into amd-staging Sep 17, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants