This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
ping: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 21 Mar 2018 20:03:16 +0100
- Subject: ping: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
- References: <20170327210753.GA29656@host1.jankratochvil.net> <20171018195237.GA19714@host1.jankratochvil.net> <867evczxik.fsf@gmail.com> <20171103220437.GA13979@host1.jankratochvil.net>
https://sourceware.org/ml/gdb-patches/2017-11/msg00090.html
Message-ID: <20171103220437.GA13979@host1.jankratochvil.net>
On Fri, 03 Nov 2017 23:04:37 +0100, Jan Kratochvil wrote:
On Mon, 30 Oct 2017 12:27:15 +0100, Yao Qi wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> > Therefore on old kernels (see below) it will now rather report watchpoint
> > which was hit only next to it (for rwatch/awatch). But it will never miss
> > a watchpoint.
>
> I am fine with the change. Does your case watchpoint-unaligned.c cover
> this? We need to have a case that show GDB misses a watchpoint (false
> negative), and with your patch applied, it becomes false positive.
> IIUC, all these change happen on the old kernel.
It is handled by the new testcase:
# We do not know if we run on a fixed kernel or not.
# Report XFAIL only in the FAIL case.
if {$expect_hit == 0 && $rdstart < $wpendaligned} {
setup_xfail external/20207 "aarch64-*-*"
}
fail $test
Sure an improvement of the testcase would be to make GDB display its internal
'have_any_contiguous' variable by some maintenance/info/debug command so that
it could display even XPASS for example. But I do not think the testcase
could be mistaken by that.
> We need a NEWS entry, since the change is user-visible.
Done.
> > There may be a regresion that it now less merges watchpoints so that with
> > multiple overlapping watchpoints it may run out of the 4 hardware watchpoint
> > registers. But as discussed in the original thread GDB needs some generic
>
> Is it a regression in your mind or a regression in some existing test
> case?
In my mind.
> If we don't have case, can you write one case to show that, we'll
> run out of watchpoint registers with this fix, but don't run out of them
> before. We can kfail the failure, which can be addressed by watchpoint
> merging stuff as you mentioned later.
Extended the attached *.exp testfile and filed for it tracker:
aarch64: watchpoints are not merged regression
https://sourceware.org/bugzilla/show_bug.cgi?id=22389
Therefore with current FSF GDB it does KPASS.
> Can you elaborate? watchpoint is quite target specific, and each
> cpu/arch has completely different watchpoint registers. What do you
> expect watchpoint merging framework do? in a target independent way, I suppose?
Some limitations are specific to arches but most of the logic would be the
same. There could be some generic merging engine with some arch-specific
parameters. Such as adjacent watchpoints (watch, not rwatch/awatch) can be
merged into a single larger watchpoint. Or one could even place there one
bigger watchpoints with possible false positives etc.
But I am not going to implement that. I consider it only as an incremental
improvement (although it would prevent the regression PR 22389).
I (also) find the code messy but I would have to rewrite it first as there are
member variables instead of accessors, it is not C++, there is missing the
arch-agnostic watchpoint splitting+merging etc. I coded the patch driven by
the testcase given it tests all the combinations (it does not test splitting of
the watchpoints which I believe is simple enough but maybe that could also be
tested).
> > aarch64_watchpoint_length (unsigned int ctrl)
>
> The comments to this function should be updated too.
Done.
> Could you split the patch, that is, patch 1 can be about changing
> aarch64_watchpoint_length and adding aarch64_watchpoint_offset? That is
> helpful to me to understand the patch.
Sorry I was looking at it for a few evenings and I do not really understand how
you wish the split to be done. Should it just add the
aarch64_watchpoint_offset function still not supporting the new kernel feature?
Then what it will do there? It will just always return 0.
If it already should support the new kernels then all the patch needs to be there.
Maybe except aarch64_downgrade_regs but that is a separate function anyway.
> > @@ -148,6 +148,7 @@ struct aarch64_debug_reg_state
> >
> > /* hardware watchpoint */
> > CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
> > + CORE_ADDR dr_addr_orig_wp[AARCH64_HWP_MAX_NUM];
>
> Could you add comments on how these two fields are used?
Done.
// Address aligned down to AARCH64_HWP_ALIGNMENT.
CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
// Address as entered by user without any alignment.
CORE_ADDR dr_addr_orig_wp[AARCH64_HWP_MAX_NUM];
> > +if {![is_aarch64_target] && ![istarget "x86_64-*-*"] && ![istarget i?86-*]} {
> > + verbose "Skipping ${gdb_test_file_name}."
> > + return
> > +}
>
> Any reason to only run this test only on these targets? Better to run
> this test on every target if possible.
OK.
Thanks,
Jan
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.
diff --git a/gdb/NEWS b/gdb/NEWS
index fbf5591..fd1d664 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -18,6 +18,11 @@
the remote inferior is started by the GDBserver, use the "unset
environment" command.
+* 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 de18edd..5cfb615 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -735,16 +735,29 @@ 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 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];
+ 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_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
+ && addr_trap >= addr_watch_aligned
&& addr_trap < addr_watch + len)
{
- *addr_p = addr_trap;
+ /* 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_p = addr_orig;
return 1;
}
}
diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
index b00d5c5..086db8e 100644
--- a/gdb/gdbserver/linux-aarch64-low.c
+++ b/gdb/gdbserver/linux-aarch64-low.c
@@ -363,14 +363,30 @@ 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 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];
+ 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_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
+ && addr_trap >= addr_watch_aligned
&& addr_trap < addr_watch + len)
- return addr_trap;
+ {
+ /* 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. */
+ return addr_orig;
+ }
}
return (CORE_ADDR) 0;
diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index 9800d9a..7a5b657 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -34,29 +34,49 @@
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);
+
+/* Return starting byte 0..7 incl. of a watchpoint encoded by CTRL. */
+
+unsigned int
+aarch64_watchpoint_offset (unsigned int ctrl)
+{
+ uint8_t mask = DR_CONTROL_MASK (ctrl);
+ unsigned retval;
+
+ // Shift out bottom zeroes.
+ for (retval = 0; mask && (mask & 1) == 0; ++retval)
+ mask >>= 1;
+
+ return retval;
+}
+
/* Utility function that returns the length in bytes of a watchpoint
according to the content of a hardware debug control register CTRL.
- Note that the kernel currently only supports the following Byte
- Address Select (BAS) values: 0x1, 0x3, 0xf and 0xff, which means
- that for a hardware watchpoint, its valid length can only be 1
- byte, 2 bytes, 4 bytes or 8 bytes. */
+ Any contiguous range of bytes in CTRL is supported. Returned value
+ can be between 0..8 incl. */
unsigned int
aarch64_watchpoint_length (unsigned int ctrl)
{
- switch (DR_CONTROL_LENGTH (ctrl))
- {
- case 0x01:
- return 1;
- case 0x03:
- return 2;
- case 0x0f:
- return 4;
- case 0xff:
- return 8;
- default:
- return 0;
- }
+ uint8_t mask = DR_CONTROL_MASK (ctrl);
+ unsigned retval;
+
+ // Shift out bottom zeroes.
+ mask >>= aarch64_watchpoint_offset (ctrl);
+
+ // Count bottom ones;
+ for (retval = 0; (mask & 1) != 0; ++retval)
+ mask >>= 1;
+
+ if (mask)
+ error (_("Unexpected hardware watchpoint length register value 0x%x"),
+ DR_CONTROL_MASK (ctrl));
+
+ return retval;
}
/* Given the hardware breakpoint or watchpoint type TYPE and its
@@ -64,10 +84,13 @@ aarch64_watchpoint_length (unsigned int ctrl)
breakpoint/watchpoint control register. */
static unsigned int
-aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
+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 + len <= AARCH64_HWP_MAX_LEN_PER_REG);
+
/* type */
switch (type)
{
@@ -89,8 +112,8 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
ctrl = ttype << 3;
- /* length bitmask */
- ctrl |= ((1 << len) - 1) << 5;
+ /* offset and length bitmask */
+ ctrl |= ((1 << len) - 1) << (5 + offset);
/* enabled at el0 */
ctrl |= (2 << 1) | 1;
@@ -134,7 +157,8 @@ aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
if (addr & (alignment - 1))
return 0;
- if (len != 8 && len != 4 && len != 2 && len != 1)
+ if ((!have_any_contiguous && len != 8 && len != 4 && len != 2 && len != 1)
+ || (have_any_contiguous && (len < 1 || len > 8)))
return 0;
return 1;
@@ -181,11 +205,12 @@ aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
static void
aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
- int *aligned_len_p, CORE_ADDR *next_addr_p,
- int *next_len_p)
+ int *aligned_offset_p, int *aligned_len_p,
+ CORE_ADDR *next_addr_p, int *next_len_p,
+ CORE_ADDR *next_addr_orig_p)
{
int aligned_len;
- unsigned int offset;
+ unsigned int offset, aligned_offset;
CORE_ADDR aligned_addr;
const unsigned int alignment = AARCH64_HWP_ALIGNMENT;
const unsigned int max_wp_len = AARCH64_HWP_MAX_LEN_PER_REG;
@@ -200,6 +225,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
must be aligned. */
offset = addr & (alignment - 1);
aligned_addr = addr - offset;
+ aligned_offset = have_any_contiguous ? addr & (alignment - 1) : 0;
gdb_assert (offset >= 0 && offset < alignment);
gdb_assert (aligned_addr >= 0 && aligned_addr <= addr);
@@ -209,7 +235,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
{
/* Need more than one watchpoint registers; truncate it at the
alignment boundary. */
- aligned_len = max_wp_len;
+ aligned_len = max_wp_len - (have_any_contiguous ? offset : 0);
len -= (max_wp_len - offset);
addr += (max_wp_len - offset);
gdb_assert ((addr & (alignment - 1)) == 0);
@@ -222,19 +248,25 @@ 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 = aligned_len_array[offset + len - 1];
+ aligned_len = (have_any_contiguous
+ ? len : aligned_len_array[offset + len - 1]);
addr += len;
len = 0;
}
if (aligned_addr_p)
*aligned_addr_p = aligned_addr;
+ if (aligned_offset_p)
+ *aligned_offset_p = aligned_offset;
if (aligned_len_p)
*aligned_len_p = aligned_len;
if (next_addr_p)
*next_addr_p = addr;
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);
}
struct aarch64_dr_update_callback_param
@@ -324,17 +356,70 @@ aarch64_notify_debug_reg_change (const struct aarch64_debug_reg_state *state,
iterate_over_lwps (pid_ptid, debug_reg_change_callback, (void *) ¶m);
}
+/* 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). */
+
+static void
+aarch64_downgrade_regs (struct aarch64_debug_reg_state *state)
+{
+ for (int i = 0; i < aarch64_num_wp_regs; ++i)
+ if ((state->dr_ctrl_wp[i] & 1) != 0)
+ {
+ 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)
+ if (mask_orig <= old_mask)
+ {
+ mask = old_mask;
+ break;
+ }
+ gdb_assert (mask != 0);
+
+ // 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;
+
+ // 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]
+ && state->dr_addr_orig_wp[j] == state->dr_addr_orig_wp[i]
+ && state->dr_ctrl_wp[j] == state->dr_ctrl_wp[i])
+ {
+ state->dr_ref_count_wp[j] += state->dr_ref_count_wp[i];
+ state->dr_ref_count_wp[i] = 0;
+ state->dr_addr_wp[i] = 0;
+ state->dr_addr_orig_wp[i] = 0;
+ state->dr_ctrl_wp[i] &= ~1;
+ break;
+ }
+
+ aarch64_notify_debug_reg_change (state, 1 /* is_watchpoint */, i);
+ }
+}
+
/* Record the insertion of one breakpoint/watchpoint, as represented
by ADDR and CTRL, in the process' arch-specific data area *STATE. */
static int
aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
enum target_hw_bp_type type,
- CORE_ADDR addr, int len)
+ CORE_ADDR addr, int offset, int len,
+ CORE_ADDR addr_orig)
{
int i, idx, num_regs, is_watchpoint;
unsigned int ctrl, *dr_ctrl_p, *dr_ref_count;
- CORE_ADDR *dr_addr_p;
+ CORE_ADDR *dr_addr_p, *dr_addr_orig_p;
/* Set up state pointers. */
is_watchpoint = (type != hw_execute);
@@ -343,6 +428,7 @@ aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
{
num_regs = aarch64_num_wp_regs;
dr_addr_p = state->dr_addr_wp;
+ dr_addr_orig_p = state->dr_addr_orig_wp;
dr_ctrl_p = state->dr_ctrl_wp;
dr_ref_count = state->dr_ref_count_wp;
}
@@ -350,11 +436,12 @@ aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
{
num_regs = aarch64_num_bp_regs;
dr_addr_p = state->dr_addr_bp;
+ dr_addr_orig_p = nullptr;
dr_ctrl_p = state->dr_ctrl_bp;
dr_ref_count = state->dr_ref_count_bp;
}
- ctrl = aarch64_point_encode_ctrl_reg (type, len);
+ ctrl = aarch64_point_encode_ctrl_reg (type, offset, len);
/* Find an existing or free register in our cache. */
idx = -1;
@@ -366,7 +453,9 @@ aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
idx = i;
/* no break; continue hunting for an exising one. */
}
- else if (dr_addr_p[i] == addr && dr_ctrl_p[i] == ctrl)
+ else if (dr_addr_p[i] == addr
+ && (dr_addr_orig_p == nullptr || dr_addr_orig_p[i] == addr_orig)
+ && dr_ctrl_p[i] == ctrl)
{
gdb_assert (dr_ref_count[i] != 0);
idx = i;
@@ -383,6 +472,8 @@ aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
{
/* new entry */
dr_addr_p[idx] = addr;
+ if (dr_addr_orig_p != nullptr)
+ dr_addr_orig_p[idx] = addr_orig;
dr_ctrl_p[idx] = ctrl;
dr_ref_count[idx] = 1;
/* Notify the change. */
@@ -403,11 +494,12 @@ aarch64_dr_state_insert_one_point (struct aarch64_debug_reg_state *state,
static int
aarch64_dr_state_remove_one_point (struct aarch64_debug_reg_state *state,
enum target_hw_bp_type type,
- CORE_ADDR addr, int len)
+ CORE_ADDR addr, int offset, int len,
+ CORE_ADDR addr_orig)
{
int i, num_regs, is_watchpoint;
unsigned int ctrl, *dr_ctrl_p, *dr_ref_count;
- CORE_ADDR *dr_addr_p;
+ CORE_ADDR *dr_addr_p, *dr_addr_orig_p;
/* Set up state pointers. */
is_watchpoint = (type != hw_execute);
@@ -415,6 +507,7 @@ aarch64_dr_state_remove_one_point (struct aarch64_debug_reg_state *state,
{
num_regs = aarch64_num_wp_regs;
dr_addr_p = state->dr_addr_wp;
+ dr_addr_orig_p = state->dr_addr_orig_wp;
dr_ctrl_p = state->dr_ctrl_wp;
dr_ref_count = state->dr_ref_count_wp;
}
@@ -422,15 +515,18 @@ aarch64_dr_state_remove_one_point (struct aarch64_debug_reg_state *state,
{
num_regs = aarch64_num_bp_regs;
dr_addr_p = state->dr_addr_bp;
+ dr_addr_orig_p = nullptr;
dr_ctrl_p = state->dr_ctrl_bp;
dr_ref_count = state->dr_ref_count_bp;
}
- ctrl = aarch64_point_encode_ctrl_reg (type, len);
+ ctrl = aarch64_point_encode_ctrl_reg (type, offset, len);
/* Find the entry that matches the ADDR and CTRL. */
for (i = 0; i < num_regs; ++i)
- if (dr_addr_p[i] == addr && dr_ctrl_p[i] == ctrl)
+ if (dr_addr_p[i] == addr
+ && (dr_addr_orig_p == nullptr || dr_addr_orig_p[i] == addr_orig)
+ && dr_ctrl_p[i] == ctrl)
{
gdb_assert (dr_ref_count[i] != 0);
break;
@@ -446,6 +542,8 @@ aarch64_dr_state_remove_one_point (struct aarch64_debug_reg_state *state,
/* Clear the enable bit. */
ctrl &= ~1;
dr_addr_p[i] = 0;
+ if (dr_addr_orig_p != nullptr)
+ dr_addr_orig_p[i] = 0;
dr_ctrl_p[i] = ctrl;
/* Notify the change. */
aarch64_notify_debug_reg_change (state, is_watchpoint, i);
@@ -472,10 +570,10 @@ aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len))
return -1;
- return aarch64_dr_state_insert_one_point (state, type, addr, len);
+ return aarch64_dr_state_insert_one_point (state, type, addr, 0, len, -1);
}
else
- return aarch64_dr_state_remove_one_point (state, type, addr, len);
+ return aarch64_dr_state_remove_one_point (state, type, addr, 0, len, -1);
}
/* This is essentially the same as aarch64_handle_breakpoint, apart
@@ -487,9 +585,9 @@ aarch64_handle_aligned_watchpoint (enum target_hw_bp_type type,
struct aarch64_debug_reg_state *state)
{
if (is_insert)
- return aarch64_dr_state_insert_one_point (state, type, addr, len);
+ return aarch64_dr_state_insert_one_point (state, type, addr, 0, len, addr);
else
- return aarch64_dr_state_remove_one_point (state, type, addr, len);
+ return aarch64_dr_state_remove_one_point (state, type, addr, 0, len, addr);
}
/* Insert/remove unaligned watchpoint by calling
@@ -504,29 +602,42 @@ aarch64_handle_unaligned_watchpoint (enum target_hw_bp_type type,
CORE_ADDR addr, int len, int is_insert,
struct aarch64_debug_reg_state *state)
{
+ CORE_ADDR addr_orig = addr;
+
while (len > 0)
{
CORE_ADDR aligned_addr;
- int aligned_len, ret;
+ int aligned_offset, aligned_len, ret;
+ CORE_ADDR addr_orig_next = addr_orig;
- aarch64_align_watchpoint (addr, len, &aligned_addr, &aligned_len,
- &addr, &len);
+ aarch64_align_watchpoint (addr, len, &aligned_addr, &aligned_offset,
+ &aligned_len, &addr, &len, &addr_orig_next);
if (is_insert)
ret = aarch64_dr_state_insert_one_point (state, type, aligned_addr,
- aligned_len);
+ aligned_offset,
+ aligned_len, addr_orig);
else
ret = aarch64_dr_state_remove_one_point (state, type, aligned_addr,
- aligned_len);
+ aligned_offset,
+ aligned_len, addr_orig);
if (show_debug_regs)
debug_printf ("handle_unaligned_watchpoint: is_insert: %d\n"
" "
"aligned_addr: %s, aligned_len: %d\n"
" "
- "next_addr: %s, next_len: %d\n",
+ "addr_orig: %s\n"
+ " "
+ "next_addr: %s, next_len: %d\n"
+ " "
+ "addr_orig_next: %s\n",
is_insert, core_addr_to_string_nz (aligned_addr),
- aligned_len, core_addr_to_string_nz (addr), len);
+ aligned_len, core_addr_to_string_nz (addr_orig),
+ core_addr_to_string_nz (addr), len,
+ core_addr_to_string_nz (addr_orig_next));
+
+ addr_orig = addr_orig_next;
if (ret != 0)
return ret;
@@ -552,7 +663,7 @@ aarch64_handle_watchpoint (enum target_hw_bp_type type, CORE_ADDR addr,
registers with data from *STATE. */
void
-aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
+aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
int tid, int watchpoint)
{
int i, count;
@@ -580,7 +691,17 @@ aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
if (ptrace (PTRACE_SETREGSET, tid,
watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
(void *) &iov))
- error (_("Unexpected error setting hardware debug registers"));
+ {
+ // Handle PR external/20207 buggy kernels?
+ if (watchpoint && errno == EINVAL && have_any_contiguous)
+ {
+ have_any_contiguous = false;
+ aarch64_downgrade_regs (state);
+ aarch64_linux_set_debug_regs (state, tid, watchpoint);
+ return;
+ }
+ error (_("Unexpected error setting hardware debug registers"));
+ }
}
/* Print the values of the cached breakpoint/watchpoint registers. */
@@ -611,8 +732,9 @@ aarch64_show_debug_reg_state (struct aarch64_debug_reg_state *state,
debug_printf ("\tWATCHPOINTs:\n");
for (i = 0; i < aarch64_num_wp_regs; i++)
- debug_printf ("\tWP%d: addr=%s, ctrl=0x%08x, ref.count=%d\n",
+ debug_printf ("\tWP%d: addr=%s (orig=%s), ctrl=0x%08x, ref.count=%d\n",
i, core_addr_to_string_nz (state->dr_addr_wp[i]),
+ core_addr_to_string_nz (state->dr_addr_orig_wp[i]),
state->dr_ctrl_wp[i], state->dr_ref_count_wp[i]);
}
diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h
index 610a5f1..7c323a2 100644
--- a/gdb/nat/aarch64-linux-hw-point.h
+++ b/gdb/nat/aarch64-linux-hw-point.h
@@ -77,13 +77,13 @@
31 13 5 3 1 0
+--------------------------------+----------+------+------+----+
- | RESERVED (SBZ) | LENGTH | TYPE | PRIV | EN |
+ | RESERVED (SBZ) | MASK | TYPE | PRIV | EN |
+--------------------------------+----------+------+------+----+
The TYPE field is ignored for breakpoints. */
#define DR_CONTROL_ENABLED(ctrl) (((ctrl) & 0x1) == 1)
-#define DR_CONTROL_LENGTH(ctrl) (((ctrl) >> 5) & 0xff)
+#define DR_CONTROL_MASK(ctrl) (((ctrl) >> 5) & 0xff)
/* Each bit of a variable of this type is used to indicate whether a
hardware breakpoint or watchpoint setting has been changed since
@@ -147,7 +147,10 @@ struct aarch64_debug_reg_state
unsigned int dr_ref_count_bp[AARCH64_HBP_MAX_NUM];
/* hardware watchpoint */
+ // Address aligned down to AARCH64_HWP_ALIGNMENT.
CORE_ADDR dr_addr_wp[AARCH64_HWP_MAX_NUM];
+ // Address as entered by user without any 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];
};
@@ -166,6 +169,7 @@ struct arch_lwp_info
extern int aarch64_num_bp_regs;
extern int aarch64_num_wp_regs;
+unsigned int aarch64_watchpoint_offset (unsigned int ctrl);
unsigned int aarch64_watchpoint_length (unsigned int ctrl);
int aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
@@ -175,7 +179,7 @@ int aarch64_handle_watchpoint (enum target_hw_bp_type type, CORE_ADDR addr,
int len, int is_insert,
struct aarch64_debug_reg_state *state);
-void aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
+void aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
int tid, int watchpoint);
void aarch64_show_debug_reg_state (struct aarch64_debug_reg_state *state,
diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.c b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
new file mode 100644
index 0000000..f653e77
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.c
@@ -0,0 +1,68 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2017 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
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ 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, see <http://www.gnu.org/licenses/>. */
+
+#include <stdint.h>
+#include <assert.h>
+
+static int again;
+
+static volatile struct
+{
+ uint64_t alignment;
+ union
+ {
+ uint64_t size8[1];
+ uint32_t size4[2];
+ uint16_t size2[4];
+ uint8_t size1[8];
+ }
+ u;
+} data;
+
+static int size = 0;
+static int offset;
+
+int
+main (void)
+{
+ volatile uint64_t local;
+
+ assert (sizeof (data) == 8 + 8);
+ while (size)
+ {
+ switch (size)
+ {
+ case 8:
+ local = data.u.size8[offset];
+ break;
+ case 4:
+ local = data.u.size4[offset];
+ break;
+ case 2:
+ local = data.u.size2[offset];
+ break;
+ case 1:
+ local = data.u.size1[offset];
+ break;
+ default:
+ assert (0);
+ }
+ size = 0;
+ size = size; /* again_start */
+ }
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
new file mode 100644
index 0000000..8d463b1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
@@ -0,0 +1,119 @@
+# Copyright 2017 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
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# 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.
+#
+# This file is part of the gdb testsuite.
+
+standard_testfile
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+ return -1
+}
+
+if ![runto_main] {
+ untested "could not run to main"
+ return -1
+}
+
+gdb_breakpoint "[gdb_get_line_number "again_start"]" "Breakpoint $decimal at $hex" "again_start"
+
+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} {
+ 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} {
+ set rdstart [expr $rdoffset * $rdsize]
+ set rdend [expr ($rdoffset + 1) * $rdsize]
+ 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 {
+ -re "Hardware read watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
+ set wpnum $expect_out(1,string)
+ }
+ }
+ gdb_test_no_output "set variable size = $rdsize" ""
+ gdb_test_no_output "set variable offset = $rdoffset" ""
+ set test "continue"
+ set did_hit 0
+ gdb_test_multiple $test $test {
+ -re "Hardware read watchpoint $wpnum:.*Value = .*\r\n$gdb_prompt $" {
+ set did_hit 1
+ send_gdb "continue\n"
+ exp_continue
+ }
+ -re " again_start .*\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} {
+ pass $test
+ } else {
+ # We do not know if we run on a fixed kernel or not.
+ # Report XFAIL only in the FAIL case.
+ if {$expect_hit == 0 && $rdstart < $wpendaligned} {
+ setup_xfail external/20207 "aarch64-*-*"
+ }
+ fail $test
+ }
+ }
+ }
+ }
+}
+
+foreach wpcount {4 7} {
+ array set wpoffset_to_wpnum {}
+ for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
+ set test "rwatch data.u.size1\[$wpoffset\]"
+ set wpnum ""
+ gdb_test_multiple $test $test {
+ -re "Hardware read watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" {
+ set wpoffset_to_wpnum($wpoffset) $expect_out(1,string)
+ }
+ }
+ }
+ gdb_test_no_output "set variable size = 1" ""
+ gdb_test_no_output "set variable offset = 1" ""
+ set test "continue"
+ set did_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
+ send_gdb "continue\n"
+ exp_continue
+ }
+ -re " again_start .*\r\n$gdb_prompt $" {
+ }
+ }
+ for {set wpoffset 1} {$wpoffset <= $wpcount} {incr wpoffset} {
+ gdb_test_no_output "delete $wpoffset_to_wpnum($wpoffset)" ""
+ }
+ set test "wpcount($wpcount)"
+ if {$wpcount > 4} {
+ setup_kfail tdep/22389 *-*-*
+ }
+ if {$did_hit} {
+ pass $test
+ } else {
+ fail $test
+ }
+}