Re: [PATCHv3 3/6] gdb: select suitable thread for gdbarch_adjust_breakpoint_address

[Pedro Alves <pedro@palves.net>]

On 2022-06-13 17:15, Andrew Burgess via Gdb-patches wrote:

> ---
>  gdb/arm-tdep.c   | 12 ++++++++----
>  gdb/breakpoint.c | 24 ++++++++++++++++++------
>  2 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 7f27d4bd6e8..e0e5c7efd63 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -541,20 +541,24 @@ arm_is_thumb (struct regcache *regcache)
>    return (cpsr & t_bit) != 0;
>  }
>  
> -/* Determine if FRAME is executing in Thumb mode.  */
> +/* Determine if FRAME is executing in Thumb mode.  FRAME must be an ARM
> +   frame.  */
>  
>  int
>  arm_frame_is_thumb (struct frame_info *frame)
>  {
> -  CORE_ADDR cpsr;
> -  ULONGEST t_bit = arm_psr_thumb_bit (get_frame_arch (frame));
> +  /* Check the architecture of FRAME.  */
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  gdb_assert (gdbarch_bfd_arch_info (target_gdbarch ())->arch == bfd_arch_arm);

I think you wanted instead:

  gdb_assert (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_arm);

?

>  
>    /* Every ARM frame unwinder can unwind the T bit of the CPSR, either
>       directly (from a signal frame or dummy frame) or by interpreting
>       the saved LR (from a prologue or DWARF frame).  So consult it and
>       trust the unwinders.  */
> -  cpsr = get_frame_register_unsigned (frame, ARM_PS_REGNUM);
> +  CORE_ADDR cpsr = get_frame_register_unsigned (frame, ARM_PS_REGNUM);
>  
> +  /* Find and extract the thumb bit.  */
> +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
>    return (cpsr & t_bit) != 0;
>  }

>  static CORE_ADDR
>  adjust_breakpoint_address (struct gdbarch *gdbarch,
> -			   CORE_ADDR bpaddr, enum bptype bptype)
> +			   CORE_ADDR bpaddr, enum bptype bptype,
> +			   struct program_space *pspace)
>  {
> +  gdb_assert (pspace != nullptr);
> +
>    if (bptype == bp_watchpoint
>        || bptype == bp_hardware_watchpoint
>        || bptype == bp_read_watchpoint
> @@ -7140,10 +7144,16 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>      {
>        CORE_ADDR adjusted_bpaddr = bpaddr;
>  
> +      /* Some targets have architectural constraints on the placement
> +	 of breakpoint instructions.  Obtain the adjusted address.  */
>        if (gdbarch_adjust_breakpoint_address_p (gdbarch))
>  	{
> -	  /* Some targets have architectural constraints on the placement
> -	     of breakpoint instructions.  Obtain the adjusted address.  */
> +	  /* Targets that implement this adjustment function will
> +	     likely inspect either the symbol table, or target memory
> +	     add BPADDR, so ensure a suitable thread (and its

s/add BPADDR/at BPADDR/, I think.

Note the comment as is doesn't justify why switch to a thread, as opposed
to just switching the inferior (e.g., with switch_to_inferior_no_thread),
as you can read memory without a thread selected (well, modulo a bug
in linux-nat.c).  We want a thread here for the arm fallback.

I'd suggest extending the comment in that direction, like:

  "... or target memory at BPADDR, or even state registers."

> +	     associated program space) are currently selected.  */
> +	  scoped_restore_current_pspace_and_thread restore_pspace_thread;
> +	  switch_to_program_space_and_thread (pspace);
>  	  adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
>  	}
Otherwise LGTM.