Re: [PATCH] Fix regression on aarch64-linux gdbserver

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

Hi Tom,

Sorry, missed this amidst all the other submissions.

On 4/22/24 14:51, Tom Tromey wrote:
> Commit 9a03f218 ("Fix gdb.base/watchpoint-unaligned.exp on aarch64")
> fixed a watchpoint bug in gdb -- but did not touch the corresponding
> code in gdbserver.
> 
> This patch moves the gdb code into gdb/nat, so that it can be shared
> with gdbserver, and then changes gdbserver to use it, fixing the bug.
> 
> This is yet another case where having a single back end would prevent
> bugs.

I agree.

> 
> I tested this using the AdaCore internal gdb testsuite.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29423

Thanks for catching this and for the refactoring.

Approved-By: Luis Machado <luis.machado@arm.com>

> ---
>  gdb/aarch64-nat.c              | 115 ---------------------------------
>  gdb/aarch64-nat.h              |   8 ---
>  gdb/nat/aarch64-hw-point.c     | 115 +++++++++++++++++++++++++++++++++
>  gdb/nat/aarch64-hw-point.h     |   8 +++
>  gdbserver/linux-aarch64-low.cc |  38 +----------
>  5 files changed, 126 insertions(+), 158 deletions(-)
> 
> diff --git a/gdb/aarch64-nat.c b/gdb/aarch64-nat.c
> index 894fb73095b..1ba9c4c19a7 100644
> --- a/gdb/aarch64-nat.c
> +++ b/gdb/aarch64-nat.c
> @@ -224,121 +224,6 @@ aarch64_remove_watchpoint (CORE_ADDR addr, int len, enum target_hw_bp_type type,
>    return ret;
>  }
>  
> -/* See aarch64-nat.h.  */
> -
> -bool
> -aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> -			      CORE_ADDR addr_trap, CORE_ADDR *addr_p)
> -{
> -  bool found = false;
> -  for (int phase = 0; phase <= 1; ++phase)
> -    for (int i = aarch64_num_wp_regs - 1; i >= 0; --i)
> -      {
> -	if (!(state->dr_ref_count_wp[i]
> -	      && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i])))
> -	  {
> -	    /* Watchpoint disabled.  */
> -	    continue;
> -	  }
> -
> -	const enum target_hw_bp_type type
> -	  = aarch64_watchpoint_type (state->dr_ctrl_wp[i]);
> -	if (type == hw_execute)
> -	  {
> -	    /* Watchpoint disabled.  */
> -	    continue;
> -	  }
> -
> -	if (phase == 0)
> -	  {
> -	    /* Phase 0: No hw_write.  */
> -	    if (type == hw_write)
> -	      continue;
> -	  }
> -	else
> -	  {
> -	    /* Phase 1: Only hw_write.  */
> -	    if (type != hw_write)
> -	      continue;
> -	  }
> -
> -	const unsigned int offset
> -	  = aarch64_watchpoint_offset (state->dr_ctrl_wp[i]);
> -	const unsigned int len
> -	  = aarch64_watchpoint_length (state->dr_ctrl_wp[i]);
> -	const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset;
> -	const CORE_ADDR addr_watch_aligned
> -	  = align_down (state->dr_addr_wp[i], AARCH64_HWP_MAX_LEN_PER_REG);
> -	const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i];
> -
> -	/* ADDR_TRAP reports the first address of the memory range
> -	   accessed by the CPU, regardless of what was the memory
> -	   range watched.  Thus, a large CPU access that straddles
> -	   the ADDR_WATCH..ADDR_WATCH+LEN range may result in an
> -	   ADDR_TRAP that is lower than the
> -	   ADDR_WATCH..ADDR_WATCH+LEN range.  E.g.:
> -
> -	   addr: |   4	 |   5	 |   6	 |   7	 |   8	 |
> -				 |---- range watched ----|
> -		 |----------- range accessed ------------|
> -
> -	   In this case, ADDR_TRAP will be 4.
> -
> -	   The access size also can be larger than that of the watchpoint
> -	   itself.  For instance, the access size of an stp instruction is 16.
> -	   So, if we use stp to store to address p, and set a watchpoint on
> -	   address p + 8, the reported ADDR_TRAP can be p + 8 (observed on
> -	   RK3399 SOC). But it also can be p (observed on M1 SOC).  Checking
> -	   for this situation introduces the possibility of false positives,
> -	   so we only do this for hw_write watchpoints.  */
> -	const CORE_ADDR max_access_size = type == hw_write ? 16 : 8;
> -	const CORE_ADDR addr_watch_base = addr_watch_aligned -
> -	  (max_access_size - AARCH64_HWP_MAX_LEN_PER_REG);
> -	if (!(addr_trap >= addr_watch_base
> -	      && addr_trap < addr_watch + len))
> -	  {
> -	    /* Not a match.  */
> -	    continue;
> -	  }
> -
> -	/* To match a watchpoint known to GDB core, we must never
> -	   report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN
> -	   range.  ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false
> -	   positive on kernels older than 4.10.  See PR
> -	   external/20207.  */
> -	if (addr_p != nullptr)
> -	  *addr_p = addr_orig;
> -
> -	if (phase == 0)
> -	  {
> -	    /* Phase 0: Return first match.  */
> -	    return true;
> -	  }
> -
> -	/* Phase 1.  */
> -	if (addr_p == nullptr)
> -	  {
> -	    /* First match, and we don't need to report an address.  No need
> -	       to look for other matches.  */
> -	    return true;
> -	  }
> -
> -	if (!found)
> -	  {
> -	    /* First match, and we need to report an address.  Look for other
> -	       matches.  */
> -	    found = true;
> -	    continue;
> -	  }
> -
> -	/* More than one match, and we need to return an address.  No need to
> -	   look for further matches.  */
> -	return false;
> -      }
> -
> -  return found;
> -}
> -
>  /* Define AArch64 maintenance commands.  */
>  
>  static void
> diff --git a/gdb/aarch64-nat.h b/gdb/aarch64-nat.h
> index 947d2805602..ec85524b2d4 100644
> --- a/gdb/aarch64-nat.h
> +++ b/gdb/aarch64-nat.h
> @@ -45,14 +45,6 @@ struct aarch64_debug_reg_state *aarch64_get_debug_reg_state (pid_t pid);
>  
>  void aarch64_remove_debug_reg_state (pid_t pid);
>  
> -/* Helper for the "stopped_data_address" target method.  Returns TRUE
> -   if a hardware watchpoint trap at ADDR_TRAP matches a set
> -   watchpoint.  The address of the matched watchpoint is returned in
> -   *ADDR_P.  */
> -
> -bool aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> -				   CORE_ADDR addr_trap, CORE_ADDR *addr_p);
> -
>  /* Helper functions used by aarch64_nat_target below.  See their
>     definitions.  */
>  
> diff --git a/gdb/nat/aarch64-hw-point.c b/gdb/nat/aarch64-hw-point.c
> index b62c4627d96..6acee0fb814 100644
> --- a/gdb/nat/aarch64-hw-point.c
> +++ b/gdb/nat/aarch64-hw-point.c
> @@ -645,3 +645,118 @@ aarch64_region_ok_for_watchpoint (CORE_ADDR addr, int len)
>       the checking is costly.  */
>    return 1;
>  }
> +
> +/* See nat/aarch64-hw-point.h.  */
> +
> +bool
> +aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> +			      CORE_ADDR addr_trap, CORE_ADDR *addr_p)
> +{
> +  bool found = false;
> +  for (int phase = 0; phase <= 1; ++phase)
> +    for (int i = aarch64_num_wp_regs - 1; i >= 0; --i)
> +      {
> +	if (!(state->dr_ref_count_wp[i]
> +	      && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i])))
> +	  {
> +	    /* Watchpoint disabled.  */
> +	    continue;
> +	  }
> +
> +	const enum target_hw_bp_type type
> +	  = aarch64_watchpoint_type (state->dr_ctrl_wp[i]);
> +	if (type == hw_execute)
> +	  {
> +	    /* Watchpoint disabled.  */
> +	    continue;
> +	  }
> +
> +	if (phase == 0)
> +	  {
> +	    /* Phase 0: No hw_write.  */
> +	    if (type == hw_write)
> +	      continue;
> +	  }
> +	else
> +	  {
> +	    /* Phase 1: Only hw_write.  */
> +	    if (type != hw_write)
> +	      continue;
> +	  }
> +
> +	const unsigned int offset
> +	  = aarch64_watchpoint_offset (state->dr_ctrl_wp[i]);
> +	const unsigned int len
> +	  = aarch64_watchpoint_length (state->dr_ctrl_wp[i]);
> +	const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset;
> +	const CORE_ADDR addr_watch_aligned
> +	  = align_down (state->dr_addr_wp[i], AARCH64_HWP_MAX_LEN_PER_REG);
> +	const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i];
> +
> +	/* ADDR_TRAP reports the first address of the memory range
> +	   accessed by the CPU, regardless of what was the memory
> +	   range watched.  Thus, a large CPU access that straddles
> +	   the ADDR_WATCH..ADDR_WATCH+LEN range may result in an
> +	   ADDR_TRAP that is lower than the
> +	   ADDR_WATCH..ADDR_WATCH+LEN range.  E.g.:
> +
> +	   addr: |   4	 |   5	 |   6	 |   7	 |   8	 |
> +				 |---- range watched ----|
> +		 |----------- range accessed ------------|
> +
> +	   In this case, ADDR_TRAP will be 4.
> +
> +	   The access size also can be larger than that of the watchpoint
> +	   itself.  For instance, the access size of an stp instruction is 16.
> +	   So, if we use stp to store to address p, and set a watchpoint on
> +	   address p + 8, the reported ADDR_TRAP can be p + 8 (observed on
> +	   RK3399 SOC). But it also can be p (observed on M1 SOC).  Checking
> +	   for this situation introduces the possibility of false positives,
> +	   so we only do this for hw_write watchpoints.  */
> +	const CORE_ADDR max_access_size = type == hw_write ? 16 : 8;
> +	const CORE_ADDR addr_watch_base = addr_watch_aligned -
> +	  (max_access_size - AARCH64_HWP_MAX_LEN_PER_REG);
> +	if (!(addr_trap >= addr_watch_base
> +	      && addr_trap < addr_watch + len))
> +	  {
> +	    /* Not a match.  */
> +	    continue;
> +	  }
> +
> +	/* To match a watchpoint known to GDB core, we must never
> +	   report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN
> +	   range.  ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false
> +	   positive on kernels older than 4.10.  See PR
> +	   external/20207.  */
> +	if (addr_p != nullptr)
> +	  *addr_p = addr_orig;
> +
> +	if (phase == 0)
> +	  {
> +	    /* Phase 0: Return first match.  */
> +	    return true;
> +	  }
> +
> +	/* Phase 1.  */
> +	if (addr_p == nullptr)
> +	  {
> +	    /* First match, and we don't need to report an address.  No need
> +	       to look for other matches.  */
> +	    return true;
> +	  }
> +
> +	if (!found)
> +	  {
> +	    /* First match, and we need to report an address.  Look for other
> +	       matches.  */
> +	    found = true;
> +	    continue;
> +	  }
> +
> +	/* More than one match, and we need to return an address.  No need to
> +	   look for further matches.  */
> +	return false;
> +      }
> +
> +  return found;
> +}
> diff --git a/gdb/nat/aarch64-hw-point.h b/gdb/nat/aarch64-hw-point.h
> index bdcca932e57..0d50eaab0de 100644
> --- a/gdb/nat/aarch64-hw-point.h
> +++ b/gdb/nat/aarch64-hw-point.h
> @@ -110,6 +110,14 @@ unsigned int aarch64_watchpoint_offset (unsigned int ctrl);
>  unsigned int aarch64_watchpoint_length (unsigned int ctrl);
>  enum target_hw_bp_type aarch64_watchpoint_type (unsigned int ctrl);
>  
> +/* Helper for the "stopped_data_address" target method.  Returns TRUE
> +   if a hardware watchpoint trap at ADDR_TRAP matches a set
> +   watchpoint.  The address of the matched watchpoint is returned in
> +   *ADDR_P.  */
> +
> +bool aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> +				   CORE_ADDR addr_trap, CORE_ADDR *addr_p);
> +
>  int aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
>  			       int len, int is_insert, ptid_t ptid,
>  			       struct aarch64_debug_reg_state *state);
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 5df67fccd08..da5c1fd0629 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -576,41 +576,9 @@ aarch64_target::low_stopped_data_address ()
>  
>    /* Check if the address matches any watched address.  */
>    state = aarch64_get_debug_reg_state (pid_of (current_thread));
> -  for (i = aarch64_num_wp_regs - 1; i >= 0; --i)
> -    {
> -      const unsigned int offset
> -	= aarch64_watchpoint_offset (state->dr_ctrl_wp[i]);
> -      const unsigned int len = aarch64_watchpoint_length (state->dr_ctrl_wp[i]);
> -      const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset;
> -      const CORE_ADDR addr_watch_aligned = align_down (state->dr_addr_wp[i], 8);
> -      const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i];
> -
> -      if (state->dr_ref_count_wp[i]
> -	  && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i])
> -	  && addr_trap >= addr_watch_aligned
> -	  && addr_trap < addr_watch + len)
> -	{
> -	  /* ADDR_TRAP reports the first address of the memory range
> -	     accessed by the CPU, regardless of what was the memory
> -	     range watched.  Thus, a large CPU access that straddles
> -	     the ADDR_WATCH..ADDR_WATCH+LEN range may result in an
> -	     ADDR_TRAP that is lower than the
> -	     ADDR_WATCH..ADDR_WATCH+LEN range.  E.g.:
> -
> -	     addr: |   4   |   5   |   6   |   7   |   8   |
> -				   |---- range watched ----|
> -		   |----------- range accessed ------------|
> -
> -	     In this case, ADDR_TRAP will be 4.
> -
> -	     To match a watchpoint known to GDB core, we must never
> -	     report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN
> -	     range.  ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false
> -	     positive on kernels older than 4.10.  See PR
> -	     external/20207.  */
> -	  return addr_orig;
> -	}
> -    }
> +  CORE_ADDR result;
> +  if (aarch64_stopped_data_address (state, addr_trap, &result))
> +    return result;
>  
>    return (CORE_ADDR) 0;
>  }