From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90564 invoked by alias); 30 Oct 2017 11:27:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 90549 invoked by uid 89); 30 Oct 2017 11:27:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS,UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=watched, hunting, Therefore, temple X-HELO: mail-wr0-f170.google.com Received: from mail-wr0-f170.google.com (HELO mail-wr0-f170.google.com) (209.85.128.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Oct 2017 11:27:26 +0000 Received: by mail-wr0-f170.google.com with SMTP id z55so12208422wrz.1 for ; Mon, 30 Oct 2017 04:27:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=LzXlxRRamgKZ93/uqbB8j0Apo+UzxUA5OqYX+KuohLA=; b=KX6QJs0j8cWJCc1Cy0SIa2avyLhdX/O7n2KFrvXMvodu+78CSOWFcajS2oHZdkcQjx LNLarL37YOkOffPDuvYn+PGcM1JuWfWsPSdI5KiF0JKBk36ujynKB45MrPbQaOB1Ox9A EWnuxvyhFKCV6ZKkQclHYMdG/MUEJpCwS2hcir5VzWlL0Yx5F+LstBqi6dgAxpAUeWdN A5f2uOwr5lZ53FhuHwJUxdqIrFitgfTeQcqySPK6lLpuyFghKcfE7un1h1oF/ExEM2Fo z29VPUGSqW7eJTVNoJeK72gapqdUU6xv/U2shodJZtizHlJJ3gZLdMTIGpT5wd6hEZQy DxHw== X-Gm-Message-State: AMCzsaUSt69Yw6SzUTghdGXD/CtnwSjPef1dq9Bboetig9IpTTxpSMC/ Z+5hyM7EOivrOt0a83/nC0TwfA== X-Google-Smtp-Source: ABhQp+SRKtGVzEQHOskd7hqg9AuH6tva4K2/ozMluJQ4VFLEcg9pklrCUolsQsX8KuChg41v9Bp4XQ== X-Received: by 10.223.134.250 with SMTP id 55mr7708542wry.249.1509362843376; Mon, 30 Oct 2017 04:27:23 -0700 (PDT) Received: from E107787-LIN (static.42.136.251.148.clients.your-server.de. [148.251.136.42]) by smtp.gmail.com with ESMTPSA id o197sm11061869wmg.3.2017.10.30.04.27.21 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 30 Oct 2017 04:27:22 -0700 (PDT) From: Yao Qi To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: ping#2: [patch] aarch64: PR 19806: watchpoints: false negatives + PR 20207 contiguous ones References: <20170327210753.GA29656@host1.jankratochvil.net> <20171018195237.GA19714@host1.jankratochvil.net> Date: Mon, 30 Oct 2017 11:27:00 -0000 In-Reply-To: <20171018195237.GA19714@host1.jankratochvil.net> (Jan Kratochvil's message of "Wed, 18 Oct 2017 21:52:37 +0200") Message-ID: <867evczxik.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00878.txt.bz2 Jan Kratochvil 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)=20 > https://sourceware.org/bugzilla/show_bug.cgi?id=3D20207 > 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 watchpo= int > 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 th= ink > 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 supp= ose? > gdb/ChangeLog > 2017-03-27 Jan Kratochvil > > 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 > > 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_o= ps *target, > state =3D aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid)); > for (i =3D aarch64_num_wp_regs - 1; i >=3D 0; --i) > { > + const unsigned int offset =3D aarch64_watchpoint_offset > + (state->dr_ctrl_wp[i]); > const unsigned int len =3D aarch64_watchpoint_length (state->dr_ct= rl_wp[i]); > const CORE_ADDR addr_trap =3D (CORE_ADDR) siginfo.si_addr; > - const CORE_ADDR addr_watch =3D state->dr_addr_wp[i]; > + const CORE_ADDR addr_watch =3D state->dr_addr_wp[i] + offset; > + const CORE_ADDR addr_watch_aligned =3D (state->dr_addr_wp[i] > + & -(CORE_ADDR) 8); > + const CORE_ADDR addr_orig =3D state->dr_addr_orig_wp[i]; >=20=20 > if (state->dr_ref_count_wp[i] > && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]) > - && addr_trap >=3D addr_watch > + && addr_trap >=3D addr_watch_aligned > && addr_trap < addr_watch + len) > { > - *addr_p =3D 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 <=3D ADDR_TRAP < ADDR_ORIG is a false positive due to > + PR external/20207 buggy kernels. */ > + *addr_p =3D addr_orig; > return 1; > } > } > diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarc= h64-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) >=20=20 > /* Check if the address matches any watched address. */ > state =3D aarch64_get_debug_reg_state (pid_of (current_thread)); > + CORE_ADDR retval (0); > for (i =3D aarch64_num_wp_regs - 1; i >=3D 0; --i) > { > + const unsigned int offset =3D aarch64_watchpoint_offset > + (state->dr_ctrl_wp[i]); > const unsigned int len =3D aarch64_watchpoint_length (state->dr_ct= rl_wp[i]); > const CORE_ADDR addr_trap =3D (CORE_ADDR) siginfo.si_addr; > - const CORE_ADDR addr_watch =3D state->dr_addr_wp[i]; > + const CORE_ADDR addr_watch =3D state->dr_addr_wp[i] + offset; > + const CORE_ADDR addr_watch_aligned =3D (state->dr_addr_wp[i] > + & -(CORE_ADDR) 8); > + const CORE_ADDR addr_orig =3D state->dr_addr_orig_wp[i]; > + > if (state->dr_ref_count_wp[i] > && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]) > - && addr_trap >=3D addr_watch > + && addr_trap >=3D 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 <=3D ADDR_TRAP < ADDR_ORIG is a false positive due to > + PR external/20207 buggy kernels. */ > + return addr_orig; > + } > } >=20=20 > 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; >=20=20 > +// 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 0xf= f. > +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 =3D DR_CONTROL_MASK (ctrl); > + unsigned retval; > + > + // Shift out bottom zeroes. > + for (retval =3D 0; mask && (mask & 1) =3D=3D 0; ++retval) > + mask >>=3D 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 =3D DR_CONTROL_MASK (ctrl); > + unsigned retval; > + > + // Shift out bottom zeroes. > + mask >>=3D aarch64_watchpoint_offset (ctrl); > + > + // Count bottom ones; > + for (retval =3D 0; (mask & 1) !=3D 0; ++retval) > + mask >>=3D 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. >=20=20 > /* Given the hardware breakpoint or watchpoint type TYPE and its > @@ -64,10 +86,13 @@ aarch64_watchpoint_length (unsigned int ctrl) > breakpoint/watchpoint control register. */ >=20=20 > 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; >=20=20 > + gdb_assert (offset =3D=3D 0 || have_any_contiguous); > + gdb_assert (offset + len <=3D 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) >=20=20 > ctrl =3D ttype << 3; >=20=20 > - /* length bitmask */ > - ctrl |=3D ((1 << len) - 1) << 5; > + /* offset and length bitmask */ > + ctrl |=3D ((1 << len) - 1) << (5 + offset); > /* enabled at el0 */ > ctrl |=3D (2 << 1) | 1; >=20=20 > @@ -134,7 +159,8 @@ aarch64_point_is_aligned (int is_watchpoint, CORE_ADD= R addr, int len) > if (addr & (alignment - 1)) > return 0; >=20=20 > - if (len !=3D 8 && len !=3D 4 && len !=3D 2 && len !=3D 1) > + if ((!have_any_contiguous && len !=3D 8 && len !=3D 4 && len !=3D 2 &&= len !=3D 1) > + || (have_any_contiguous && (len < 1 || len > 8))) > return 0; >=20=20 > return 1; > @@ -181,11 +207,12 @@ aarch64_point_is_aligned (int is_watchpoint, CORE_A= DDR addr, int len) >=20=20 > static void > aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_ad= dr_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 =3D AARCH64_HWP_ALIGNMENT; > const unsigned int max_wp_len =3D AARCH64_HWP_MAX_LEN_PER_REG; > @@ -200,6 +227,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CO= RE_ADDR *aligned_addr_p, > must be aligned. */ > offset =3D addr & (alignment - 1); > aligned_addr =3D addr - offset; > + aligned_offset =3D have_any_contiguous ? addr & (alignment - 1) : 0; >=20=20 > gdb_assert (offset >=3D 0 && offset < alignment); > gdb_assert (aligned_addr >=3D 0 && aligned_addr <=3D addr); > @@ -209,7 +237,7 @@ aarch64_align_watchpoint (CORE_ADDR addr, int len, CO= RE_ADDR *aligned_addr_p, > { > /* Need more than one watchpoint registers; truncate it at the > alignment boundary. */ > - aligned_len =3D max_wp_len; > + aligned_len =3D max_wp_len - (have_any_contiguous ? offset : 0); > len -=3D (max_wp_len - offset); > addr +=3D (max_wp_len - offset); > gdb_assert ((addr & (alignment - 1)) =3D=3D 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] =3D > { 1, 2, 4, 4, 8, 8, 8, 8 }; >=20=20 > - aligned_len =3D aligned_len_array[offset + len - 1]; > + aligned_len =3D (have_any_contiguous > + ? len : aligned_len_array[offset + len - 1]); > addr +=3D len; > len =3D 0; > } >=20=20 > if (aligned_addr_p) > *aligned_addr_p =3D aligned_addr; > + if (aligned_offset_p) > + *aligned_offset_p =3D aligned_offset; > if (aligned_len_p) > *aligned_len_p =3D aligned_len; > if (next_addr_p) > *next_addr_p =3D addr; > if (next_len_p) > *next_len_p =3D len; > + if (next_addr_orig_p) > + *next_addr_orig_p =3D (((*next_addr_orig_p) + alignment) > + & -(CORE_ADDR) alignment); > } >=20=20 > struct aarch64_dr_update_callback_param > @@ -324,17 +358,70 @@ aarch64_notify_debug_reg_change (const struct aarch= 64_debug_reg_state *state, > iterate_over_lwps (pid_ptid, debug_reg_change_callback, (void *) ¶= m); > } >=20=20 > +/* 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 =3D 0; i < aarch64_num_wp_regs; ++i) > + if ((state->dr_ctrl_wp[i] & 1) !=3D 0) > + { > + gdb_assert (state->dr_ref_count_wp[i] !=3D 0); > + uint8_t mask_orig =3D (state->dr_ctrl_wp[i] >> 5) & 0xff; > + gdb_assert (mask_orig !=3D 0); > + const std::array old_valid ({ 0x01, 0x03, 0x0f, 0xff }); > + uint8_t mask (0); > + for (const uint8_t old_mask:old_valid) > + if (mask_orig <=3D old_mask) > + { > + mask =3D old_mask; > + break; > + } > + gdb_assert (mask !=3D 0); > + > + // No update needed for this watchpoint? > + if (mask =3D=3D mask_orig) > + continue; > + state->dr_ctrl_wp[i] |=3D mask << 5; > + state->dr_addr_wp[i] &=3D -(CORE_ADDR) AARCH64_HWP_ALIGNMENT; > + > + // Try to match duplicate entries. > + for (int j =3D 0; j < i; ++j) > + if ((state->dr_ctrl_wp[j] & 1) !=3D 0 > + && state->dr_addr_wp[j] =3D=3D state->dr_addr_wp[i] > + && state->dr_addr_orig_wp[j] =3D=3D state->dr_addr_orig_wp[i] > + && state->dr_ctrl_wp[j] =3D=3D state->dr_ctrl_wp[i]) > + { > + state->dr_ref_count_wp[j] +=3D state->dr_ref_count_wp[i]; > + state->dr_ref_count_wp[i] =3D 0; > + state->dr_addr_wp[i] =3D 0; > + state->dr_addr_orig_wp[i] =3D 0; > + state->dr_ctrl_wp[i] &=3D ~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. */ >=20=20 > 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; >=20=20 > /* Set up state pointers. */ > is_watchpoint =3D (type !=3D hw_execute); > @@ -343,6 +430,7 @@ aarch64_dr_state_insert_one_point (struct aarch64_deb= ug_reg_state *state, > { > num_regs =3D aarch64_num_wp_regs; > dr_addr_p =3D state->dr_addr_wp; > + dr_addr_orig_p =3D state->dr_addr_orig_wp; > dr_ctrl_p =3D state->dr_ctrl_wp; > dr_ref_count =3D state->dr_ref_count_wp; > } > @@ -350,11 +438,12 @@ aarch64_dr_state_insert_one_point (struct aarch64_d= ebug_reg_state *state, > { > num_regs =3D aarch64_num_bp_regs; > dr_addr_p =3D state->dr_addr_bp; > + dr_addr_orig_p =3D nullptr; > dr_ctrl_p =3D state->dr_ctrl_bp; > dr_ref_count =3D state->dr_ref_count_bp; > } >=20=20 > - ctrl =3D aarch64_point_encode_ctrl_reg (type, len); > + ctrl =3D aarch64_point_encode_ctrl_reg (type, offset, len); >=20=20 > /* Find an existing or free register in our cache. */ > idx =3D -1; > @@ -366,7 +455,9 @@ aarch64_dr_state_insert_one_point (struct aarch64_deb= ug_reg_state *state, > idx =3D i; > /* no break; continue hunting for an exising one. */ > } > - else if (dr_addr_p[i] =3D=3D addr && dr_ctrl_p[i] =3D=3D ctrl) > + else if (dr_addr_p[i] =3D=3D addr > + && (dr_addr_orig_p =3D=3D nullptr || dr_addr_orig_p[i] =3D=3D ad= dr_orig) > + && dr_ctrl_p[i] =3D=3D ctrl) > { > gdb_assert (dr_ref_count[i] !=3D 0); > idx =3D i; > @@ -383,6 +474,8 @@ aarch64_dr_state_insert_one_point (struct aarch64_deb= ug_reg_state *state, > { > /* new entry */ > dr_addr_p[idx] =3D addr; > + if (dr_addr_orig_p !=3D nullptr) > + dr_addr_orig_p[idx] =3D addr_orig; > dr_ctrl_p[idx] =3D ctrl; > dr_ref_count[idx] =3D 1; > /* Notify the change. */ > @@ -403,11 +496,12 @@ aarch64_dr_state_insert_one_point (struct aarch64_d= ebug_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; >=20=20 > /* Set up state pointers. */ > is_watchpoint =3D (type !=3D hw_execute); > @@ -415,6 +509,7 @@ aarch64_dr_state_remove_one_point (struct aarch64_deb= ug_reg_state *state, > { > num_regs =3D aarch64_num_wp_regs; > dr_addr_p =3D state->dr_addr_wp; > + dr_addr_orig_p =3D state->dr_addr_orig_wp; > dr_ctrl_p =3D state->dr_ctrl_wp; > dr_ref_count =3D state->dr_ref_count_wp; > } > @@ -422,15 +517,18 @@ aarch64_dr_state_remove_one_point (struct aarch64_d= ebug_reg_state *state, > { > num_regs =3D aarch64_num_bp_regs; > dr_addr_p =3D state->dr_addr_bp; > + dr_addr_orig_p =3D nullptr; > dr_ctrl_p =3D state->dr_ctrl_bp; > dr_ref_count =3D state->dr_ref_count_bp; > } >=20=20 > - ctrl =3D aarch64_point_encode_ctrl_reg (type, len); > + ctrl =3D aarch64_point_encode_ctrl_reg (type, offset, len); >=20=20 > /* Find the entry that matches the ADDR and CTRL. */ > for (i =3D 0; i < num_regs; ++i) > - if (dr_addr_p[i] =3D=3D addr && dr_ctrl_p[i] =3D=3D ctrl) > + if (dr_addr_p[i] =3D=3D addr > + && (dr_addr_orig_p =3D=3D nullptr || dr_addr_orig_p[i] =3D=3D addr_orig) > + && dr_ctrl_p[i] =3D=3D ctrl) > { > gdb_assert (dr_ref_count[i] !=3D 0); > break; > @@ -446,6 +544,8 @@ aarch64_dr_state_remove_one_point (struct aarch64_deb= ug_reg_state *state, > /* Clear the enable bit. */ > ctrl &=3D ~1; > dr_addr_p[i] =3D 0; > + if (dr_addr_orig_p !=3D nullptr) > + dr_addr_orig_p[i] =3D 0; > dr_ctrl_p[i] =3D 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 t= ype, CORE_ADDR addr, > if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len)) > return -1; >=20=20 > - return aarch64_dr_state_insert_one_point (state, type, addr, len); > + return aarch64_dr_state_insert_one_point (state, type, addr, 0, le= n, -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); > } >=20=20 > /* 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); > } >=20=20 > /* 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 =3D addr; > + > while (len > 0) > { > CORE_ADDR aligned_addr; > - int aligned_len, ret; > + int aligned_offset, aligned_len, ret; > + CORE_ADDR addr_orig_next =3D addr_orig; >=20=20 > - aarch64_align_watchpoint (addr, len, &aligned_addr, &aligned_len, > - &addr, &len); > + aarch64_align_watchpoint (addr, len, &aligned_addr, &aligned_offse= t, > + &aligned_len, &addr, &len, &addr_orig_next); >=20=20 > if (is_insert) > ret =3D aarch64_dr_state_insert_one_point (state, type, aligned_addr, > - aligned_len); > + aligned_offset, > + aligned_len, addr_orig); > else > ret =3D aarch64_dr_state_remove_one_point (state, type, aligned_addr, > - aligned_len); > + aligned_offset, > + aligned_len, addr_orig); >=20=20 > 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 =3D addr_orig_next; >=20=20 > if (ret !=3D 0) > return ret; > @@ -552,7 +665,7 @@ aarch64_handle_watchpoint (enum target_hw_bp_type typ= e, CORE_ADDR addr, > registers with data from *STATE. */ >=20=20 > void > -aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *stat= e, > +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_d= ebug_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 =3D=3D EINVAL && have_any_contiguous) > + { > + have_any_contiguous =3D false; > + aarch64_downgrade_regs (state); > + aarch64_linux_set_debug_regs (state, tid, watchpoint); > + return; > + } > + error (_("Unexpected error setting hardware debug registers")); > + } > } >=20=20 > /* Print the values of the cached breakpoint/watchpoint registers. */ > @@ -611,8 +734,9 @@ aarch64_show_debug_reg_state (struct aarch64_debug_re= g_state *state, >=20=20 > debug_printf ("\tWATCHPOINTs:\n"); > for (i =3D 0; i < aarch64_num_wp_regs; i++) > - debug_printf ("\tWP%d: addr=3D%s, ctrl=3D0x%08x, ref.count=3D%d\n", > + debug_printf ("\tWP%d: addr=3D%s (orig=3D%s), ctrl=3D0x%08x, ref.cou= nt=3D%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]); > } >=20=20 > 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 @@ >=20=20 > 31 13 5 3 1 0 > +--------------------------------+----------+------+------+----+ > - | RESERVED (SBZ) | LENGTH | TYPE | PRIV | EN | > + | RESERVED (SBZ) | MASK | TYPE | PRIV | EN | > +--------------------------------+----------+------+------+----+ >=20=20 > The TYPE field is ignored for breakpoints. */ >=20=20 > #define DR_CONTROL_ENABLED(ctrl) (((ctrl) & 0x1) =3D=3D 1) > -#define DR_CONTROL_LENGTH(ctrl) (((ctrl) >> 5) & 0xff) > +#define DR_CONTROL_MASK(ctrl) (((ctrl) >> 5) & 0xff) >=20=20 > /* 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 >=20=20 > /* 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; >=20=20 > +unsigned int aarch64_watchpoint_offset (unsigned int ctrl); > unsigned int aarch64_watchpoint_length (unsigned int ctrl); >=20=20 > int aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR ad= dr, > @@ -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); >=20=20 > -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); >=20=20 > void aarch64_show_debug_reg_state (struct aarch64_debug_reg_state *state, > diff --git a/gdb/testsuite/gdb.base/watchpoint-unaligned.exp b/gdb/testsu= ite/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. --=20 Yao (=E9=BD=90=E5=B0=A7)