> On 6 Feb 2019, at 00:51, Weimin Pan wrote: > > Would you please review this aarch64-specific patch? Thanks. Had a few problems applying the patch directly. Also, the changelog entry should be in the body of the email and not the patch. But, ignoring that for now... I ran the test case with the current HEAD on AArch64, and it passed. I think that your issue might have already been fixed with the following patch: commit 754e31689866524049b9cfc68053ed4e1293cfac Author: Alan Hayward Date: 9 weeks ago AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread My patch is roughly doing the same thing, but in a different way. Could you check this please? I’ve not yet read the test case in detail. If the bug is already fixed, then the test might still be useful. Alan. > > On 7/11/2018 5:30 PM, Wei-min Pan wrote: >> >> On 6/27/2018 6:10 PM, Weimin Pan wrote: >>> At issue here is that ptrace() does not validate either address or size >>> when setting a hardware watchpoint/breakpoint. As a result, watchpoints >>> were set at address 0, the initial value of aarch64_debug_reg_state in >>> aarch64_process_info, and DR_CONTROL_LENGTH (dreg_state.dbg_regs[i].ctrl) >>> got changed to a non-zero default value, when the PTRACE_SETREGSET request >>> was made in aarch64_linux_set_debug_regs(), in preparation for resuming >>> the thread. >>> >>> I sent out a patch upstream for review last year. Yao Qi commented that >>> he needed to take a look at ptrace implementation to better understand >>> why this was happening. Unfortunately he didn't get a chance to do so. >>> >>> This is a revised patch which calls ptrace only if any debug register >>> has changed, either address or control containing a non-zero value, >>> in aarch64_linux_set_debug_regs() and is similar to what x86 does - >>> x86_linux_update_debug_registers() checks if a debug register has >>> non-zero reference count before setting it. >>> >>> Tested on aarch64-linux-gnu. No regressions. >>> --- >>> gdb/ChangeLog | 6 + >>> gdb/nat/aarch64-linux-hw-point.c | 7 + >>> gdb/testsuite/ChangeLog | 20 ++- >>> gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c | 179 ++++++++++++++++++++++ >>> gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp | 40 +++++ >>> 5 files changed, 244 insertions(+), 8 deletions(-) >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c >>> create mode 100644 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp >>> >>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog >>> index 7b108bf..d5bf15f 100644 >>> --- a/gdb/ChangeLog >>> +++ b/gdb/ChangeLog >>> @@ -1,3 +1,9 @@ >>> +2018-06-27 Weimin Pan >>> + >>> + PR gdb/21870 >>> + * nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs): >>> + Validate debug register contents before calling ptrace. >>> + >>> 2018-06-18 Tom Tromey >>> * solib-aix.c (solib_aix_get_section_offsets): Return >>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c >>> index a3931ea..41ac304 100644 >>> --- a/gdb/nat/aarch64-linux-hw-point.c >>> +++ b/gdb/nat/aarch64-linux-hw-point.c >>> @@ -694,11 +694,18 @@ aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state, >>> iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs) >>> + count * sizeof (regs.dbg_regs[0])); >>> + int changed_regs = 0; >>> for (i = 0; i < count; i++) >>> { >>> regs.dbg_regs[i].addr = addr[i]; >>> regs.dbg_regs[i].ctrl = ctrl[i]; >>> + /* Check to see if any of these debug registers contains valid data, >>> + e.g. non-zero, before calling ptrace. See PR gdb/21870. */ >>> + if (ctrl[i] || addr[i]) >>> + changed_regs++; >>> } >>> + if (changed_regs == 0) >>> + return; >>> if (ptrace (PTRACE_SETREGSET, tid, >>> watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK, >>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog >>> index a812061..0ce2d34 100644 >>> --- a/gdb/testsuite/ChangeLog >>> +++ b/gdb/testsuite/ChangeLog >>> @@ -1,4 +1,9 @@ >>> +2018-06-27 Weimin Pan >>> + >>> + PR gdb/21870 >>> + * gdb.arch/aarch64-dbreg-contents.c: New file. >>> + * gdb.arch/aarch64-dbreg-contents.exp: New file. >>> + >>> 2018-06-18 Tom de Vries >>> * gdb.ada/bp_inlined_func.exp: Allow 5 breakpoint locations. >>> diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c >>> new file mode 100644 >>> index 0000000..85d4a03 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c >>> @@ -0,0 +1,179 @@ >>> +/* Test case for setting a memory-write unaligned watchpoint on aarch64. >>> + >>> + This software is provided 'as-is', without any express or implied >>> + warranty. In no event will the authors be held liable for any damages >>> + arising from the use of this software. >>> + >>> + Permission is granted to anyone to use this software for any purpose, >>> + including commercial applications, and to alter it and redistribute it >>> + freely. */ >>> + >>> +#define _GNU_SOURCE 1 >>> +#ifdef __ia64__ >>> +#define ia64_fpreg ia64_fpreg_DISABLE >>> +#define pt_all_user_regs pt_all_user_regs_DISABLE >>> +#endif /* __ia64__ */ >>> +#include >>> +#ifdef __ia64__ >>> +#undef ia64_fpreg >>> +#undef pt_all_user_regs >>> +#endif /* __ia64__ */ >>> +#include >>> +#include >>> +#include >>> +#if defined __i386__ || defined __x86_64__ >>> +#include >>> +#endif >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +static __attribute__((unused)) pid_t child; >>> + >>> +static __attribute__((unused)) void >>> +cleanup (void) >>> +{ >>> + if (child > 0) >>> + kill (child, SIGKILL); >>> + child = 0; >>> +} >>> + >>> +static __attribute__((unused)) void >>> +handler_fail (int signo) >>> +{ >>> + cleanup (); >>> + signal (signo, SIG_DFL); >>> + raise (signo); >>> +} >>> + >>> +#ifdef __aarch64__ >>> + >>> +#define SET_WATCHPOINT set_watchpoint >>> + >>> +/* Macros to extract fields from the hardware debug information word. */ >>> +#define AARCH64_DEBUG_NUM_SLOTS(x) ((x) & 0xff) >>> +#define AARCH64_DEBUG_ARCH(x) (((x) >> 8) & 0xff) >>> +/* Macro for the expected version of the ARMv8-A debug architecture. */ >>> +#define AARCH64_DEBUG_ARCH_V8 0x6 >>> +#define DR_CONTROL_ENABLED(ctrl) (((ctrl) & 0x1) == 1) >>> +#define DR_CONTROL_LENGTH(ctrl) (((ctrl) >> 5) & 0xff) >>> + >>> +static void >>> +set_watchpoint (pid_t pid, volatile void *addr, unsigned len_mask) >>> +{ >>> + struct user_hwdebug_state dreg_state; >>> + struct iovec iov; >>> + long l; >>> + >>> + assert (len_mask >= 0x01); >>> + assert (len_mask <= 0xff); >>> + >>> + iov.iov_base = &dreg_state; >>> + iov.iov_len = sizeof (dreg_state); >>> + errno = 0; >>> + l = ptrace (PTRACE_GETREGSET, pid, NT_ARM_HW_WATCH, &iov); >>> + assert (l == 0); >>> + assert (AARCH64_DEBUG_ARCH (dreg_state.dbg_info) == AARCH64_DEBUG_ARCH_V8); >>> + assert (AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info) >= 1); >>> + >>> + assert (!DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl)); >>> + dreg_state.dbg_regs[0].ctrl |= 1; >>> + assert ( DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl)); >>> + >>> + assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == 0); >>> + dreg_state.dbg_regs[0].ctrl |= len_mask << 5; >>> + assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) == len_mask); >>> + >>> + dreg_state.dbg_regs[0].ctrl |= 2 << 3; // write >>> + dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0 >>> + //printf("ctrl=0x%x\n",dreg_state.dbg_regs[0].ctrl); >>> + dreg_state.dbg_regs[0].addr = (uintptr_t) addr; >>> + >>> + iov.iov_base = &dreg_state; >>> + iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs) >>> + + sizeof (dreg_state.dbg_regs[0])); >>> + errno = 0; >>> + l = ptrace (PTRACE_SETREGSET, pid, NT_ARM_HW_WATCH, &iov); >>> + if (errno != 0) >>> + error (1, errno, "PTRACE_SETREGSET: NT_ARM_HW_WATCH"); >>> + assert (l == 0); >>> +} >>> + >>> +#endif >>> + >>> +#ifndef SET_WATCHPOINT >>> + >>> +int >>> +main (void) >>> +{ >>> + return 77; >>> +} >>> +#else >>> + >>> +static volatile long long check; >>> + >>> +int >>> +main (void) >>> +{ >>> + pid_t got_pid; >>> + int i, status; >>> + long l; >>> + >>> + atexit (cleanup); >>> + signal (SIGABRT, handler_fail); >>> + signal (SIGINT, handler_fail); >>> + >>> + child = fork (); >>> + assert (child >= 0); >>> + if (child == 0) >>> + { >>> + l = ptrace (PTRACE_TRACEME, 0, NULL, NULL); >>> + assert (l == 0); >>> + i = raise (SIGUSR1); >>> + assert (i == 0); >>> + check = -1; >>> + i = raise (SIGUSR2); >>> + /* NOTREACHED */ >>> + assert (0); >>> + } >>> + >>> + got_pid = waitpid (child, &status, 0); >>> + assert (got_pid == child); >>> + assert (WIFSTOPPED (status)); >>> + assert (WSTOPSIG (status) == SIGUSR1); >>> + >>> + // PASS: >>> + //SET_WATCHPOINT (child, &check, 0xff); >>> + // FAIL: >>> + SET_WATCHPOINT (child, &check, 0x02); >>> + >>> + errno = 0; >>> + l = ptrace (PTRACE_CONT, child, 0l, 0l); >>> + assert_perror (errno); >>> + assert (l == 0); >>> + >>> + got_pid = waitpid (child, &status, 0); >>> + assert (got_pid == child); >>> + assert (WIFSTOPPED (status)); >>> + if (WSTOPSIG (status) == SIGUSR2) >>> + { >>> + /* We missed the watchpoint - unsupported by hardware? */ >>> + cleanup (); >>> + return 2; >>> + } >>> + assert (WSTOPSIG (status) == SIGTRAP); >>> + >>> + cleanup (); >>> + return 0; >>> +} >>> + >>> +#endif >>> + >>> diff --git a/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp >>> new file mode 100644 >>> index 0000000..6aa23b0 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp >>> @@ -0,0 +1,40 @@ >>> +# Copyright 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 . >>> +# >>> +# Make sure that the inferior doesn't assert and exits successfully. >>> + >>> +if {![is_aarch64_target]} { >>> + verbose "Skipping ${gdb_test_file_name}." >>> + return >>> +} >>> + >>> +standard_testfile .c >>> + >>> +if { [gdb_compile ${srcdir}/${subdir}/${srcfile} ${binfile} executable {debug}] != "" } { >>> + untested "failed to compile" >>> + return -1 >>> +} >>> + >>> +clean_restart $testfile >>> + >>> +set test "run to exit" >>> +gdb_test_multiple "run" "$test" { >>> + -re "exited with code 01.*$gdb_prompt $" { >>> + pass "$test" >>> + } >>> + -re "exited normally.*$gdb_prompt $" { >>> + pass "$test" >>> + } >>> +} >> &j!z޶םib֫rnr