Re: [PATCH][AArch64] Fix missing watchpoint hits/endless loop

[Luis Machado <luis.machado@linaro.org>]

On 6/2/21 1:57 PM, Alan Hayward wrote:
> 
> 
>> On 2 Jun 2021, at 15:27, Luis Machado <luis.machado@linaro.org> wrote:
>>
>> I ran into a situation where a hardware watchpoint hit is not detected
>> correctly, misleading GDB into thinking it was a delayed breakpoint hit.
>>
>> The problem is that hardware watchpoints are not skippable on AArch64, so
>> that makes GDB loop endlessly trying to run past the instruction.
>>
>> The most obvious case where this happens is when the load/store pair
>> instructions access 16 bytes of memory.
>>
>> Suppose we have a stp instruction that will write a couple 64-bit registers
>> to address 0x10 (stp x3,x4 [x2]). It will write data from 0x10 up to 0x20.
>>
>> Now suppose a write watchpoint is created to monitor memory address 0x18,
>> which is the start of the second register write. It can have whatever length,
>> but let's assume it has length 8.
>>
>> When we execute that stp instruction, it will trap and the reported stopped
>> data address from the kernel will be 0x10 (the beginning of the memory range
>> accessed by the instruction).
>>
>> The current code won't be able to detect a valid trigger because it assumes an
>> alignment of 8 bytes for the watchpoint address. Forcing that kind of alignment
>> won't be enough to detect a 16-byte access if the trap address falls outside of
>> the 8-byte alignment window. We need to know how many bytes the instruction
>> will access, but we won't have that data unless we go parsing instructions.
>>
>> Another issue with the current code seems to be that it assumes the accesses
>> will always be 8 bytes in size, since it wants to align the watchpoint address
>> to that particular boundary. This leads to problems when we have unaligned
>> addresses and unaligned watchpoints.
>>
>> For example, suppose we have a str instruction storing 8 bytes to memory
>> address 0xf. Now suppose we have a write watchpoint at address 0x16,
>> monitoring 8 bytes.
>>
>> The trap address will be 0xf, but forcing 0x16 to 8-byte alignment yields
>> 0x10, and so GDB doesn't think this is a watchpoint hit.
> 
> Ouch.
> I’m starting to wonder now if I have encountered this bug myself, and just
> shrugged it off thinking the variable wasn’t being hit.
> 

One more detail worth mentioning is that different CPU's may report trap 
addresses that are more or less precise.

D05's Cortex A72's are precise, while Ampere's Altra and Cavium's 
ThunderX2 CPU's report the trap address from the beginning of the memory 
range, so that's less precise, but still valid according to the spec.

>>
>> I believe you can trigger the same problem with smaller memory accesses,
>> except one that accesses a single byte.
>>
>> As I said earlier, ideally we'd go parsing instructions to figure out how many
>> bytes they access. That is a bit complex though, so meanwhile I think we should
>> go with a simpler solution.
>>
>> We keep the assumption that the instructions access 8 bytes, except when we
>> detect one of the instructions that access 16 bytes.
> 
> What about NEON and SVE instructions? They can access a lot of data at once.
> 

I haven't written a test for those yet, but my guess is that our 
handling of watchpoint triggers caused by those instructions is not 
entirely correct either. If the trap address falls outside of the 
expected alignment window, we won't notice the hit.

>>
>> For the trigger detection, instead of forcing an alignment and then checking
>> if the trap address falls within the range of the watchpoint, we just check
>> if the memory access and the watchpoint range overlap. If they do, then we
>> have a watchpoint hit.
>>
>> This still has potential to give false positives if we choose the right
>> combination of memory address, access size and watchpoint address. But it
>> should match the old code in this regard, while giving better results for
>> accesses of 16 bytes and unaligned accesses.
> 
> Is there a plan for fixing those?
> 

There is, but I wanted to at least get this patch out before GDB 11 
branches. That ought to improve things for CPU's that don't report a 
precise trap address.

I think the best course of action is to find a fast way to determine how 
many bytes a particular instruction accesses. Given the differences in 
encoding between regular ld/st instructions, NEON and SVE variants, I'll 
need to come up with a more thorough testcase to make sure we're dealing 
with those correctly.

>>
>> It also fixes these two failures in the testsuite:
>>
>> FAIL: gdb.base/watchpoint-unaligned.exp: continue (timeout)
>> FAIL: gdb.base/watchpoint-unaligned.exp: size8twice write
>>
>> Regression tested on aarch64-linux Ubuntu/20.04.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* aarch64-linux-nat.c
>> 	(aarch64_linux_nat_target::stopped_data_address): Refactor.
>> 	* nat/aarch64-linux-hw-point.c (hw_watch_is_16b_ldst)
>> 	(hw_watch_regions_overlap, hw_watch_detect_trigger): New functions.
>> 	* nat/aarch64-linux-hw-point.h (LDP_STP_MASK, STP_OPC_64)
>> 	(LDP_OPC_64): New constants.
>> 	(hw_watch_is_16b_ldst, hw_watch_regions_overlap)
>> 	(hw_watch_detect_trigger): New prototypes.
>>
>> gdbserver/ChangeLog:
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* linux-aarch64-low.cc
>> 	(aarch64_target::low_stopped_data_address): Refactor.
>> ---
>> gdb/aarch64-linux-nat.c          |  45 ++----------
>> gdb/nat/aarch64-linux-hw-point.c | 120 +++++++++++++++++++++++++++++++
>> gdb/nat/aarch64-linux-hw-point.h |  16 +++++
>> gdbserver/linux-aarch64-low.cc   |  49 ++++---------
>> 4 files changed, 154 insertions(+), 76 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 61224022f6a..de405a5e351 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -951,7 +951,6 @@ bool
>> aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
>> {
>>    siginfo_t siginfo;
>> -  int i;
>>    struct aarch64_debug_reg_state *state;
>>
>>    if (!linux_nat_get_siginfo (inferior_ptid, &siginfo))
>> @@ -969,46 +968,14 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
>>    const CORE_ADDR addr_trap
>>      = address_significant (gdbarch, (CORE_ADDR) siginfo.si_addr);
>>
>> +  struct regcache *regs = get_thread_regcache (this, inferior_ptid);
>> +  CORE_ADDR trigger_pc = regcache_read_pc (regs);
>> +  uint32_t insn;
>> +  read_memory (trigger_pc, (gdb_byte *) &insn, 4);
>> +
>>    /* Check if the address matches any watched address.  */
>>    state = aarch64_get_debug_reg_state (inferior_ptid.pid ());
>> -  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.  */
>> -	  *addr_p = addr_orig;
>> -	  return true;
>> -	}
>> -    }
>> -
>> -  return false;
>> +  return hw_watch_detect_trigger (state, insn, addr_trap, addr_p);
>> }
>>
>> /* Implement the "stopped_by_watchpoint" target_ops method.  */
>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>> index af2cc4254e2..32ed76157ac 100644
>> --- a/gdb/nat/aarch64-linux-hw-point.c
>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>> @@ -865,3 +865,123 @@ aarch64_linux_region_ok_for_watchpoint (CORE_ADDR addr, int len)
>>       the checking is costly.  */
>>    return 1;
>> }
>> +
>> +/* Return true if INSN is a ldp/stp that accesses 16 bytes of memory.
>> +   Return false otherwise.  */
>> +
>> +bool
>> +hw_watch_is_16b_ldst (CORE_ADDR insn)
>> +{
>> +  return ((insn & LDP_STP_MASK) == STP_OPC_64
>> +	  || (insn & LDP_STP_MASK) == LDP_OPC_64);
>> +}
>> +
>> +/* Return true if the regions [mem_addr, mem_addr + mem_len] and
>> +   [watch_addr, watch_addr + watch_len] overlap.  False otherwise.  */
>> +
>> +bool
>> +hw_watch_regions_overlap (CORE_ADDR mem_addr, size_t mem_len,
>> +			  CORE_ADDR watch_addr, size_t watch_len)
>> +{
>> +  if (watch_addr > (mem_addr + mem_len)
>> +      || mem_addr > (watch_addr + watch_len))
>> +    return false;
>> +
>> +  CORE_ADDR start = std::max (mem_addr, watch_addr);
>> +  CORE_ADDR end = std::min (mem_addr + mem_len, watch_addr + watch_len);
>> +
>> +  return ((end - start) > 0);
>> +}
>> +
>> +/* Check if a hardware watchpoint has triggered.  If a trigger is detected,
>> +   return true and update ADDR_P with the stopped data address.
>> +   Otherwise return false.
>> +
>> +   STATE is the debug register's state, INSN is the instruction the inferior
>> +   stopped at and ADDR_TRAP is the reported stopped data address.  */
>> +
>> +bool
>> +hw_watch_detect_trigger (const struct aarch64_debug_reg_state *state,
>> +			 CORE_ADDR insn, CORE_ADDR addr_trap,
>> +			 CORE_ADDR *addr_p)
>> +{
>> +  /* There are 6 variations of watchpoint range and memory access
>> +     range positioning:
>> +
>> +     - W is the byte in the watchpoint range only.
>> +
>> +     - M is the byte in the memory access range ony.
>> +
>> +     - O is the byte in the overlapping region of the watchpoint range and
>> +       the memory access range.
>> +
>> +     1 - Non-overlapping, no triggers.
>> +
>> +     [WWWWWWWW]...[MMMMMMMM]
>> +
>> +     2 - Non-overlapping, no triggers.
>> +
>> +     [MMMMMMMM]...[WWWWWWWW]
>> +
>> +     3 - Overlapping partially, triggers.
>> +
>> +     [MMMMOOOOWWWW]
>> +
>> +     4 - Overlapping partially, triggers.
>> +
>> +     [WWWWOOOOMMMM]
>> +
>> +     5 - Memory access contained in watchpoint range, triggers.
>> +
>> +     [WWWWOOOOOOOOWWWW]
>> +
>> +     6 - Memory access containing watchpoint range, triggers.
>> +
>> +     [MMMMOOOOOOOOMMMM]
> 
> Very minor nit: M looks like an upside down W, so there was an initial slight confusion.
> Not worth changing, because M and W are the right letter choices.
> 

I wonder if we can improve that in some way. Maybe name it B for "Memory 
Byte"?

>> +  */
>> +
>> +  /* We assume the memory access size is 8 bytes.  */
>> +  unsigned int memory_access_size = 8;
>> +
>> +  /* Check if the memory access size is 16 bytes.  */
>> +  if (hw_watch_is_16b_ldst (insn))
>> +    memory_access_size = 16;
>> +
>> +  for (int 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_orig = state->dr_addr_orig_wp[i];
>> +
>> +      if ((state->dr_ref_count_wp[i]
>> +	  && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]))
>> +	  && hw_watch_regions_overlap (addr_trap, memory_access_size,
>> +				       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.  */
>> +	  *addr_p = addr_orig;
>> +	  return true;
>> +	}
>> +    }
>> +
>> +  return false;
>> +}
>> diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h
>> index 2fc4b400ece..62e755dd1e9 100644
>> --- a/gdb/nat/aarch64-linux-hw-point.h
>> +++ b/gdb/nat/aarch64-linux-hw-point.h
>> @@ -132,6 +132,13 @@ typedef ULONGEST dr_changed_t;
>> #define DR_HAS_CHANGED(x) ((x) != 0)
>> #define DR_N_HAS_CHANGED(x, n) ((x) & ((dr_changed_t)1 << (n)))
>>
>> +/* Mask for matching LDP and STP instruction variants.  */
>> +#define LDP_STP_MASK  0xFE400000
>> +/* stp and stnp with 64-bit registers.  */
>> +#define STP_OPC_64    0xA8000000
>> +/* ldp and ldnp with 64-bit registers.  */
>> +#define LDP_OPC_64    0xA8400000
>> +
>> /* Structure for managing the hardware breakpoint/watchpoint resources.
>>     DR_ADDR_* stores the address, DR_CTRL_* stores the control register
>>     content, and DR_REF_COUNT_* counts the numbers of references to the
>> @@ -197,4 +204,13 @@ struct aarch64_debug_reg_state *aarch64_get_debug_reg_state (pid_t pid);
>>
>> int aarch64_linux_region_ok_for_watchpoint (CORE_ADDR addr, int len);
>>
>> +bool hw_watch_is_16b_ldst (CORE_ADDR insn);
>> +
>> +bool hw_watch_regions_overlap (CORE_ADDR mem_addr, size_t mem_len,
>> +			       CORE_ADDR watch_addr, size_t watch_len);
>> +
>> +bool hw_watch_detect_trigger (const struct aarch64_debug_reg_state *state,
>> +			      CORE_ADDR insn, CORE_ADDR stopped_data_address,
>> +			      CORE_ADDR *addr_p);
>> +
>> #endif /* NAT_AARCH64_LINUX_HW_POINT_H */
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index daccfef746e..5df632fe724 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -518,7 +518,7 @@ CORE_ADDR
>> aarch64_target::low_stopped_data_address ()
>> {
>>    siginfo_t siginfo;
>> -  int pid, i;
>> +  int pid;
>>    struct aarch64_debug_reg_state *state;
>>
>>    pid = lwpid_of (current_thread);
>> @@ -538,45 +538,20 @@ aarch64_target::low_stopped_data_address ()
>>    const CORE_ADDR addr_trap
>>      = address_significant ((CORE_ADDR) siginfo.si_addr);
>>
>> +  struct regcache *regs = get_thread_regcache (current_thread, 1);
>> +  CORE_ADDR trigger_pc = regcache_read_pc (regs);
>> +  uint32_t insn;
>> +  read_memory (trigger_pc, (gdb_byte *) &insn, 4);
>> +
>>    /* 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;
>> -	}
>> -    }
>>
>> -  return (CORE_ADDR) 0;
>> +  CORE_ADDR trigger_addr;
>> +
>> +  if (hw_watch_detect_trigger (state, insn, addr_trap, &trigger_addr))
>> +    return trigger_addr;
>> +
>> +  return 0;
>> }
>>
>> /* Implementation of linux target ops method "low_stopped_by_watchpoint".  */
>> -- 
>> 2.25.1
>>
>