[PING] [PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[PING] [PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan]

On 7/3/2018 00:07 AM, 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, with a non-zero value, in aarch64_linux_set_debug_regs()
> and is similar to what x86 does - x86_linux_update_debug_registers()
> checks whether or not 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-28  Weimin Pan  <weimin.pan@oracle.com>
> +
> +	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  <tom@tromey.com>
>   
>   	* 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-28  Weimin Pan  <weimin.pan@oracle.com>
> +
> +	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  <tdevries@suse.de>
>   
>   	* 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 <sys/ptrace.h>
> +#ifdef __ia64__
> +#undef ia64_fpreg
> +#undef pt_all_user_regs
> +#endif  /* __ia64__ */
> +#include <linux/ptrace.h>
> +#include <sys/types.h>
> +#include <sys/user.h>
> +#if defined __i386__ || defined __x86_64__
> +#include <sys/debugreg.h>
> +#endif
> +
> +#include <assert.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <errno.h>
> +#include <sys/uio.h>
> +#include <elf.h>
> +#include <error.h>
> +
> +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 <http://www.gnu.org/licenses/>.
> +#
> +# 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"
> +    }
> +}

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Weimin Pan]

Would you please review this aarch64-specific patch?  Thanks.

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  <weimin.pan@oracle.com>
>> +
>> +    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  <tom@tromey.com>
>>         * 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  <weimin.pan@oracle.com>
>> +
>> +    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  <tdevries@suse.de>
>>         * 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 <sys/ptrace.h>
>> +#ifdef __ia64__
>> +#undef ia64_fpreg
>> +#undef pt_all_user_regs
>> +#endif  /* __ia64__ */
>> +#include <linux/ptrace.h>
>> +#include <sys/types.h>
>> +#include <sys/user.h>
>> +#if defined __i386__ || defined __x86_64__
>> +#include <sys/debugreg.h>
>> +#endif
>> +
>> +#include <assert.h>
>> +#include <unistd.h>
>> +#include <sys/wait.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stddef.h>
>> +#include <errno.h>
>> +#include <sys/uio.h>
>> +#include <elf.h>
>> +#include <error.h>
>> +
>> +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 <http://www.gnu.org/licenses/>.
>> +#
>> +# 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"
>> +    }
>> +}
>

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Alan Hayward]

> On 6 Feb 2019, at 00:51, Weimin Pan <weimin.pan@oracle.com> 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 <alan.hayward@arm.com>
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  <weimin.pan@oracle.com>
>>> +
>>> +    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  <tom@tromey.com>
>>>         * 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  <weimin.pan@oracle.com>
>>> +
>>> +    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  <tdevries@suse.de>
>>>         * 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 <sys/ptrace.h>
>>> +#ifdef __ia64__
>>> +#undef ia64_fpreg
>>> +#undef pt_all_user_regs
>>> +#endif  /* __ia64__ */
>>> +#include <linux/ptrace.h>
>>> +#include <sys/types.h>
>>> +#include <sys/user.h>
>>> +#if defined __i386__ || defined __x86_64__
>>> +#include <sys/debugreg.h>
>>> +#endif
>>> +
>>> +#include <assert.h>
>>> +#include <unistd.h>
>>> +#include <sys/wait.h>
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <stddef.h>
>>> +#include <errno.h>
>>> +#include <sys/uio.h>
>>> +#include <elf.h>
>>> +#include <error.h>
>>> +
>>> +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 <http://www.gnu.org/licenses/>.
>>> +#
>>> +# 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"
>>> +    }
>>> +}
>> 

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan]

On 2/6/2019 4:43 AM, Alan Hayward wrote:
 >
 >
 >> On 6 Feb 2019, at 00:51, Weimin Pan <weimin.pan@oracle.com> 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.

Thanks for pointing it out, noted.

 > 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 <alan.hayward@arm.com>
 > 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?

Yes, your patch does fix the problem.

 >
 > I've not yet read the test case in detail. If the bug is already fixed,
 > then the test might still be useful.

I can push the test, which is included in the bug report, upstream.

Thanks.

> 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  <weimin.pan@oracle.com>
>>>> +
>>>> +    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  <tom@tromey.com>
>>>>          * 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  <weimin.pan@oracle.com>
>>>> +
>>>> +    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  <tdevries@suse.de>
>>>>          * 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 <sys/ptrace.h>
>>>> +#ifdef __ia64__
>>>> +#undef ia64_fpreg
>>>> +#undef pt_all_user_regs
>>>> +#endif  /* __ia64__ */
>>>> +#include <linux/ptrace.h>
>>>> +#include <sys/types.h>
>>>> +#include <sys/user.h>
>>>> +#if defined __i386__ || defined __x86_64__
>>>> +#include <sys/debugreg.h>
>>>> +#endif
>>>> +
>>>> +#include <assert.h>
>>>> +#include <unistd.h>
>>>> +#include <sys/wait.h>
>>>> +#include <stdio.h>
>>>> +#include <stdlib.h>
>>>> +#include <stddef.h>
>>>> +#include <errno.h>
>>>> +#include <sys/uio.h>
>>>> +#include <elf.h>
>>>> +#include <error.h>
>>>> +
>>>> +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 <http://www.gnu.org/licenses/>.
>>>> +#
>>>> +# 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"
>>>> +    }
>>>> +}

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Alan Hayward]

> On 6 Feb 2019, at 22:36, Wei-min Pan <weimin.pan@oracle.com> wrote:
> 
> On 2/6/2019 4:43 AM, Alan Hayward wrote:
> >
> >
> >> On 6 Feb 2019, at 00:51, Weimin Pan <weimin.pan@oracle.com> 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.
> 
> Thanks for pointing it out, noted.
> 
> > 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 <alan.hayward@arm.com>
> > 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?
> 
> Yes, your patch does fix the problem.

Excellent!

> 
> >
> > I've not yet read the test case in detail. If the bug is already fixed,
> > then the test might still be useful.
> 
> I can push the test, which is included in the bug report, upstream.

Ok - I'll review the test case either tomorrow or early next week.


Alan.

> 
> Thanks.
> 
>> 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  <weimin.pan@oracle.com>
>>>>> +
>>>>> +    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  <tom@tromey.com>
>>>>>         * 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  <weimin.pan@oracle.com>
>>>>> +
>>>>> +    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  <tdevries@suse.de>
>>>>>         * 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 <sys/ptrace.h>
>>>>> +#ifdef __ia64__
>>>>> +#undef ia64_fpreg
>>>>> +#undef pt_all_user_regs
>>>>> +#endif  /* __ia64__ */
>>>>> +#include <linux/ptrace.h>
>>>>> +#include <sys/types.h>
>>>>> +#include <sys/user.h>
>>>>> +#if defined __i386__ || defined __x86_64__
>>>>> +#include <sys/debugreg.h>
>>>>> +#endif
>>>>> +
>>>>> +#include <assert.h>
>>>>> +#include <unistd.h>
>>>>> +#include <sys/wait.h>
>>>>> +#include <stdio.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <stddef.h>
>>>>> +#include <errno.h>
>>>>> +#include <sys/uio.h>
>>>>> +#include <elf.h>
>>>>> +#include <error.h>
>>>>> +
>>>>> +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 <http://www.gnu.org/licenses/>.
>>>>> +#
>>>>> +# 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"
>>>>> +    }
>>>>> +}

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan]

On 2/7/2019 4:49 AM, Alan Hayward wrote:
>
>> On 6 Feb 2019, at 22:36, Wei-min Pan <weimin.pan@oracle.com> wrote:
>>
>> On 2/6/2019 4:43 AM, Alan Hayward wrote:
>>>
>>>> On 6 Feb 2019, at 00:51, Weimin Pan <weimin.pan@oracle.com> 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.
>> Thanks for pointing it out, noted.
>>
>>> 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 <alan.hayward@arm.com>
>>> 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?
>> Yes, your patch does fix the problem.
> Excellent!
>
>>> I've not yet read the test case in detail. If the bug is already fixed,
>>> then the test might still be useful.
>> I can push the test, which is included in the bug report, upstream.
> Ok - I'll review the test case either tomorrow or early next week.

OK, thanks.

>
>
> Alan.
>
>> Thanks.
>>
>>> 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  <weimin.pan@oracle.com>
>>>>>> +
>>>>>> +    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  <tom@tromey.com>
>>>>>>          * 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  <weimin.pan@oracle.com>
>>>>>> +
>>>>>> +    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  <tdevries@suse.de>
>>>>>>          * 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 <sys/ptrace.h>
>>>>>> +#ifdef __ia64__
>>>>>> +#undef ia64_fpreg
>>>>>> +#undef pt_all_user_regs
>>>>>> +#endif  /* __ia64__ */
>>>>>> +#include <linux/ptrace.h>
>>>>>> +#include <sys/types.h>
>>>>>> +#include <sys/user.h>
>>>>>> +#if defined __i386__ || defined __x86_64__
>>>>>> +#include <sys/debugreg.h>
>>>>>> +#endif
>>>>>> +
>>>>>> +#include <assert.h>
>>>>>> +#include <unistd.h>
>>>>>> +#include <sys/wait.h>
>>>>>> +#include <stdio.h>
>>>>>> +#include <stdlib.h>
>>>>>> +#include <stddef.h>
>>>>>> +#include <errno.h>
>>>>>> +#include <sys/uio.h>
>>>>>> +#include <elf.h>
>>>>>> +#include <error.h>
>>>>>> +
>>>>>> +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 <http://www.gnu.org/licenses/>.
>>>>>> +#
>>>>>> +# 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"
>>>>>> +    }
>>>>>> +}

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Alan Hayward]

>>>> 
>>>> 
>>>>> On 7/11/2018 5:30 PM, Wei-min Pan wrote:
>>>>>> On 6/27/2018 6:10 PM, Weimin Pan wrote:
>>>>>>> 

<SNIP>

>>>>>>>   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 <sys/ptrace.h>
>>>>>>> +#ifdef __ia64__
>>>>>>> +#undef ia64_fpreg
>>>>>>> +#undef pt_all_user_regs
>>>>>>> +#endif  /* __ia64__ */
>>>>>>> +#include <linux/ptrace.h>
>>>>>>> +#include <sys/types.h>
>>>>>>> +#include <sys/user.h>
>>>>>>> +#if defined __i386__ || defined __x86_64__
>>>>>>> +#include <sys/debugreg.h>
>>>>>>> +#endif
>>>>>>> +

I’m not sure why you have all the x86 and IA64 checks.
The test will only be executed on AArch64 (because of the checks in the .exp file).
Could you remove all of those checks please.

>>>>>>> +#include <assert.h>
>>>>>>> +#include <unistd.h>
>>>>>>> +#include <sys/wait.h>
>>>>>>> +#include <stdio.h>
>>>>>>> +#include <stdlib.h>
>>>>>>> +#include <stddef.h>
>>>>>>> +#include <errno.h>
>>>>>>> +#include <sys/uio.h>
>>>>>>> +#include <elf.h>
>>>>>>> +#include <error.h>
>>>>>>> +
>>>>>>> +static __attribute__((unused)) pid_t child;
>>>>>>> +
>>>>>>> +static __attribute__((unused)) void

Why are these marked as "static __attribute__((unused))” ?

>>>>>>> +cleanup (void)
>>>>>>> +{
>>>>>>> +  if (child > 0)
>>>>>>> +    kill (child, SIGKILL);
>>>>>>> +  child = 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static __attribute__((unused)) void

Same as above.

>>>>>>> +handler_fail (int signo)
>>>>>>> +{
>>>>>>> +  cleanup ();
>>>>>>> +  signal (signo, SIG_DFL);
>>>>>>> +  raise (signo);
>>>>>>> +}
>>>>>>> +
>>>>>>> +#ifdef __aarch64__

Again, as before, you shouldn’t need this check. If the test is only run
on AArch64 then it isn’t needed.

>>>>>>> +
>>>>>>> +#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);

Remove the commented out code.

>>>>>>> +  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

Having the executable exit with error on not AArch64 is not useful.
Again, this can be cut.


>>>>>>> +
>>>>>>> +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);

I’m not sure on the point of the handler_fail?
Would the test be simpler without them?

>>>>>>> +
>>>>>>> +  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:

Remove the commented out code.

>>>>>>> +  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);
>>>>>>> +

It’s not immediately clear to me what is going on above.
A few comments are probably needed to make it clear:
*Add a watchpoint to check.
*Restart the child. It will write to check.
*Check child has stopped on the watchpoint.

>>>>>>> +  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.

2019

>>>>>>> +
>>>>>>> +# This program is free software; you can redistribute it and/or modify
>>>>>>> +# it under the terms of the GNU General Public License as published by
>>>>>>> +# the Free Software Foundation; either version 3 of the License, or
>>>>>>> +# (at your option) any later version.
>>>>>>> +#
>>>>>>> +# This program is distributed in the hope that it will be useful,
>>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>>> +# GNU General Public License for more details.
>>>>>>> +#
>>>>>>> +# You should have received a copy of the GNU General Public License
>>>>>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>>>> +#
>>>>>>> +# Make sure that the inferior doesn't assert and exits successfully.

Could you expand the description here please.  Add something to state the error
case being tested. Something like:

“This test checks that GDB does not alter watchpoints set by an inferior. The
tests sets a watchpoint on memory then writes to the watched memory. It will exit
with 1 if the watchpoint is not reached."

Also add a reference to the PR bug number in the correct format.

>>>>>>> +
>>>>>>> +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"
>>>>>>> +    }

The whole .exp is a little strange as it doesn’t do anything - but - it’s
probably the easiest way to test the bug. So, I’m ok with this.

>>>>>>> +}


Alan.

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Weimin Pan]

[-- Attachment #1: Type: text/plain, Size: 10260 bytes --]


On 2/11/2019 7:24 AM, Alan Hayward wrote:
>
>>>>>>>>    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 <sys/ptrace.h>
>>>>>>>> +#ifdef __ia64__
>>>>>>>> +#undef ia64_fpreg
>>>>>>>> +#undef pt_all_user_regs
>>>>>>>> +#endif  /* __ia64__ */
>>>>>>>> +#include <linux/ptrace.h>
>>>>>>>> +#include <sys/types.h>
>>>>>>>> +#include <sys/user.h>
>>>>>>>> +#if defined __i386__ || defined __x86_64__
>>>>>>>> +#include <sys/debugreg.h>
>>>>>>>> +#endif
>>>>>>>> +
> I’m not sure why you have all the x86 and IA64 checks.
> The test will only be executed on AArch64 (because of the checks in the .exp file).
> Could you remove all of those checks please.

The test case is likely to have been used for other targets as well. 
I've removed
all non-aarch64 code and unused header files.

>>>>>>>> +#include <assert.h>
>>>>>>>> +#include <unistd.h>
>>>>>>>> +#include <sys/wait.h>
>>>>>>>> +#include <stdio.h>
>>>>>>>> +#include <stdlib.h>
>>>>>>>> +#include <stddef.h>
>>>>>>>> +#include <errno.h>
>>>>>>>> +#include <sys/uio.h>
>>>>>>>> +#include <elf.h>
>>>>>>>> +#include <error.h>
>>>>>>>> +
>>>>>>>> +static __attribute__((unused)) pid_t child;
>>>>>>>> +
>>>>>>>> +static __attribute__((unused)) void
> Why are these marked as "static __attribute__((unused))” ?

It instructs GCC not to produce a warning if the function is unused.

>>>>>>>> +cleanup (void)
>>>>>>>> +{
>>>>>>>> +  if (child > 0)
>>>>>>>> +    kill (child, SIGKILL);
>>>>>>>> +  child = 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static __attribute__((unused)) void
> Same as above.
>
>>>>>>>> +handler_fail (int signo)
>>>>>>>> +{
>>>>>>>> +  cleanup ();
>>>>>>>> +  signal (signo, SIG_DFL);
>>>>>>>> +  raise (signo);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +#ifdef __aarch64__
> Again, as before, you shouldn’t need this check. If the test is only run
> on AArch64 then it isn’t needed.
Done.
>
>>>>>>>> +
>>>>>>>> +#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);
> Remove the commented out code.
>
>>>>>>>> +  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
> Having the executable exit with error on not AArch64 is not useful.
> Again, this can be cut.
>
>
>>>>>>>> +
>>>>>>>> +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);
> I’m not sure on the point of the handler_fail?
> Would the test be simpler without them?
Yes, and the function is removed.
>
>>>>>>>> +
>>>>>>>> +  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:
> Remove the commented out code.
>
>>>>>>>> +  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);
>>>>>>>> +
> It’s not immediately clear to me what is going on above.
> A few comments are probably needed to make it clear:
> *Add a watchpoint to check.
> *Restart the child. It will write to check.
> *Check child has stopped on the watchpoint.
>
>>>>>>>> +  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.
> 2019
>
>>>>>>>> +
>>>>>>>> +# This program is free software; you can redistribute it and/or modify
>>>>>>>> +# it under the terms of the GNU General Public License as published by
>>>>>>>> +# the Free Software Foundation; either version 3 of the License, or
>>>>>>>> +# (at your option) any later version.
>>>>>>>> +#
>>>>>>>> +# This program is distributed in the hope that it will be useful,
>>>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>>>> +# GNU General Public License for more details.
>>>>>>>> +#
>>>>>>>> +# You should have received a copy of the GNU General Public License
>>>>>>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>>>>> +#
>>>>>>>> +# Make sure that the inferior doesn't assert and exits successfully.
> Could you expand the description here please.  Add something to state the error
> case being tested. Something like:
>
> “This test checks that GDB does not alter watchpoints set by an inferior. The
> tests sets a watchpoint on memory then writes to the watched memory. It will exit
> with 1 if the watchpoint is not reached."
>
> Also add a reference to the PR bug number in the correct format.

Done.

I'm attaching the revised patch. Thanks for your comments.

>>>>>>>> +
>>>>>>>> +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"
>>>>>>>> +    }
> The whole .exp is a little strange as it doesn’t do anything - but - it’s
> probably the easiest way to test the bug. So, I’m ok with this.
>
>>>>>>>> +}
>
> Alan.
>
>

[-- Attachment #2: 0001-Adding-a-test-case.patch --]
[-- Type: text/plain, Size: 6434 bytes --]

From 69499587c60a2ae24aa358f40e17ae8f92750d0a Mon Sep 17 00:00:00 2001
From: Weimin Pan <weimin.pan@oracle.com>
Date: Mon, 11 Feb 2019 16:07:09 -0800
Subject: [PATCH PR gdb/21870] aarch64: Leftover uncleared debug registers

Adding a test case

gdb/testsuite/ChangeLog
2019-02-11  Weimin Pan  <weimin.pan@oracle.com>

            PR breakpoints/21870
            * gdb.arch/aarch64-dbreg-contents.exp: New file.
            * gdb.arch/aarch64-dbreg-contents.c: New file.
---
 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c   | 129 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp |  46 ++++++++
 2 files changed, 175 insertions(+)
 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/testsuite/gdb.arch/aarch64-dbreg-contents.c b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
new file mode 100644
index 0000000..753037d
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.c
@@ -0,0 +1,129 @@
+
+/* 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
+#include <sys/ptrace.h>
+#include <assert.h>
+#include <sys/wait.h>
+#include <stddef.h>
+#include <errno.h>
+#include <sys/uio.h>
+#include <elf.h>
+
+static __attribute__((unused)) pid_t child;
+
+static __attribute__((unused)) void
+cleanup (void)
+{
+  if (child > 0)
+    kill (child, SIGKILL);
+  child = 0;
+}
+
+#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
+  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);
+}
+
+static volatile long long check;
+
+int
+main (void)
+{
+  pid_t got_pid;
+  int i, status;
+  long l;
+
+  atexit (cleanup);
+
+  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);
+
+  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;
+}
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..f6bee02
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-dbreg-contents.exp
@@ -0,0 +1,46 @@
+# Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Make sure that the inferior doesn't assert and exits successfully.
+#
+# This test checks that GDB does not alter watchpoints set by an inferior.
+# It sets a watchpoint on memory then writes to the watched memory.
+# It will exit with 1 if the watchpoint is not reached.
+#
+# See PR breakpoints/21870.
+
+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"
+    }
+}
-- 
1.9.1

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Alan Hayward]

> On 12 Feb 2019, at 01:10, Weimin Pan <weimin.pan@oracle.com> wrote:
> 
> 
> On 2/11/2019 7:24 AM, Alan Hayward wrote:
>> 
>>>>>>>>>   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 <sys/ptrace.h>
>>>>>>>>> +#ifdef __ia64__
>>>>>>>>> +#undef ia64_fpreg
>>>>>>>>> +#undef pt_all_user_regs
>>>>>>>>> +#endif  /* __ia64__ */
>>>>>>>>> +#include <linux/ptrace.h>
>>>>>>>>> +#include <sys/types.h>
>>>>>>>>> +#include <sys/user.h>
>>>>>>>>> +#if defined __i386__ || defined __x86_64__
>>>>>>>>> +#include <sys/debugreg.h>
>>>>>>>>> +#endif
>>>>>>>>> +
>> I’m not sure why you have all the x86 and IA64 checks.
>> The test will only be executed on AArch64 (because of the checks in the .exp file).
>> Could you remove all of those checks please.
> 
> The test case is likely to have been used for other targets as well. I've removed
> all non-aarch64 code and unused header files.
> 
>>>>>>>>> +#include <assert.h>
>>>>>>>>> +#include <unistd.h>
>>>>>>>>> +#include <sys/wait.h>
>>>>>>>>> +#include <stdio.h>
>>>>>>>>> +#include <stdlib.h>
>>>>>>>>> +#include <stddef.h>
>>>>>>>>> +#include <errno.h>
>>>>>>>>> +#include <sys/uio.h>
>>>>>>>>> +#include <elf.h>
>>>>>>>>> +#include <error.h>
>>>>>>>>> +
>>>>>>>>> +static __attribute__((unused)) pid_t child;
>>>>>>>>> +
>>>>>>>>> +static __attribute__((unused)) void
>> Why are these marked as "static __attribute__((unused))” ?
> 
> It instructs GCC not to produce a warning if the function is unused.

Now that you have removed the x86/ia64 code, you should be able to
remove the static attribute too.

> 
>>>>>>>>> +cleanup (void)
>>>>>>>>> +{
>>>>>>>>> +  if (child > 0)
>>>>>>>>> +    kill (child, SIGKILL);
>>>>>>>>> +  child = 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static __attribute__((unused)) void
>> Same as above.
>> 
>>>>>>>>> +handler_fail (int signo)
>>>>>>>>> +{
>>>>>>>>> +  cleanup ();
>>>>>>>>> +  signal (signo, SIG_DFL);
>>>>>>>>> +  raise (signo);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +#ifdef __aarch64__
>> Again, as before, you shouldn’t need this check. If the test is only run
>> on AArch64 then it isn’t needed.
> Done.
>> 
>>>>>>>>> +
>>>>>>>>> +#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);
>> Remove the commented out code.
>> 
>>>>>>>>> +  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
>> Having the executable exit with error on not AArch64 is not useful.
>> Again, this can be cut.
>> 
>> 
>>>>>>>>> +
>>>>>>>>> +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);
>> I’m not sure on the point of the handler_fail?
>> Would the test be simpler without them?
> Yes, and the function is removed.
>> 
>>>>>>>>> +
>>>>>>>>> +  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:
>> Remove the commented out code.
>> 
>>>>>>>>> +  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);
>>>>>>>>> +
>> It’s not immediately clear to me what is going on above.
>> A few comments are probably needed to make it clear:
>> *Add a watchpoint to check.
>> *Restart the child. It will write to check.
>> *Check child has stopped on the watchpoint.

Could you add some comments to the C file please.


>> 
> <0001-Adding-a-test-case.patch>

Attached patch looks ok.
Happy for you to push if you make the two changes above.


Thanks,
Alan.

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Weimin Pan]

On 2/12/2019 6:46 AM, Alan Hayward wrote:
>
>> On 12 Feb 2019, at 01:10, Weimin Pan <weimin.pan@oracle.com> wrote:
>>
>>
>> On 2/11/2019 7:24 AM, Alan Hayward wrote:
>>>>>>>>>>    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 <sys/ptrace.h>
>>>>>>>>>> +#ifdef __ia64__
>>>>>>>>>> +#undef ia64_fpreg
>>>>>>>>>> +#undef pt_all_user_regs
>>>>>>>>>> +#endif  /* __ia64__ */
>>>>>>>>>> +#include <linux/ptrace.h>
>>>>>>>>>> +#include <sys/types.h>
>>>>>>>>>> +#include <sys/user.h>
>>>>>>>>>> +#if defined __i386__ || defined __x86_64__
>>>>>>>>>> +#include <sys/debugreg.h>
>>>>>>>>>> +#endif
>>>>>>>>>> +
>>> I’m not sure why you have all the x86 and IA64 checks.
>>> The test will only be executed on AArch64 (because of the checks in the .exp file).
>>> Could you remove all of those checks please.
>> The test case is likely to have been used for other targets as well. I've removed
>> all non-aarch64 code and unused header files.
>>
>>>>>>>>>> +#include <assert.h>
>>>>>>>>>> +#include <unistd.h>
>>>>>>>>>> +#include <sys/wait.h>
>>>>>>>>>> +#include <stdio.h>
>>>>>>>>>> +#include <stdlib.h>
>>>>>>>>>> +#include <stddef.h>
>>>>>>>>>> +#include <errno.h>
>>>>>>>>>> +#include <sys/uio.h>
>>>>>>>>>> +#include <elf.h>
>>>>>>>>>> +#include <error.h>
>>>>>>>>>> +
>>>>>>>>>> +static __attribute__((unused)) pid_t child;
>>>>>>>>>> +
>>>>>>>>>> +static __attribute__((unused)) void
>>> Why are these marked as "static __attribute__((unused))” ?
>> It instructs GCC not to produce a warning if the function is unused.
> Now that you have removed the x86/ia64 code, you should be able to
> remove the static attribute too.
>
>>>>>>>>>> +cleanup (void)
>>>>>>>>>> +{
>>>>>>>>>> +  if (child > 0)
>>>>>>>>>> +    kill (child, SIGKILL);
>>>>>>>>>> +  child = 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static __attribute__((unused)) void
>>> Same as above.
>>>
>>>>>>>>>> +handler_fail (int signo)
>>>>>>>>>> +{
>>>>>>>>>> +  cleanup ();
>>>>>>>>>> +  signal (signo, SIG_DFL);
>>>>>>>>>> +  raise (signo);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +#ifdef __aarch64__
>>> Again, as before, you shouldn’t need this check. If the test is only run
>>> on AArch64 then it isn’t needed.
>> Done.
>>>>>>>>>> +
>>>>>>>>>> +#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);
>>> Remove the commented out code.
>>>
>>>>>>>>>> +  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
>>> Having the executable exit with error on not AArch64 is not useful.
>>> Again, this can be cut.
>>>
>>>
>>>>>>>>>> +
>>>>>>>>>> +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);
>>> I’m not sure on the point of the handler_fail?
>>> Would the test be simpler without them?
>> Yes, and the function is removed.
>>>>>>>>>> +
>>>>>>>>>> +  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:
>>> Remove the commented out code.
>>>
>>>>>>>>>> +  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);
>>>>>>>>>> +
>>> It’s not immediately clear to me what is going on above.
>>> A few comments are probably needed to make it clear:
>>> *Add a watchpoint to check.
>>> *Restart the child. It will write to check.
>>> *Check child has stopped on the watchpoint.
> Could you add some comments to the C file please.
>
>
>> <0001-Adding-a-test-case.patch>
> Attached patch looks ok.
> Happy for you to push if you make the two changes above.

Made these two changes and pushed. Thanks.

>
>
> Thanks,
> Alan.
>
>

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Pedro Alves]

On 02/12/2019 01:10 AM, Weimin Pan wrote:
> +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"
> +    }
> +}

A naked "run" command doesn't work when testing against
gdbserver with --target_board=native-gdbserver.

Is "run" important here?  Could this use runto_main + "continue" instead?

Also, the comment at the top of the file says:

 # This test checks that GDB does not alter watchpoints set by an inferior.
 # It sets a watchpoint on memory then writes to the watched memory.
 # It will exit with 1 if the watchpoint is not reached.

But I couldn't spot where that "exit with 1" happens in the .c file.
Also, when that happens, we're issuing a pass, as seen above.
Is that intended?

Thanks,
Pedro Alves

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan]

On 2/13/2019 3:40 AM, Pedro Alves wrote:
> On 02/12/2019 01:10 AM, Weimin Pan wrote:
>> +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"
>> +    }
>> +}
> A naked "run" command doesn't work when testing against
> gdbserver with --target_board=native-gdbserver.
>
> Is "run" important here?  Could this use runto_main + "continue" instead?

As long as the test run doesn't assert, we could instead use
"runto_main + "continue". Thanks for pointing this out.

> Also, the comment at the top of the file says:
>
>   # This test checks that GDB does not alter watchpoints set by an inferior.
>   # It sets a watchpoint on memory then writes to the watched memory.
>   # It will exit with 1 if the watchpoint is not reached.
>
> But I couldn't spot where that "exit with 1" happens in the .c file.

You are right, it should be "exit with 2".

> Also, when that happens, we're issuing a pass, as seen above.
> Is that intended?

"Exit with 1" could happen if the PTRACE_SETREGSET call should
fail which is ok as long as it doesn't cause assertion.

Thanks for your comments.

>
> Thanks,
> Pedro Alves

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Pedro Alves]

On 02/13/2019 09:56 PM, Wei-min Pan wrote:
> 
> On 2/13/2019 3:40 AM, Pedro Alves wrote:
>> On 02/12/2019 01:10 AM, Weimin Pan wrote:
>>> +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"
>>> +    }
>>> +}
>> A naked "run" command doesn't work when testing against
>> gdbserver with --target_board=native-gdbserver.
>>
>> Is "run" important here?  Could this use runto_main + "continue" instead?
> 
> As long as the test run doesn't assert, we could instead use
> "runto_main + "continue". Thanks for pointing this out.

OOC, why does asserting make a difference?

> 
>> Also, the comment at the top of the file says:
>>
>>   # This test checks that GDB does not alter watchpoints set by an inferior.
>>   # It sets a watchpoint on memory then writes to the watched memory.
>>   # It will exit with 1 if the watchpoint is not reached.
>>
>> But I couldn't spot where that "exit with 1" happens in the .c file.
> 
> You are right, it should be "exit with 2".
> 
>> Also, when that happens, we're issuing a pass, as seen above.
>> Is that intended?
> 
> "Exit with 1" could happen if the PTRACE_SETREGSET call should
> fail which is ok as long as it doesn't cause assertion.

I see.  Could you add some comments, please?

Could you send a patch?

BTW:

 - why is the SET_WATCHPOINT macro necessary?

 - what does the ??? below mean?

    dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0

 - The code has:

  atexit (cleanup);

  (...)

  but then also calls cleanup() return returning (twice, end of main),
  which seems unnecessary?

 - While at it, the gdb_compile + clean_restart in the .exp file
   could be replaced with a single prepare_for_testing call.

> 
> Thanks for your comments.
> 

Thanks,
Pedro Alves

Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan]

On 2/14/2019 5:01 AM, Pedro Alves wrote:
> On 02/13/2019 09:56 PM, Wei-min Pan wrote:
>> On 2/13/2019 3:40 AM, Pedro Alves wrote:
>>> On 02/12/2019 01:10 AM, Weimin Pan wrote:
>>>> +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"
>>>> +    }
>>>> +}
>>> A naked "run" command doesn't work when testing against
>>> gdbserver with --target_board=native-gdbserver.
>>>
>>> Is "run" important here?  Could this use runto_main + "continue" instead?
>> As long as the test run doesn't assert, we could instead use
>> "runto_main + "continue". Thanks for pointing this out.
> OOC, why does asserting make a difference?
>
>>> Also, the comment at the top of the file says:
>>>
>>>    # This test checks that GDB does not alter watchpoints set by an inferior.
>>>    # It sets a watchpoint on memory then writes to the watched memory.
>>>    # It will exit with 1 if the watchpoint is not reached.
>>>
>>> But I couldn't spot where that "exit with 1" happens in the .c file.
>> You are right, it should be "exit with 2".
>>
>>> Also, when that happens, we're issuing a pass, as seen above.
>>> Is that intended?
>> "Exit with 1" could happen if the PTRACE_SETREGSET call should
>> fail which is ok as long as it doesn't cause assertion.
> I see.  Could you add some comments, please?

Done.

> Could you send a patch?

Just pushed the updated patch.


> BTW:
>
>   - why is the SET_WATCHPOINT macro necessary?

It's not necessary. Removed.

>
>   - what does the ??? below mean?
>
>      dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0

Removed the "GDB: ???:" part.

>
>   - The code has:
>
>    atexit (cleanup);
>
>    (...)
>
>    but then also calls cleanup() return returning (twice, end of main),
>    which seems unnecessary?

You're right, the second call is redundant and is removed.

>
>   - While at it, the gdb_compile + clean_restart in the .exp file
>     could be replaced with a single prepare_for_testing call.

Nice. Replaced.

Thanks.

>
>> Thanks for your comments.
>>
> Thanks,
> Pedro Alves