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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Alan Hayward
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  <[hidden email]>

        * 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  <[hidden email]>

        * 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.
+
 * Testsuite
 
   The testsuite now creates the files gdb.cmd (containing the arguments
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 9b6324f0fc..236456399d 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -273,6 +273,17 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
   return addr;
 }
 
+/* Implement the "get_pc_address_flags" gdbarch method.  */
+
+static std::string
+aarch64_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
+{
+  if (pc != 0 && get_frame_pc_masked (frame))
+    return "PAC";
+
+  return "";
+}
+
 /* Analyze a prologue, looking for a recognizable stack frame
    and frame pointer.  Scan until we encounter a store that could
    clobber the stack frame unexpectedly, or an unknown instruction.  */
@@ -3382,6 +3393,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_gen_return_address (gdbarch, aarch64_gen_return_address);
 
+  set_gdbarch_get_pc_address_flags (gdbarch, aarch64_get_pc_address_flags);
+
   tdesc_use_registers (gdbarch, tdesc, tdesc_data);
 
   /* Add standard register aliases.  */
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index bfcb58207a..c61fa6f051 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -995,6 +995,14 @@ default_type_align (struct gdbarch *gdbarch, struct type *type)
   return 0;
 }
 
+/* See arch-utils.h.  */
+
+std::string
+default_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
+{
+  return "";
+}
+
 void
 _initialize_gdbarch_utils (void)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 588e7c0762..e5bbcd1f95 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -272,4 +272,8 @@ extern bool default_in_indirect_branch_thunk (gdbarch *gdbarch,
 extern ULONGEST default_type_align (struct gdbarch *gdbarch,
     struct type *type);
 
+/* Default implementation of gdbarch get_pc_address_flags method.  */
+extern std::string default_get_pc_address_flags (frame_info *frame,
+ CORE_ADDR pc);
+
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index c8ca757989..433b7eb260 100644
--- 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
 
 @node i386
 @subsection x86 Architecture-specific Issues
@@ -28925,6 +28926,11 @@ breakpoint; or the string @samp{<MULTIPLE>}, for a breakpoint with
 multiple locations.  This field will not be present if no address can
 be determined.  For example, a watchpoint does not have an address.
 
+@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.
+
 @item func
 If known, the function in which the breakpoint appears.
 If not known, this field is not present.
@@ -29025,6 +29031,11 @@ Note that this is not the same as the field @code{enable}.
 @item addr
 The address of this location as an hexidecimal number.
 
+@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.
+
 @item func
 If known, the function in which the location appears.
 If not known, this field is not present.
@@ -29077,6 +29088,11 @@ be absent if @value{GDBN} is unable to determine the function name.
 @item addr
 The code address for the frame.  This field is always present.
 
+@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.
+
 @item file
 The name of the source files that correspond to the frame's code
 address.  This field may be absent.
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 725b98fcd9..7b93d003a7 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -358,6 +358,7 @@ struct gdbarch
   char ** disassembler_options;
   const disasm_options_and_args_t * valid_disassembler_options;
   gdbarch_type_align_ftype *type_align;
+  gdbarch_get_pc_address_flags_ftype *get_pc_address_flags;
 };
 
 /* Create a new ``struct gdbarch'' based on information provided by
@@ -473,6 +474,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
   gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   gdbarch->type_align = default_type_align;
+  gdbarch->get_pc_address_flags = default_get_pc_address_flags;
   /* gdbarch_alloc() */
 
   return gdbarch;
@@ -721,6 +723,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of disassembler_options, invalid_p == 0 */
   /* Skip verify of valid_disassembler_options, invalid_p == 0 */
   /* Skip verify of type_align, invalid_p == 0 */
+  /* Skip verify of get_pc_address_flags, invalid_p == 0 */
   if (!log.empty ())
     internal_error (__FILE__, __LINE__,
                     _("verify_gdbarch: the following are invalid ...%s"),
@@ -1065,6 +1068,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: get_longjmp_target = <%s>\n",
                       host_address_to_string (gdbarch->get_longjmp_target));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: get_pc_address_flags = <%s>\n",
+                      host_address_to_string (gdbarch->get_pc_address_flags));
   fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_get_siginfo_type_p() = %d\n",
                       gdbarch_get_siginfo_type_p (gdbarch));
@@ -5156,6 +5162,23 @@ set_gdbarch_type_align (struct gdbarch *gdbarch,
   gdbarch->type_align = type_align;
 }
 
+std::string
+gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, frame_info *frame, CORE_ADDR pc)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->get_pc_address_flags != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_get_pc_address_flags called\n");
+  return gdbarch->get_pc_address_flags (frame, pc);
+}
+
+void
+set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch,
+                                  gdbarch_get_pc_address_flags_ftype get_pc_address_flags)
+{
+  gdbarch->get_pc_address_flags = get_pc_address_flags;
+}
+
 
 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index c3556d3841..3c6efae895 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1640,6 +1640,12 @@ typedef ULONGEST (gdbarch_type_align_ftype) (struct gdbarch *gdbarch, struct typ
 extern ULONGEST gdbarch_type_align (struct gdbarch *gdbarch, struct type *type);
 extern void set_gdbarch_type_align (struct gdbarch *gdbarch, gdbarch_type_align_ftype *type_align);
 
+/* Return a string containing any flags for the given PC in the given FRAME. */
+
+typedef std::string (gdbarch_get_pc_address_flags_ftype) (frame_info *frame, CORE_ADDR pc);
+extern std::string gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, frame_info *frame, CORE_ADDR pc);
+extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_get_pc_address_flags_ftype *get_pc_address_flags);
+
 extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);
 
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 2f9fbbc56c..d589b2c49a 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1210,6 +1210,9 @@ v;const disasm_options_and_args_t *;valid_disassembler_options;;;0;0;;0;host_add
 # default rules as laid out in gdbtypes.c:type_align.
 m;ULONGEST;type_align;struct type *type;type;;default_type_align;;0
 
+# Return a string containing any flags for the given PC in the given FRAME.
+f;std::string;get_pc_address_flags;frame_info *frame, CORE_ADDR pc;frame, pc;;default_get_pc_address_flags;;0
+
 EOF
 }
 
diff --git a/gdb/stack.c b/gdb/stack.c
index 49b9100485..06431ea354 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -923,6 +923,23 @@ print_frame_info_to_print_what (const char *print_frame_info)
   print_frame_info);
 }
 
+/* Print the PC from FRAME, plus any flags, to UIOUT.  */
+
+static void
+print_pc (struct ui_out *uiout, struct gdbarch *gdbarch, frame_info *frame,
+  CORE_ADDR pc)
+{
+  uiout->field_core_addr ("addr", gdbarch, pc);
+
+  std::string flags = gdbarch_get_pc_address_flags (gdbarch, frame, pc);
+  if (!flags.empty ())
+  {
+    uiout->text (" [");
+    uiout->field_string ("addr_flags", flags);
+    uiout->text ("]");
+  }
+}
+
 /* See stack.h.  */
 
 void
@@ -980,8 +997,7 @@ print_frame_info (const frame_print_options &fp_opts,
       if (uiout->is_mi_like_p ())
         {
           annotate_frame_address ();
-          uiout->field_core_addr ("addr",
-  gdbarch, get_frame_pc (frame));
+  print_pc (uiout, gdbarch, frame, get_frame_pc (frame));
           annotate_frame_address_end ();
         }
 
@@ -1065,8 +1081,7 @@ print_frame_info (const frame_print_options &fp_opts,
      ability to decide for themselves if it is desired.  */
   if (opts.addressprint && mid_statement)
     {
-      uiout->field_core_addr ("addr",
-      gdbarch, get_frame_pc (frame));
+      print_pc (uiout, gdbarch, frame, get_frame_pc (frame));
       uiout->text ("\t");
     }
 
@@ -1292,11 +1307,7 @@ print_frame (const frame_print_options &fp_opts,
  {
   annotate_frame_address ();
   if (pc_p)
-    {
-      uiout->field_core_addr ("addr", gdbarch, pc);
-      if (get_frame_pc_masked (frame))
- uiout->field_string ("pac", " [PAC]");
-    }
+    print_pc (uiout, gdbarch, frame, pc);
   else
     uiout->field_string ("addr", "<unavailable>",
  ui_out_style_kind::ADDRESS);
--
2.20.1 (Apple Git-117)

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Alan Hayward <[hidden email]>
> CC: nd <[hidden email]>, Alan Hayward <[hidden email]>
> 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  <[hidden email]>
>
> * 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  <[hidden email]>
>
> * 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.

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

> +@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.

Thanks.
Reply | Threaded
Open this post in threaded view
|

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

Alan Hayward


> On 12 Aug 2019, at 16:37, Eli Zaretskii <[hidden email]> wrote:
>
>> From: Alan Hayward <[hidden email]>
>> CC: nd <[hidden email]>, Alan Hayward <[hidden email]>
>> 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  <[hidden email]>
>>
>> * 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  <[hidden email]>
>>
>> * 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.
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Alan Hayward <[hidden email]>
> CC: "[hidden email]" <[hidden email]>, nd <[hidden email]>
> Date: Tue, 13 Aug 2019 09:19:26 +0000
> 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.

LGTM.

> >> +@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.

Yes, I definitely meant to keep the first sentence.

Thanks.
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Alan Hayward
On 8/12/19 4:13 PM, Alan Hayward wrote:

> gdb/ChangeLog:
>
> 2019-08-12  Alan Hayward  <[hidden email]>
>
> * 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.

OK.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

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

Alan Hayward



> On 13 Aug 2019, at 15:28, Eli Zaretskii <[hidden email]> wrote:
>
>> From: Alan Hayward <[hidden email]>
>> CC: "[hidden email]" <[hidden email]>, nd <[hidden email]>
>> Date: Tue, 13 Aug 2019 09:19:26 +0000
>> 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.
>
> LGTM.
>
>>>> +@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.
>
> Yes, I definitely meant to keep the first sentence.
>
> Thanks.



> On 15 Aug 2019, at 18:39, Pedro Alves <[hidden email]> wrote:
>
> On 8/12/19 4:13 PM, Alan Hayward wrote:
>
>> gdb/ChangeLog:
>>
>> 2019-08-12  Alan Hayward  <[hidden email]>
>>
>> * 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.
>
> OK.
>
> Thanks,
> Pedro Alves


Pushed with the doc changes. Patch below.

Thanks,
Alan.



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 151f78f2b8..08a77e8c82 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2019-08-16  Alan Hayward  <[hidden email]>
+
+       * 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.
+
 2019-08-16  Tom de Vries  <[hidden email]>

        * maint.c (maintenance_info_sections): Also handle !ALLOBJ case using
diff --git a/gdb/NEWS b/gdb/NEWS
index 462247f486..0a4e0f260f 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -287,6 +287,13 @@ 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.  On AArch64 this contains PAC if the address
+    has been masked in the frame.  On all other targets the field is not
+    present.
+
 * Testsuite

   The testsuite now creates the files gdb.cmd (containing the arguments
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e9f7b8ee0..e512118579 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -273,6 +273,17 @@ aarch64_frame_unmask_lr (struct gdbarch_tdep *tdep,
   return addr;
 }

+/* Implement the "get_pc_address_flags" gdbarch method.  */
+
+static std::string
+aarch64_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
+{
+  if (pc != 0 && get_frame_pc_masked (frame))
+    return "PAC";
+
+  return "";
+}
+
 /* Analyze a prologue, looking for a recognizable stack frame
    and frame pointer.  Scan until we encounter a store that could
    clobber the stack frame unexpectedly, or an unknown instruction.  */
@@ -3370,6 +3381,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)

   set_gdbarch_gen_return_address (gdbarch, aarch64_gen_return_address);

+  set_gdbarch_get_pc_address_flags (gdbarch, aarch64_get_pc_address_flags);
+
   tdesc_use_registers (gdbarch, tdesc, tdesc_data);

   /* Add standard register aliases.  */
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index bfcb58207a..c61fa6f051 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -995,6 +995,14 @@ default_type_align (struct gdbarch *gdbarch, struct type *type)
   return 0;
 }

+/* See arch-utils.h.  */
+
+std::string
+default_get_pc_address_flags (frame_info *frame, CORE_ADDR pc)
+{
+  return "";
+}
+
 void
 _initialize_gdbarch_utils (void)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 588e7c0762..e5bbcd1f95 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -272,4 +272,8 @@ extern bool default_in_indirect_branch_thunk (gdbarch *gdbarch,
 extern ULONGEST default_type_align (struct gdbarch *gdbarch,
                                    struct type *type);

+/* Default implementation of gdbarch get_pc_address_flags method.  */
+extern std::string default_get_pc_address_flags (frame_info *frame,
+                                                CORE_ADDR pc);
+
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 971e9c311a..339f3375e0 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,9 @@
+2019-08-16  Alan Hayward  <[hidden email]>
+
+       * gdb.texinfo (AArch64 Pointer Authentication)
+       (GDB/MI Breakpoint Information) (Frame Information): Document
+       addr_field.
+
 2019-08-12  Tom Tromey  <[hidden email]>

        * gdb.texinfo (Configure Options): Document minimum version of
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 523e3d0bfe..bcf0420779 100644
--- 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.

 @node i386
 @subsection x86 Architecture-specific Issues
@@ -28925,6 +28926,11 @@ breakpoint; or the string @samp{<MULTIPLE>}, for a breakpoint with
 multiple locations.  This field will not be present if no address can
 be determined.  For example, a watchpoint does not have an address.

+@item addr_flags
+Optional field containing any flags related to the address.  These flags are
+architecture-dependent; see @ref{Architectures} for their meaning for a
+particular CPU.
+
 @item func
 If known, the function in which the breakpoint appears.
 If not known, this field is not present.
@@ -29025,6 +29031,11 @@ Note that this is not the same as the field @code{enable}.
 @item addr
 The address of this location as an hexidecimal number.

+@item addr_flags
+Optional field containing any flags related to the address.  These flags are
+architecture-dependent; see @ref{Architectures} for their meaning for a
+particular CPU.
+
 @item func
 If known, the function in which the location appears.
 If not known, this field is not present.
@@ -29077,6 +29088,11 @@ be absent if @value{GDBN} is unable to determine the function name.
 @item addr
 The code address for the frame.  This field is always present.

+@item addr_flags
+Optional field containing any flags related to the address.  These flags are
+architecture-dependent; see @ref{Architectures} for their meaning for a
+particular CPU.
+
 @item file
 The name of the source files that correspond to the frame's code
 address.  This field may be absent.
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 725b98fcd9..7b93d003a7 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -358,6 +358,7 @@ struct gdbarch
   char ** disassembler_options;
   const disasm_options_and_args_t * valid_disassembler_options;
   gdbarch_type_align_ftype *type_align;
+  gdbarch_get_pc_address_flags_ftype *get_pc_address_flags;
 };

 /* Create a new ``struct gdbarch'' based on information provided by
@@ -473,6 +474,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->gnu_triplet_regexp = default_gnu_triplet_regexp;
   gdbarch->addressable_memory_unit_size = default_addressable_memory_unit_size;
   gdbarch->type_align = default_type_align;
+  gdbarch->get_pc_address_flags = default_get_pc_address_flags;
   /* gdbarch_alloc() */

   return gdbarch;
@@ -721,6 +723,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of disassembler_options, invalid_p == 0 */
   /* Skip verify of valid_disassembler_options, invalid_p == 0 */
   /* Skip verify of type_align, invalid_p == 0 */
+  /* Skip verify of get_pc_address_flags, invalid_p == 0 */
   if (!log.empty ())
     internal_error (__FILE__, __LINE__,
                     _("verify_gdbarch: the following are invalid ...%s"),
@@ -1065,6 +1068,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   fprintf_unfiltered (file,
                       "gdbarch_dump: get_longjmp_target = <%s>\n",
                       host_address_to_string (gdbarch->get_longjmp_target));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: get_pc_address_flags = <%s>\n",
+                      host_address_to_string (gdbarch->get_pc_address_flags));
   fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_get_siginfo_type_p() = %d\n",
                       gdbarch_get_siginfo_type_p (gdbarch));
@@ -5156,6 +5162,23 @@ set_gdbarch_type_align (struct gdbarch *gdbarch,
   gdbarch->type_align = type_align;
 }

+std::string
+gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, frame_info *frame, CORE_ADDR pc)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->get_pc_address_flags != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_get_pc_address_flags called\n");
+  return gdbarch->get_pc_address_flags (frame, pc);
+}
+
+void
+set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch,
+                                  gdbarch_get_pc_address_flags_ftype get_pc_address_flags)
+{
+  gdbarch->get_pc_address_flags = get_pc_address_flags;
+}
+

 /* Keep a registry of per-architecture data-pointers required by GDB
    modules.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index c3556d3841..3c6efae895 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1640,6 +1640,12 @@ typedef ULONGEST (gdbarch_type_align_ftype) (struct gdbarch *gdbarch, struct typ
 extern ULONGEST gdbarch_type_align (struct gdbarch *gdbarch, struct type *type);
 extern void set_gdbarch_type_align (struct gdbarch *gdbarch, gdbarch_type_align_ftype *type_align);

+/* Return a string containing any flags for the given PC in the given FRAME. */
+
+typedef std::string (gdbarch_get_pc_address_flags_ftype) (frame_info *frame, CORE_ADDR pc);
+extern std::string gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, frame_info *frame, CORE_ADDR pc);
+extern void set_gdbarch_get_pc_address_flags (struct gdbarch *gdbarch, gdbarch_get_pc_address_flags_ftype *get_pc_address_flags);
+
 extern struct gdbarch_tdep *gdbarch_tdep (struct gdbarch *gdbarch);


diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 2f9fbbc56c..d589b2c49a 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1210,6 +1210,9 @@ v;const disasm_options_and_args_t *;valid_disassembler_options;;;0;0;;0;host_add
 # default rules as laid out in gdbtypes.c:type_align.
 m;ULONGEST;type_align;struct type *type;type;;default_type_align;;0

+# Return a string containing any flags for the given PC in the given FRAME.
+f;std::string;get_pc_address_flags;frame_info *frame, CORE_ADDR pc;frame, pc;;default_get_pc_address_flags;;0
+
 EOF
 }

diff --git a/gdb/stack.c b/gdb/stack.c
index 49b9100485..06431ea354 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -923,6 +923,23 @@ print_frame_info_to_print_what (const char *print_frame_info)
                  print_frame_info);
 }

+/* Print the PC from FRAME, plus any flags, to UIOUT.  */
+
+static void
+print_pc (struct ui_out *uiout, struct gdbarch *gdbarch, frame_info *frame,
+         CORE_ADDR pc)
+{
+  uiout->field_core_addr ("addr", gdbarch, pc);
+
+  std::string flags = gdbarch_get_pc_address_flags (gdbarch, frame, pc);
+  if (!flags.empty ())
+  {
+    uiout->text (" [");
+    uiout->field_string ("addr_flags", flags);
+    uiout->text ("]");
+  }
+}
+
 /* See stack.h.  */

 void
@@ -980,8 +997,7 @@ print_frame_info (const frame_print_options &fp_opts,
       if (uiout->is_mi_like_p ())
         {
           annotate_frame_address ();
-          uiout->field_core_addr ("addr",
-                                 gdbarch, get_frame_pc (frame));
+         print_pc (uiout, gdbarch, frame, get_frame_pc (frame));
           annotate_frame_address_end ();
         }

@@ -1065,8 +1081,7 @@ print_frame_info (const frame_print_options &fp_opts,
             ability to decide for themselves if it is desired.  */
          if (opts.addressprint && mid_statement)
            {
-             uiout->field_core_addr ("addr",
-                                     gdbarch, get_frame_pc (frame));
+             print_pc (uiout, gdbarch, frame, get_frame_pc (frame));
              uiout->text ("\t");
            }

@@ -1292,11 +1307,7 @@ print_frame (const frame_print_options &fp_opts,
        {
          annotate_frame_address ();
          if (pc_p)
-           {
-             uiout->field_core_addr ("addr", gdbarch, pc);
-             if (get_frame_pc_masked (frame))
-               uiout->field_string ("pac", " [PAC]");
-           }
+           print_pc (uiout, gdbarch, frame, pc);
          else
            uiout->field_string ("addr", "<unavailable>",
                                 ui_out_style_kind::ADDRESS);