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

[Luis Machado <luis.machado@arm.com>]

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?

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?