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

[Alan Hayward <Alan.Hayward@arm.com>]

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