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

Alan Hayward
2019-08-09T13:22:19Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4083 bytes --]

> 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.\x16º&Öéj×!zÊÞ¶êç×Ÿ4ó™b²Ö«r\x18\x1dn–­r\x17¬