[v2,gdb/tdep] Fix gdb.base/watchpoint-unaligned.exp on aarch64
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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
...
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 allowing a match 8 bytes below addr_watch_aligned. This
introduces the possibility for false positives, so we only do this for regular
"value changed" watchpoints.
An earlier version of this patch worked by aligning addr_watch_aligned to 16
instead of 8:
...
- 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);
...
but while that fixed the test-case, it didn't fix the problem completely, so
extend the test-case to check more scenarios.
Tested on aarch64-linux.
PR tdep/29423
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29423
---
gdb/aarch64-nat.c | 17 +++-
gdb/testsuite/gdb.base/watchpoint-unaligned.c | 11 +--
.../gdb.base/watchpoint-unaligned.exp | 78 ++++++++++++-------
3 files changed, 68 insertions(+), 38 deletions(-)
base-commit: 4810a2d92b9e1a13774c1286cd8a0f718f81abba
Comments
Hi Tom,
On 3/13/24 16:31, 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
> ...
>
> 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 allowing a match 8 bytes below addr_watch_aligned. This
> introduces the possibility for false positives, so we only do this for regular
> "value changed" watchpoints.
>
> An earlier version of this patch worked by aligning addr_watch_aligned to 16
> instead of 8:
> ...
> - 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);
> ...
> but while that fixed the test-case, it didn't fix the problem completely, so
> extend the test-case to check more scenarios.
>
> Tested on aarch64-linux.
>
> PR tdep/29423
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29423
> ---
> gdb/aarch64-nat.c | 17 +++-
> gdb/testsuite/gdb.base/watchpoint-unaligned.c | 11 +--
> .../gdb.base/watchpoint-unaligned.exp | 78 ++++++++++++-------
> 3 files changed, 68 insertions(+), 38 deletions(-)
>
> diff --git a/gdb/aarch64-nat.c b/gdb/aarch64-nat.c
> index 6c72a8d6d9f..802bab6d682 100644
> --- a/gdb/aarch64-nat.c
> +++ b/gdb/aarch64-nat.c
> @@ -269,7 +269,7 @@ aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> = 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);
> + = 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
> @@ -283,8 +283,19 @@ aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
> |---- range watched ----|
> |----------- range accessed ------------|
>
> - In this case, ADDR_TRAP will be 4. */
> - if (!(addr_trap >= addr_watch_aligned
> + 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. */
> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.c b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
> index 64728bb9ea3..6f709259b6c 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-unaligned.c
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
> @@ -29,7 +29,7 @@ static volatile struct
> uint32_t size4[2];
> uint16_t size2[4];
> uint8_t size1[8];
> - uint64_t size8twice[2];
> + uint64_t size8twice[3];
> }
> u;
> } data;
> @@ -44,13 +44,14 @@ write_size8twice (void)
> static const uint64_t second = 2;
>
> #ifdef __aarch64__
> + volatile void *p = &data.u.size8twice[offset];
> asm volatile ("stp %1, %2, [%0]"
> : /* output */
> - : "r" (data.u.size8twice), "r" (first), "r" (second) /* input */
> + : "r" (p), "r" (first), "r" (second) /* input */
> : "memory" /* clobber */);
> #else
> - data.u.size8twice[0] = first;
> - data.u.size8twice[1] = second;
> + data.u.size8twice[offset] = first;
> + data.u.size8twice[offset + 1] = second;
> #endif
> }
>
> @@ -59,7 +60,7 @@ main (void)
> {
> volatile uint64_t local;
>
> - assert (sizeof (data) == 8 + 2 * 8);
> + assert (sizeof (data) == 8 + 3 * 8);
>
> write_size8twice ();
>
> diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> index 8d985c03bfc..35e8868d39d 100644
> --- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -151,38 +151,56 @@ foreach wpcount {4 7} {
> gdb_assert $got_hit $test
> }
>
> -if ![runto_main] {
> - return -1
> -}
> -gdb_breakpoint [gdb_get_line_number "final_return"] "Breakpoint $decimal at $hex" "final_return"
> -set test {watch data.u.size8twice[1]}
> -set wpnum 0
> -gdb_test_multiple $test $test {
> - -re "Hardware watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
> - set wpnum $expect_out(1,string)
> - pass $gdb_test_name
> - }
> - -re "Watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
> - if {[istarget "arm*-*-*"]} {
> - untested $gdb_test_name
> - } else {
> - fail $gdb_test_name
> - }
> - }
> -}
> -if {$wpnum} {
> - set test "continue"
> - set got_hit 0
> - gdb_test_multiple $test $test {
> - -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
> +# We've got an array with 3 8-byte elements. Do a store of 16 bytes,
> +# to:
> +# - elements 0 and 1 (offset == 0), and
> +# - elements 1 and 2 (offset == 1).
> +# For each case, check setting a watchpoint at:
> +# - the first written element (index == 0), and
> +# - the second element (index == 1).
> +foreach_with_prefix offset { 0 1 } {
> + foreach_with_prefix index { 0 1 } {
> +
> + clean_restart $binfile
> +
> + if ![runto_main] {
> + return -1
> }
> - -re "Hardware watchpoint $wpnum:.*New value = .*\r\n$gdb_prompt $" {
> - set got_hit 1
> - send_gdb "continue\n"
> - exp_continue
> +
> + gdb_test_no_output "set var offset = $offset"
> + gdb_breakpoint [gdb_get_line_number "final_return"] \
> + "Breakpoint $decimal at $hex" "final_return"
> + set watch_index [expr $offset + $index]
> + set test "watch data.u.size8twice\[$watch_index\]"
> + set wpnum 0
> + gdb_test_multiple $test $test {
> + -re "Hardware watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
> + set wpnum $expect_out(1,string)
> + pass $gdb_test_name
> + }
> + -re "Watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
> + if {[istarget "arm*-*-*"]} {
> + untested $gdb_test_name
> + } else {
> + fail $gdb_test_name
> + }
> + }
> }
> - -re " final_return .*\r\n$gdb_prompt $" {
> + if {$wpnum} {
> + set test "continue"
> + set got_hit 0
> + gdb_test_multiple $test $test {
> + -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
> + }
> + -re "Hardware watchpoint $wpnum:.*New value = .*\r\n$gdb_prompt $" {
> + set got_hit 1
> + send_gdb "continue\n"
> + exp_continue
> + }
> + -re " final_return .*\r\n$gdb_prompt $" {
> + }
> + }
> + gdb_assert $got_hit "size8twice write"
> }
> }
> - gdb_assert $got_hit "size8twice write"
> }
>
> base-commit: 4810a2d92b9e1a13774c1286cd8a0f718f81abba
I tried this on my end and didn't see any regressions. Then again, my system behaves slightly different, and
I don't get a FAIL if a run the patched testcase with an unpatched gdb.
This is a good improvement nonetheless. At least until we can get more precise information about hardware
watchpoint events.
Thanks.
Tested-By: Luis Machado <luis.machado@arm.com>
Approved-By: Luis Machado <luis.machado@arm.com>
@@ -269,7 +269,7 @@ aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
= 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);
+ = 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
@@ -283,8 +283,19 @@ aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state,
|---- range watched ----|
|----------- range accessed ------------|
- In this case, ADDR_TRAP will be 4. */
- if (!(addr_trap >= addr_watch_aligned
+ 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. */
@@ -29,7 +29,7 @@ static volatile struct
uint32_t size4[2];
uint16_t size2[4];
uint8_t size1[8];
- uint64_t size8twice[2];
+ uint64_t size8twice[3];
}
u;
} data;
@@ -44,13 +44,14 @@ write_size8twice (void)
static const uint64_t second = 2;
#ifdef __aarch64__
+ volatile void *p = &data.u.size8twice[offset];
asm volatile ("stp %1, %2, [%0]"
: /* output */
- : "r" (data.u.size8twice), "r" (first), "r" (second) /* input */
+ : "r" (p), "r" (first), "r" (second) /* input */
: "memory" /* clobber */);
#else
- data.u.size8twice[0] = first;
- data.u.size8twice[1] = second;
+ data.u.size8twice[offset] = first;
+ data.u.size8twice[offset + 1] = second;
#endif
}
@@ -59,7 +60,7 @@ main (void)
{
volatile uint64_t local;
- assert (sizeof (data) == 8 + 2 * 8);
+ assert (sizeof (data) == 8 + 3 * 8);
write_size8twice ();
@@ -151,38 +151,56 @@ foreach wpcount {4 7} {
gdb_assert $got_hit $test
}
-if ![runto_main] {
- return -1
-}
-gdb_breakpoint [gdb_get_line_number "final_return"] "Breakpoint $decimal at $hex" "final_return"
-set test {watch data.u.size8twice[1]}
-set wpnum 0
-gdb_test_multiple $test $test {
- -re "Hardware watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
- set wpnum $expect_out(1,string)
- pass $gdb_test_name
- }
- -re "Watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
- if {[istarget "arm*-*-*"]} {
- untested $gdb_test_name
- } else {
- fail $gdb_test_name
- }
- }
-}
-if {$wpnum} {
- set test "continue"
- set got_hit 0
- gdb_test_multiple $test $test {
- -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
+# We've got an array with 3 8-byte elements. Do a store of 16 bytes,
+# to:
+# - elements 0 and 1 (offset == 0), and
+# - elements 1 and 2 (offset == 1).
+# For each case, check setting a watchpoint at:
+# - the first written element (index == 0), and
+# - the second element (index == 1).
+foreach_with_prefix offset { 0 1 } {
+ foreach_with_prefix index { 0 1 } {
+
+ clean_restart $binfile
+
+ if ![runto_main] {
+ return -1
}
- -re "Hardware watchpoint $wpnum:.*New value = .*\r\n$gdb_prompt $" {
- set got_hit 1
- send_gdb "continue\n"
- exp_continue
+
+ gdb_test_no_output "set var offset = $offset"
+ gdb_breakpoint [gdb_get_line_number "final_return"] \
+ "Breakpoint $decimal at $hex" "final_return"
+ set watch_index [expr $offset + $index]
+ set test "watch data.u.size8twice\[$watch_index\]"
+ set wpnum 0
+ gdb_test_multiple $test $test {
+ -re "Hardware watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
+ set wpnum $expect_out(1,string)
+ pass $gdb_test_name
+ }
+ -re "Watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
+ if {[istarget "arm*-*-*"]} {
+ untested $gdb_test_name
+ } else {
+ fail $gdb_test_name
+ }
+ }
}
- -re " final_return .*\r\n$gdb_prompt $" {
+ if {$wpnum} {
+ set test "continue"
+ set got_hit 0
+ gdb_test_multiple $test $test {
+ -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
+ }
+ -re "Hardware watchpoint $wpnum:.*New value = .*\r\n$gdb_prompt $" {
+ set got_hit 1
+ send_gdb "continue\n"
+ exp_continue
+ }
+ -re " final_return .*\r\n$gdb_prompt $" {
+ }
+ }
+ gdb_assert $got_hit "size8twice write"
}
}
- gdb_assert $got_hit "size8twice write"
}