From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 80408 invoked by alias); 7 Feb 2019 12:49:55 -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 78938 invoked by uid 89); 7 Feb 2019 12:49:55 -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,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: EUR01-DB5-obe.outbound.protection.outlook.com Received: from mail-eopbgr150083.outbound.protection.outlook.com (HELO EUR01-DB5-obe.outbound.protection.outlook.com) (40.107.15.83) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 07 Feb 2019 12:49:52 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1vU7DS5f1TnQxQfwFADZhbkJfPXdWczyV5qRyCP4FBM=; b=PRFsP7RxvskboAjCmigC4u2vw6NuBy7w4rDCMNqYLyAb5+HejZ9/dxDZBV7mZYNiDat/Upcp+H8AxuufqPupkFcXQVQWXCfKC7/iNIDW2CAP0xlwq+7KAfLxAWEalQCEJJmDNLphDo7LgEpzgF52u42LKeMg73CoVVC+5IN4jtM= Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.227.22) by DB6PR0802MB2277.eurprd08.prod.outlook.com (10.172.226.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1580.17; Thu, 7 Feb 2019 12:49:48 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::acd7:a958:2aaa:562e]) by DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::acd7:a958:2aaa:562e%5]) with mapi id 15.20.1580.019; Thu, 7 Feb 2019 12:49:48 +0000 From: Alan Hayward To: Wei-min Pan CC: "gdb-patches@sourceware.org" , nd Subject: Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers Date: Thu, 07 Feb 2019 12:49:00 -0000 Message-ID: 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> In-Reply-To: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 Content-Type: text/plain; charset="us-ascii" Content-ID: <9B1BC56C7FCC56438F0443FCF90EE660@eurprd08.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00059.txt.bz2 > On 6 Feb 2019, at 22:36, Wei-min Pan wrote: >=20 > 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. >=20 > Thanks for pointing it out, noted. >=20 > > 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? >=20 > Yes, your patch does fix the problem. Excellent! >=20 > > > > I've not yet read the test case in detail. If the bug is already fixed, > > then the test might still be useful. >=20 > 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. Alan. >=20 > Thanks. >=20 >> Alan. >>=20 >>=20 >>> 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 si= ze >>>>> when setting a hardware watchpoint/breakpoint. As a result, watchpoin= ts >>>>> 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].c= trl) >>>>> got changed to a non-zero default value, when the PTRACE_SETREGSET re= quest >>>>> was made in aarch64_linux_set_debug_regs(), in preparation for resumi= ng >>>>> the thread. >>>>>=20 >>>>> I sent out a patch upstream for review last year. Yao Qi commented th= at >>>>> 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. >>>>>=20 >>>>> 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. >>>>>=20 >>>>> 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 >>>>>=20 >>>>> 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_de= bug_reg_state *state, >>>>> iov.iov_len =3D (offsetof (struct user_hwdebug_state, dbg_regs) >>>>> + count * sizeof (regs.dbg_regs[0])); >>>>> + int changed_regs =3D 0; >>>>> for (i =3D 0; i < count; i++) >>>>> { >>>>> regs.dbg_regs[i].addr =3D addr[i]; >>>>> regs.dbg_regs[i].ctrl =3D 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 =3D=3D 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 aarc= h64. >>>>> + >>>>> + This software is provided 'as-is', without any express or implied >>>>> + warranty. In no event will the authors be held liable for any dam= ages >>>>> + arising from the use of this software. >>>>> + >>>>> + Permission is granted to anyone to use this software for any purpo= se, >>>>> + including commercial applications, and to alter it and redistribut= e 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 =3D 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) =3D=3D 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 >=3D 0x01); >>>>> + assert (len_mask <=3D 0xff); >>>>> + >>>>> + iov.iov_base =3D &dreg_state; >>>>> + iov.iov_len =3D sizeof (dreg_state); >>>>> + errno =3D 0; >>>>> + l =3D ptrace (PTRACE_GETREGSET, pid, NT_ARM_HW_WATCH, &iov); >>>>> + assert (l =3D=3D 0); >>>>> + assert (AARCH64_DEBUG_ARCH (dreg_state.dbg_info) =3D=3D AARCH64_DE= BUG_ARCH_V8); >>>>> + assert (AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info) >=3D 1); >>>>> + >>>>> + assert (!DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl)); >>>>> + dreg_state.dbg_regs[0].ctrl |=3D 1; >>>>> + assert ( DR_CONTROL_ENABLED (dreg_state.dbg_regs[0].ctrl)); >>>>> + >>>>> + assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) =3D=3D 0); >>>>> + dreg_state.dbg_regs[0].ctrl |=3D len_mask << 5; >>>>> + assert (DR_CONTROL_LENGTH (dreg_state.dbg_regs[0].ctrl) =3D=3D len= _mask); >>>>> + >>>>> + dreg_state.dbg_regs[0].ctrl |=3D 2 << 3; // write >>>>> + dreg_state.dbg_regs[0].ctrl |=3D 2 << 1; // GDB: ???: enabled at e= l0 >>>>> + //printf("ctrl=3D0x%x\n",dreg_state.dbg_regs[0].ctrl); >>>>> + dreg_state.dbg_regs[0].addr =3D (uintptr_t) addr; >>>>> + >>>>> + iov.iov_base =3D &dreg_state; >>>>> + iov.iov_len =3D (offsetof (struct user_hwdebug_state, dbg_regs) >>>>> + + sizeof (dreg_state.dbg_regs[0])); >>>>> + errno =3D 0; >>>>> + l =3D ptrace (PTRACE_SETREGSET, pid, NT_ARM_HW_WATCH, &iov); >>>>> + if (errno !=3D 0) >>>>> + error (1, errno, "PTRACE_SETREGSET: NT_ARM_HW_WATCH"); >>>>> + assert (l =3D=3D 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 =3D fork (); >>>>> + assert (child >=3D 0); >>>>> + if (child =3D=3D 0) >>>>> + { >>>>> + l =3D ptrace (PTRACE_TRACEME, 0, NULL, NULL); >>>>> + assert (l =3D=3D 0); >>>>> + i =3D raise (SIGUSR1); >>>>> + assert (i =3D=3D 0); >>>>> + check =3D -1; >>>>> + i =3D raise (SIGUSR2); >>>>> + /* NOTREACHED */ >>>>> + assert (0); >>>>> + } >>>>> + >>>>> + got_pid =3D waitpid (child, &status, 0); >>>>> + assert (got_pid =3D=3D child); >>>>> + assert (WIFSTOPPED (status)); >>>>> + assert (WSTOPSIG (status) =3D=3D SIGUSR1); >>>>> + >>>>> + // PASS: >>>>> + //SET_WATCHPOINT (child, &check, 0xff); >>>>> + // FAIL: >>>>> + SET_WATCHPOINT (child, &check, 0x02); >>>>> + >>>>> + errno =3D 0; >>>>> + l =3D ptrace (PTRACE_CONT, child, 0l, 0l); >>>>> + assert_perror (errno); >>>>> + assert (l =3D=3D 0); >>>>> + >>>>> + got_pid =3D waitpid (child, &status, 0); >>>>> + assert (got_pid =3D=3D child); >>>>> + assert (WIFSTOPPED (status)); >>>>> + if (WSTOPSIG (status) =3D=3D SIGUSR2) >>>>> + { >>>>> + /* We missed the watchpoint - unsupported by hardware? */ >>>>> + cleanup (); >>>>> + return 2; >>>>> + } >>>>> + assert (WSTOPSIG (status) =3D=3D 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 modi= fy >>>>> +# 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} executab= le {debug}] !=3D "" } { >>>>> + 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" >>>>> + } >>>>> +}