Re: [PATCH] AArch64 pauth: Indicate unmasked addresses in backtrace

[Pedro Alves <palves@redhat.com>]

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 "<unmasked>" 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<U> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604[U] in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<M> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<P> in bar () at cbreak-lib.c:12
> 2  0x0000000000400604<PAC> 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