Re: [PATCH] Move [PAC] into a new MI field addr_flags

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

> On 12 Aug 2019, at 16:37, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> CC: nd <nd@arm.com>, Alan Hayward <Alan.Hayward@arm.com>
>> Date: Mon, 12 Aug 2019 15:13:52 +0000
>> 
>> Add a new print_pc which prints both the PC and a new field addr_flags.
>> Call this wherever the PC is printed in stack.c.
>> 
>> Add a new gdbarch method get_pc_address_flags to obtain the addr_flag
>> contents. By default returns an empty string, on AArch64 this returns
>> PAC if the address has been masked in the frame.
>> 
>> Document this in the manual and NEWS file.
>> 
>> gdb/ChangeLog:
>> 
>> 2019-08-12  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* NEWS (Other MI changes): New subsection.
>> 	* aarch64-tdep.c (aarch64_get_pc_address_flags): New function.
>> 	(aarch64_gdbarch_init): Add aarch64_get_pc_address_flags.
>> 	* arch-utils.c (default_get_pc_address_flags): New function.
>> 	* arch-utils.h (default_get_pc_address_flags): New declaration.
>> 	* gdbarch.sh: Add get_pc_address_flags.
>> 	* gdbarch.c: Regenerate.
>> 	* gdbarch.h: Likewise.
>> 	* stack.c (print_pc): New function.
>> 	(print_frame_info) (print_frame): Call print_pc.
>> 
>> gdb/doc/ChangeLog:
>> 
>> 2019-08-12  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* gdb.texinfo (AArch64 Pointer Authentication)
>> 	(GDB/MI Breakpoint Information) (Frame Information): Document
>> 	addr_field.
>> ---
>> gdb/NEWS            |  6 ++++++
>> gdb/aarch64-tdep.c  | 13 +++++++++++++
>> gdb/arch-utils.c    |  8 ++++++++
>> gdb/arch-utils.h    |  4 ++++
>> gdb/doc/gdb.texinfo | 18 +++++++++++++++++-
>> gdb/gdbarch.c       | 23 +++++++++++++++++++++++
>> gdb/gdbarch.h       |  6 ++++++
>> gdb/gdbarch.sh      |  3 +++
>> gdb/stack.c         | 29 ++++++++++++++++++++---------
>> 9 files changed, 100 insertions(+), 10 deletions(-)
>> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index fa01adf6e8..42b2ba3d2b 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -287,6 +287,12 @@ maint show test-options-completion-result
>>   These can be used to catch C++ exceptions in a similar fashion to
>>   the CLI commands 'catch throw', 'catch rethrow', and 'catch catch'.
>> 
>> +* Other MI changes
>> +
>> + ** Backtraces and frames include a new optional field addr_flags which is
>> +    given after the addr field.  Currently this is only used by AArch64
>> +    for indicating PAC encyrpted addresses.
> 
> I think your original description at the beginning of your message
> describes the purpose of this field more clearly.
> 
> Also, "encyrpted" is a typo.

How about this:


* Other MI changes

 ** Backtraces and frames include a new optional field addr_flags which is
    given after the addr field.  On AArch64 this contains PAC if the address
    has been masked in the frame.  On all other targets the field is not
    present.


> 
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -24397,7 +24397,8 @@ 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 its value will be masked.
>> When GDB prints a backtrace, any addresses that required unmasking will be
>> -postfixed with the marker [PAC].
>> +postfixed with the marker [PAC].  When using the MI, this is printed as part
>> +of the @code{addr_flags}. field
>                           ^
> That period should be moved to after "field”.

Oops. Ok.

> 
>> +@item addr_flags
>> +Optional field containing any flags related to the address.  If there
>> +are any flags defined for the current target then they are documented in
>> +the @xref{Architectures} section.
> 
> I suggest to reword:
> 
>  These flags are architecture-dependent; see @ref{Architectures} for
>  their meaning for a particular CPU.

Maybe keep the first sentence? Giving:

Optional field containing any flags related to the address.  These flags are
architecture-dependent; see @ref{Architectures} for their meaning for a
particular CPU.



Thanks for the review!

Alan.