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

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

> On 13 Aug 2019, at 15:28, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Alan Hayward <Alan.Hayward@arm.com>
>> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, nd <nd@arm.com>
>> 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 <palves@redhat.com> wrote:
> 
> On 8/12/19 4:13 PM, Alan Hayward wrote:
> 
>> 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.
> 
> 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  <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.
+
 2019-08-16  Tom de Vries  <tdevries@suse.de>

        * 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  <alan.hayward@arm.com>
+
+       * gdb.texinfo (AArch64 Pointer Authentication)
+       (GDB/MI Breakpoint Information) (Frame Information): Document
+       addr_field.
+
 2019-08-12  Tom Tromey  <tom@tromey.com>

        * 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);