From patchwork Tue May 1 23:06:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kratochvil X-Patchwork-Id: 27069 Received: (qmail 36025 invoked by alias); 1 May 2018 23:06:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 36010 invoked by uid 89); 1 May 2018 23:06:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-22.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT, SPF_HELO_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=STATE, hunting, n1 X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 01 May 2018 23:06:10 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 67DF2407574E; Tue, 1 May 2018 23:06:09 +0000 (UTC) Received: from host1.jankratochvil.net (unknown [10.36.118.9]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3EF787C3C; Tue, 1 May 2018 23:06:08 +0000 (UTC) Date: Wed, 2 May 2018 01:06:06 +0200 From: Jan Kratochvil To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones Message-ID: <20180501230606.GA3138797@host1.jankratochvil.net> References: <20170327210753.GA29656@host1.jankratochvil.net> <20171018195237.GA19714@host1.jankratochvil.net> <867evczxik.fsf@gmail.com> <20171103220437.GA13979@host1.jankratochvil.net> <20180321190316.GA32598@host1.jankratochvil.net> <1e06eb53-60f4-0800-a4f6-458e02f840bd@redhat.com> <20180426201216.GA218540@host1.jankratochvil.net> <20180501230339.GA3136080@host1.jankratochvil.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180501230339.GA3136080@host1.jankratochvil.net> User-Agent: Mutt/1.9.2 (2017-12-15) On Wed, 02 May 2018 01:03:39 +0200, Jan Kratochvil wrote: > On Thu, 26 Apr 2018 22:12:16 +0200, Jan Kratochvil wrote: > > On Fri, 20 Apr 2018 16:49:39 +0200, Pedro Alves wrote: > > > ~~~~~~~~~~~~ > > > 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. > > > > This is new for me what you found. I just did not expect the changed region > > region could be larger than aligned 8 bytes. > > > > Unfortunately I cannot reproduce that so I cannot do much with that. > > Does anyone know how to reproduce it? > > Attaching the change I made, I will repost the patch in a next mail. Here is the new patch, if that new testcase PASS: gdb.base/watchpoint-unaligned.exp: size8twice write fails for anyone I can check how to avoid it. Thanks, Jan gdb/ChangeLog: yyyy-mm-dd Jan Kratochvil Pedro Alves 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 Pedro Alves * linux-aarch64-low.c (aarch64_stopped_data_address): Likewise. gdb/testsuite/ChangeLog: yyyy-mm-dd Jan Kratochvil Pedro Alves 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 6193070023..46f6635dda 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -42,6 +42,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 diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index 9385659f14..421d044e10 100644 --- a/gdb/aarch64-linux-nat.c +++ b/gdb/aarch64-linux-nat.c @@ -735,16 +735,38 @@ 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 = 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] && 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 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 80de826ba7..8d839d10fa 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 5408c35469..7bc6e90f05 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 eccac4da13..7ea24c2363 100644 --- a/gdb/gdbserver/linux-aarch64-low.c +++ b/gdb/gdbserver/linux-aarch64-low.c @@ -360,14 +360,39 @@ 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 = 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] && 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 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; + } } return (CORE_ADDR) 0; diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c index ce26f28fad..a3931ea6a9 100644 --- a/gdb/nat/aarch64-linux-hw-point.c +++ b/gdb/nat/aarch64-linux-hw-point.c @@ -34,29 +34,52 @@ int aarch64_num_bp_regs; int aarch64_num_wp_regs; +/* 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. */ + +unsigned int +aarch64_watchpoint_offset (unsigned int ctrl) +{ + uint8_t mask = DR_CONTROL_MASK (ctrl); + unsigned retval; + + /* Shift out bottom zeros. */ + 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. The returned + value can be between 0..8 (inclusive). */ 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 zeros. */ + mask >>= aarch64_watchpoint_offset (ctrl); + + /* Count bottom ones. */ + for (retval = 0; (mask & 1) != 0; ++retval) + mask >>= 1; + + if (mask != 0) + 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 +87,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 || kernel_supports_any_contiguous_range); + gdb_assert (offset + len <= AARCH64_HWP_MAX_LEN_PER_REG); + /* type */ switch (type) { @@ -89,8 +115,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,58 +160,65 @@ 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 ((!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, - 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; @@ -196,10 +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 + = kernel_supports_any_contiguous_range ? addr & (alignment - 1) : 0; gdb_assert (offset >= 0 && offset < alignment); gdb_assert (aligned_addr >= 0 && aligned_addr <= addr); @@ -207,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; + 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); @@ -222,19 +258,24 @@ 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 = (kernel_supports_any_contiguous_range + ? 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 = align_down (*next_addr_orig_p + alignment, alignment); } struct aarch64_dr_update_callback_param @@ -324,17 +365,73 @@ 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 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) +{ + 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); + 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; + 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] + = align_down (state->dr_addr_wp[i], 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 +440,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 +448,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 +465,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 +484,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 +506,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 +519,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 +527,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 +554,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 +582,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 +597,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 +614,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 +675,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 +703,18 @@ 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 Linux kernels with the PR external/20207 bug. */ + if (watchpoint && errno == EINVAL + && kernel_supports_any_contiguous_range) + { + kernel_supports_any_contiguous_range = 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 +745,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 7c42b96d1b..1940b06a89 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 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]; }; @@ -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 0000000000..97832a05d4 --- /dev/null +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.c @@ -0,0 +1,89 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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 + 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 . */ + +#include +#include + +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]; + uint64_t size8twice[2]; + } + u; +} data; + +static int size = 0; +static int offset; + +static void +write_size8twice (void) +{ + static const uint64_t first = 1; + static const uint64_t second = 2; + +#ifdef __aarch64__ + asm volatile ("stp %1, %2, [%0]" + : /* output */ + : "r" (data.u.size8twice), "r" (first), "r" (second) /* input */ + : "memory" /* clobber */); +#else + data.u.size8twice[0] = first; + data.u.size8twice[1] = second; +#endif +} + +int +main (void) +{ + volatile uint64_t local; + + assert (sizeof (data) == 8 + 2 * 8); + + write_size8twice (); + + 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; /* start_again */ + } + return 0; /* final_return */ +} diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp new file mode 100644 index 0000000000..25a15a89fd --- /dev/null +++ b/gdb/testsuite/gdb.base/watchpoint-unaligned.exp @@ -0,0 +1,142 @@ +# 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 +# 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 . +# +# 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 +} + +if ![runto_main] { + untested "could not run to main" + return -1 +} + +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} { + 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 got_hit 0 + gdb_test_multiple $test $test { + -re "Hardware read watchpoint $wpnum:.*Value = .*\r\n$gdb_prompt $" { + set got_hit 1 + send_gdb "continue\n" + exp_continue + } + -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 == $got_hit} { + pass $test + } else { + # 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*-*-linux*" + } + 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 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 got_hit 1 + send_gdb "continue\n" + exp_continue + } + -re " start_again .*\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 *-*-* + } + gdb_assert $got_hit $test +} + +if ![runto_main] { + return -1 +} +gdb_breakpoint [gdb_get_line_number "final_return"] "Breakpoint $decimal at $hex" "final_return" +set test {watch data.u.size8twice[1]} +set wpnum "" +gdb_test_multiple $test $test { + -re "Hardware watchpoint (\[0-9\]+): .*\r\n$gdb_prompt $" { + set wpnum $expect_out(1,string) + } +} +set test "continue" +set got_hit 0 +gdb_test_multiple $test $test { + -re "\r\nCould not insert hardware watchpoint .*\r\n$gdb_prompt $" { + } + -re "Hardware watchpoint $wpnum:.*New value = .*\r\n$gdb_prompt $" { + set got_hit 1 + send_gdb "continue\n" + exp_continue + } + -re " final_return .*\r\n$gdb_prompt $" { + } +} +gdb_assert $got_hit "size8twice write" diff --git a/gdb/utils.c b/gdb/utils.c index b957b0dc5c..e274f02676 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 4dec889d83..c728449429 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