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

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

Hi!

On 6/13/22 17:15, Andrew Burgess wrote:
> The three targets that implement gdbarch_adjust_breakpoint_address are
> arm, frv, and mips.  In each of these targets the adjust breakpoint
> address function does some combination of reading the symbol table, or
> reading memory at the location the breakpoint could be placed.
> 
> The problem is that performing these actions requires that the current
> inferior and program space be the one in which the breakpoint will be
> placed, and this is not currently always the case.
> 
> Consider a GDB session with multiple inferiors.  One inferior might be
> a native target while another could be a remote target of a completely
> different architecture.  Alternatively, if we consider ARM and
> AArch64, one native inferior might be AArch64, while a second native
> inferior could be ARM.
> 
> In these cases it is possible, and valid, for a user to have one
> inferior selected, and place a breakpoint in the other inferior by
> placing a breakpoint on a particular symbol.
> 
> If this happens, then currently, when
> gdbarch_adjust_breakpoint_address is called, the wrong inferior (and
> program space) will be selected, and memory reads, and symbol look
> ups, will not return the expected results, this could lead to
> breakpoints being placed in the wrong location.
> 
> There are currently two places where gdbarch_adjust_breakpoint_address
> is called:
> 
>    1. In infrun.c, in the function handle_step_into_function.  In this
>    case, I believe that the correct inferior and program space will
>    already be selected as this is called as part of the stop event
>    handling, so I don't think we need to worry about this case, and
> 
>    2. In breakpoint.c, in the function adjust_breakpoint_address, which
>    is itself called from code_breakpoint::add_location and
>    watch_command_1.
> 
>    The watch_command_1 case I don't think we need to worry about, this
>    is for when a local watch expression is created, which can only be
>    in the currently selected inferior, so this case should be fine.
> 
>    The code_breakpoint::add_location case is the one that needs fixing,
>    this is what allows a breakpoint to be created between inferiors.
> 
> To fix the code_breakpoint::add_location case, I propose that we pass
> the "correct" program_space (i.e. the program space in which the
> breakpoint will be created) to the adjust_breakpoint_address function.
> Then in adjust_breakpoint_address we can make use of
> switch_to_program_space_and_thread to switch program_space and
> inferior before calling gdbarch_adjust_breakpoint_address.
> 
> I discovered this issue while working on a later patch in this
> series.  This later patch will detect when we cast the result of
> gdbarch_tdep to the wrong type.
> 
> With this later patch in place I ran gdb.multi/multi-arch.exp on an
> AArch64 target.  In this situation, two inferiors are created, an
> AArch64 inferior, and an ARM inferior.  The test selected the AArch64
> inferior and tries to create a breakpoint in the ARM inferior.
> 
> As a result of this we end up in arm_adjust_breakpoint_address, which
> calls arm_pc_is_thumb.  Before this commit the AArch64 inferior would
> be current.  As a result, all of the checks in arm_pc_is_thumb would
> fail (they rely on reading symbols from the current program space),
> and so, at the end of arm_pc_is_thumb we would call
> arm_frame_is_thumb.  However, remember, at this point the current
> inferior is the AArch64 inferior, so the current frame is an AArch64
> frame.
> 
> In arm_frame_is_thumb we call arm_psr_thumb_bit, which calls
> gdbarch_tdep and casts the result to arm_gdbarch_tdep.  This is wrong,
> the tdep field is of type aarch64_gdbarch_tdep.  After this we have
> undefined behaviour.
> 
> With this patch in place, we will have switched to a thread in the ARM
> program space before calling arm_adjust_breakpoint_address.  As a
> result, we now succeed in looking up the required symbols in
> arm_pc_is_thumb, and so we never call arm_frame_is_thumb.
> 
> However, in the worst case scenario, if we did end up calling
> arm_frame_is_thumb, as the current inferior should now be the ARM
> inferior, the current frame should be an ARM frame, so we still should
> not hit undefined behaviour.
> 
> I have added an assert to arm_frame_is_thumb.
> ---
>   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);
>   
>     /* 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;
>   }
>   
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index ed932a19ed7..5370e70f15e 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -124,7 +124,8 @@ static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
>   
>   static CORE_ADDR adjust_breakpoint_address (struct gdbarch *gdbarch,
>   					    CORE_ADDR bpaddr,
> -					    enum bptype bptype);
> +					    enum bptype bptype,
> +					    struct program_space *pspace);
>   
>   static int watchpoint_locations_match (struct bp_location *loc1,
>   				       struct bp_location *loc2);
> @@ -7114,8 +7115,11 @@ breakpoint_adjustment_warning (CORE_ADDR from_addr, CORE_ADDR to_addr,
>   
>   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

Is there something funny in the above line? Should add->and?

Other than this, LGTM.

Again, thanks for taking the time to address this.