This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



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.




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]