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

Weimin Pan
2019-02-12T01:10:23Z

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