Re: ping: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones

Pedro Alves
2018-04-20T14:49:47Z

Hi,

On 03/21/2018 07:03 PM, Jan Kratochvil wrote:

> gdb/ChangeLog
> 2017-03-27 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> PR breakpoints/19806 and support for PR external/20207.
> * NEWS (Changes since GDB 8.0): Mention unaligned hardware watchpoints.
> * aarch64-linux-nat.c (aarch64_linux_stopped_data_address): Fix missed
> watchpoints and PR external/20207 watchpoints.
> * gdbserver/linux-aarch64-low.c (aarch64_stopped_data_address):
> Likewise.
> * nat/aarch64-linux-hw-point.c (have_any_contiguous): New.
> (aarch64_watchpoint_offset): New.
> (aarch64_watchpoint_length): Support PR external/20207 watchpoints.
> (aarch64_point_encode_ctrl_reg): New parameter offset, new asserts.
> (aarch64_point_is_aligned): Support PR external/20207 watchpoints.
> (aarch64_align_watchpoint): New parameters aligned_offset_p and
> next_addr_orig_p. Support PR external/20207 watchpoints.
> (aarch64_downgrade_regs): New.
> (aarch64_dr_state_insert_one_point): New parameters offset and
> addr_orig.
> (aarch64_dr_state_remove_one_point): Likewise.
> (aarch64_handle_breakpoint): Update caller.
> (aarch64_handle_aligned_watchpoint): Likewise.
> (aarch64_handle_unaligned_watchpoint): Support addr_orig and
> aligned_offset.
> (aarch64_linux_set_debug_regs): Remove const from state. Call
> aarch64_downgrade_regs.
> (aarch64_show_debug_reg_state): Print also dr_addr_orig_wp.
> * nat/aarch64-linux-hw-point.h (DR_CONTROL_LENGTH): Rename to ...
> (DR_CONTROL_MASK): ... here.
> (struct aarch64_debug_reg_state): New field dr_addr_orig_wp.
> (unsigned int aarch64_watchpoint_offset): New prototype.
> (aarch64_linux_set_debug_regs): Remove const from state.
>
> gdb/testsuite/ChangeLog
> 2017-03-27 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> PR breakpoints/19806 and support for PR external/20207.
> * gdb.base/watchpoint-unaligned.c: New file.
> * gdb.base/watchpoint-unaligned.exp: New file.

So I spent a couple days this week working through this.

The patch looks largely good to me, though as I was going
through this, I was having trouble understanding some of the
details, and kept wishing for clearer comments, so I tried to
clarify and copy/edit the comments as I went. I also noticed a
few places more where we should update the comments to newer
reality, like the comments on top of aarch64_align_watchpoint.

Other copy/edits:

- gdbserver has its own ChangeLog file
- move align_down (and align_up) to common/, so we
can use it in gdbserver too, and use it to make
code a little bit clearer.
- rename the have_any_contiguous global as it wasn't
immediately clear what it meant when I started
reading this. Use copy-init while at it.
- // -> /**/ comments.
- Misc formatting.
- Expanded/clarified the git commit log entry.

Here's the patch that I intend to squash with yours before
merging (keeping you as --author).

Could you double-check to see if I missed something?

I have one question though. In aarch64_linux_stopped_data_address,
where we match ADDR_TRAP from siginfo to a watchpoint, we check
whether that address is within the aligned watch regions:

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)
{

However, by reading
<http://lkml.iu.edu/hypermail/linux/kernel/1611.1/05226.html>:

~~~~~~~~~~~~
Previously, when the hardware reported a watchpoint hit on an address
that did not match our watchpoint (this happens in case of instructions
which access large chunks of memory such as "stp") the process would
enter a loop where we would be continually resuming it (because we did
not recognise that watchpoint hit) and it would keep hitting the
watchpoint again and again. The tracing process would never get
notified of the watchpoint hit.
~~~~~~~~~~~~

... I'm left with the impression that ADDR_TRAP could be even
lower than addr_watch_aligned, in which case we'll still miss
watchpoints. I wondering whether GDB should be using a similar
trick as that kernel patch does. I may well be missing something,
though, as I only tried the patch on an older kernel. WDYT?

From f265a8e13984aaa40a5ed59913ed14923bc67d9d Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 20 Apr 2018 14:55:03 +0100
Subject: [PATCH] Aarch64: Fix watchpoints set on non-8-byte-aligned addresses
are always missed (PR 19806)

Ref: https://sourceware.org/bugzilla/show_bug.cgi?id=19806

As described in detail on the bug report, on Aarch64, some unaligned
watchpoints are currently missed. For example, with:

union
{
char buf[4];
unsigned int ul;
} u;

int
main ()
{
u.ul = 0xffffffff;
return 0;
}

on x86-64, we get:

(gdb) watch u.buf[1]
Hardware watchpoint 1: u.buf[1]
(gdb) c
Continuing.
Hardware watchpoint 1: u.buf[1]

Old value = 0 '\000'
New value = -1 '\377'
main () at watch.c:11
11 return 0;
(gdb)

While on Aarch64, GDB misses the watchpoint hit.

Actually, the kernel reports the hit to gdb, and linux-nat.c forwards
the event to infrun.c. However, it doesn't work as expected because
the Aarch64 backend code aligns the inserted watchpoint's address to
an 8-byte boundary, and then when the watchpoint triggers, the backend
reports that aligned address as the watchpoint stop address. Since
that address falls out of any memory range covered by watchpoints that
the core of GDB knows about, the watchpoint hit is not reported to the
user (it's considered a spurious/moribund watchpoint trap, and
ignored).

This patch fixes it, by trying to match the kernel-reported trapped
address (ADDR_TRAP) with a watched region, so that the core of gdb
figures out which watchpoint triggered.

Additionally this patch makes GDB support any watchpoint masks as
described here:

kernel RFE: aarch64: ptrace: BAS: Support any contiguous range
https://sourceware.org/bugzilla/show_bug.cgi?id=20207

The above is already fixed in current Linux kernels.

The patch trades missing watchpoints (false negatives) for the
occasional rwatch/awatch false positive on unfixed kernels. The
latter can happen if you have watchpoints set in near addresses; gdb
may report the wrong watchpoint being hit.

Note the change causes the aarch64 backend to merge fewer watchpoints,
since it now only considers the requested ranges for merging instead
of the enlarged/aligned ranges. I.e., with multiple overlapping
watchpoints, gdb may run out of the 4 hardware watchpoint registers
earlier than before. While we could make gdb considered the enlarged
ranges when running on an older kernel, I do not think it's worth the
bother, since older kernels will eventually be phased out.

Tested on RHEL-7.{3,4} for no regressions on:
kernel-4.10.0-6.el7.aarch64 (contiguous watchpoints supported)
kernel-4.5.0-15.el7.aarch64 (contiguous watchpoints unsupported)

gdb/ChangeLog:
yyyy-mm-dd Jan Kratochvil <jan.kratochvil@redhat.com>

PR breakpoints/19806 and support for PR external/20207.

* NEWS: Mention Aarch64 watchpoint improvements.

* aarch64-linux-nat.c (aarch64_linux_stopped_data_address): Fix missed
watchpoints and PR external/20207 watchpoints.
* nat/aarch64-linux-hw-point.c
(kernel_supports_any_contiguous_range): New.
(aarch64_watchpoint_offset): New.
(aarch64_watchpoint_length): Support PR external/20207 watchpoints.
(aarch64_point_encode_ctrl_reg): New parameter offset, new asserts.
(aarch64_point_is_aligned): Support PR external/20207 watchpoints.
(aarch64_align_watchpoint): New parameters aligned_offset_p and
next_addr_orig_p. Support PR external/20207 watchpoints.
(aarch64_downgrade_regs): New.
(aarch64_dr_state_insert_one_point): New parameters offset and
addr_orig.
(aarch64_dr_state_remove_one_point): Likewise.
(aarch64_handle_breakpoint): Update caller.
(aarch64_handle_aligned_watchpoint): Likewise.
(aarch64_handle_unaligned_watchpoint): Support addr_orig and
aligned_offset.
(aarch64_linux_set_debug_regs): Remove const from state. Call
aarch64_downgrade_regs.
(aarch64_show_debug_reg_state): Print also dr_addr_orig_wp.
* nat/aarch64-linux-hw-point.h (DR_CONTROL_LENGTH): Rename to ...
(DR_CONTROL_MASK): ... this.
(struct aarch64_debug_reg_state): New field dr_addr_orig_wp.
(unsigned int aarch64_watchpoint_offset): New prototype.
(aarch64_linux_set_debug_regs): Remove const from state.
* utils.c (align_up, align_down): Move to ...
* common/common-utils.c (align_up, align_down): ... here.
* utils.h (align_up, align_down): Move to ...
* common/common-utils.h (align_up, align_down): ... here.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd Jan Kratochvil <jan.kratochvil@redhat.com>

* linux-aarch64-low.c (aarch64_stopped_data_address):
Likewise.

gdb/testsuite/ChangeLog:
yyyy-mm-dd Jan Kratochvil <jan.kratochvil@redhat.com>

PR breakpoints/19806 and support for PR external/20207.
* gdb.base/watchpoint-unaligned.c: New file.
* gdb.base/watchpoint-unaligned.exp: New file.
---
gdb/NEWS | 15 ++-
gdb/aarch64-linux-nat.c | 33 +++---
gdb/common/common-utils.c | 20 ++++
gdb/common/common-utils.h | 32 ++++++
gdb/gdbserver/linux-aarch64-low.c | 33 +++---
gdb/nat/aarch64-linux-hw-point.c | 129 +++++++++++++-----------
gdb/nat/aarch64-linux-hw-point.h | 4 +-
gdb/testsuite/gdb.base/watchpoint-unaligned.c | 4 +-
gdb/testsuite/gdb.base/watchpoint-unaligned.exp | 41 ++++----
gdb/utils.c | 16 ---
gdb/utils.h | 32 ------
11 files changed, 198 insertions(+), 161 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 25c404bfc37..38043c4ff2e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -35,6 +35,16 @@ SH-5/SH64 ELF sh64-*-elf*, SH-5/SH64 support in sh*
SH-5/SH64 running GNU/Linux SH-5/SH64 support in sh*-*-linux*
SH-5/SH64 running OpenBSD SH-5/SH64 support in sh*-*-openbsd*

+* Aarch64/Linux hardware watchpoints improvements
+
+ Hardware watchpoints on unaligned addresses are now properly
+ supported when running Linux kernel 4.10 or higher: read and access
+ watchpoints are no longer spuriously missed, and all watchpoints
+ lengths between 1 and 8 bytes are supported. On older kernels,
+ watchpoints set on unaligned addresses are no longer missed, with
+ the tradeoff that there is a possibility of false hits being
+ reported.
+
*** Changes in GDB 8.1

* GDB now supports dynamically creating arbitrary register groups specified
@@ -148,11 +158,6 @@ SH-5/SH64 running OpenBSD SH-5/SH64 support in sh*-*-openbsd*

Tab completion was adjusted accordingly as well.

-* Unaligned hardware watchpoints on aarch64-linux are now properly supported
- with Linux kernel 4.10 and higher. On older kernels some unaligned
- watchpoints are no longer missed - but other unaligned watchpoints may make
- false hits now.
-
* Python Scripting

** New events gdb.new_inferior, gdb.inferior_deleted, and
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index b69dfb2fce7..421d044e108 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -735,13 +735,12 @@ aarch64_linux_stopped_data_address (struct target_ops *target,
state = aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
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 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_trap = (CORE_ADDR) siginfo.si_addr;
const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset;
- const CORE_ADDR addr_watch_aligned = (state->dr_addr_wp[i]
- & -(CORE_ADDR) 8);
+ 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]
@@ -749,14 +748,24 @@ aarch64_linux_stopped_data_address (struct target_ops *target,
&& addr_trap >= addr_watch_aligned
&& addr_trap < addr_watch + len)
{
- /* ADDR_TRAP reports first address of the range touched by CPU.
- ADDR_TRAP may be before the ADDR_WATCH..ADDR_WATCH+LEN range
- for larger CPU access hitting the watchpoint by its tail part.
- ADDR_TRAP must be in the ADDR_WATCH_ALIGNED..ADDR_WATCH+LEN range.
- Never report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN
- range (not matching any known watchpoint range).
- ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false positive due to
- PR external/20207 buggy kernels. */
+ /* 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 1;
}
diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index 80de826ba78..8d839d10fa8 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -440,3 +440,23 @@ is_regular_file (const char *name, int *errno_ptr)
*errno_ptr = EINVAL;
return false;
}
+
+/* See common/common-utils.h. */
+
+ULONGEST
+align_up (ULONGEST v, int n)
+{
+ /* Check that N is really a power of two. */
+ gdb_assert (n && (n & (n-1)) == 0);
+ return (v + n - 1) & -n;
+}
+
+/* See common/common-utils.h. */
+
+ULONGEST
+align_down (ULONGEST v, int n)
+{
+ /* Check that N is really a power of two. */
+ gdb_assert (n && (n & (n-1)) == 0);
+ return (v & -n);
+}
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 5408c354693..7bc6e90f05c 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -151,4 +151,36 @@ in_inclusive_range (T value, T low, T high)
we're expecting a regular file. */
extern bool is_regular_file (const char *name, int *errno_ptr);

+/* Ensure that V is aligned to an N byte boundary (B's assumed to be a
+ power of 2). Round up/down when necessary. Examples of correct
+ use include:
+
+ addr = align_up (addr, 8); -- VALUE needs 8 byte alignment
+ write_memory (addr, value, len);
+ addr += len;
+
+ and:
+
+ sp = align_down (sp - len, 16); -- Keep SP 16 byte aligned
+ write_memory (sp, value, len);
+
+ Note that uses such as:
+
+ write_memory (addr, value, len);
+ addr += align_up (len, 8);
+
+ and:
+
+ sp -= align_up (len, 8);
+ write_memory (sp, value, len);
+
+ are typically not correct as they don't ensure that the address (SP
+ or ADDR) is correctly aligned (relying on previous alignment to
+ keep things right). This is also why the methods are called
+ "align_..." instead of "round_..." as the latter reads better with
+ this incorrect coding style. */
+
+extern ULONGEST align_up (ULONGEST v, int n);
+extern ULONGEST align_down (ULONGEST v, int n);
+
#endif
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index 133572ef0c5..7ea24c23634 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -360,13 +360,12 @@ aarch64_stopped_data_address (void)
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 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_trap = (CORE_ADDR) siginfo.si_addr;
const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset;
- const CORE_ADDR addr_watch_aligned = (state->dr_addr_wp[i]
- & -(CORE_ADDR) 8);
+ 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]
@@ -374,14 +373,24 @@ aarch64_stopped_data_address (void)
&& addr_trap >= addr_watch_aligned
&& addr_trap < addr_watch + len)
{
- /* ADDR_TRAP reports first address of the range touched by CPU.
- ADDR_TRAP may be before the ADDR_WATCH..ADDR_WATCH+LEN range
- for larger CPU access hitting the watchpoint by its tail part.
- ADDR_TRAP must be in the ADDR_WATCH_ALIGNED..ADDR_WATCH+LEN range.
- Never report RETVAL outside of any ADDR_WATCH..ADDR_WATCH+LEN
- range (not matching any known watchpoint range).
- ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false positive due to
- PR external/20207 buggy kernels. */
+ /* 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;
}
}
diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index ef0022ac0e1..a3931ea6a94 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -34,10 +34,13 @@
int aarch64_num_bp_regs;
int aarch64_num_wp_regs;

-/* TRUE means this kernel has fixed PR external/20207.
- Fixed kernel supports any contiguous range of bits in 8-bit byte
- DR_CONTROL_MASK. Buggy kernel supports only 0x01, 0x03, 0x0f and 0xff. */
-static bool have_any_contiguous (true);
+/* True if this kernel does not have the bug described by PR
+ external/20207 (Linux >= 4.10). A fixed kernel supports any
+ contiguous range of bits in 8-bit byte DR_CONTROL_MASK. A buggy
+ kernel supports only 0x01, 0x03, 0x0f and 0xff. We start by
+ assuming the bug is fixed, and then detect the bug at
+ PTRACE_SETREGSET time. */
+static bool kernel_supports_any_contiguous_range = true;

/* Return starting byte 0..7 incl. of a watchpoint encoded by CTRL. */

@@ -47,7 +50,7 @@ aarch64_watchpoint_offset (unsigned int ctrl)
uint8_t mask = DR_CONTROL_MASK (ctrl);
unsigned retval;

- // Shift out bottom zeroes.
+ /* Shift out bottom zeros. */
for (retval = 0; mask && (mask & 1) == 0; ++retval)
mask >>= 1;

@@ -56,8 +59,8 @@ aarch64_watchpoint_offset (unsigned int ctrl)

/* Utility function that returns the length in bytes of a watchpoint
according to the content of a hardware debug control register CTRL.
- Any contiguous range of bytes in CTRL is supported. Returned value
- can be between 0..8 incl. */
+ Any contiguous range of bytes in CTRL is supported. The returned
+ value can be between 0..8 (inclusive). */

unsigned int
aarch64_watchpoint_length (unsigned int ctrl)
@@ -65,16 +68,16 @@ aarch64_watchpoint_length (unsigned int ctrl)
uint8_t mask = DR_CONTROL_MASK (ctrl);
unsigned retval;

- // Shift out bottom zeroes.
+ /* Shift out bottom zeros. */
mask >>= aarch64_watchpoint_offset (ctrl);

- // Count bottom ones;
+ /* Count bottom ones. */
for (retval = 0; (mask & 1) != 0; ++retval)
mask >>= 1;

- if (mask)
+ if (mask != 0)
error (_("Unexpected hardware watchpoint length register value 0x%x"),
- DR_CONTROL_MASK (ctrl));
+ DR_CONTROL_MASK (ctrl));

return retval;
}
@@ -88,7 +91,7 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int offset, int len)
{
unsigned int ctrl, ttype;

- gdb_assert (offset == 0 || have_any_contiguous);
+ gdb_assert (offset == 0 || kernel_supports_any_contiguous_range);
gdb_assert (offset + len <= AARCH64_HWP_MAX_LEN_PER_REG);

/* type */
@@ -157,51 +160,56 @@ aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
if (addr & (alignment - 1))
return 0;

- if ((!have_any_contiguous && len != 8 && len != 4 && len != 2 && len != 1)
- || (have_any_contiguous && (len < 1 || len > 8)))
+ if ((!kernel_supports_any_contiguous_range
+ && len != 8 && len != 4 && len != 2 && len != 1)
+ || (kernel_supports_any_contiguous_range
+ && (len < 1 || len > 8)))
return 0;

return 1;
}

/* Given the (potentially unaligned) watchpoint address in ADDR and
- length in LEN, return the aligned address and aligned length in
- *ALIGNED_ADDR_P and *ALIGNED_LEN_P, respectively. The returned
- aligned address and length will be valid values to write to the
- hardware watchpoint value and control registers.
+ length in LEN, return the aligned address, offset from that base
+ address, and aligned length in *ALIGNED_ADDR_P, *ALIGNED_OFFSET_P
+ and *ALIGNED_LEN_P, respectively. The returned values will be
+ valid values to write to the hardware watchpoint value and control
+ registers.

The given watchpoint may get truncated if more than one hardware
register is needed to cover the watched region. *NEXT_ADDR_P
and *NEXT_LEN_P, if non-NULL, will return the address and length
of the remaining part of the watchpoint (which can be processed
- by calling this routine again to generate another aligned address
- and length pair.
+ by calling this routine again to generate another aligned address,
+ offset and length tuple.

Essentially, unaligned watchpoint is achieved by minimally
enlarging the watched area to meet the alignment requirement, and
if necessary, splitting the watchpoint over several hardware
- watchpoint registers. The trade-off is that there will be
- false-positive hits for the read-type or the access-type hardware
- watchpoints; for the write type, which is more commonly used, there
- will be no such issues, as the higher-level breakpoint management
- in gdb always examines the exact watched region for any content
- change, and transparently resumes a thread from a watchpoint trap
- if there is no change to the watched region.
+ watchpoint registers.
+
+ On kernels that predate the support for Byte Address Select (BAS)
+ in the hardware watchpoint control register, the offset from the
+ base address is always zero, and so in that case the trade-off is
+ that there will be false-positive hits for the read-type or the
+ access-type hardware watchpoints; for the write type, which is more
+ commonly used, there will be no such issues, as the higher-level
+ breakpoint management in gdb always examines the exact watched
+ region for any content change, and transparently resumes a thread
+ from a watchpoint trap if there is no change to the watched region.

Another limitation is that because the watched region is enlarged,
- the watchpoint fault address returned by
+ the watchpoint fault address discovered by
aarch64_stopped_data_address may be outside of the original watched
region, especially when the triggering instruction is accessing a
larger region. When the fault address is not within any known
range, watchpoints_triggered in gdb will get confused, as the
higher-level watchpoint management is only aware of original
watched regions, and will think that some unknown watchpoint has
- been triggered. In such a case, gdb may stop without displaying
- any detailed information.
-
- Once the kernel provides the full support for Byte Address Select
- (BAS) in the hardware watchpoint control register, these
- limitations can be largely relaxed with some further work. */
+ been triggered. To prevent such a case,
+ aarch64_stopped_data_address implementations in gdb and gdbserver
+ try to match the trapped address with a watched region, and return
+ an address within the latter. */

static void
aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
@@ -221,11 +229,12 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
if (len <= 0)
return;

- /* Address to be put into the hardware watchpoint value register
- must be aligned. */
+ /* The address put into the hardware watchpoint value register must
+ be aligned. */
offset = addr & (alignment - 1);
aligned_addr = addr - offset;
- aligned_offset = have_any_contiguous ? addr & (alignment - 1) : 0;
+ aligned_offset
+ = kernel_supports_any_contiguous_range ? addr & (alignment - 1) : 0;

gdb_assert (offset >= 0 && offset < alignment);
gdb_assert (aligned_addr >= 0 && aligned_addr <= addr);
@@ -233,9 +242,10 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,

if (offset + len >= max_wp_len)
{
- /* Need more than one watchpoint registers; truncate it at the
+ /* Need more than one watchpoint register; truncate at the
alignment boundary. */
- aligned_len = max_wp_len - (have_any_contiguous ? offset : 0);
+ aligned_len
+ = max_wp_len - (kernel_supports_any_contiguous_range ? offset : 0);
len -= (max_wp_len - offset);
addr += (max_wp_len - offset);
gdb_assert ((addr & (alignment - 1)) == 0);
@@ -248,7 +258,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
aligned_len_array[AARCH64_HWP_MAX_LEN_PER_REG] =
{ 1, 2, 4, 4, 8, 8, 8, 8 };

- aligned_len = (have_any_contiguous
+ aligned_len = (kernel_supports_any_contiguous_range
? len : aligned_len_array[offset + len - 1]);
addr += len;
len = 0;
@@ -265,8 +275,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
if (next_len_p)
*next_len_p = len;
if (next_addr_orig_p)
- *next_addr_orig_p = (((*next_addr_orig_p) + alignment)
- & -(CORE_ADDR) alignment);
+ *next_addr_orig_p = align_down (*next_addr_orig_p + alignment, alignment);
}

struct aarch64_dr_update_callback_param
@@ -356,13 +365,15 @@ aarch64_notify_debug_reg_change (const struct aarch64_debug_reg_state *state,
iterate_over_lwps (pid_ptid, debug_reg_change_callback, (void *) &param);
}

-/* Reconfigure STATE to be compatible with Linux kernel with buggy
- PR external/20207 during HAVE_ANY_CONTIGUOUS true->false change.
- A regression for buggy kernels is that originally GDB could support
- more watchpoint combinations that had matching (and thus shared)
- masks. On such buggy kernels new GDB will try to first setup the
- perfect matching ranges which will run out of registers (before this
- function can merge them). */
+/* Reconfigure STATE to be compatible with Linux kernels with the PR
+ external/20207 bug. This is called when
+ KERNEL_SUPPORTS_ANY_CONTIGUOUS_RANGE transitions to false. Note we
+ don't try to support combining watchpoints with matching (and thus
+ shared) masks, as it's too late when we get here. On buggy
+ kernels, GDB will try to first setup the perfect matching ranges,
+ which will run out of registers before this function can merge
+ them. It doesn't look like worth the effort to improve that, given
+ eventually buggy kernels will be phased out. */

static void
aarch64_downgrade_regs (struct aarch64_debug_reg_state *state)
@@ -373,9 +384,9 @@ aarch64_downgrade_regs (struct aarch64_debug_reg_state *state)
gdb_assert (state->dr_ref_count_wp[i] != 0);
uint8_t mask_orig = (state->dr_ctrl_wp[i] >> 5) & 0xff;
gdb_assert (mask_orig != 0);
- const std::array<uint8_t, 4> old_valid ({ 0x01, 0x03, 0x0f, 0xff });
- uint8_t mask (0);
- for (const uint8_t old_mask:old_valid)
+ static const uint8_t old_valid[] = { 0x01, 0x03, 0x0f, 0xff };
+ uint8_t mask = 0;
+ for (const uint8_t old_mask : old_valid)
if (mask_orig <= old_mask)
{
mask = old_mask;
@@ -383,13 +394,14 @@ aarch64_downgrade_regs (struct aarch64_debug_reg_state *state)
}
gdb_assert (mask != 0);

- // No update needed for this watchpoint?
+ /* No update needed for this watchpoint? */
if (mask == mask_orig)
continue;
state->dr_ctrl_wp[i] |= mask << 5;
- state->dr_addr_wp[i] &= -(CORE_ADDR) AARCH64_HWP_ALIGNMENT;
+ state->dr_addr_wp[i]
+ = align_down (state->dr_addr_wp[i], AARCH64_HWP_ALIGNMENT);

- // Try to match duplicate entries.
+ /* Try to match duplicate entries. */
for (int j = 0; j < i; ++j)
if ((state->dr_ctrl_wp[j] & 1) != 0
&& state->dr_addr_wp[j] == state->dr_addr_wp[i]
@@ -692,10 +704,11 @@ aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
(void *) &iov))
{
- // Handle PR external/20207 buggy kernels?
- if (watchpoint && errno == EINVAL && have_any_contiguous)
+ /* Handle Linux kernels with the PR external/20207 bug. */
+ if (watchpoint && errno == EINVAL
+ && kernel_supports_any_contiguous_range)
{
- have_any_contiguous = false;
+ kernel_supports_any_contiguous_range = false;
aarch64_downgrade_regs (state);
aarch64_linux_set_debug_regs (state, tid, watchpoint);
return;
diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h
index 7c3513e301e..1940b06a89e 100644
--- a/gdb/nat/aarch64-linux-hw-point.h
+++ b/gdb/nat/aarch64-linux-hw-point.h
@@ -147,9 +147,9 @@ struct aarch64_debug_reg_state
unsigned int dr_ref_count_bp[AARCH64_HBP_MAX_NUM];

/* hardware watchpoint */
- // Address aligned down to AARCH64_HWP_ALIGNMENT.
+ /* Address aligned down to AARCH64_HWP_ALIGNMENT. */
CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
- // Address as entered by user without any alignment.
+ /* Address as entered by user without any forced alignment. */
CORE_ADDR dr_addr_orig_wp[AARCH64_HWP_MAX_NUM];
unsigned int dr_ctrl_wp[AARCH64_HWP_MAX_NUM];
unsigned int dr_ref_count_wp[AARCH64_HWP_MAX_NUM];
diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.c b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
index f653e77529d..ea844e947f6 100644
--- a/gdb/testsuite/gdb.base/watchpoint-unaligned.c
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
@@ -1,6 +1,6 @@
/* This testcase is part of GDB, the GNU debugger.

- Copyright 2017 Free Software Foundation, Inc.
+ Copyright 2017-2018 Free Software Foundation, Inc.

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -62,7 +62,7 @@ main (void)
assert (0);
}
size = 0;
- size = size; /* again_start */
+ size = size; /* start_again */
}
return 0;
}
diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
index 8d463b12fad..833997b04d7 100644
--- a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
@@ -1,4 +1,4 @@
-# Copyright 2017 Free Software Foundation, Inc.
+# Copyright 2017-2018 Free Software Foundation, Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -11,11 +11,12 @@
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
# This file is part of the gdb testsuite.

+# Test inserting read watchpoints on unaligned addresses.
+
standard_testfile
if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
return -1
@@ -26,21 +27,21 @@ if ![runto_main] {
return -1
}

-gdb_breakpoint "[gdb_get_line_number "again_start"]" "Breakpoint $decimal at $hex" "again_start"
+gdb_breakpoint [gdb_get_line_number "start_again"] "Breakpoint $decimal at $hex" "start_again"

set sizes {1 2 4 8}
array set alignedend {1 1 2 2 3 4 4 4 5 8 6 8 7 8 8 8}

foreach wpsize $sizes {
- for {set wpoffset 0} {$wpoffset < 8/$wpsize} {incr wpoffset} {
+ for {set wpoffset 0} {$wpoffset < 8 / $wpsize} {incr wpoffset} {
set wpstart [expr $wpoffset * $wpsize]
set wpend [expr ($wpoffset + 1) * $wpsize]
set wpendaligned $alignedend($wpend)
foreach rdsize $sizes {
- for {set rdoffset 0} {$rdoffset < 8/$rdsize} {incr rdoffset} {
+ for {set rdoffset 0} {$rdoffset < 8 / $rdsize} {incr rdoffset} {
set rdstart [expr $rdoffset * $rdsize]
set rdend [expr ($rdoffset + 1) * $rdsize]
- set expect_hit [expr max($wpstart,$rdstart) < min($wpend,$rdend)]
+ set expect_hit [expr max ($wpstart, $rdstart) < min ($wpend, $rdend)]
set test "rwatch data.u.size$wpsize\[$wpoffset\]"
set wpnum ""
gdb_test_multiple $test $test {
@@ -51,25 +52,25 @@ foreach wpsize $sizes {
gdb_test_no_output "set variable size = $rdsize" ""
gdb_test_no_output "set variable offset = $rdoffset" ""
set test "continue"
- set did_hit 0
+ set got_hit 0
gdb_test_multiple $test $test {
-re "Hardware read watchpoint $wpnum:.*Value = .*\r\n$gdb_prompt $" {
- set did_hit 1
+ set got_hit 1
send_gdb "continue\n"
exp_continue
}
- -re " again_start .*\r\n$gdb_prompt $" {
+ -re " start_again .*\r\n$gdb_prompt $" {
}
}
gdb_test_no_output "delete $wpnum" ""
set test "wp(size=$wpsize offset=$wpoffset) rd(size=$rdsize offset=$rdoffset) expect=$expect_hit"
- if {$expect_hit == $did_hit} {
+ if {$expect_hit == $got_hit} {
pass $test
} else {
- # We do not know if we run on a fixed kernel or not.
- # Report XFAIL only in the FAIL case.
+ # We do not know if we run on a fixed Linux kernel
+ # or not. Report XFAIL only in the FAIL case.
if {$expect_hit == 0 && $rdstart < $wpendaligned} {
- setup_xfail external/20207 "aarch64-*-*"
+ setup_xfail external/20207 "aarch64*-*-linux*"
}
fail $test
}
@@ -92,16 +93,16 @@ foreach wpcount {4 7} {
gdb_test_no_output "set variable size = 1" ""
gdb_test_no_output "set variable offset = 1" ""
set test "continue"
- set did_hit 0
+ set got_hit 0
gdb_test_multiple $test $test {
-re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" {
}
-re "Hardware read watchpoint $wpoffset_to_wpnum(1):.*Value = .*\r\n$gdb_prompt $" {
- set did_hit 1
+ set got_hit 1
send_gdb "continue\n"
exp_continue
}
- -re " again_start .*\r\n$gdb_prompt $" {
+ -re " start_again .*\r\n$gdb_prompt $" {
}
}
for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
@@ -111,9 +112,5 @@ foreach wpcount {4 7} {
if {$wpcount > 4} {
setup_kfail tdep/22389 *-*-*
}
- if {$did_hit} {
- pass $test
- } else {
- fail $test
- }
+ gdb_assert $got_hit $test
}
diff --git a/gdb/utils.c b/gdb/utils.c
index b957b0dc5c2..e274f026764 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2849,22 +2849,6 @@ gdb_realpath_tests ()

#endif /* GDB_SELF_TEST */

-ULONGEST
-align_up (ULONGEST v, int n)
-{
- /* Check that N is really a power of two. */
- gdb_assert (n && (n & (n-1)) == 0);
- return (v + n - 1) & -n;
-}
-
-ULONGEST
-align_down (ULONGEST v, int n)
-{
- /* Check that N is really a power of two. */
- gdb_assert (n && (n & (n-1)) == 0);
- return (v & -n);
-}
-
/* Allocation function for the libiberty hash table which uses an
obstack. The obstack is passed as DATA. */

diff --git a/gdb/utils.h b/gdb/utils.h
index 4dec889d83e..c728449429e 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -494,38 +494,6 @@ extern pid_t wait_to_die_with_timeout (pid_t pid, int *status, int timeout);

extern int myread (int, char *, int);

-/* Ensure that V is aligned to an N byte boundary (B's assumed to be a
- power of 2). Round up/down when necessary. Examples of correct
- use include:
-
- addr = align_up (addr, 8); -- VALUE needs 8 byte alignment
- write_memory (addr, value, len);
- addr += len;
-
- and:
-
- sp = align_down (sp - len, 16); -- Keep SP 16 byte aligned
- write_memory (sp, value, len);
-
- Note that uses such as:
-
- write_memory (addr, value, len);
- addr += align_up (len, 8);
-
- and:
-
- sp -= align_up (len, 8);
- write_memory (sp, value, len);
-
- are typically not correct as they don't ensure that the address (SP
- or ADDR) is correctly aligned (relying on previous alignment to
- keep things right). This is also why the methods are called
- "align_..." instead of "round_..." as the latter reads better with
- this incorrect coding style. */
-
-extern ULONGEST align_up (ULONGEST v, int n);
-extern ULONGEST align_down (ULONGEST v, int n);
-
/* Resource limits used by getrlimit and setrlimit. */

enum resource_limit_kind
--
2.14.3