From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113596 invoked by alias); 6 Feb 2019 00:51:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 113487 invoked by uid 89); 6 Feb 2019 00:51:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=nat, Yao, damages, yao X-HELO: userp2130.oracle.com Received: from userp2130.oracle.com (HELO userp2130.oracle.com) (156.151.31.86) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Feb 2019 00:51:37 +0000 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x160nnWm146873; Wed, 6 Feb 2019 00:51:32 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : references : cc : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=DI0fyUljJmvbugY8EEEFcJr8Poai1TFYGH4jyC79cvs=; b=BkE50E/YKtZP6q6qFLW7ZHCwJdN694a09NlRRvDrPat9BzIdElHTfm3QIaBDwwgqRsA/ ox1ncM0OL3DOYthFagbyuv1kaYcfWfcScZAT8uUkyat9wawYxcDPSxA/VBRrl2pQDwuH xREDqXZfKXDRv3+40861GEPQBiuHeO+WpXLIUmLha+7QoRgVwCZCpO85WGiJlCWHfm3g fOEjk4X8NwNKluXtg8Bbb7SZOWfwU8UxfTCWp3vxm7nSNxSv3qY+dE1gHzVdbD8G7pyh JVALS+xb6j/byQoYWYI6iEot1mm9Cs9a0cTySicR2dWd+GLAG/Ny5rON+9dVrtYkZzLh CQ== Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by userp2130.oracle.com with ESMTP id 2qd9arebp6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 06 Feb 2019 00:51:32 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id x160pVOa028612 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 6 Feb 2019 00:51:31 GMT Received: from abhmp0006.oracle.com (abhmp0006.oracle.com [141.146.116.12]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x160pUse020153; Wed, 6 Feb 2019 00:51:30 GMT Received: from [10.132.97.56] (/10.132.97.56) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 06 Feb 2019 00:51:30 +0000 Subject: Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers To: Alan.Hayward@arm.com References: <1530148222-12558-1-git-send-email-weimin.pan@oracle.com> <145f2e8d-4321-00a6-650a-bf8f0a483b6f@oracle.com> Cc: "gdb-patches@sourceware.org" From: Weimin Pan Message-ID: Date: Wed, 06 Feb 2019 00:51:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: <145f2e8d-4321-00a6-650a-bf8f0a483b6f@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2019-02/txt/msg00032.txt.bz2 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  >> + >> +    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  >>         * 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  >> + >> +    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  >>         * 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 >> +#ifdef __ia64__ >> +#undef ia64_fpreg >> +#undef pt_all_user_regs >> +#endif  /* __ia64__ */ >> +#include >> +#include >> +#include >> +#if defined __i386__ || defined __x86_64__ >> +#include >> +#endif >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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 . >> +# >> +# 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" >> +    } >> +} >