From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120691 invoked by alias); 17 Jul 2019 14:44:04 -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 120680 invoked by uid 89); 17 Jul 2019 14:44:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=H*M:7b3f X-HELO: mail-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Jul 2019 14:44:02 +0000 Received: by mail-wr1-f67.google.com with SMTP id g17so25136919wrr.5 for ; Wed, 17 Jul 2019 07:44:02 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id s3sm24119055wmh.27.2019.07.17.07.43.59 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Wed, 17 Jul 2019 07:44:00 -0700 (PDT) Subject: Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace To: Alan Hayward References: <20190717081336.68835-1-alan.hayward@arm.com> <68E9D3EF-D6A5-44C9-A87C-916EC6970435@arm.com> Cc: "gdb-patches@sourceware.org" , nd From: Pedro Alves Message-ID: Date: Wed, 17 Jul 2019 14:44:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <68E9D3EF-D6A5-44C9-A87C-916EC6970435@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2019-07/txt/msg00389.txt.bz2 On 7/17/19 2:35 PM, Alan Hayward wrote: > >> Once they learn something, often being concise >> helps -- or in other words, once you learn what "" or "U" or whatever >> is, and you're used to it, what would you rather see? What's the main >> information you're looking for when staring at the backtrace? Thoughts >> like that should guide the output too, IMO. > > PAC is the official abbreviation for the feature, so maybe :PAC works best. Reading https://lwn.net/Articles/767695/ , I see mention of Insert a PAC into a pointer Strip a PAC from a pointer Would ":PAC" make it read like the address _includes_ PAC bits? > > (gdb) bt > 0 0x0000000000400490 in puts@plt () > 1 0x00000000004005dc in foo ("hello") at cbreak-lib.c:6 > 2 0x0000000000400604:PAC in bar () at cbreak-lib.c:12 > 3 0x0000000000400620:PAC in barbar () at cbreak.c:17 > 4 0x00000000004005b4 in main () at cbreak-3.c:10 > > > Some of my attempts at different representations: > 2 0x0000000000400604* in bar () at cbreak-lib.c:12 > 2 0x0000000000400604! in bar () at cbreak-lib.c:12 > 2 0x0000000000400604U in bar () at cbreak-lib.c:122 > 2 0x0000000000400604:U in bar () at cbreak-lib.c:122 > 2 0x0000000000400604 in bar () at cbreak-lib.c:12 > 2 0x0000000000400604[U] in bar () at cbreak-lib.c:12 > 2 0x0000000000400604 in bar () at cbreak-lib.c:12 > 2 0x0000000000400604

in bar () at cbreak-lib.c:12 > 2 0x0000000000400604 in bar () at cbreak-lib.c:12 > 2 0x0000000000400604PAC in bar () at cbreak-lib.c:12 > 2 0x0000000000400604:PAC in bar () at cbreak-lib.c:12 > 2 0x0000000000400604,PAC in bar () at cbreak-lib.c:12 > > I found a single character was too hidden. A single character or symbol was also > a little confusing - my brain read U as unsigned, * as pointer, [] as an array. > > I also like ,PAC as it might be easier to add future extensions. I'd go with either: 2 0x0000000000400604 (PAC) in bar () at cbreak-lib.c:12 2 0x0000000000400604 [PAC] in bar () at cbreak-lib.c:12 Not having the space may make it a little bit harder to focus on low digits of the address. > my brain read U as unsigned, * as pointer, [] as an array. If you make it like 0x0000000000400604U, then I can see that. But not so much with: 2 0x0000000000400604 [U] in bar () at cbreak-lib.c:12 You don't have to use a single letter, though: 2 0x0000000000400604 [UN] in bar () at cbreak-lib.c:12 [] seems natural as a way to group some flags/properties to me. We already use it here for example: (top-gdb) info registers $eflags eflags 0x206 [ PF IF ] I guess I'm saying that it depends on context, and I wouldn't be worried with [] being confused with C arrays. Afterall, < and > also have meaning in C/C++... More than one meaning, actually. :-) >>> /* If address signing is enabled, mask off the signature bits from ADDR, using >>> - the register values in THIS_FRAME. */ >>> + the register values in THIS_FRAME. IS_LR indicates if ADDR is from the link >>> + register for the current frame. */ >>> >>> static CORE_ADDR >>> aarch64_frame_unmask_address (struct gdbarch_tdep *tdep, >>> - struct frame_info *this_frame, >>> - CORE_ADDR addr) >>> + struct frame_info *this_frame, CORE_ADDR addr, >>> + bool is_lr = true) >> >> I didn't see anywhere in the patch that passes is_lr == true? >> >> Looks like the patch is a nop? > > Yes, the is_lr check will always be true. > The function is designed to be for any address, but right now it’s only used for > the link register. But, I didn’t want someone in the future using it as a generic > address unmask function and it causing the link register to be marked. > > An other option was to add a warning to the comment block above the function. Or > do the lr marking in the caller (but then that adds extra logic). > > Maybe the better option would be to rename the function to > aarch64_frame_unmask_lr_address. Yes, I think that renaming is the best option. People remove unnecessary parameters all the time, when they notice some parameter isn't really necessary. If someone needs a more generic aarch64_frame_unmask_address for all kinds of addresses, they can always factor out a new aarch64_frame_unmask_address that looks like today's version, and make aarch64_frame_unmask_lr_address call it + mark the frame. Thanks, Pedro Alves