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

[Alan Hayward <Alan dot Hayward at arm dot com>]

> On 8 Aug 2019, at 11:33, Pedro Alves <palves@redhat.com> wrote:
> 
> On 8/8/19 9:55 AM, Alan Hayward wrote:
> 
>> diff --git a/gdb/stack.c b/gdb/stack.c
>> index 0859815baf..c599caf51c 100644
>> --- a/gdb/stack.c
>> +++ b/gdb/stack.c
>> @@ -1301,7 +1301,7 @@ print_frame (const frame_print_options &fp_opts,
>>            {
>>              uiout->field_core_addr ("addr", gdbarch, pc);
>>              if (get_frame_pc_masked (frame))
>> -               uiout->field_string ("pac", " [PAC]");
>> +               uiout->field_string ("addr", " [PAC]");
>>            }
>>          else
>>            uiout->field_string ("addr", "<unavailable>",
>> 
> 
> ... because I think that this results in MI printing two different "addr" attributes.
> 
> Instead, you'll need to build a string, with e.g., string_printf,
> and use uiout->field_string with ui_out_style_kind::ADDRESS style,
> so that MI outputs one single "addr" attribute.
> 
> Please try "gdb -i=mi".  You can still type CLI commands, so just "(gdb) start"
> and running to main, so that GDB prints the frame in the *stop event should
> be sufficient to trigger this.

stop doesn’t trigger a PAC because it’s only once we reference a function via the
link register that the PAC unmasking happens.

However, selecting a previous frame does.... and the issues are obvious now:

=thread-selected,id="1",frame={level="1",addr="0x00000000004005b0",pac=" [PAC]",func="main3",args=[],file="cbreak-3.c",fullname="/root/cbreak-3.c",line="9",arch="aarch64"}


> 
> BTW, there are two other places where we output the "addr" field
> in the file.  Do you want to include "[PAC]" in those?  If so,
> then factoring out the "addr" printing to a separate function
> would be appropriate.
> 

Ok, I can do that.




> On 8 Aug 2019, at 17:58, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Hmm, I had suggested considering MI in the previous iteration, but
> Pedro> I was just thinking of including the "[PAC]" text in the
> Pedro> "addr" field.  If we're adding a new field, then a few extra
> Pedro> things need to be considered:
> 
> Pedro>  #1 - documentation, both manual and NEWS should mention this new MI field.
> 
> Oops, I forgot about this.  Sorry about that.

I’ll add something.

> 
> I don't think putting this information into the "addr" field is a good
> idea.  It's better, IMO, to let MI field names provide the structure,
> rather than requiring clients to also parse the values of fields.
> 
> I realize MI isn't 100% clean on this topic, but we can still not make
> it worse.
> 
> Pedro>  #2 - calling the attribute "pac" makes it architecture specific. 
> 
> I don't think this is such a big deal but at the same time any
> reasonable name is fine by me.
> 
> Pedro>  #3 - The MI attribute is called "pac", and its content is
> Pedro>       literally " [PAC]".  I'd find that odd if I were a frontend author:
> Pedro>       the content is right aligned with a space, making doing anything with
> Pedro>       it other than appending it to the address text probably look odd,
> Pedro>       unless you bake in awareness of the attribute's text...  If I saw
> Pedro>       an attribute named "pac", I'd expect it to be a boolean?  At the
> Pedro>       least, the left space should not be part of the field, I think?
> 
> I think part of the pain here is an internal constraint, namely that the
> CLI ui-out wouldn't know to rewrite the boolean value to something else
> here.  But perhaps that's something that could just be addressed
> directly.

It looks like fixing the space just requires an additional call to uiout->text (" “).


How about I create a new field addr_flags? It would be a generic field into which
targets can add whichever fields they want to.

I then could add a call to a new function gdbarch_print_addr_flags() which prints the
PAC on AArch64 and nothing on all other targets?




Alan.