Skip to content

gdb/testsuite: add HIP graph-launch kernel debugging test#197

Open
spatrang wants to merge 1 commit into
amd-stagingfrom
users/spatrang/hip-graph-launch-tests
Open

gdb/testsuite: add HIP graph-launch kernel debugging test#197
spatrang wants to merge 1 commit into
amd-stagingfrom
users/spatrang/hip-graph-launch-tests

Conversation

@spatrang

@spatrang spatrang commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a new gdb.rocm dejagnu test, hip-graph-launch, covering debugging of
kernels dispatched through a HIP graph (hipGraphLaunch) rather than a plain
kernel<<<>>>() launch. The program captures two kernels (add_one then
times_three) from a stream into a single graph, instantiates it, and replays
it 3 times; each replay computes out = (out + 1) * 3 (0 -> 3 -> 12 -> 39).

The test exercises the graph-specific debugger behavior:

  • pending breakpoints in graph-launched kernels resolve and hit,
  • both graph nodes stop in order on every replay (dispatch numbering / re-dispatch),
  • correct per-replay data is observed at each node,
  • symbol breakpoint + backtrace + kernel-argument inspection,
  • single-stepping inside a graph-launched kernel mutates device memory,
  • info dispatches reports the active dispatch and associates it with the kernel.

Test plan

Validated 22/22 assertions under both GCC and LLVM host compilers on gfx942
(MI300X), against:

  • locally built ROCgdb (gdb 18.0.50), and
  • nightly ROCm 7.14 rocgdb (gdb 16.3).

JIRA

AIROCGDB-580

@spatrang spatrang requested a review from a team as a code owner July 8, 2026 09:24
@spatrang spatrang marked this pull request as draft July 8, 2026 09:40
@spatrang spatrang marked this pull request as ready for review July 8, 2026 10:17
@spatrang spatrang force-pushed the users/spatrang/hip-graph-launch-tests branch from e542624 to d32f447 Compare July 10, 2026 10:19
@spatrang

Copy link
Copy Markdown
Contributor Author

Rebased onto latest amd-staging to pick up the record-full.c build fix and re-trigger CI.

@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.

Just minor comments for now, will get a deeper review at a later time.

Comment thread gdb/testsuite/gdb.rocm/hip-graph-launch.cpp Outdated
Comment thread gdb/testsuite/gdb.rocm/hip-graph-launch.cpp Outdated
Comment thread gdb/testsuite/gdb.rocm/hip-graph-launch.exp Outdated
@spatrang spatrang assigned spatrang and unassigned lancesix Jul 13, 2026
Add gdb.rocm/hip-graph-launch test covering debugging of kernels
dispatched via hipGraphLaunch (stream-captured graph, replayed).
@spatrang spatrang force-pushed the users/spatrang/hip-graph-launch-tests branch from d32f447 to 448cbc2 Compare July 13, 2026 10:15
@spatrang

spatrang commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed review comments :

  • Use the shared CHECK macro from rocm-test-utils.h instead of a local copy.
  • num_elems is now constexpr.
  • Keep the replay count in sync: the .exp passes -DNUM_REPLAYS=$num_replays and the source guards the define with #ifndef (per the nonstop-mode.exp pattern).

Also rebased onto latest amd-staging.

@spatrang spatrang removed their assignment Jul 13, 2026
@spatrang spatrang marked this pull request as draft July 13, 2026 14:58
@spatrang spatrang marked this pull request as ready for review July 13, 2026 14:58
@aktemur

aktemur commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Assigning to myself to reduce the load on @lancesix.
@lancesix, feel free to assign it back to yourself.

@aktemur aktemur assigned aktemur and unassigned lancesix Jul 15, 2026
Comment on lines +66 to +69
int *result_ptr, result;
hipStream_t stream;
hipGraph_t graph;
hipGraphExec_t graph_exec;

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.

It's not a hard requirement, but the code style prefers to declare variables at the first time of definition. E.g. Decls above can be moved down, like so:

hipStream_t stream;
CHECK (hipStreamCreate (&stream));
...
hipGraph_t graph;
CHECK (hipStreamEndCapture (stream, &graph));

hipGraphExec_t graph_exec;
CHECK (hipGraphInstantiate (&graph_exec, graph, nullptr, nullptr, 0))
...

return
}

# Line numbers of the breakpoint markers in the two kernels.

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.

Nit: I think this comment can be removed. It's rather repeating the code.

gdb_test "print out\[0\]" " = $val" \
"add_one sees value from previous replay"

continue_to_kernel "times_three" $::times_three_line \

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.

It's mostly a personal taste: The proc continue_to_kernel is a wrapper around one gdb_test only. We can rather inline it and remove the proc. What we are testing would become clearer.

# test description.
proc continue_to_kernel {kernel line desc} {
gdb_test "continue" \
"Thread $::decimal .* hit Breakpoint $::decimal, with lane 0, $kernel \\(.*\\) at .*$::srcfile:$line.*" \

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.

If we want to make this shorter, this would also be ok, without losing quality of the test:
Breakpoint .* $kernel .*$::srcfile:$line.*

Comment on lines +64 to +65
clean_restart
gdb_load $::binfile

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.

clean_restart $::testfile

# After all replays the final result must be 39, and the program exits
# cleanly.
gdb_test "continue" \
"result is $val.*\\\[Inferior 1 .* exited normally\\\]" \

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.

Can we use $inferior_exited_re here?

Comment on lines +116 to +117
clean_restart
gdb_load $::binfile

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.

clearn_restart $::testfile


gdb_breakpoint "add_one" -allow-pending
gdb_test "continue" \
"Thread $::decimal .* hit Breakpoint $::decimal, with lane 0, add_one \\(.*\\) at .*$::srcfile:.*" \

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.

Nit: The shorter variant I gave above may be used.

Comment on lines +132 to +133
gdb_test "print out" " = \\(int \\*\\) $::hex" \
"kernel argument is readable"

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.

We can print and check the value of out[0], too. The value can be passed as an argument to the proc.

Comment on lines +169 to +171
test_breakpoint_by_name_and_backtrace
test_single_step
test_info_dispatches

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.

I'm not sure what testing value we gain by these 3 tests. The first one tested being able to hit the breakpoint inside graph-launched kernels, that's understood. But the other 3 seem like they test debug info and basic debug functionality, for which there already exist tests. I don't see why it should matter to do these for graph-launched kernels. I'd be in favor of removing them for making this test file simpler and less expensive (such tests accumulate and the overall test time may increase to unpleasant levels over time).

@aktemur aktemur assigned spatrang and unassigned aktemur Jul 15, 2026
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