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

Alan Hayward
2019-02-12T14:46:41Z

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