[PING] [PATCH v3] gdb: Modify until_break_command to act correctly on SIGTRAMP_FRAME

[Tomas Vanek <vanekt@fbl.cz>]

On 11/01/2023 19:01, Tomas Vanek wrote:
> 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:
> STM32F4, 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
>
> The commit 7eb895307f53 Skip unwritable frames in command "finish"
> added the gdbarch based mechanism for distinguishing the frames not
> suitable for setting a breakpoint. It unfortunately limited the changes
> to the 'finish' command and left other commands potentially setting
> a breakpoint at magic address.
>
> This patch introduces get_prev_frame_for_breakpoint () for skipping
> over frames that are not suitable for guarding with a breakpoint.
> If no suitable frame is found, a momentary breakpoint is not set.
> Instead of several independent accesses to frame_unwind_caller_id (),
> frame_unwind_caller_pc () and frame_unwind_caller_arch () referencing
> the current frame, frame_info_ptr of the real caller is resolved
> first and then used to get_frame_id (), get_frame_pc () and
> get_frame_arch ().
>
> v2: Comment fixes, bug reference.
>
> v3: get_prev_frame_for_breakpoint () introduced.
>      Fixed skipping inline frames.
>      Extended to a local variable scope guard breakpoint of the 'watch'
>      command and to insert_step_resume_breakpoint_at_caller ().
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28683
> Signed-off-by: Tomas Vanek <vanekt@fbl.cz>
> ---
>   gdb/breakpoint.c | 26 +++++++++++++++-----------
>   gdb/frame.c      | 37 +++++++++++++++++++++++++++++++++++++
>   gdb/frame.h      | 10 +++++++++-
>   gdb/infrun.c     | 15 ++++++++++-----
>   4 files changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 00cc2ab..e1f04b6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -10212,12 +10212,13 @@ enum print_stop_action
>        that we will encounter it first in bpstat_stop_status.  */
>     if (exp_valid_block != NULL && wp_frame != NULL)
>       {
> -      frame_id caller_frame_id = frame_unwind_caller_id (wp_frame);
> +      frame_info_ptr caller_frame;
> +      caller_frame = get_prev_frame_for_breakpoint (wp_frame);
>   
> -      if (frame_id_p (caller_frame_id))
> +      if (caller_frame)
>   	{
> -	  gdbarch *caller_arch = frame_unwind_caller_arch (wp_frame);
> -	  CORE_ADDR caller_pc = frame_unwind_caller_pc (wp_frame);
> +	  gdbarch *caller_arch = get_frame_arch (caller_frame);
> +	  CORE_ADDR caller_pc = get_frame_pc (caller_frame);
>   
>   	  scope_breakpoint
>   	    = create_internal_breakpoint (caller_arch, caller_pc,
> @@ -10232,7 +10233,7 @@ enum print_stop_action
>   	  scope_breakpoint->disposition = disp_del;
>   
>   	  /* Only break in the proper frame (help with recursion).  */
> -	  scope_breakpoint->frame_id = caller_frame_id;
> +	  scope_breakpoint->frame_id = get_frame_id (caller_frame);
>   
>   	  /* Set the address at which we will stop.  */
>   	  scope_breakpoint->loc->gdbarch = caller_arch;
> @@ -10592,9 +10593,9 @@ 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;
>     int thread;
>     struct thread_info *tp;
>   
> @@ -10630,7 +10631,7 @@ 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_for_breakpoint (frame);
>   
>     /* Keep within the current frame, or in frames called by the current
>        one.  */
> @@ -10639,14 +10640,17 @@ 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;
> +      struct frame_id caller_frame_id;
> +      CORE_ADDR pc = get_frame_pc (caller_frame);
>   
> -      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 (pc, 0);
> +      sal2.pc = pc;
> +      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,
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 2f9622a..c547037 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -537,6 +537,43 @@ struct frame_info
>     return frame;
>   }
>   
> +/* Similar to skip_artificial_frames () but skips also unwritable frames.  */
> +
> +static frame_info_ptr
> +skip_artificial_unwritable_frames (frame_info_ptr frame)
> +{
> +  while (get_frame_type (frame) == INLINE_FRAME
> +	 || get_frame_type (frame) == TAILCALL_FRAME
> +	 || gdbarch_code_of_frame_writable (get_frame_arch (frame), frame) == 0)
> +    {
> +      frame = get_prev_frame_always (frame);
> +      if (frame == NULL)
> +	break;
> +    }
> +
> +  return frame;
> +}
> +
> +/* Similar to get_prev_frame_always () but skips also unwritable frames.
> +   Returns nullptr if no suitable frame found.
> +   Use for setting a guard breakpoint after return to the caller.  */
> +
> +frame_info_ptr
> +get_prev_frame_for_breakpoint (frame_info_ptr next_frame)
> +{
> +  frame_info_ptr this_frame;
> +
> +  next_frame = skip_artificial_unwritable_frames (next_frame);
> +  if (next_frame == NULL)
> +    return nullptr;
> +
> +  this_frame = get_prev_frame_always (next_frame);
> +  if (this_frame == NULL)
> +    return nullptr;
> +
> +  return skip_artificial_unwritable_frames (this_frame);
> +}
> +
>   /* See frame.h.  */
>   
>   frame_info_ptr
> diff --git a/gdb/frame.h b/gdb/frame.h
> index 5935465..9198061 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -279,6 +279,12 @@ extern void restore_selected_frame (frame_id frame_id, int frame_level)
>      frame.  */
>   extern frame_info_ptr get_prev_frame_always (frame_info_ptr);
>   
> +/* Return a "struct frame_info" corresponding to the frame that called
> +   THIS_FRAME.  Returns NULL if there is no such frame.
> +
> +   Unlike get_prev_frame_always, this function skips unwritable frames.  */
> +extern frame_info_ptr get_prev_frame_for_breakpoint (frame_info_ptr);
> +
>   /* Given a frame's ID, relocate the frame.  Returns NULL if the frame
>      is not found.  */
>   extern frame_info_ptr frame_find_by_id (frame_id id);
> @@ -533,7 +539,9 @@ extern void put_frame_register_bytes (frame_info_ptr frame, int regnum,
>   
>   /* Unwind the PC.  Strictly speaking return the resume address of the
>      calling frame.  For GDB, `pc' is the resume address and not a
> -   specific register.  */
> +   specific register.
> +   Does not skip unwritable frames. Do not use for setting breakpoint
> +   after returnt to the caller.  */
>   
>   extern CORE_ADDR frame_unwind_caller_pc (frame_info_ptr frame);
>   
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 181d961..0822b0d 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7930,20 +7930,25 @@ static bool restart_stepped_thread (process_stratum_target *resume_target,
>   static void
>   insert_step_resume_breakpoint_at_caller (frame_info_ptr next_frame)
>   {
> +  frame_info_ptr caller_frame;
> +  struct frame_id caller_frame_id;
> +
> +  caller_frame = get_prev_frame_for_breakpoint (next_frame);
> +  caller_frame_id = get_frame_id (caller_frame);
> +
>     /* We shouldn't have gotten here if we don't know where the call site
>        is.  */
> -  gdb_assert (frame_id_p (frame_unwind_caller_id (next_frame)));
> +  gdb_assert (frame_id_p (caller_frame_id));
>   
> -  struct gdbarch *gdbarch = frame_unwind_caller_arch (next_frame);
> +  struct gdbarch *gdbarch = get_frame_arch (caller_frame);
>   
>     symtab_and_line sr_sal;
>     sr_sal.pc = gdbarch_addr_bits_remove (gdbarch,
> -					frame_unwind_caller_pc (next_frame));
> +					get_frame_pc (caller_frame));
>     sr_sal.section = find_pc_overlay (sr_sal.pc);
>     sr_sal.pspace = frame_unwind_program_space (next_frame);
>   
> -  insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
> -					frame_unwind_caller_id (next_frame));
> +  insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal, caller_frame_id);
>   }
>   
>   /* Insert a "longjmp-resume" breakpoint at PC.  This is used to set a