From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3779 invoked by alias); 7 Feb 2019 21:39:39 -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 3761 invoked by uid 89); 7 Feb 2019 21:39:38 -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= X-HELO: aserp2130.oracle.com Received: from aserp2130.oracle.com (HELO aserp2130.oracle.com) (141.146.126.79) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 07 Feb 2019 21:39:35 +0000 Received: from pps.filterd (aserp2130.oracle.com [127.0.0.1]) by aserp2130.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x17LXqA3170818; Thu, 7 Feb 2019 21:39:30 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=1zEkKiYMQR73j3qD9WAtASYmk/TOOU/p+QIDL2dx7tQ=; b=qLdc9Uj9vqDyf/6Cq8p71uMbir2ZjXoYbAzqM/hD1PHsBjl62LUDbg1PuV9bqohP+Nfy V9Iomz1kEd1eaPTsj6nN7C3lSsont3Oxq/Rp9k9U9aCBv14coSDaTUa2gnDDHxGiTsFI Gpx7Gz6olM9Sjv8hxFsYDdasFFYH/xPwJzcgnfykZ3hCFa2y8aJxL4tpy0Z+8HolytTS mcEvNgW2t3iPiXDHcIOMn+gpG+0b/7R0sqOWx1xJ3wYt9gmhX9+KqQVPbAUxj0xf7tn0 VfJ5xxXu+vrVRr5VCrQ3t2wtNRlDBhWswNJ4YCM1CC4k1gktnHmF8CHzoUwUwDOdqDVL QQ== Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by aserp2130.oracle.com with ESMTP id 2qd97f9jca-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 07 Feb 2019 21:39:30 +0000 Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id x17LdOdh016811 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 7 Feb 2019 21:39:24 GMT Received: from abhmp0003.oracle.com (abhmp0003.oracle.com [141.146.116.9]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id x17LdNrJ024323; Thu, 7 Feb 2019 21:39:23 GMT Received: from [10.159.253.50] (/10.159.253.50) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 07 Feb 2019 13:39:23 -0800 Subject: Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers To: Alan Hayward Cc: "gdb-patches@sourceware.org" , nd References: <1530148222-12558-1-git-send-email-weimin.pan@oracle.com> <145f2e8d-4321-00a6-650a-bf8f0a483b6f@oracle.com> <7861E9FA-0293-4C16-857A-6935579C7042@arm.com> From: Wei-min Pan Message-ID: <3eea53e4-dfc6-ef59-f88f-d35797c26ba6@oracle.com> Date: Thu, 07 Feb 2019 21:39: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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2019-02/txt/msg00075.txt.bz2 On 2/7/2019 4:49 AM, Alan Hayward wrote: > >> On 6 Feb 2019, at 22:36, Wei-min Pan wrote: >> >> On 2/6/2019 4:43 AM, Alan Hayward wrote: >>> >>>> On 6 Feb 2019, at 00:51, Weimin Pan wrote: >>>> >>>> Would you please review this aarch64-specific patch? Thanks. >>> >>> Had a few problems applying the patch directly. >>> Also, the changelog entry should be in the body of the email and not >>> the patch. >> Thanks for pointing it out, noted. >> >>> But, ignoring that for now... >>> >>> I ran the test case with the current HEAD on AArch64, and it passed. >>> I think that your issue might have already been fixed with the following >>> patch: >>> >>> commit 754e31689866524049b9cfc68053ed4e1293cfac >>> Author: Alan Hayward >>> Date: 9 weeks ago >>> >>> AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread >>> >>> >>> My patch is roughly doing the same thing, but in a different way. >>> Could you check this please? >> Yes, your patch does fix the problem. > Excellent! > >>> I've not yet read the test case in detail. If the bug is already fixed, >>> then the test might still be useful. >> I can push the test, which is included in the bug report, upstream. > Ok - I'll review the test case either tomorrow or early next week. OK, thanks. > > > Alan. > >> Thanks. >> >>> Alan. >>> >>> >>>> 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" >>>>>> + } >>>>>> +}