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

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

> On 7 Aug 2019, at 20:24, Pedro Alves <palves@redhat.com> wrote:
> 
> On 7/30/19 3:41 PM, Alan Hayward wrote:
> 
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 0fcd131f71..b7dba2f918 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -24380,6 +24380,14 @@ but the lengths of the @code{z} and @code{p} registers will not change.  This
>> is a known limitation of @value{GDBN} and does not affect the execution of the
>> target process.
>> 
>> +@subsubsection AArch64 Pointer Authentication.
>> +@cindex AArch64 Pointer Authentication.
>> +
>> +When @value{GDBN} is debugging the AArch64 architecture, and the program is
>> +using the v8.3-A feature Pointer Authentication (PAC), then whenever the link
>> +register @code{$lr} is pointing to an PAC function it's value will be masked.
> 
> s/it's value/its value/
> 
>> +When GDB prints a backtrace, any addresses that required unmasking will be
>> +postfixed with the marker [PAC].
>> 
> 
>> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
>> index a2a96ac0d3..d805ec68f2 100644
>> --- a/gdb/python/py-framefilter.c
>> +++ b/gdb/python/py-framefilter.c
>> @@ -901,6 +901,8 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>> 	    {
>> 	      annotate_frame_address ();
>> 	      out->field_core_addr ("addr", gdbarch, address);
>> +	      if (get_frame_pc_masked (frame))
>> +		out->field_string ("pac", " [PAC]");
>> 	      annotate_frame_address_end ();
>> 	      out->text (" in ");
>> 	    }
>> diff --git a/gdb/stack.c b/gdb/stack.c
>> index 7833ca4aeb..9d49809895 100644
>> --- a/gdb/stack.c
>> +++ b/gdb/stack.c
>> @@ -1298,7 +1298,11 @@ print_frame (const frame_print_options &fp_opts,
>> 	{
>> 	  annotate_frame_address ();
>> 	  if (pc_p)
>> -	    uiout->field_core_addr ("addr", gdbarch, pc);
>> +	    {
>> +	      uiout->field_core_addr ("addr", gdbarch, pc);
>> +	      if (get_frame_pc_masked (frame))
>> +		uiout->field_string ("pac", " [PAC]");
> 
> Hmm, I had suggested considering MI in the previous iteration, but
> I was just thinking of including the "[PAC]" text in the
> "addr" field.  If we're adding a new field, then a few extra
> things need to be considered:
> 
> #1 - documentation, both manual and NEWS should mention this new MI field.
> 
> #2 - calling the attribute "pac" makes it architecture specific. 
>      I.e., to make use of it, a frontend will have to have Aarch64 awareness?
>      Not sure that is a good thing.
> 
> #3 - The MI attribute is called "pac", and its content is
>      literally " [PAC]".  I'd find that odd if I were a frontend author:
>      the content is right aligned with a space, making doing anything with
>      it other than appending it to the address text probably look odd,
>      unless you bake in awareness of the attribute's text...  If I saw
>      an attribute named "pac", I'd expect it to be a boolean?  At the
>      least, the left space should not be part of the field, I think?
>      Maybe we should rename the field to something else, like "addr_attr"
>     for "address attributes" or something.


I hadn’t realised the implications doing that would have, and had assumed
you couldn’t add to a field that had already been used.

I had (prematurely) pushed the patch. Is this additional fix ok?

Alan.



    Move backtrace PAC marker into addr field

    PAC does not need its own field and should instead be part of
    the addr field.

    gdb/ChangeLog:

    2019-08-08  Alan Hayward  <alan.hayward@arm.com>

            * stack.c (print_frame): Move PAC into addr field.

    gdb/doc/ChangeLog:

    2019-08-08  Alan Hayward  <alan.hayward@arm.com>

            * gdb.texinfo (AArch64 Pointer Authentication): Fix typo.

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7f8c0aff1c..c8ca757989 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24395,7 +24395,7 @@ target process.

 When @value{GDBN} is debugging the AArch64 architecture, and the program is
 using the v8.3-A feature Pointer Authentication (PAC), then whenever the link
-register @code{$lr} is pointing to an PAC function it's value will be masked.
+register @code{$lr} is pointing to an PAC function its value will be masked.
 When GDB prints a backtrace, any addresses that required unmasking will be
 postfixed with the marker [PAC].

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>",