From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126171 invoked by alias); 13 Feb 2019 01:05:36 -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 126160 invoked by uid 89); 13 Feb 2019 01:05:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,UNSUBSCRIBE_BODY 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; Wed, 13 Feb 2019 01:05:34 +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 x1D13hYH167665; Wed, 13 Feb 2019 01:05:29 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=dO0Wx8wXye3IfS4p7rRY52+McyW/DTGSgAqvNcmy9lw=; b=UkJ59GEWTvu/sCQdu57451gHXgCXBPnh7UBtyz8Oh4rRtKfwPnTVjZ59YbDqrh7dccUq qQbSGEFzqdQMQmplHIZRcmjEYYk9+oYix5+NO1U0soQEJ+a2gesMucjAWwXQmJoU6ryb mitObCJcZoL0knVU6DrH3/kJDvMfKuTc50XZyI9XTRp42ASAz001GcYw30HYVvSuMXSG XdQKyPIzPP8cCjYO7IaMCiqCCeTl1gLEAzXj3v88v3Q3tM/WhWc2E3bDVEarTdkqmMIT gtSTjm2OZt1c5LuK4a9V/Hn0VmWcR+lf98ydVfF7Pi0b2ZWZtRXfnHRsAZIpUoRBKpib nQ== Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by aserp2130.oracle.com with ESMTP id 2qhre5f7ka-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 13 Feb 2019 01:05:29 +0000 Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id x1D15Mol011768 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 13 Feb 2019 01:05:23 GMT Received: from abhmp0008.oracle.com (abhmp0008.oracle.com [141.146.116.14]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x1D15MUY020679; Wed, 13 Feb 2019 01:05:22 GMT Received: from [10.132.96.98] (/10.132.96.98) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 12 Feb 2019 17:05:22 -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> <3eea53e4-dfc6-ef59-f88f-d35797c26ba6@oracle.com> <31396591-A287-40C5-A4D0-6EAEC8077D6B@arm.com> <563997a0-cd3c-cf6e-8418-bbd452436b2b@oracle.com> <68F734F2-CEAC-437E-903B-C7490C13AC1B@arm.com> From: Weimin Pan Message-ID: <10492121-c55c-d529-f850-a26e74708a01@oracle.com> Date: Wed, 13 Feb 2019 01:05: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: <68F734F2-CEAC-437E-903B-C7490C13AC1B@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2019-02/txt/msg00169.txt.bz2 On 2/12/2019 6:46 AM, Alan Hayward wrote: > >> On 12 Feb 2019, at 01:10, Weimin Pan 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 >>>>>>>>>> +#ifdef __ia64__ >>>>>>>>>> +#undef ia64_fpreg >>>>>>>>>> +#undef pt_all_user_regs >>>>>>>>>> +#endif /* __ia64__ */ >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#if defined __i386__ || defined __x86_64__ >>>>>>>>>> +#include >>>>>>>>>> +#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 >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> +#include >>>>>>>>>> + >>>>>>>>>> +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. Made these two changes and pushed. Thanks. > > > Thanks, > Alan. > >