Re: [PATCH v2] gdb: Modify until_break_command to act correctly on SIGTRAMP_FRAME

[Tomas Vanek <vanekt@fbl.cz>]

On 27/10/2022 19:46, Tomas Vanek wrote:
> Hi Luis,
>
> On 27/10/2022 12:31, Luis Machado wrote:
>> Hi Tomas,
>>
>> On 10/21/22 12:58, Tomas Vanek wrote:
>>> This patch partially depends on
>>> gdb/arm: Terminate frame unwinding in M-profile lockup state
>>> (without it lockup state is unwound as if it were a normal
>>> stack frame).
>>>
>>> The commands 'advance' and 'until' try to set a breakpoint
>>> on the bogus return address derived from Arm M-profile magic
>>> address (actually EXC_RETURN or a PC value indicating lockup).
>>>
>>> The offending breakpoint should be set at the return address in
>>> the caller. The magic value 0xffffffff in LR indicates
>>> there is no caller (return to this address would lock up the CPU).
>>>
>>> Similar behaviour of 'advance' and 'until' is observed in
>>> an exception handler routine. In this case LR contains e.g.
>>> 0xfffffff1 (EXC_RETURN) and GDB tries to set a breakpoint at
>>> 0xfffffff0. It should use a return value stacked by the exception
>>> instead.
>>>
>>> Testbench setup:
>>> STM32G474, a Cortex-M4 device. Any Cortex-M device can be used.
>>> A test application (an ordinary blink) with a standard startup
>>> is loaded to the device flash.
>>>
>>> Steps to reproduce the problem:
>>>
>>> start GDB server
>>>   $ openocd -f interface/cmsis-dap.cfg -f target/stm32g4x.cfg
>>>
>>> start GDB in second terminal
>>>   $ arm-none-eabi-gdb blink.elf
>>>
>>>   (gdb) target extended-remote localhost:3333
>>>
>>> Reset the device and halt it:
>>>   (gdb) monitor reset halt
>>>   target halted due to debug-request, current mode: Thread
>>>   xPSR: 0x01000000 pc: 0x08000e14 msp: 0x20020000
>>>
>>> Step by one instruction to re-read GDB register cache:
>>>   (gdb) stepi
>>>
>>> Check registers, LR should be 0xffffffff after reset:
>>>   (gdb) info registers
>>>   ...
>>>   sp             0x20020000          0x20020000
>>>   lr             0xffffffff          -1
>>>   pc             0x8000e16           0x8000e16
>>>   xPSR           0x1000000           16777216
>>>   ...
>>>
>>>   (gdb) set debug remote
>>>
>>> Issue 'advance' command:
>>>   (gdb) advance main
>>>   [remote] Sending packet: $mfffffffe,2#fa
>>>   [remote] Packet received: 0000
>>>   [remote] Sending packet: $mfffffffe,2#fa
>>>   [remote] Packet received: 0000
>>>   [remote] Sending packet: $m8000526,2#30
>>>   [remote] Packet received: 2046
>>>   [remote] Sending packet: $Z1,8000526,2#7a
>>>   [remote] Packet received: OK
>>>   [remote] packet_ok: Packet Z1 (hardware-breakpoint) is supported
>>>   [remote] Sending packet: $Z0,fffffffe,2#43
>>>   [remote] Packet received: E0E
>>>   [remote] packet_ok: Packet Z0 (software-breakpoint) is supported
>>>   Warning:
>>>   Cannot insert breakpoint 0.
>>>   Cannot access memory at address 0xfffffffe
>>>
>>>   Command aborted.
>>>   (gdb)
>>>
>>> Relevant messages from OpenOCD:
>>>   Error: Failed to read memory at 0xfffff000
>>>   Error: can't add breakpoint: unknown reason
>>>
>>> This patch adds skipping over frames that are not suitable for
>>> guarding with a breakpoint inspired by 'finish' command.
>>> If no suitable frame is found, a momentary breakpoint is not set.
>>>
>>> v2: Comment fixes, bug reference.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28683
>>> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
>>> ---
>>>   gdb/breakpoint.c | 22 +++++++++++++++++-----
>>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index f6591d4..bb85342 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -10467,6 +10467,7 @@ enum async_reply_reason
>>>   until_break_command (const char *arg, int from_tty, int anywhere)
>>>   {
>>>     frame_info_ptr frame;
>>> +  frame_info_ptr caller_frame;
>>>     struct gdbarch *frame_gdbarch;
>>>     struct frame_id stack_frame_id;
>>>     struct frame_id caller_frame_id;
>>> @@ -10505,7 +10506,17 @@ enum async_reply_reason
>>>     frame = get_selected_frame (NULL);
>>>     frame_gdbarch = get_frame_arch (frame);
>>>     stack_frame_id = get_stack_frame_id (frame);
>>> -  caller_frame_id = frame_unwind_caller_id (frame);
>>> +
>>> +  caller_frame = get_prev_frame_always (frame);
>>> +
>>> +  while (caller_frame)
>>> +    {
>>> +      if (get_frame_type (caller_frame) != TAILCALL_FRAME
>>> +      && gdbarch_code_of_frame_writable (get_frame_arch 
>>> (caller_frame), caller_frame))
>>> +    break;
>>> +
>>> +      caller_frame = get_prev_frame_always (caller_frame);
>>> +    }
>>>       /* Keep within the current frame, or in frames called by the 
>>> current
>>>        one.  */
>>> @@ -10514,14 +10525,15 @@ enum async_reply_reason
>>>       gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
>>>   -  if (frame_id_p (caller_frame_id))
>>> +  if (caller_frame)
>>>       {
>>>         struct symtab_and_line sal2;
>>>         struct gdbarch *caller_gdbarch;
>>>   -      sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);
>>> -      sal2.pc = frame_unwind_caller_pc (frame);
>>> -      caller_gdbarch = frame_unwind_caller_arch (frame);
>>> +      sal2 = find_pc_line (get_frame_pc (caller_frame), 0);
>>> +      sal2.pc = get_frame_pc (caller_frame);
>>> +      caller_gdbarch = get_frame_arch (caller_frame);
>>> +      caller_frame_id = get_frame_id (caller_frame);
>>>           breakpoint_up caller_breakpoint
>>>       = set_momentary_breakpoint (caller_gdbarch, sal2,
>>
>> My understanding is that these changes are meant to prevent both 
>> commands (until/advance) from
>> attempting to place a breakpoint in a caller that doesn't really 
>> exist, right?
>>
> Yes.
>
>> The finish command, as you mentioned, seems to have a similar 
>> treatment in "skip_finish_frames".
>>
>> Would it be possible to factor out that code into a common function 
>> that we can call to determine
>> if we have a valid caller whose PC we can breakpoint?
>
> Of course it was also my original idea.
>
> Unfortunately skip_finish_frames() uses skip_tailcall_frames() and 
> skip_unwritable_frames()
> which both call get_prev_frame(). get_prev_frame() stops if the 
> current frame is main() or startup.
> This is probably sufficient for the 'finish' command (until the user 
> requests to 'finish' the main() function:
> 'finish' refuses to do so with message "finish not meaningful in the 
> outermost frame").
>
> 'advance' places one breakpoint according to the user request.
> The second one is set as a safety measure for the case the first one 
> is not reached.
> It is quite common to use 'advance' in the main() function and even to 
> execute the startup code
> and stop at the main() begin. IMO gdb should treat main() as any other 
> function and place the safety
> breakpoint at its return if possible.
>
> That's why I use get_prev_frame_always() instead of get_prev_frame().
> And this is also the reason why there is no simple and elegant way to 
> factor out a common function for
> both 'advance' and 'finish'.
>
> Tomas

Luis,

do you have more comments on this patch?
Or do we need it reviewed by somebody else?

Tomas