This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH V2] AArch64 pauth: Indicate unmasked addresses in backtrace
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Pedro Alves <palves at redhat dot com>, Tom Tromey <tom at tromey dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Fri, 9 Aug 2019 13:22:00 +0000
- Subject: Re: [PATCH V2] AArch64 pauth: Indicate unmasked addresses in backtrace
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=2iusAHB0Fc4Qk3gM+MjmKn49POBYYz4CD2DMYeM919k=; b=hiC21lu6y593UtQnJ2f5pR5mcsLh6p+2XuQNFXLd6baGLBTJFe1KHuMjUhmPDzrNdyg/Ua2LPLq8AvpMPAUdYDYa/itSQweHKKYVMKwiUMarlnj2w45vrfsBmuBNq0z4vmEkKP5QXDWuJxJVH5UOf0rlbAR+Rxx0q8tZyU10sbk4/2skjHGjdLgQNNKx53yBcFv4Qxe7qU06c2dLCPXujMFG11WpjAuA4fbGnz5QuqeFSfMiZhzyr8lBVoWZg9MDI+Itr++NWVT2Fzoylh1E5Di1QFzD5Ykw42P5hwHXJtQvbBd4LerGsPenxlIbmGQR0oxWj1VnwSlyPwJIXrUPxA==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QFjCKZkge+XQ6YnvwZ8VKr6gWwkKE0eOmTRdwxRnzON0EZoXRVy35WaFeUrDR9rL5ZD4HQAr3V1hmofw/+E35faFfrD+PxlsZ4axAyiTDnEvj5ymd4okeLCWFp4rXQQc1UnDDx8/g6RxQlcqKDRKfIpwVk4L9Fj/a0XUjS8LqXyGJoosFPMNo4Q5/byBz+7DYEZ9RFV2qmauZIcwB66s+CXk4g+jSzAu6dhS+fyb4pameMXLSFiQpA6+hrwYM5iBB4iconjsNFBDibZe4BFpYjn1P9hxSxpuJD7DLi0sRaQf3xvwJrMj81xlzn43u0ZWXIHuG87gMxbxnXk1Ejf3og==
- Original-authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan dot Hayward at arm dot com;
- References: <20190730144123.11135-1-alan.hayward@arm.com> <728af5fa-8e3d-845c-d72f-60b1d2067643@redhat.com> <F3D7A960-329A-4BDA-A251-87828CF3E459@arm.com> <474e8e87-50d4-874f-787f-ef5f5fbb6cc3@redhat.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.