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

[Andrew Burgess <aburgess@redhat.com>]

Luis Machado <luis.machado@arm.com> writes:

> 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?
>

Yeah, that doesn't seem right.  I think I meant to see 'target memory at
BPADDR'.

The patch below has this fix, nothing else changed.

Thanks,
Andrew

---

commit b72622f7f8c979ebf2f923fe3f8bfb304843c2d3
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Jun 13 14:34:01 2022 +0100

    gdb: select suitable thread for gdbarch_adjust_breakpoint_address
    
    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.

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..4b8e2d66267 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,11 +7144,18 @@ 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.  */
-	  adjusted_bpaddr = gdbarch_adjust_breakpoint_address (gdbarch, bpaddr);
+	  /* Targets that implement this adjustment function will likely
+	     inspect either the symbol table, or target memory at BPADDR,
+	     so ensure a suitable thread (and its 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);
 	}
 
       adjusted_bpaddr = address_significant (gdbarch, adjusted_bpaddr);
@@ -8098,7 +8109,8 @@ code_breakpoint::add_location (const symtab_and_line &sal)
      not want its scan of the location chain to find a breakpoint and
      location that's only been partially initialized.  */
   adjusted_address = adjust_breakpoint_address (loc_gdbarch,
-						sal.pc, type);
+						sal.pc, type,
+						sal.pspace);
 
   /* Sort the locations by their ADDRESS.  */
   new_loc = allocate_location ();
@@ -10085,7 +10097,8 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
 	  scope_breakpoint->loc->address
 	    = adjust_breakpoint_address (scope_breakpoint->loc->gdbarch,
 					 scope_breakpoint->loc->requested_address,
-					 scope_breakpoint->type);
+					 scope_breakpoint->type,
+					 current_program_space);
 	}
     }