Re: [PATCH 1/2] [gdb/tdep] Fix gdb.base/watchpoint-unaligned.exp on aarch64

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

Hi Tom,

On 2/20/24 20:54, Tom de Vries wrote:
> On aarch64-linux, with test-case gdb.base/watchpoint-unaligned.exp I run into:
> ...
> (gdb) watch data.u.size8twice[1]^M
> Hardware watchpoint 241: data.u.size8twice[1]^M
> (gdb) PASS: gdb.base/watchpoint-unaligned.exp: watch data.u.size8twice[1]
> continue^M
> Continuing.^M
> FAIL: gdb.base/watchpoint-unaligned.exp: continue (timeout)
> FAIL: gdb.base/watchpoint-unaligned.exp: size8twice write
> ...
> 
> This happens as follows.
> 
> We start the exec and set an 8-byte hardware watchpoint on
> data.u.size8twice[1] at address 0x440048:
> ...
> (gdb) p sizeof (data.u.size8twice[1])
> $1 = 8
> (gdb) p &data.u.size8twice[1]
> $2 = (uint64_t *) 0x440048 <data+16>
> ...
> 
> We continue execution, and a 16-byte write at address 0x440040 triggers the
> hardware watchpoint:
> ...
>   4101c8:       a9000801        stp     x1, x2, [x0]
> ...
> 
> When checking whether a watchpoint has triggered in
> aarch64_stopped_data_address, we check against address 0x440040 (passed in
> parameter addr_trap).  This behaviour is documented:
> ...
> 	  /* ADDR_TRAP reports the first address of the memory range
> 	     accessed by the CPU, regardless of what was the memory
> 	     range watched.  ...  */
> ...
> and consequently the matching logic compares against an addr_watch_aligned:
> ...
> 	  && addr_trap >= addr_watch_aligned
> 	  && addr_trap < addr_watch + len)
> ...
> 
> However, the comparison fails:
> ...
> (gdb) p /x addr_watch_aligned
> $3 = 0x440048
> (gdb) p addr_trap >= addr_watch_aligned
> $4 = false
> ...
> because the computation of addr_watch_aligned doesn't take 16-byte memory
> accesses into account:
> ...
>     const CORE_ADDR addr_watch_aligned = align_down (state->dr_addr_wp[i], 8);
> ...
> 
> Consequently, aarch64_stopped_data_address returns false, and
> stopped_by_watchpoint returns false, and watchpoints_triggered returns 0,
> which make infrun think it's looking at a delayed hardware
> breakpoint/watchpoint trap:
> ...
>   [infrun] handle_signal_stop: stop_pc=0x4101c8
>   [infrun] handle_signal_stop: delayed hardware breakpoint/watchpoint trap, ignoring
> ...
> Infrun then ignores the trap and continues, but runs into the same situation
> again and again, causing a hang which then causes the test timeout.
> 
> Fix this by using 16 instead of 8 in the addr_watch_aligned computation, such
> that we have:
> ...
> (gdb) p /x addr_watch_aligned
> $2 = 0x440040
> (gdb) p addr_trap >= addr_watch_aligned
> $3 = true
> ...
> 
> PR tdep/29423
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29423
> ---
>  gdb/aarch64-nat.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-nat.c b/gdb/aarch64-nat.c
> index 2e8c0d80182..5d7d2be2827 100644
> --- a/gdb/aarch64-nat.c
> +++ b/gdb/aarch64-nat.c
> @@ -239,7 +239,8 @@ aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
>  	= 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_watch_aligned
> +	= align_down (state->dr_addr_wp[i], 16);
>        const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i];
>  
>        if (state->dr_ref_count_wp[i]
> 
> base-commit: 94a75b0363b1e09416e9bd24cac72d98864688d8

Raising the alignment enforcement means we will cover a bigger range of addresses, potentially covering our target watchpoint/trap address,
but I'm afraid it also means we will raise the potential for false positives, if watchpoints are placed within the alignment range.

Furthermore, we are not limited to 16-byte accesses. For SVE and SME we may be looking at even bigger accesses. And, more generally, the memset/memcpy
instructions (not yet widely used) can potentially access arbitrary amounts of memory. So tweaking the alignment is only a focused fix towards the most often
used instructions and access sizes at the moment.

The more general problem of not being able to tell which particular watchpoint caused the trap remains.

How does the above fix behave on the overall testsuite in terms of watchpoint tests?