gdb/infrun.c: fix handle_step_into_function lane divergence - #181
Conversation
|
Function foo is called in an execution path where lane 2 is inactive. The lane becomes active temporarily for spilling/restoring, but that sounds like an implementation detail. Can we not hide these temporary state changes, by noticing that the frame for foo is deeper than the frame in which we started stepping? |
|
Will probably need few discussions for this but:
That second bit would probably be the most efficient approach to address stepping with divergent lanes. I can look into this. |
This makes sense to me. |
I have added a new patch to the branch implementing this. Let me know what you think. |
aktemur
left a comment
aktemur
left a comment
There was a problem hiding this comment.
The second patch would then make the first patch unnecessary, wouldn't it?
| && (!ecs->event_thread->is_simd_lane_active | ||
| (ecs->event_thread->current_simd_lane ()))) | ||
| { | ||
| infrun_debug_printf |
There was a problem hiding this comment.
The commit message of the second patch says we stop at line 4 and then execute "step", which makes us reach the end of line. However, on line 4 there is "foo ()". We should've entered foo and stopped there. I think you meant to have on line 4 a statement without a function call.
|
|
||
| Because the lane was inactive on the call site, we expect that | ||
| lane to remain inactive for the entire frame. Therefore, even if | ||
| we issued a next, the current lane should ont become active until |
There was a problem hiding this comment.
"issue a next" -> Did you mean a "step"?
Also there is a typo: "ont" -> "not"
|
@lancesix we are planning to land our compiler merge friday/monday which will epose this issue. can this get landed soon ? |
|
@lancesix This will have to land on both amd-staging and amd-staging-rocgdb-16. |
trending to Monday 7/13 to land. |
palves
left a comment
palves
left a comment
There was a problem hiding this comment.
The commit log of patch 1, here:
Dump of assembler code for function _Z3fooi:
.../gdb/testsuite/gdb.rocm/lane-execution.cpp:
68 {
0x00007ffff5f29dc0 <+0>: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
0x00007ffff5f29dc4 <+4>: s_mov_b32 s25, s33
0x00007ffff5f29dc8 <+8>: s_mov_b32 s33, s32
0x00007ffff5f29dcc <+12>: s_xor_saveexec_b32 s16, -1
0x00007ffff5f29dd0 <+16>: buffer_store_dword v3, off, s[0:3], s33 offset:4
0x00007ffff5f29dd8 <+24>: s_mov_b32 exec_lo, s16
[...]
When single stepping the first 5 instructions, because the current lane
is inactive, we just ignore the stop and continue single stepping
directly. However, the 5th instruction (at offset foo+12) activates all
the lanes of the wave for spilling using a full VGPR, ...
A couple things wrong there:
-
foo+12 is the 4th instruction, not the 5th.
-
"activates all the lanes" isn't correct, s_xor_saveexec_b32 is xor'ing the exec mask with -1, which means it's inverting the mask not setting it all to ones.
It doesn't change the conclusion, but it made me confused for a while.
But, I also don't see the point of patch #1 if we have patch #2. That patch reads to me like it is working around the fact that we're skipping set_step_info calls because the "If this lane is divergent, keep stepping until it becomes active" logic is too early. I'd rather drop it for now.
Patch #2 seems right to me. It is a similar idea to the "Skip divergent code regions by running to breakpoint" patch's logic from the lane divergence PR (#124), see: ecad366
Of course, without the debug info to figure out the logical PC, we can't do that. Skipping calls ends up giving a lot of the same benefit.
| lane to remain inactive for the entire frame. Therefore, even if | ||
| we issued a next, the current lane should ont become active until | ||
| we return from the current frame. We can return directly to | ||
| the caller. */ |
There was a problem hiding this comment.
I'd suggest this reword:
/* We just stepped inside a new function, and the current
lane is inactive. None of the architectures we support
can atomically jump to a new frame while changing the set
of active lanes, so the current lane was inactive at the
call site.
Because the lane is already inactive on entry, we expect
it to remain logically inactive for the entire frame.
The function may still activate the lane transiently,
e.g., for a whole-wave vector register spill, but that's
an implementation detail that the user should not see.
Therefore, even if we issued a step rather than a next,
there is nothing to stop for in this frame, so we can
return directly to the caller. */
(Edit: see the other review comment for a branch with this comment already added in.)
I.e.,
-
make it a little more clear what the first paragraph is talking about -- it took me a couple reads to figure out what you meant.
-
talk about the frame remaining inactive in logical terms, and mention that the lane may be turn active temporarily (and give the example why) but we don't care about stopping for that.
-
say "step rather than a next", as I think that's what you meant.
-
use emacs' word wrap. :-)
| != ecs->event_thread->control.step_stack_frame_id) | ||
| && (frame_unwind_caller_id (frame) | ||
| == ecs->event_thread->control.step_stack_frame_id) | ||
| && !ecs->event_thread->control.in_step_start_function (frame)) |
There was a problem hiding this comment.
This in_step_start_function check here is wrong, if you don't bring in the outer_frame_id check as well.
See commit 005ca36. In the original code you're copying this from, the code looks like this:
&& ((ecs->event_thread->control.step_stack_frame_id
!= outer_frame_id)
|| !ecs->event_thread->control.in_step_start_function (frame))))
This is for handling instruction-stepping through startup code, like in _start, before the stack is set up.
The in_step_start_function call is only reached when the outer_frame_id check is false, i.e., when step_stack_frame_id == outer_frame_id.
In normal stepping, started from a real function with a valid frame id, step frame id is not outer_frame_id, so the first check is true and the call to in_step_start_function doesn't happen. So that check does nothing except in the outermost-frame case.
When you stepi through _start while SP is still invalid, so step_stack_frame_id == outer_frame_id, you step over the instruction that sets SP from invalid to valid. Now the frame_unwind_caller_id (frame) == outer_frame_id, which matches step_stack_frame_id. So GDB thinks you stepped into a subroutine from the outermost function. But you didn't, you're still in _start. The !in_step_start_function check catches that -- you're still in the same function you started in => not really a call.
The new code makes the !in_step_start_function unconditional, so the result is that it misdetects stepping into a recursive call of the same function:
same find_pc_function
=> in_step_start_function true
=> predicate false
=> not treated as a subroutine call
Not sure we can have that outer_frame_id turning into a real frame on AMD GPUs. The AMD GPU heuristic unwinder does use outer_frame_id, so maybe we can. And who knows on other archs...
This suggests to me that it'd be better to factor out the "Check for subroutine calls" logic to a separate function and call it here as well. Let's call that function "stepped_into_subroutine".
The part of that logic that gave me the most headaches is the SIGTRAMP case, but I convinced myself that we want it in this divergent stepping code too. Let me explain.
There are two cases around single-stepping into a signal handler:
- case 1 - return trampoline (the common case). The kernel jumps straight to the signal handler.
The trampoline is the handler's return address. It runs after the handler. When the handler finishes, it "returns" into the trampoline, which callssigreturnto unwind back to the interrupted code. This is the frame chain while you're stopped in the handler:
handler <- current frame
sigtramp <- handler's caller (its return addr)
step frame <- interrupted code
This is why the comment says "the current frame is a grandchild of the step frame".
- case 2 - the entry trampoline case (some platforms).
In this case, the kernel does not jump straight to the handler. Instead, it first transfers control to a trampoline, which then calls the handler. When the signal is delivered, the current frame is the trampoline itself:
sigtramp <- current frame
step frame <- interrupted code
This is the case that looks like a subroutine call, when it really isn't. The sigtramp frame is an async call activation, unrelated to whatever the mainline code was doing.
Let's imagine that someday we'll have a SIMT model implemented on top of a CPU's SIMD unit, so we'll have lanes on the CPU side as well. Even if the current lane was inactive for the mainline code, the signal handler may run with the lane enabled. Presumably, for case 1, the kernel would already reenable all lanes so we wouldn't enter the "step divergent lanes" code, but the kernel could also always disable all lanes on signal handler entry. Who knows. But it wouldn't matter, as the "stepped into subroutine" would return false for case 1 (because grandchild, not child), so we'd always single step over the signal handler.
For case 2 (the trampoline case), though, it could well be that the signal handler jumps to the trampoline frame with the exec mask unmodified, which makes GDB's think that we stepped into a subroutine, and then skip the whole signal handler. Moving the SIGTRAMP_FRAME check into the new stepped_into_subroutine function too would handle this, making us single step into the signal handler here too, in case the trampoline enables lanes and then calls the handler with the current lane enabled.
If we don't have a mix of signal handler and lanes, the SIGTRAMP_FRAME check is harmless.
So I'm thinking that it's just simpler to factor out the whole logic to the new stepped_into_subroutine and call it from the divergent lane case.
It's very hard for me to reason about this stuff without playing with the code, so I went ahead and did the refactor, to confirm it all still works. I did this as two patches on top of yours:
-
a patch doing the refactor. The way I did that with breaking the predicates into early returns with their own comment I think makes it much easier to understand. Let me know what you think of that.
-
a patch adjusting your new code to use the new refactored function. Oh, and it tweaks the comments too, like I suggested in another review comment.
I put it in the users/palves/fix-lane-divergence-stepping branch, see:
amd-staging...users/palves/fix-lane-divergence-stepping
The intention is to do the obvious fold the second patch into yours, of course.
I think I could send the stepped_into_subroutine refactor upstream immediately as a cleanup.
56206a0 to
559b246
Compare
|
Thanks for the in depth review. Last push:
I don't see why we could not send the |
process_event_stop_test uses a long expression with several non-obvious predicates to decide whether the thread has stepped into a subroutine call, with a big block of comments explaining each of its parts above it. A following patch wants the same "stepped into subroutine" logic from another place, so factor the expression out into a new stepped_into_subroutine function. Rather than carrying the expression over verbatim, write it as a series of early-return checks, with each (smaller) predicate having the comment that explains it right above it. This is a pure refactor, with no change in behavior. Change-Id: Id027e7e3dcdce8bbb38df79b70a01752f2927392
When stepping through code with lane-divergence support enabled, we can
currently have the following:
1 if (pred ())
2 quite_expensive ();
3 else
4 foo ();
5
6 bar ();
Consider we stopped on foo, with a lane selected active at this point and
issue a "step" command. GDB will start single stepping until we reach a
new line. Because our compiler emits the code for the else branch
before the then branch, the execution will single step the end of line
4, go back to line 1 to switch the EXEC mask, and will proceed to L2.
Because the selected lane is inactive at this point, infrun will single
step all the way through L2, including in the called function, until we
return from quite_expensive and eventually reach L6.
This is quite wasteful. When setpping through L2, as soon as we step
inside the "quite_expensive" function, we can actually know that nothing
of interest can happen while in this function. The current lane was
inactive before calling the function, so it cannot become active while
in that call. Even if we used "step", because the lane was inactive on
the function call, we can actually behave as "next" and resume execution
until we return from "quite_expensive".
This patch proposes to implement the shortcut, and detect when we step
in a function with the current lane is active. In this case, we do as
we do for next: insert a breakpoint on the return address, and resume
execution. This way, we execute "quite_expensive" at full speed instead
of single stepping every single instruction.
This scenario is already exercised by the gdb.rocm/lane-execution.exp
testcase. We just used to take the long approach.
Co-authored-by: Pedro Alves <pedro@palves.net>
Change-Id: Iecd0c50a40b99c4c2e16fa43d78f66814f59c803
559b246 to
73190dd
Compare
|
last push:
|
palves
left a comment
palves
left a comment
There was a problem hiding this comment.
LGTM, let's merge this.
I'll send the stepped_into_subroutine refactor upstream soon.
5 participants
Pending compiler changes (current tip of
amd-stagingof the compiler[1]) shows failures in gdb.rocm/lane-execution.exp:In this test, we have:
The program stops at L104 (active lane is 2), set scheduler-locking on and issues a "step" command. Because the compiler emits the "else" branch before the "then" branch, the "step" command will single step through L104, then will change the active lanes and single step through L102. The expectation is that we single step through L102 until we reach L108, which will be the first line with lane 2 active we will execute next.
However, something goes wrong when stepping L102, and the wave will run to completion. Because we have scheduler-locking on, once the wave has terminated, no further events will reach GDB, hanging it, causing the timeout.
When stepping with lane divergence support enabled, GDB tries to single step until the current lane becomes active again and the PC of the wave reaches a new line. If we follow this process, here is what happens:
First GDB steps through L104 as expected, and does not change as it is still on the starting line. We then go to the code changing the EXEC mask (mapped to L101, but not marked as statement so skipped), and end-up on L102. When executing L102, lane 2 is inactive, so we continue single stepping instructions, which includes calling the function foo.
In the function foo, we have:
When single stepping the first 5 instructions, because the current lane is inactive, we just ignore the stop and continue single stepping directly. However, the 5th instruction (at offset foo+12) activates all the lanes of the wave for spilling using a full VGPR, so lane 2 is active when executing the buffer_store_dword instruction. As a consequence, GDB cannot dismiss this instruction as the others, and needs to consider it fully.
At this point, in process_event_stop_test, GDB will realize that the current frame changed compared to when we started the step command. The process from this point is quite simple, we just stepped inside a new function, we are almost done. Last thing to do is to skip the function prologue, and continue execution. This is done by handle_step_into_function: we set a breakpoint after the prologue (at foo+104: 0x00007ffff7fa9f28), resume execution and hope to be done.
We resume execution, which executes the buffer_store, then restores the EXEC mask (disabling lane 2) and execute until the end of the prologue. Because lane 2 is now inactive, we have to continue stepping, so we execute all of the foo function. This includes a call to the bar function which never touches the EXEC mask, so nothing important happening there.
After we return from bar, we need to execute the epilogue of function foo which looks like:
Between offset +192 and +204, we need to restore the state that was spilled during the prologue, so we do the same thing: activate all the lanes, do the load and restore lanes. When stopping on the buffer_load instruction, GDB sees that lane 2 is active again, and gets back to "normal" processing. We see that we are on a frame different from the one we had when starting the step, so are about to stop and try to skip the prologue. However, this goes quite wrong, as shown by the infrun logs:
We insert the breakpoint at the end of the function prologue and resume execution. But the PC is currently in the epilogue, not in the prologue! Because of this, we resume execution without single stepping, with a breakpoint inserted at an instruction we will not execute, which allows the wave to run to completion.
I noted earlier that those failures are regressions with a new compiler, so why didn't we see this issue before? With the previous version of the compiler, the bar function (called by foo) also had some spill / restore logic, changing slightly the sequence. Because bar was also spilling, it has an instruction where lane 2 was active, causing GDB to consider the stop without the lane divergence bypass.
When evaluating if we frame changed, we executed this condition:
Here, the first two predicates hold (current frame is not the same as when we started stepping, and it is not a SIGTRAMP frame), but things go south when checking
This one is not true. Here frame is bar, so the caller is foo, and the step_stack_frame we compare it to is lane_pc_test. As a consequence, this check fails, which gets us to fallback to another case: stepping got us out of the original step_range we set, but the current instruction is not a statement either. So we reset the stepping range information (set_step_info) and continue single stepping through the current range. This gets us all the way through bar (let's ignore the spill-restore bit), then in foo where we also do the spill-restore. Because the same check is done, and the current frame (foo) is not called by where we started (bar), we continue stepping, and eventually finish the operation normally.
So now the question is: how do we fix this? I see 2 approaches for this. One is to reset the "step range" when we see lane 2 active in foo the first time. This would avoid us the incorrect detection of a new frame when doing the spill-restore. However, there is something even more broken exposed with this: why do we allow resuming the wave to skip the function prologue if we are not in the function prologue?
Because the resume seems universally wrong, this patch proposes to fix this first. When doing the resume, check that the thread's PC is not past the end of the prologue before we use
insert_step_resume_breakpoint_at_sal_1. With this simple adjustment, the gdb.rocm/lane-execution.exp testcase passes correctly.
I will explore reseting the stepping range in followup work.
Tested on amdgpu GFX1031.
[1] ROCm/llvm-project@a6fa68a
Change-Id: I7f57c51a430e9a87bddc4877ebddf5deec8a0de8
Bug: LCOMPILER-2337