This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: ping#2: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 30 Oct 2017 11:27:15 +0000
- Subject: Re: ping#2: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones
- Authentication-results: sourceware.org; auth=none
- References: <20170327210753.GA29656@host1.jankratochvil.net> <20171018195237.GA19714@host1.jankratochvil.net>
Jan Kratochvil <jan.kratochvil@redhat.com> writes:
Hi Jan,
the target description change for sve and regcache changes are sort of
in shape, so I have some chance to look at this patch. Sorry for
delayed response.
> 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. We need a NEWS entry,
since the change is user-visible.
>
> Additionally it now supports any watchpoint masks as described in:
> kernel RFE: aarch64: ptrace: BAS: Support any contiguous range (edit)
> https://sourceware.org/bugzilla/show_bug.cgi?id=20207
> which is already fixed in recent Linux kernels.
>
> 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)
> I have seen (not investigated):
> -FAIL: gdb.threads/thread-specific-bp.exp: non-stop:
> thread-specific breakpoint was deleted (timeout)
> +PASS: gdb.threads/thread-specific-bp.exp: non-stop:
> thread-specific breakpoint was deleted
>
> 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? 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.
> watchpoints merging framework to be used by all the target specific code.
> Even current FSF GDB code does not merge it perfectly. Also with the more
> precise watchpoints one can technically merge them less. And I do not think
> it matters too much to improve mergeability only for old kernels.
> Still even on new kernels some better merging logic would make sense.
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?
> gdb/ChangeLog
> 2017-03-27 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> PR breakpoints/19806 and support for PR external/20207.
> * 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/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 3f5b30e..b26d0a2 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 334310b..abbb43a 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -372,16 +372,33 @@ aarch64_stopped_data_address (void)
>
> /* Check if the address matches any watched address. */
> state = aarch64_get_debug_reg_state (pid_of (current_thread));
> + CORE_ADDR retval (0);
> 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..f8a7938 100644
> --- a/gdb/nat/aarch64-linux-hw-point.c
> +++ b/gdb/nat/aarch64-linux-hw-point.c
> @@ -34,6 +34,26 @@
> 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) 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
> @@ -44,19 +64,21 @@ int aarch64_num_wp_regs;
> unsigned int
> aarch64_watchpoint_length (unsigned int ctrl)
The comments to this function should be updated too.
> {
> - 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;
> }
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.
>
> /* Given the hardware breakpoint or watchpoint type TYPE and its
> @@ -64,10 +86,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 +114,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 +159,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 +207,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 +227,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 +237,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 +250,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 +358,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 +430,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 +438,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 +455,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 +474,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 +496,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 +509,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 +517,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 +544,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 +572,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 +587,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 +604,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 +665,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 +693,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 +734,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..de2206d 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
> @@ -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?
> unsigned int dr_ctrl_wp[AARCH64_HWP_MAX_NUM];
> unsigned int dr_ref_count_wp[AARCH64_HWP_MAX_NUM];
> };
> @@ -166,6 +167,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 +177,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.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> new file mode 100644
> index 0000000..ea8b9e3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp
> @@ -0,0 +1,84 @@
> +# 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.
> +
> +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.
--
Yao (齐尧)