Re: [PATCH v4 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history

[Guinevere Larsen <blarsen@redhat.com>]

On 4/5/24 06:12, Metzger, Markus T wrote:
>>> When trying to step over a breakpoint at the end of the trace, the
>>> step-over will fail with no-history.  This does not clear step_over_info
>>> so a subsequent resume will cause GDB to not resume the thread and
>> expect
>>> a SIGTRAP to complete the step-over.  This will never come causing GDB to
>>> hang in the wait-for-event poll.
>>>
>>> That step-over failed after actually completing the step.  This is wrong.
>>> The step-over itself should have failed and the step should not have
>>> completed.  Fix it by moving the end of execution history check to before
>>> we are stepping.
>>>
>>> This exposes another issue, however.  When completing a step-over at the
>>> end of the execution history, we implicitly stop replaying that thread.  A
>>> continue command would resume after the step-over and, since we're no
>>> longer replaying, would continue recording.
>>>
>>> Fix that by recording the replay state in the thread's control state and
>>> failing with no-history in keep_going if we're switching from replay to
>>> recording.
>> I am a little confused by your test case. From the commit message
>> description, I expected the following GDB session to have been problematic:
>>
>> $ gdb record_goto
>> (gdb) start
>> 49 fun4 ();
>> (gdb) record btrace
>> (gdb) break fun1
>> (gdb) continue
>> in frame fun1 ()
>> 23 }
>> (gdb) reverse-next
>> In frame fun4 ()
>> 41 fun1 ();
>> (gdb) next
>> No more reverse-execution history.
>> 23 }
>> (gdb) next
>> # problems due to the SIGTRAP from the previous line
>>
>> (Pardon my hand written gdb session, my new system has an AMD cpu, so I
>> seem to be locked out of btrace).
>>
>> My confusion comes from your test case never explicitly setting any
>> breakpoints. I don't see why the inferior would have a sigtrap generated
>> to test it. Am I misunderstanding the issue you described?
> There are three tests contained in this patch.  Two of them do

My bad, I should have said that no test set the breakpoint to the 
furthest point that is executed.

I didn't understand that the break should be in the previous 
instruction. Now that I'm rethinking my example, it doesn't really make 
sense, since as the testcase says, GDB wouldn't be replaying at that 
point. With this new understanding, this change LGTM.

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
>>> +# We need to be replaying, otherwise, we'd just continue recording.
>>> +gdb_test "reverse-stepi"
>>> +gdb_test "break"
>>> +
>>> +# Continuing will step over the breakpoint and then run into the end of
>>> +# the execution history.  This ends replay, so we can continue recording.
>>> +gdb_test "continue" "No more reverse-execution history.*"
>>> +gdb_continue_to_end
> The third test makes sure that 'step n' doesn't suddenly start recording.
>
> Regards,
> Markus.
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928