[PATCH 0/5] Python Unwinders and Inline Frames

[PATCH 0/5] Python Unwinders and Inline Frames

[Andrew Burgess]

I ran into an issue with Python frame unwinders and inline frames.
The fix for which is in patch #5 of this series.

In order to write a test for patch #5 I needed some extra
functionality from the Python API, which resulted in patches #2 - #4.

And while writing patch #2 I noticed the cleanup that's in patch #1.

Feedback welcome,

Thanks,
Andrew


---

Andrew Burgess (5):
  gdb: Remove deprecated_set_gdbarch_data
  gdb/python: Add architecture method to gdb.PendingFrame
  gdb/python: Add gdb.Architecture.registers method
  gdb/python: New method to access list of register groups
  gdb: Fix Python unwinders and inline frames

 gdb/ChangeLog                                 |  61 +++
 gdb/Makefile.in                               |   1 +
 gdb/NEWS                                      |  10 +
 gdb/doc/ChangeLog                             |  21 +
 gdb/doc/gdb.texinfo                           |   1 +
 gdb/doc/python.texi                           |  70 +++
 gdb/findvar.c                                 |  28 +-
 gdb/gdbarch.c                                 |  22 +-
 gdb/gdbarch.h                                 |   3 -
 gdb/gdbarch.sh                                |  25 +-
 gdb/ia64-libunwind-tdep.c                     |  15 +-
 gdb/python/py-arch.c                          |  45 ++
 gdb/python/py-registers.c                     | 464 ++++++++++++++++++
 gdb/python/py-unwind.c                        |  20 +
 gdb/python/python-internal.h                  |   6 +
 gdb/python/python.c                           |   1 +
 gdb/testsuite/ChangeLog                       |  19 +
 .../gdb.python/py-arch-reg-groups.exp         |  87 ++++
 .../gdb.python/py-arch-reg-names.exp          |  87 ++++
 gdb/testsuite/gdb.python/py-unwind-inline.c   |  37 ++
 gdb/testsuite/gdb.python/py-unwind-inline.exp |  49 ++
 gdb/testsuite/gdb.python/py-unwind-inline.py  |  71 +++
 gdb/testsuite/gdb.python/py-unwind.py         |  10 +-
 gdb/user-regs.c                               |  18 +-
 24 files changed, 1097 insertions(+), 74 deletions(-)
 create mode 100644 gdb/python/py-registers.c
 create mode 100644 gdb/testsuite/gdb.python/py-arch-reg-groups.exp
 create mode 100644 gdb/testsuite/gdb.python/py-arch-reg-names.exp
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py

-- 
2.25.4

[PATCH 1/5] gdb: Remove deprecated_set_gdbarch_data

[Andrew Burgess]

There are currently two remaining uses of deprecated_set_gdbarch_data,
both of which are needed because during gdbarch initialisation we call
gdbarch_data for a data field that is registered using:

  gdbarch_data_register_post_init (....)

However, in both of these cases, the only thing that the call back
needs from the gdbarch struct is its obstack.  Given this there is
nothing stopping us changing the post-init hooks into pre-init hooks.
The pre-init hooks don't get passed the full gdbarch, they only get
passed its obstack.

The IA64 change is completely untested.  The user-regs change has been
tested a little by locally adding some user-regs to the x86-64 target,
and also by running the RISC-V tests, which do use user-regs.

gdb/ChangeLog:

	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh (deprecated_set_gdbarch_data): Delete.
	(gdbarch_data): Use internal_error for the case where
	deprecated_set_gdbarch_data was originally needed.
	* ia64-libunwind-tdep.c (libunwind_descr_init): Update parameters,
	and use passed in obstack.
	(libunwind_frame_set_descr): Should no longer get back NULL from
	gdbarch_data.
	(_initialize_libunwind_frame): Register as a pre-init gdbarch data
	type.
	* user-regs.c (user_regs_init): Update parameters, and use passed
	in obstack.
	(user_reg_add): Should no longer get back NULL from gdbarch_data.
	(_initialize_user_regs): Register as a pre-init gdbarch data type.
---
 gdb/ChangeLog             | 18 ++++++++++++++++++
 gdb/gdbarch.c             | 22 +++-------------------
 gdb/gdbarch.h             |  3 ---
 gdb/gdbarch.sh            | 25 +++----------------------
 gdb/ia64-libunwind-tdep.c | 15 ++++-----------
 gdb/user-regs.c           | 18 +++++-------------
 6 files changed, 33 insertions(+), 68 deletions(-)

diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 55e4a67e2c5..cc3962d951a 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -5240,20 +5240,6 @@ alloc_gdbarch_data (struct gdbarch *gdbarch)
   gdbarch->data = GDBARCH_OBSTACK_CALLOC (gdbarch, gdbarch->nr_data, void *);
 }
 
-/* Initialize the current value of the specified per-architecture
-   data-pointer.  */
-
-void
-deprecated_set_gdbarch_data (struct gdbarch *gdbarch,
-			     struct gdbarch_data *data,
-			     void *pointer)
-{
-  gdb_assert (data->index < gdbarch->nr_data);
-  gdb_assert (gdbarch->data[data->index] == NULL);
-  gdb_assert (data->pre_init == NULL);
-  gdbarch->data[data->index] = pointer;
-}
-
 /* Return the current value of the specified per-architecture
    data-pointer.  */
 
@@ -5283,11 +5269,9 @@ gdbarch_data (struct gdbarch *gdbarch, struct gdbarch_data *data)
 	  data->init_p = 1;
 	}
       else
-	/* The architecture initialization hasn't completed - punt -
-	 hope that the caller knows what they are doing.  Once
-	 deprecated_set_gdbarch_data has been initialized, this can be
-	 changed to an internal error.  */
-	return NULL;
+	internal_error (__FILE__, __LINE__,
+			_("gdbarch post-init data field can only be used "
+			  "after gdbarch is fully initialised"));
       gdb_assert (gdbarch->data[data->index] != NULL);
     }
   return gdbarch->data[data->index];
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index c7621f2fda1..ee42972d66f 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1849,9 +1849,6 @@ typedef void *(gdbarch_data_pre_init_ftype) (struct obstack *obstack);
 extern struct gdbarch_data *gdbarch_data_register_pre_init (gdbarch_data_pre_init_ftype *init);
 typedef void *(gdbarch_data_post_init_ftype) (struct gdbarch *gdbarch);
 extern struct gdbarch_data *gdbarch_data_register_post_init (gdbarch_data_post_init_ftype *init);
-extern void deprecated_set_gdbarch_data (struct gdbarch *gdbarch,
-                                         struct gdbarch_data *data,
-			                 void *pointer);
 
 extern void *gdbarch_data (struct gdbarch *gdbarch, struct gdbarch_data *);
 
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index e297b1c56a0..f1e948e6037 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1634,9 +1634,6 @@ typedef void *(gdbarch_data_pre_init_ftype) (struct obstack *obstack);
 extern struct gdbarch_data *gdbarch_data_register_pre_init (gdbarch_data_pre_init_ftype *init);
 typedef void *(gdbarch_data_post_init_ftype) (struct gdbarch *gdbarch);
 extern struct gdbarch_data *gdbarch_data_register_post_init (gdbarch_data_post_init_ftype *init);
-extern void deprecated_set_gdbarch_data (struct gdbarch *gdbarch,
-                                         struct gdbarch_data *data,
-			                 void *pointer);
 
 extern void *gdbarch_data (struct gdbarch *gdbarch, struct gdbarch_data *);
 
@@ -2241,20 +2238,6 @@ alloc_gdbarch_data (struct gdbarch *gdbarch)
   gdbarch->data = GDBARCH_OBSTACK_CALLOC (gdbarch, gdbarch->nr_data, void *);
 }
 
-/* Initialize the current value of the specified per-architecture
-   data-pointer.  */
-
-void
-deprecated_set_gdbarch_data (struct gdbarch *gdbarch,
-			     struct gdbarch_data *data,
-			     void *pointer)
-{
-  gdb_assert (data->index < gdbarch->nr_data);
-  gdb_assert (gdbarch->data[data->index] == NULL);
-  gdb_assert (data->pre_init == NULL);
-  gdbarch->data[data->index] = pointer;
-}
-
 /* Return the current value of the specified per-architecture
    data-pointer.  */
 
@@ -2284,11 +2267,9 @@ gdbarch_data (struct gdbarch *gdbarch, struct gdbarch_data *data)
 	  data->init_p = 1;
 	}
       else
-	/* The architecture initialization hasn't completed - punt -
-	 hope that the caller knows what they are doing.  Once
-	 deprecated_set_gdbarch_data has been initialized, this can be
-	 changed to an internal error.  */
-	return NULL;
+	internal_error (__FILE__, __LINE__,
+			_("gdbarch post-init data field can only be used "
+			  "after gdbarch is fully initialised"));
       gdb_assert (gdbarch->data[data->index] != NULL);
     }
   return gdbarch->data[data->index];
diff --git a/gdb/ia64-libunwind-tdep.c b/gdb/ia64-libunwind-tdep.c
index 59b7df13d2f..94881bbfb5d 100644
--- a/gdb/ia64-libunwind-tdep.c
+++ b/gdb/ia64-libunwind-tdep.c
@@ -133,10 +133,10 @@ libunwind_descr (struct gdbarch *gdbarch)
 }
 
 static void *
-libunwind_descr_init (struct gdbarch *gdbarch)
+libunwind_descr_init (struct obstack *obstack)
 {
   struct libunwind_descr *descr
-    = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct libunwind_descr);
+    = OBSTACK_ZALLOC (obstack, struct libunwind_descr);
 
   return descr;
 }
@@ -151,14 +151,7 @@ libunwind_frame_set_descr (struct gdbarch *gdbarch,
 
   arch_descr = ((struct libunwind_descr *)
 		gdbarch_data (gdbarch, libunwind_descr_handle));
-
-  if (arch_descr == NULL)
-    {
-      /* First time here.  Must initialize data area.  */
-      arch_descr = (struct libunwind_descr *) libunwind_descr_init (gdbarch);
-      deprecated_set_gdbarch_data (gdbarch,
-				   libunwind_descr_handle, arch_descr);
-    }
+  gdb_assert (arch_descr != NULL);
 
   /* Copy new descriptor info into arch descriptor.  */
   arch_descr->gdb2uw = descr->gdb2uw;
@@ -596,7 +589,7 @@ void
 _initialize_libunwind_frame ()
 {
   libunwind_descr_handle
-    = gdbarch_data_register_post_init (libunwind_descr_init);
+    = gdbarch_data_register_pre_init (libunwind_descr_init);
 
   libunwind_initialized = libunwind_load ();
 }
diff --git a/gdb/user-regs.c b/gdb/user-regs.c
index cb922313b0c..d461f857390 100644
--- a/gdb/user-regs.c
+++ b/gdb/user-regs.c
@@ -98,16 +98,15 @@ user_reg_add_builtin (const char *name, user_reg_read_ftype *xread,
 static struct gdbarch_data *user_regs_data;
 
 static void *
-user_regs_init (struct gdbarch *gdbarch)
+user_regs_init (struct obstack *obstack)
 {
   struct user_reg *reg;
-  struct gdb_user_regs *regs 
-    = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct gdb_user_regs);
+  struct gdb_user_regs *regs = OBSTACK_ZALLOC (obstack, struct gdb_user_regs);
 
   regs->last = &regs->first;
   for (reg = builtin_user_regs.first; reg != NULL; reg = reg->next)
     append_user_reg (regs, reg->name, reg->xread, reg->baton,
-		     GDBARCH_OBSTACK_ZALLOC (gdbarch, struct user_reg));
+		     OBSTACK_ZALLOC (obstack, struct user_reg));
   return regs;
 }
 
@@ -117,14 +116,7 @@ user_reg_add (struct gdbarch *gdbarch, const char *name,
 {
   struct gdb_user_regs *regs
     = (struct gdb_user_regs *) gdbarch_data (gdbarch, user_regs_data);
-
-  if (regs == NULL)
-    {
-      /* ULGH, called during architecture initialization.  Patch
-         things up.  */
-      regs = (struct gdb_user_regs *) user_regs_init (gdbarch);
-      deprecated_set_gdbarch_data (gdbarch, user_regs_data, regs);
-    }
+  gdb_assert (regs != NULL);
   append_user_reg (regs, name, xread, baton,
 		   GDBARCH_OBSTACK_ZALLOC (gdbarch, struct user_reg));
 }
@@ -240,7 +232,7 @@ void _initialize_user_regs ();
 void
 _initialize_user_regs ()
 {
-  user_regs_data = gdbarch_data_register_post_init (user_regs_init);
+  user_regs_data = gdbarch_data_register_pre_init (user_regs_init);
 
   add_cmd ("user-registers", class_maintenance,
 	   maintenance_print_user_registers,
-- 
2.25.4

Re: [PATCH 1/5] gdb: Remove deprecated_set_gdbarch_data

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> 	* gdbarch.c: Regenerate.
Andrew> 	* gdbarch.h: Regenerate.
Andrew> 	* gdbarch.sh (deprecated_set_gdbarch_data): Delete.
Andrew> 	(gdbarch_data): Use internal_error for the case where
Andrew> 	deprecated_set_gdbarch_data was originally needed.
Andrew> 	* ia64-libunwind-tdep.c (libunwind_descr_init): Update parameters,
Andrew> 	and use passed in obstack.
Andrew> 	(libunwind_frame_set_descr): Should no longer get back NULL from
Andrew> 	gdbarch_data.
Andrew> 	(_initialize_libunwind_frame): Register as a pre-init gdbarch data
Andrew> 	type.
Andrew> 	* user-regs.c (user_regs_init): Update parameters, and use passed
Andrew> 	in obstack.
Andrew> 	(user_reg_add): Should no longer get back NULL from gdbarch_data.
Andrew> 	(_initialize_user_regs): Register as a pre-init gdbarch data type.

Seems fine to me.

Tom

[PATCH 2/5] gdb/python: Add architecture method to gdb.PendingFrame

[Andrew Burgess]

It could be useful to determine the architecture of a frame being
unwound during the frame unwind process, that is, before we have a
gdb.Frame, but when we only have a gdb.PendingFrame.

The PendingFrame already has a pointer to the gdbarch internally, this
commit just exposes an 'architecture' method to Python, and has this
return a gdb.Architecture object (list gdb.Frame.architecture does).

gdb/ChangeLog:

	* python/py-unwind.c (pending_framepy_architecture): New function.
	(pending_frame_object_methods): Add architecture method.

gdb/testsuite/ChangeLog:

	* gdb.python/py-unwind.py (TestUnwinder::__call__): Add test for
	gdb.PendingFrame.architecture method.

gdb/doc/ChangeLog:

	* python.texi (Unwinding Frames in Python): Document
	PendingFrame.architecture method.
---
 gdb/ChangeLog                         |  5 +++++
 gdb/doc/ChangeLog                     |  5 +++++
 gdb/doc/python.texi                   |  6 ++++++
 gdb/python/py-unwind.c                | 20 ++++++++++++++++++++
 gdb/testsuite/ChangeLog               |  5 +++++
 gdb/testsuite/gdb.python/py-unwind.py | 10 +++++++++-
 6 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index a38f1dab426..e919b3cfd7e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2518,6 +2518,12 @@
 
 @end defun
 
+@defun PendingFrame.architecture ()
+Return the @code{gdb.Architecture} (@pxref{Architectures In Python})
+for this @code{gdb.PendingFrame}.  This represents the architecture of
+the particual frame being unwound.
+@end defun
+
 @subheading Unwinder Output: UnwindInfo
 
 Use @code{PendingFrame.create_unwind_info} method described above to
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index d583ff462b0..1cef491cedf 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -441,6 +441,22 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
                                     frame_id_build_special (sp, pc, special));
 }
 
+/* Implementation of PendingFrame.architecture (self) -> gdb.Architecture.  */
+
+static PyObject *
+pending_framepy_architecture (PyObject *self, PyObject *args)
+{
+  pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+  if (pending_frame->frame_info == NULL)
+    {
+      PyErr_SetString (PyExc_ValueError,
+                       "Attempting to read register from stale PendingFrame");
+      return NULL;
+    }
+  return gdbarch_to_arch_object (pending_frame->gdbarch);
+}
+
 /* frame_unwind.this_id method.  */
 
 static void
@@ -671,6 +687,10 @@ static PyMethodDef pending_frame_object_methods[] =
     "create_unwind_info (FRAME_ID) -> gdb.UnwindInfo\n"
     "Construct UnwindInfo for this PendingFrame, using FRAME_ID\n"
     "to identify it." },
+  { "architecture",
+    pending_framepy_architecture, METH_NOARGS,
+    "architecture () -> gdb.Architecture\n"
+    "The architecture for this PendingFrame." },
   {NULL}  /* Sentinel */
 };
 
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index 42dd6fe138c..d01da80f25b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -30,7 +30,6 @@ class FrameId(object):
     def pc(self):
         return self._pc
 
-
 class TestUnwinder(Unwinder):
     AMD64_RBP = 6
     AMD64_RSP = 7
@@ -69,6 +68,15 @@ class TestUnwinder(Unwinder):
         This unwinder recognizes the corrupt frames by checking that
         *RBP == RBP, and restores previous RBP from the word above it.
         """
+
+        # Check that we can access the architecture of the pending
+        # frame, and that this is the same architecture as for the
+        # currently selected inferior.
+        inf_arch = gdb.selected_inferior ().architecture ()
+        frame_arch = pending_frame.architecture ()
+        if (inf_arch != frame_arch):
+            raise gdb.GdbError ("architecture mismatch")
+
         try:
             # NOTE: the registers in Unwinder API can be referenced
             # either by name or by number. The code below uses both
-- 
2.25.4

Re: [PATCH 2/5] gdb/python: Add architecture method to gdb.PendingFrame

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Wed, 17 Jun 2020 18:38:06 +0100
> 
> gdb/ChangeLog:
> 
> 	* python/py-unwind.c (pending_framepy_architecture): New function.
> 	(pending_frame_object_methods): Add architecture method.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-unwind.py (TestUnwinder::__call__): Add test for
> 	gdb.PendingFrame.architecture method.
> 
> gdb/doc/ChangeLog:
> 
> 	* python.texi (Unwinding Frames in Python): Document
> 	PendingFrame.architecture method.

Thanks, the documentation part is OK.

Re: [PATCH 2/5] gdb/python: Add architecture method to gdb.PendingFrame

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> +@defun PendingFrame.architecture ()
Andrew> +Return the @code{gdb.Architecture} (@pxref{Architectures In Python})
Andrew> +for this @code{gdb.PendingFrame}.  This represents the architecture of
Andrew> +the particual frame being unwound.

Typo in "particual".

Also, I think we put new Python APIs into NEWS.

Otherwise this looks good.  Thanks.

Tom

[PATCH 3/5] gdb/python: Add gdb.Architecture.registers method

[Andrew Burgess]

This commit adds a new method gdb.Architecture.registers that returns
an object of the new type gdb.RegisterDescriptorIterator.  This
iterator returns objects of the new type gdb.RegisterDescriptor.

A RegisterDescriptor is not a way to read the value of a register,
this is already covered by Frame.read_register, a RegisterDescriptor
is simply a way to discover from Python, which registers are
available for a given architecture.

I did consider just returning a string, the name of each register,
instead of a RegisterDescriptor, however, I'm aware that it we don't
want to break the existing Python API in any way, so if I return just
a string now, but in the future we want more information about a
register then we would have to add a second API to get that
information.  By going straight to a descriptor object now, it is easy
to add additional properties in the future should we wish to.

Right now the only property of a register that a user can access is
the name of the register.

In future we might want to be able to ask the register about is
register groups, or its type.

gdb/ChangeLog:

	* Makefile.in (SUBDIR_PYTHON_SRCS): Add py-registers.c
	* python/py-arch.c (archpy_registers): New function.
	(arch_object_methods): Add 'registers' method.
	* python/py-registers.c: New file.
	* python/python-internal.h
	(gdbpy_new_register_descriptor_iterator): Declare.
	(gdbpy_initialize_registers): Declare.
	* python/python.c (do_start_initialization): Call
	gdbpy_initialize_registers.
	* NEWS: Mention additions to the Python API.

gdb/testsuite/ChangeLog:

	* gdb.python/py-arch-reg-names.exp: New file.

gdb/doc/ChangeLog:

	* python.texi (Python API): Add new section the menu.
	(Frames In Python): Add new @anchor.
	(Architectures In Python): Document new registers method.
	(Registers In Python): New section.
---
 gdb/ChangeLog                                 |  13 +
 gdb/Makefile.in                               |   1 +
 gdb/NEWS                                      |   5 +
 gdb/doc/ChangeLog                             |   7 +
 gdb/doc/python.texi                           |  32 +++
 gdb/python/py-arch.c                          |  27 ++
 gdb/python/py-registers.c                     | 269 ++++++++++++++++++
 gdb/python/python-internal.h                  |   5 +
 gdb/python/python.c                           |   1 +
 gdb/testsuite/ChangeLog                       |   4 +
 .../gdb.python/py-arch-reg-names.exp          |  87 ++++++
 11 files changed, 451 insertions(+)
 create mode 100644 gdb/python/py-registers.c
 create mode 100644 gdb/testsuite/gdb.python/py-arch-reg-names.exp

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 32d0eee7c63..38816e4f74e 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -400,6 +400,7 @@ SUBDIR_PYTHON_SRCS = \
 	python/py-record.c \
 	python/py-record-btrace.c \
 	python/py-record-full.c \
+	python/py-registers.c \
 	python/py-signalevent.c \
 	python/py-stopevent.c \
 	python/py-symbol.c \
diff --git a/gdb/NEWS b/gdb/NEWS
index cebfd18f0c6..0dc467dc95e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -96,6 +96,11 @@ GNU/Linux/RISC-V (gdbserver)	riscv*-*-linux*
   ** Commands written in Python can be in the "TUI" help class by
      registering with the new constant gdb.COMMAND_TUI.
 
+  ** New gdb.Architecture.registers method that returns a
+     gdb.RegisterDescriptorIterator object, an iterator that returns
+     gdb.RegisterDescriptor objects.  The new RegisterDescriptor is a
+     way to query the registers available for an architecture.
+
 *** Changes in GDB 9
 
 * 'thread-exited' event is now available in the annotations interface.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index e919b3cfd7e..7503063af61 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -163,6 +163,7 @@
                                 using Python.
 * Lazy Strings In Python::      Python representation of lazy strings.
 * Architectures In Python::     Python representation of architectures.
+* Registers In Python::         Python representation of registers.
 * TUI Windows In Python::       Implementing new TUI windows.
 @end menu
 
@@ -4684,6 +4685,7 @@
 @xref{Symbol Tables In Python}.
 @end defun
 
+@anchor{gdbpy_frame_read_register}
 @defun Frame.read_register (register)
 Return the value of @var{register} in this frame.  The @var{register}
 argument must be a string (e.g., @code{'sp'} or @code{'rax'}).
@@ -5715,6 +5717,36 @@
 @end table
 @end defun
 
+@anchor{gdbpy_architecture_registers}
+@defun Architecture.registers (@r{[} @var{reggroup} @r{]})
+Return a @code{gdb.RegisterDescriptorIterator} (@pxref{Registers In
+Python}) for all of the registers in @var{reggroup}, a string that is
+the name of a register group.  If @var{reggroup} is missing, or is the
+empty string, then the register group @dfn{all} is assumed.
+@end defun
+
+@node Registers In Python
+@subsubsection Registers In Python
+@cindex Registers In Python
+
+Python code can request from a @code{gdb.Architecture} information
+about the set of registers available
+(@pxref{gdbpy_architecture_registers,,@code{Architecture.registers}}).
+The register information is returned as a
+@code{gdb.RegisterDescriptorIterator}, which is an iterator that in
+turn returns @code{gdb.RegisterDescriptor} objects.
+
+A @code{gdb.RegisterDescriptor} does not provide the value of a
+register (@pxref{gdbpy_frame_read_register,,@code{Frame.read_register}} for reading a registers
+value), instead the @code{RegisterDescriptor} is a way to discover
+which registers are available for a particular architecture.
+
+A @code{gdb.RegisterDescriptor} has the following read-only properties:
+
+@defvar RegisterDescriptor.name
+The name of this register.
+@end defvar
+
 @node TUI Windows In Python
 @subsubsection Implementing new TUI windows
 @cindex Python TUI Windows
diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index 853c7a9e7ee..15f9f50d7d7 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -226,6 +226,28 @@ archpy_disassemble (PyObject *self, PyObject *args, PyObject *kw)
   return result_list.release ();
 }
 
+/* Implementation of gdb.Architecture.registers (self, reggroup) -> Iterator.
+   Returns an iterator over register descriptors for registers in GROUP
+   within the architecture SELF.  */
+
+static PyObject *
+archpy_registers (PyObject *self, PyObject *args, PyObject *kw)
+{
+  static const char *keywords[] = { "reggroup", NULL };
+  struct gdbarch *gdbarch = NULL;
+  const char *group_name = NULL;
+
+  /* Parse method arguments.  */
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "|s", keywords,
+					&group_name))
+    return NULL;
+
+  /* Extract the gdbarch from the self object.  */
+  ARCHPY_REQUIRE_VALID (self, gdbarch);
+
+  return gdbpy_new_register_descriptor_iterator (gdbarch, group_name);
+}
+
 /* Initializes the Architecture class in the gdb module.  */
 
 int
@@ -249,6 +271,11 @@ Return the name of the architecture as a string value." },
     "disassemble (start_pc [, end_pc [, count]]) -> List.\n\
 Return a list of at most COUNT disassembled instructions from START_PC to\n\
 END_PC." },
+  { "registers", (PyCFunction) archpy_registers,
+    METH_VARARGS | METH_KEYWORDS,
+    "registers ([ group-name ]) -> Iterator.\n\
+Return an iterator of register descriptors for the registers in register\n\
+group GROUP-NAME." },
   {NULL}  /* Sentinel */
 };
 
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
new file mode 100644
index 00000000000..b78e7426c98
--- /dev/null
+++ b/gdb/python/py-registers.c
@@ -0,0 +1,269 @@
+/* Python interface to register, and register group information.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "gdbarch.h"
+#include "arch-utils.h"
+#include "disasm.h"
+#include "reggroups.h"
+#include "python-internal.h"
+
+/* Structure for iterator over register descriptors.  */
+typedef struct {
+  PyObject_HEAD
+
+  /* The register group that the user is iterating over.  This will never
+     be NULL.  */
+  struct reggroup *reggroup;
+
+  /* The next register number to lookup.  Starts at 0 and counts up.  */
+  int regnum;
+
+  /* Pointer back to the architecture we're finding registers for.  */
+  struct gdbarch *gdbarch;
+} register_descriptor_iterator_object;
+
+extern PyTypeObject register_descriptor_iterator_object_type
+    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("register_descriptor_iterator_object");
+
+/* A register descriptor.  */
+typedef struct {
+  PyObject_HEAD
+
+  /* The register this is a descriptor for.  */
+  int regnum;
+
+  /* The architecture this is a register for.  */
+  struct gdbarch *gdbarch;
+} register_descriptor_object;
+
+extern PyTypeObject register_descriptor_object_type
+    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("register_descriptor_object");
+
+/* Create an return a new gdb.RegisterDescriptor object.  */
+static PyObject *
+gdbpy_new_register_descriptor (struct gdbarch *gdbarch,
+			       int regnum)
+{
+  /* Create a new object and fill in its details.  */
+  register_descriptor_object *reg
+    = PyObject_New (register_descriptor_object,
+		    &register_descriptor_object_type);
+  if (reg == NULL)
+    return NULL;
+  reg->regnum = regnum;
+  reg->gdbarch = gdbarch;
+  return (PyObject *) reg;
+}
+
+/* Convert the register descriptor to a string.  */
+
+static PyObject *
+gdbpy_register_descriptor_to_string (PyObject *self)
+{
+  register_descriptor_object *reg
+    = (register_descriptor_object *) self;
+  struct gdbarch *gdbarch = reg->gdbarch;
+  int regnum = reg->regnum;
+
+  const char *name = gdbarch_register_name (gdbarch, regnum);
+  return PyString_FromString (name);
+}
+
+/* Implement gdb.RegisterDescriptor.name attribute get function.  Return a
+   string that is the name of this register.  Due to checking when register
+   descriptors are created the name will never by the empty string.  */
+
+static PyObject *
+gdbpy_register_descriptor_name (PyObject *self, void *closure)
+{
+  return gdbpy_register_descriptor_to_string (self);
+}
+
+/* Create and return a new gdb.RegisterDescriptorIterator object which
+   will iterate over all registers in GROUP_NAME for GDBARCH.  If
+   GROUP_NAME is either NULL or the empty string then the ALL_REGGROUP is
+   used, otherwise lookup the register group matching GROUP_NAME and use
+   that.
+
+   This function can return NULL if GROUP_NAME isn't found.  */
+
+PyObject *
+gdbpy_new_register_descriptor_iterator (struct gdbarch *gdbarch,
+					const char *group_name)
+{
+  struct reggroup *grp = NULL;
+
+  /* Lookup the requested register group, or find the default.  */
+  if (group_name == NULL || *group_name == '\0')
+    grp = all_reggroup;
+  else
+    {
+      grp = reggroup_find (gdbarch, group_name);
+      if (grp == NULL)
+	{
+	  PyErr_SetString (PyExc_ValueError,
+			   _("Unknown register group name."));
+	  return NULL;
+	}
+    }
+  /* Create a new iterator object initialised for this architecture and
+     fill in all of the details.  */
+  register_descriptor_iterator_object *iter
+    = PyObject_New (register_descriptor_iterator_object,
+		    &register_descriptor_iterator_object_type);
+  if (iter == NULL)
+    return NULL;
+  iter->regnum = 0;
+  iter->gdbarch = gdbarch;
+  gdb_assert (grp != NULL);
+  iter->reggroup = grp;
+
+  return (PyObject *) iter;
+}
+
+/* Return a reference to the gdb.RegisterDescriptorIterator object.  */
+
+static PyObject *
+gdbpy_register_descriptor_iter (PyObject *self)
+{
+  Py_INCREF (self);
+  return self;
+}
+
+/* Return the next register name.  */
+
+static PyObject *
+gdbpy_register_descriptor_iter_next (PyObject *self)
+{
+  register_descriptor_iterator_object *iter_obj
+    = (register_descriptor_iterator_object *) self;
+  struct gdbarch *gdbarch = iter_obj->gdbarch;
+
+  do
+    {
+      if (iter_obj->regnum >= gdbarch_num_cooked_regs (gdbarch))
+	{
+	  PyErr_SetString (PyExc_StopIteration, _("No more registers"));
+	  return NULL;
+	}
+
+      const char *name = nullptr;
+      int regnum = iter_obj->regnum;
+      if (gdbarch_register_reggroup_p (gdbarch, regnum,
+				       iter_obj->reggroup))
+	name = gdbarch_register_name (gdbarch, regnum);
+      iter_obj->regnum++;
+
+      if (name != nullptr && *name != '\0')
+	return gdbpy_new_register_descriptor (gdbarch, regnum);
+    }
+  while (true);
+}
+
+/* Initializes the new Python classes from this file in the gdb module.  */
+
+int
+gdbpy_initialize_registers (void)
+{
+  register_descriptor_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&register_descriptor_object_type) < 0)
+    return -1;
+  if (gdb_pymodule_addobject
+      (gdb_module, "RegisterDescriptor",
+       (PyObject *) &register_descriptor_object_type) < 0)
+    return -1;
+
+  register_descriptor_iterator_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&register_descriptor_iterator_object_type) < 0)
+    return -1;
+  return (gdb_pymodule_addobject
+	  (gdb_module, "RegisterDescriptorIterator",
+	   (PyObject *) &register_descriptor_iterator_object_type));
+}
+
+PyTypeObject register_descriptor_iterator_object_type = {
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.RegisterDescriptorIterator",	  	/*tp_name*/
+  sizeof (register_descriptor_iterator_object),	/*tp_basicsize*/
+  0,				  /*tp_itemsize*/
+  0,				  /*tp_dealloc*/
+  0,				  /*tp_print*/
+  0,				  /*tp_getattr*/
+  0,				  /*tp_setattr*/
+  0,				  /*tp_compare*/
+  0,				  /*tp_repr*/
+  0,				  /*tp_as_number*/
+  0,				  /*tp_as_sequence*/
+  0,				  /*tp_as_mapping*/
+  0,				  /*tp_hash */
+  0,				  /*tp_call*/
+  0,				  /*tp_str*/
+  0,				  /*tp_getattro*/
+  0,				  /*tp_setattro*/
+  0,				  /*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,			/*tp_flags*/
+  "GDB architecture register descriptor iterator object",	/*tp_doc */
+  0,				  /*tp_traverse */
+  0,				  /*tp_clear */
+  0,				  /*tp_richcompare */
+  0,				  /*tp_weaklistoffset */
+  gdbpy_register_descriptor_iter,	  /*tp_iter */
+  gdbpy_register_descriptor_iter_next,  /*tp_iternext */
+  0				  /*tp_methods */
+};
+
+static gdb_PyGetSetDef gdbpy_register_descriptor_getset[] = {
+  { "name", gdbpy_register_descriptor_name, NULL,
+    "The name of this register.", NULL },
+  { NULL }  /* Sentinel */
+};
+
+PyTypeObject register_descriptor_object_type = {
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.RegisterDescriptor",	  /*tp_name*/
+  sizeof (register_descriptor_object),	/*tp_basicsize*/
+  0,				  /*tp_itemsize*/
+  0,				  /*tp_dealloc*/
+  0,				  /*tp_print*/
+  0,				  /*tp_getattr*/
+  0,				  /*tp_setattr*/
+  0,				  /*tp_compare*/
+  0,				  /*tp_repr*/
+  0,				  /*tp_as_number*/
+  0,				  /*tp_as_sequence*/
+  0,				  /*tp_as_mapping*/
+  0,				  /*tp_hash */
+  0,				  /*tp_call*/
+  gdbpy_register_descriptor_to_string,			/*tp_str*/
+  0,				  /*tp_getattro*/
+  0,				  /*tp_setattro*/
+  0,				  /*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
+  "GDB architecture register descriptor object",	/*tp_doc */
+  0,				  /*tp_traverse */
+  0,				  /*tp_clear */
+  0,				  /*tp_richcompare */
+  0,				  /*tp_weaklistoffset */
+  0,				  /*tp_iter */
+  0,				  /*tp_iternext */
+  0,				  /*tp_methods */
+  0,				  /*tp_members */
+  gdbpy_register_descriptor_getset			/*tp_getset */
+};
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index e352b303828..91cac319059 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -473,6 +473,9 @@ PyObject *gdbpy_lookup_objfile (PyObject *self, PyObject *args, PyObject *kw);
 
 PyObject *gdbarch_to_arch_object (struct gdbarch *gdbarch);
 
+PyObject *gdbpy_new_register_descriptor_iterator (struct gdbarch *gdbarch,
+						  const char *group_name);
+
 gdbpy_ref<thread_object> create_thread_object (struct thread_info *tp);
 gdbpy_ref<> thread_to_thread_object (thread_info *thr);;
 gdbpy_ref<inferior_object> inferior_to_inferior_object (inferior *inf);
@@ -540,6 +543,8 @@ int gdbpy_initialize_py_events (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_arch (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
+int gdbpy_initialize_registers (void)
+  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_xmethods (void)
   CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
 int gdbpy_initialize_unwind (void)
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 4bdd2201abc..7787dce4b4c 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1759,6 +1759,7 @@ do_start_initialization ()
       || gdbpy_initialize_py_events () < 0
       || gdbpy_initialize_event () < 0
       || gdbpy_initialize_arch () < 0
+      || gdbpy_initialize_registers () < 0
       || gdbpy_initialize_xmethods () < 0
       || gdbpy_initialize_unwind () < 0
       || gdbpy_initialize_tui () < 0)
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
new file mode 100644
index 00000000000..14bc0a822a4
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -0,0 +1,87 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check the gdb.Architecture.registers functionality.
+
+load_lib gdb-python.exp
+standard_testfile py-arch.c
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] {
+   return -1
+}
+
+# First, use 'info registers' to get a list of register names.
+set regs {}
+gdb_test_multiple "info registers general" "info registers general" {
+    -re "^info registers general\r\n" {
+	exp_continue
+    }
+    -re "^(\[^ \t\]+)\[ \t\]+\[^\r\n\]+\r\n" {
+	set reg $expect_out(1,string)
+	lappend regs $reg
+	exp_continue
+    }
+    -re "^$gdb_prompt " {
+    }
+}
+gdb_assert {[llength $regs] > 0} \
+    "Found at least one register"
+
+# Now get the same register names using Python API.
+gdb_py_test_silent_cmd \
+    "python frame = gdb.selected_frame()" "get frame" 0
+gdb_py_test_silent_cmd \
+    "python arch = frame.architecture()" "get arch" 0
+gdb_py_test_silent_cmd \
+    "python regs = list (arch.registers (\"general\"))" \
+    "get general registers" 0
+gdb_py_test_silent_cmd \
+    "python regs = map (lambda r : r.name, regs)" \
+    "get names of general registers" 0
+
+set py_regs {}
+gdb_test_multiple "python print (\"\\n\".join (regs))" \
+    "general register from python" {
+	-re "^python print \[^\r\n\]+\r\n" {
+	    exp_continue
+	}
+	-re "^(\[^\r\n\]+)\r\n" {
+	    set reg $expect_out(1,string)
+	    lappend py_regs $reg
+	    exp_continue
+	}
+	-re "^$gdb_prompt " {
+	}
+    }
+
+gdb_assert {[llength $py_regs] > 0} \
+    "Found at least one register from python"
+gdb_assert {[llength $py_regs] == [llength $regs]} \
+    "Same numnber of registers found"
+
+set found_non_match 0
+for { set i 0 } { $i < [llength $regs] } { incr i } {
+    if {[lindex $regs $i] != [lindex $py_regs $i]} {
+	set found_non_match 1
+    }
+}
+gdb_assert { $found_non_match == 0 } "all registers match"
-- 
2.25.4

Re: [PATCH 3/5] gdb/python: Add gdb.Architecture.registers method

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Wed, 17 Jun 2020 18:38:07 +0100
> gdb/ChangeLog:
> 
> 	* Makefile.in (SUBDIR_PYTHON_SRCS): Add py-registers.c
> 	* python/py-arch.c (archpy_registers): New function.
> 	(arch_object_methods): Add 'registers' method.
> 	* python/py-registers.c: New file.
> 	* python/python-internal.h
> 	(gdbpy_new_register_descriptor_iterator): Declare.
> 	(gdbpy_initialize_registers): Declare.
> 	* python/python.c (do_start_initialization): Call
> 	gdbpy_initialize_registers.
> 	* NEWS: Mention additions to the Python API.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-arch-reg-names.exp: New file.
> 
> gdb/doc/ChangeLog:
> 
> 	* python.texi (Python API): Add new section the menu.
                                                    ^^^^^^^^
"to the menu".

> 	(Frames In Python): Add new @anchor.
> 	(Architectures In Python): Document new registers method.
> 	(Registers In Python): New section.

The documentation parts are approved, with a few nits:

> +@anchor{gdbpy_architecture_registers}
> +@defun Architecture.registers (@r{[} @var{reggroup} @r{]})
> +Return a @code{gdb.RegisterDescriptorIterator} (@pxref{Registers In
> +Python}) for all of the registers in @var{reggroup}, a string that is
> +the name of a register group.  If @var{reggroup} is missing, or is the
                                                       ^^^^^^^
"omitted" is better here.

> +empty string, then the register group @dfn{all} is assumed.
                                         ^^^^^^^^^
Please use @samp here instead of @dfn.  The latter is for markup of
terminology, which is not what you want here.

> +A @code{gdb.RegisterDescriptor} does not provide the value of a
> +register (@pxref{gdbpy_frame_read_register,,@code{Frame.read_register}} for reading a registers
                                                                                         ^^^^^^^^^
You meant "register's" here, with the apostrophe, right?

Thanks.

Re: [PATCH 3/5] gdb/python: Add gdb.Architecture.registers method

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Thanks for doing this.

Andrew> This commit adds a new method gdb.Architecture.registers that returns
Andrew> an object of the new type gdb.RegisterDescriptorIterator.  This
Andrew> iterator returns objects of the new type gdb.RegisterDescriptor.

Andrew> A RegisterDescriptor is not a way to read the value of a register,
Andrew> this is already covered by Frame.read_register, a RegisterDescriptor
Andrew> is simply a way to discover from Python, which registers are
Andrew> available for a given architecture.

Andrew> I did consider just returning a string, the name of each register,
Andrew> instead of a RegisterDescriptor, however, I'm aware that it we don't
Andrew> want to break the existing Python API in any way, so if I return just
Andrew> a string now, but in the future we want more information about a
Andrew> register then we would have to add a second API to get that
Andrew> information.  By going straight to a descriptor object now, it is easy
Andrew> to add additional properties in the future should we wish to.

Andrew> Right now the only property of a register that a user can access is
Andrew> the name of the register.

Andrew> In future we might want to be able to ask the register about is
Andrew> register groups, or its type.

This all seems reasonable.

Andrew> +int
Andrew> +gdbpy_initialize_registers (void)

No need for "void" here in new code.
Same in the .h.

Looks good to me with those nits fixed.

Tom

[PATCH 4/5] gdb/python: New method to access list of register groups

[Andrew Burgess]

Add a new method gdb.Architecture.register_groups which returns a new
object of type gdb.RegisterGroupsIterator.  This new iterator then
returns objects of type gdb.RegisterGroup.

Each gdb.RegisterGroup object just wraps a single reggroup pointer,
and (currently) has just one read-only property 'name' that is a
string, the name of the register group.

As with the previous commit (adding gdb.RegisterDescriptor) I made
gdb.RegisterGroup an object rather than just a string in case we want
to add additional properties in the future.

gdb/ChangeLog:

	* NEWS: Mention additions to Python API.
	* python/py-arch.c (archpy_register_groups): New function.
	(arch_object_methods): Add 'register_groups' method.
	* python/py-registers.c (reggroup_iterator_object): New struct.
	(reggroup_object): New struct.
	(gdbpy_new_reggroup): New function.
	(gdbpy_reggroup_to_string): New function.
	(gdbpy_reggroup_name): New function.
	(gdbpy_reggroup_iter): New function.
	(gdbpy_reggroup_iter_next): New function.
	(gdbpy_new_reggroup_iterator): New function
	(gdbpy_initialize_registers): Register new types.
	(reggroup_iterator_object_type): Define new Python type.
	(gdbpy_reggroup_getset): New static global.
	(reggroup_object_type): Define new Python type.
	* python/python-internal.h

gdb/testsuite/ChangeLog:

	* gdb.python/py-arch-reg-groups.exp: New file.

gdb/doc/ChangeLog:

	* gdb.texi (Registers): Add @anchor for 'info registers
	<reggroup>' command.
	* python.texi (Architectures In Python): Document new
	register_groups method.
	(Registers In Python): Document two new object types related to
	register groups.
---
 gdb/ChangeLog                                 |  19 ++
 gdb/NEWS                                      |   5 +
 gdb/doc/ChangeLog                             |   9 +
 gdb/doc/gdb.texinfo                           |   1 +
 gdb/doc/python.texi                           |  32 +++
 gdb/python/py-arch.c                          |  18 ++
 gdb/python/py-registers.c                     | 195 ++++++++++++++++++
 gdb/python/python-internal.h                  |   1 +
 gdb/testsuite/ChangeLog                       |   4 +
 .../gdb.python/py-arch-reg-groups.exp         |  87 ++++++++
 10 files changed, 371 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-arch-reg-groups.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 0dc467dc95e..1c684427b1d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -101,6 +101,11 @@ GNU/Linux/RISC-V (gdbserver)	riscv*-*-linux*
      gdb.RegisterDescriptor objects.  The new RegisterDescriptor is a
      way to query the registers available for an architecture.
 
+  ** New gdb.Architecture.register_groups method that returns a
+     gdb.RegisterGroupIterator object, an iterator that returns
+     gdb.RegisterGroup objects.  The new RegisterGroup is a way to
+     discover the available register groups.
+
 *** Changes in GDB 9
 
 * 'thread-exited' event is now available in the annotations interface.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 59e3e75d18a..56cd10ec2e2 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -12344,6 +12344,7 @@
 Print the names and values of all registers, including floating-point
 and vector registers (in the selected stack frame).
 
+@anchor{info_registers_reggroup}
 @item info registers @var{reggroup} @dots{}
 Print the name and value of the registers in each of the specified
 @var{reggroup}s.  The @var{reggroup} can be any of those returned by
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7503063af61..6ecc3ea63f0 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5725,6 +5725,13 @@
 empty string, then the register group @dfn{all} is assumed.
 @end defun
 
+@anchor{gdbpy_architecture_reggroups}
+@defun Architecture.register_groups ()
+Return a @code{gdb.RegisterGroupsIterator} (@pxref{Registers In
+Python}) for all of the register groups available for the
+@code{gdb.Architecture}.
+@end defun
+
 @node Registers In Python
 @subsubsection Registers In Python
 @cindex Registers In Python
@@ -5747,6 +5754,31 @@
 The name of this register.
 @end defvar
 
+Python code can also request from a @code{gdb.Architecture}
+information about the set of register groups available on a given
+architecture
+(@pxref{gdbpy_architecture_reggroups,,@code{Architecture.register_groups}}).
+
+Every register can be a member of zero or more register groups.  Some
+register groups are used internally within @value{GDBN} to control
+things like which registers must be saved when calling into the
+program being debugged (@pxref{Calling,,Calling Program Functions}).
+Other register groups exist to allow users to easily see related sets
+of registers in commands like @code{info registers}
+(@pxref{info_registers_reggroup,,@code{info registers
+@var{reggroup}}}).
+
+The register groups information is returned as a
+@code{gdb.RegisterGroupsIterator}, which is an iterator that in turn
+returns @code{gdb.RegisterGroup} objects.
+
+A @code{gdb.RegisterGroup} object has the following read-only
+properties:
+
+@defvar RegisterGroup.name
+A string that is the name of this register group.
+@end defvar
+
 @node TUI Windows In Python
 @subsubsection Implementing new TUI windows
 @cindex Python TUI Windows
diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
index 15f9f50d7d7..d9eaf81a30a 100644
--- a/gdb/python/py-arch.c
+++ b/gdb/python/py-arch.c
@@ -248,6 +248,20 @@ archpy_registers (PyObject *self, PyObject *args, PyObject *kw)
   return gdbpy_new_register_descriptor_iterator (gdbarch, group_name);
 }
 
+/* Implementation of gdb.Architecture.register_groups (self) -> Iterator.
+   Returns an iterator that will give up all valid register groups in the
+   architecture SELF.  */
+
+static PyObject *
+archpy_register_groups (PyObject *self, PyObject *args)
+{
+  struct gdbarch *gdbarch = NULL;
+
+  /* Extract the gdbarch from the self object.  */
+  ARCHPY_REQUIRE_VALID (self, gdbarch);
+  return gdbpy_new_reggroup_iterator (gdbarch);
+}
+
 /* Initializes the Architecture class in the gdb module.  */
 
 int
@@ -276,6 +290,10 @@ END_PC." },
     "registers ([ group-name ]) -> Iterator.\n\
 Return an iterator of register descriptors for the registers in register\n\
 group GROUP-NAME." },
+  { "register_groups", archpy_register_groups,
+    METH_NOARGS,
+    "register_groups () -> Iterator.\n\
+Return an iterator over all of the register groups in this architecture." },
   {NULL}  /* Sentinel */
 };
 
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index b78e7426c98..3a6bc276d5b 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -56,6 +56,67 @@ typedef struct {
 extern PyTypeObject register_descriptor_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("register_descriptor_object");
 
+/* Structure for iterator over register groups.  */
+typedef struct {
+  PyObject_HEAD
+
+  /* The last register group returned.  Initially this will be NULL.  */
+  struct reggroup *reggroup;
+
+  /* Pointer back to the architecture we're finding registers for.  */
+  struct gdbarch *gdbarch;
+} reggroup_iterator_object;
+
+extern PyTypeObject reggroup_iterator_object_type
+    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_iterator_object");
+
+/* A register group object.  */
+typedef struct {
+  PyObject_HEAD
+
+  /* The register group being described.  */
+  struct reggroup *reggroup;
+} reggroup_object;
+
+extern PyTypeObject reggroup_object_type
+    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_object");
+
+/* Create a new gdb.RegisterGroup object wrapping REGGROUP.  */
+
+static PyObject *
+gdbpy_new_reggroup (struct reggroup *reggroup)
+{
+  /* Create a new object and fill in its details.  */
+  reggroup_object *group
+    = PyObject_New (reggroup_object, &reggroup_object_type);
+  if (group == NULL)
+    return NULL;
+  group->reggroup = reggroup;
+  return (PyObject *) group;
+}
+
+/* Convert a gdb.RegisterGroup to a string, it just returns the name of
+   the register group.  */
+
+static PyObject *
+gdbpy_reggroup_to_string (PyObject *self)
+{
+  reggroup_object *group = (reggroup_object *) self;
+  struct reggroup *reggroup = group->reggroup;
+
+  const char *name = reggroup_name (reggroup);
+  return PyString_FromString (name);
+}
+
+/* Implement gdb.RegisterGroup.name (self) -> String.
+   Return a string that is the name of this register group.  */
+
+static PyObject *
+gdbpy_reggroup_name (PyObject *self, void *closure)
+{
+  return gdbpy_reggroup_to_string (self);
+}
+
 /* Create an return a new gdb.RegisterDescriptor object.  */
 static PyObject *
 gdbpy_new_register_descriptor (struct gdbarch *gdbarch,
@@ -96,6 +157,54 @@ gdbpy_register_descriptor_name (PyObject *self, void *closure)
   return gdbpy_register_descriptor_to_string (self);
 }
 
+/* Return a reference to the gdb.RegisterGroupsIterator object.  */
+
+static PyObject *
+gdbpy_reggroup_iter (PyObject *self)
+{
+  Py_INCREF (self);
+  return self;
+}
+
+/* Return the next gdb.RegisterGroup object from the iterator.  */
+
+static PyObject *
+gdbpy_reggroup_iter_next (PyObject *self)
+{
+  reggroup_iterator_object *iter_obj
+    = (reggroup_iterator_object *) self;
+  struct gdbarch *gdbarch = iter_obj->gdbarch;
+
+  struct reggroup *next_group = reggroup_next (gdbarch, iter_obj->reggroup);
+  if (next_group == NULL)
+    {
+      PyErr_SetString (PyExc_StopIteration, _("No more groups"));
+      return NULL;
+    }
+
+  iter_obj->reggroup = next_group;
+  return gdbpy_new_reggroup (iter_obj->reggroup);
+}
+
+/* Return a new gdb.RegisterGroupsIterator over all the register groups in
+   GDBARCH.  */
+
+PyObject *
+gdbpy_new_reggroup_iterator (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != nullptr);
+
+  /* Create a new object and fill in its internal state.  */
+  reggroup_iterator_object *iter
+    = PyObject_New (reggroup_iterator_object,
+		    &reggroup_iterator_object_type);
+  if (iter == NULL)
+    return NULL;
+  iter->reggroup = NULL;
+  iter->gdbarch = gdbarch;
+  return (PyObject *) iter;
+}
+
 /* Create and return a new gdb.RegisterDescriptorIterator object which
    will iterate over all registers in GROUP_NAME for GDBARCH.  If
    GROUP_NAME is either NULL or the empty string then the ALL_REGGROUP is
@@ -190,6 +299,22 @@ gdbpy_initialize_registers (void)
        (PyObject *) &register_descriptor_object_type) < 0)
     return -1;
 
+  reggroup_iterator_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&reggroup_iterator_object_type) < 0)
+    return -1;
+  if (gdb_pymodule_addobject
+      (gdb_module, "RegisterGroupsIterator",
+       (PyObject *) &reggroup_iterator_object_type) < 0)
+    return -1;
+
+  reggroup_object_type.tp_new = PyType_GenericNew;
+  if (PyType_Ready (&reggroup_object_type) < 0)
+    return -1;
+  if (gdb_pymodule_addobject
+      (gdb_module, "RegisterGroup",
+       (PyObject *) &reggroup_object_type) < 0)
+    return -1;
+
   register_descriptor_iterator_object_type.tp_new = PyType_GenericNew;
   if (PyType_Ready (&register_descriptor_iterator_object_type) < 0)
     return -1;
@@ -267,3 +392,73 @@ PyTypeObject register_descriptor_object_type = {
   0,				  /*tp_members */
   gdbpy_register_descriptor_getset			/*tp_getset */
 };
+
+PyTypeObject reggroup_iterator_object_type = {
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.RegisterGroupsIterator",	  /*tp_name*/
+  sizeof (reggroup_iterator_object),		/*tp_basicsize*/
+  0,				  /*tp_itemsize*/
+  0,				  /*tp_dealloc*/
+  0,				  /*tp_print*/
+  0,				  /*tp_getattr*/
+  0,				  /*tp_setattr*/
+  0,				  /*tp_compare*/
+  0,				  /*tp_repr*/
+  0,				  /*tp_as_number*/
+  0,				  /*tp_as_sequence*/
+  0,				  /*tp_as_mapping*/
+  0,				  /*tp_hash */
+  0,				  /*tp_call*/
+  0,				  /*tp_str*/
+  0,				  /*tp_getattro*/
+  0,				  /*tp_setattro*/
+  0,				  /*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_ITER,	/*tp_flags*/
+  "GDB register groups iterator object",	/*tp_doc */
+  0,				  /*tp_traverse */
+  0,				  /*tp_clear */
+  0,				  /*tp_richcompare */
+  0,				  /*tp_weaklistoffset */
+  gdbpy_reggroup_iter,		  /*tp_iter */
+  gdbpy_reggroup_iter_next,	  /*tp_iternext */
+  0				  /*tp_methods */
+};
+
+static gdb_PyGetSetDef gdbpy_reggroup_getset[] = {
+  { "name", gdbpy_reggroup_name, NULL,
+    "The name of this register group.", NULL },
+  { NULL }  /* Sentinel */
+};
+
+PyTypeObject reggroup_object_type = {
+  PyVarObject_HEAD_INIT (NULL, 0)
+  "gdb.RegisterGroup",		  /*tp_name*/
+  sizeof (reggroup_object),	  /*tp_basicsize*/
+  0,				  /*tp_itemsize*/
+  0,				  /*tp_dealloc*/
+  0,				  /*tp_print*/
+  0,				  /*tp_getattr*/
+  0,				  /*tp_setattr*/
+  0,				  /*tp_compare*/
+  0,				  /*tp_repr*/
+  0,				  /*tp_as_number*/
+  0,				  /*tp_as_sequence*/
+  0,				  /*tp_as_mapping*/
+  0,				  /*tp_hash */
+  0,				  /*tp_call*/
+  gdbpy_reggroup_to_string,	  /*tp_str*/
+  0,				  /*tp_getattro*/
+  0,				  /*tp_setattro*/
+  0,				  /*tp_as_buffer*/
+  Py_TPFLAGS_DEFAULT,		  /*tp_flags*/
+  "GDB register group object",	  /*tp_doc */
+  0,				  /*tp_traverse */
+  0,				  /*tp_clear */
+  0,				  /*tp_richcompare */
+  0,				  /*tp_weaklistoffset */
+  0,				  /*tp_iter */
+  0,				  /*tp_iternext */
+  0,				  /*tp_methods */
+  0,				  /*tp_members */
+  gdbpy_reggroup_getset		  /*tp_getset */
+};
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 91cac319059..3f4fd2a886d 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -475,6 +475,7 @@ PyObject *gdbarch_to_arch_object (struct gdbarch *gdbarch);
 
 PyObject *gdbpy_new_register_descriptor_iterator (struct gdbarch *gdbarch,
 						  const char *group_name);
+PyObject *gdbpy_new_reggroup_iterator (struct gdbarch *gdbarch);
 
 gdbpy_ref<thread_object> create_thread_object (struct thread_info *tp);
 gdbpy_ref<> thread_to_thread_object (thread_info *thr);;
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-groups.exp b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
new file mode 100644
index 00000000000..ea9aa77b0fa
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
@@ -0,0 +1,87 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check the gdb.Architecture.register_groups functionality.
+
+load_lib gdb-python.exp
+standard_testfile py-arch.c
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if ![runto_main] {
+   return -1
+}
+
+# First, use 'maint print reggroups' to get a list of all register
+# groups.
+set groups {}
+set test "maint print reggroups"
+gdb_test_multiple $test $test {
+    -re ".*Group\[ \t\]+Type\[ \t\]+\r\n" {
+	exp_continue
+    }
+    -re "^ (\[^ \t\]+)\[ \t\]+\[^\r\n\]+\r\n" {
+	lappend groups $expect_out(1,string)
+	exp_continue
+    }
+    -re "^$gdb_prompt " {
+    }
+}
+gdb_assert {[llength $groups] > 0} \
+    "Found at least one register group"
+
+# Now get the same register names using Python API.
+gdb_py_test_silent_cmd \
+    "python frame = gdb.selected_frame()" "get frame" 0
+gdb_py_test_silent_cmd \
+    "python arch = frame.architecture()" "get arch" 0
+gdb_py_test_silent_cmd \
+    "python groups = list (arch.register_groups ())" \
+    "get register groups" 0
+gdb_py_test_silent_cmd \
+    "python groups = map (lambda obj: obj.name, groups)" \
+    "convert groups to names" 0
+
+set py_groups {}
+gdb_test_multiple "python print (\"\\n\".join (groups))" \
+    "register groups from python" {
+	-re "^python print \[^\r\n\]+\r\n" {
+	    exp_continue
+	}
+	-re "^(\[^\r\n\]+)\r\n" {
+	    lappend py_groups $expect_out(1,string)
+	    exp_continue
+	}
+	-re "^$gdb_prompt " {
+	}
+    }
+
+gdb_assert {[llength $py_groups] > 0} \
+    "Found at least one register group from python"
+gdb_assert {[llength $py_groups] == [llength $groups]} \
+    "Same numnber of registers groups found"
+
+set found_non_match 0
+for { set i 0 } { $i < [llength $groups] } { incr i } {
+    if {[lindex $groups $i] != [lindex $py_groups $i]} {
+	set found_non_match 1
+    }
+}
+gdb_assert { $found_non_match == 0 } "all register groups match"
-- 
2.25.4

Re: [PATCH 4/5] gdb/python: New method to access list of register groups

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Wed, 17 Jun 2020 18:38:08 +0100
> 
> gdb/ChangeLog:
> 
> 	* NEWS: Mention additions to Python API.
> 	* python/py-arch.c (archpy_register_groups): New function.
> 	(arch_object_methods): Add 'register_groups' method.
> 	* python/py-registers.c (reggroup_iterator_object): New struct.
> 	(reggroup_object): New struct.
> 	(gdbpy_new_reggroup): New function.
> 	(gdbpy_reggroup_to_string): New function.
> 	(gdbpy_reggroup_name): New function.
> 	(gdbpy_reggroup_iter): New function.
> 	(gdbpy_reggroup_iter_next): New function.
> 	(gdbpy_new_reggroup_iterator): New function
> 	(gdbpy_initialize_registers): Register new types.
> 	(reggroup_iterator_object_type): Define new Python type.
> 	(gdbpy_reggroup_getset): New static global.
> 	(reggroup_object_type): Define new Python type.
> 	* python/python-internal.h
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-arch-reg-groups.exp: New file.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texi (Registers): Add @anchor for 'info registers
> 	<reggroup>' command.
> 	* python.texi (Architectures In Python): Document new
> 	register_groups method.
> 	(Registers In Python): Document two new object types related to
> 	register groups.

The documentation parts are OK, thanks.

Re: [PATCH 4/5] gdb/python: New method to access list of register groups

[Christian Biesinger]

On Wed, Jun 17, 2020 at 12:38 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> Add a new method gdb.Architecture.register_groups which returns a new
> object of type gdb.RegisterGroupsIterator.  This new iterator then
> returns objects of type gdb.RegisterGroup.
>
> Each gdb.RegisterGroup object just wraps a single reggroup pointer,
> and (currently) has just one read-only property 'name' that is a
> string, the name of the register group.
>
> As with the previous commit (adding gdb.RegisterDescriptor) I made
> gdb.RegisterGroup an object rather than just a string in case we want
> to add additional properties in the future.

So this adds just the ability to get the names of the groups, without
a way to associate specific registers with the group? Out of
curiosity, what's the use case for that?

Christian

Re: [PATCH 4/5] gdb/python: New method to access list of register groups

[Andrew Burgess]

* Christian Biesinger <cbiesinger@google.com> [2020-06-17 13:40:47 -0500]:

> On Wed, Jun 17, 2020 at 12:38 PM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> >
> > Add a new method gdb.Architecture.register_groups which returns a new
> > object of type gdb.RegisterGroupsIterator.  This new iterator then
> > returns objects of type gdb.RegisterGroup.
> >
> > Each gdb.RegisterGroup object just wraps a single reggroup pointer,
> > and (currently) has just one read-only property 'name' that is a
> > string, the name of the register group.
> >
> > As with the previous commit (adding gdb.RegisterDescriptor) I made
> > gdb.RegisterGroup an object rather than just a string in case we want
> > to add additional properties in the future.
> 
> So this adds just the ability to get the names of the groups, without
> a way to associate specific registers with the group? Out of
> curiosity, what's the use case for that?

I did consider that, but remember that groups exist across
architectures.  That is, there's only one "all" group that everyone
shares.

As such to go from group -> registers you would have to supply an
architecture.  And by that point you may just as well call the
registers method on the architecture.

As for use case the only thing that comes immediately to mind would be
if you wanted to implement a new kind of register viewing window in
the TUI.  This would provide a way for Python to figure out which
register groups exist.

Honestly, I needed the registers API to write my test (for a later
patch in the series), and I added this one mostly just because I was
working in the area and it seemed kind-of related.

Thanks,
Andrew

Re: [PATCH 4/5] gdb/python: New method to access list of register groups

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Add a new method gdb.Architecture.register_groups which returns a new
Andrew> object of type gdb.RegisterGroupsIterator.  This new iterator then
Andrew> returns objects of type gdb.RegisterGroup.

Andrew> Each gdb.RegisterGroup object just wraps a single reggroup pointer,
Andrew> and (currently) has just one read-only property 'name' that is a
Andrew> string, the name of the register group.

Andrew> As with the previous commit (adding gdb.RegisterDescriptor) I made
Andrew> gdb.RegisterGroup an object rather than just a string in case we want
Andrew> to add additional properties in the future.

FWIW this looks good to me.

thanks,
Tom

[PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

I observed that when making use of a Python frame unwinder, if the
frame sniffing code accessed any registers within an inline frame then
GDB would crash with this error:

  gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.

The problem is that, when in the Python unwinder I write this:

  pending_frame.read_register ("register-name")

This is translated internally into a call to `value_of_register',
which in turn becomes a call to `value_of_register_lazy'.

Usually this isn't a problem, `value_of_register_lazy' requires the
next frame (more inner) to have a valid frame_id, which will be the
case (if we're sniffing frame #1, then frame #0 will have had its
frame-id figured out).

Unfortunately if frame #0 is inline within frame #1, then the frame-id
for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
frame #0 requires us to first compute the frame-id for frame #1.  As a
result it is not same to call `value_of_register_lazy' before the
frame-id of the current frame is known.

The solution I propose here is that `value_of_register' stops making a
lazy register value (the step that requires the frame-id, and thus
causes the problems), and instead calls `get_frame_register_value'.
This avoids the need to calculate the frame-id and so avoids the
problem.

This bug has crept in because we allowed calls to the function
`value_of_register_lazy' at a time when the frame-id of the frame
passed to the function didn't have its frame-id calculated.  This is
only ever a problem for inline frames.  To prevent bugs like this
getting into GDB in the future I've added an assert to the function
`value_of_register_lazy' to require the current frame have its id
calculated.

If we are calling from outside of the frame sniffer then this will
_always_ be true, so is not a problem.  If we are calling this
function from within a frame sniffer then it will always trigger.

gdb/ChangeLog:

	* findvar.c (value_of_register): Use get_frame_register_value
	rather than value_of_register_lazy.
	(value_of_register_lazy): Add new assert, and update comments.

gdb/testsuite/ChangeLog:

	* gdb.python/py-unwind-inline.c: New file.
	* gdb.python/py-unwind-inline.exp: New file.
	* gdb.python/py-unwind-inline.py: New file.
---
 gdb/ChangeLog                                 |  6 ++
 gdb/findvar.c                                 | 28 ++++++--
 gdb/testsuite/ChangeLog                       |  6 ++
 gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
 gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
 gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
 6 files changed, 192 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
 create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py

diff --git a/gdb/findvar.c b/gdb/findvar.c
index c7cd31ce1a6..e5445b34930 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -263,16 +263,13 @@ struct value *
 value_of_register (int regnum, struct frame_info *frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  struct value *reg_val;
 
   /* User registers lie completely outside of the range of normal
      registers.  Catch them early so that the target never sees them.  */
   if (regnum >= gdbarch_num_cooked_regs (gdbarch))
     return value_of_user_reg (regnum, frame);
 
-  reg_val = value_of_register_lazy (frame, regnum);
-  value_fetch_lazy (reg_val);
-  return reg_val;
+  return get_frame_register_value (frame, regnum);
 }
 
 /* Return a `value' with the contents of (virtual or cooked) register
@@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 
   next_frame = get_next_frame_sentinel_okay (frame);
 
-  /* We should have a valid next frame.  */
+  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
+     for the next frame will require that FRAME has a valid frame-id.
+     Usually this is not a problem, however, if this function is ever
+     called from a frame sniffer, the small window of time when not all
+     frames have yet had their frame-id calculated, this function will
+     trigger an assert within get_frame_id that a frame at a level > 0
+     doesn't have a frame id.
+
+     Instead of waiting for an inline frame to reveal an invalid use of
+     this function, we instead demand that FRAME must have had its frame-id
+     calculated before we use this function.
+
+     The one time it is safe to call this function when FRAME does not yet
+     have a frame id is when FRAME is at level 0, in this case the
+     assertion in get_frame_id doesn't fire, and instead get_frame_id will
+     correctly compute the frame id for us.  */
+  gdb_assert (frame_relative_level (frame) == 0
+	      || frame_id_computed_p (frame));
+
+  /* We should have a valid next frame too.  Given that FRAME is more
+     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
+     then this must always be true.  */
   gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
   reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
new file mode 100644
index 00000000000..0cfe06dd273
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
@@ -0,0 +1,37 @@
+/* Copyright 2019-2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+volatile int global_var;
+
+int  __attribute__ ((noinline))
+bar ()
+{
+  return global_var;
+}
+
+static inline int __attribute__ ((always_inline))
+foo ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  int ans;
+  global_var = 0;
+  ans = foo ();
+  return ans;
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
new file mode 100644
index 00000000000..f7c65f6afc8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
@@ -0,0 +1,49 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This script tests GDB's handling of using a Python unwinder in the
+# presence of inlined frames.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  debug] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+# The following tests require execution.
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+gdb_breakpoint "foo"
+
+gdb_test "source ${pyfile}" "Python script imported" \
+        "import python scripts"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
+    "\\r\\n#0  foo \\(\\)"
+    "\\r\\n#1  main \\(\\)"
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
new file mode 100644
index 00000000000..7206a0b2830
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
@@ -0,0 +1,71 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# A dummy stack unwinder used for testing the Python unwinders when we
+# have inline frames.  This unwinder will never claim any frames,
+# instead, all it does it try to read all registers possible target
+# registers as part of the frame sniffing process..
+
+import gdb
+from gdb.unwinder import Unwinder
+
+apb_global = None
+
+class dummy_unwinder (Unwinder):
+    """ A dummy unwinder that looks at a bunch of registers as part of
+    the unwinding process."""
+
+    class frame_id (object):
+        """ Basic frame id."""
+
+        def __init__ (self, sp, pc):
+            """ Create the frame id."""
+            self.sp = sp
+            self.pc = pc
+
+    def __init__ (self):
+        """Create the unwinder."""
+        Unwinder.__init__ (self, "dummy stack unwinder")
+        self.void_ptr_t = gdb.lookup_type("void").pointer()
+        self.regs = None
+
+    def get_regs (self, pending_frame):
+        """Return a list of register names that should be read.  Only
+        gathers the list once, then caches the result."""
+        if (self.regs != None):
+            return self.regs
+
+        # Collect the names of all registers to read.
+        self.regs = list (pending_frame.architecture ()
+                          .register_names ())
+
+        return self.regs
+
+    def __call__ (self, pending_frame):
+        """Actually performs the unwind, or at least sniffs this frame
+        to see if the unwinder should claim it, which is never does."""
+        try:
+            for r in (self.get_regs (pending_frame)):
+                v = pending_frame.read_register (r).cast (self.void_ptr_t)
+        except:
+            print ("Dummy unwinder, exception")
+            raise
+
+        return None
+
+# Register the ComRV stack unwinder.
+gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
+
+print ("Python script imported")
-- 
2.25.4

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Luis Machado]

On 6/17/20 2:38 PM, Andrew Burgess wrote:
> I observed that when making use of a Python frame unwinder, if the
> frame sniffing code accessed any registers within an inline frame then
> GDB would crash with this error:
> 
>    gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
> 
> The problem is that, when in the Python unwinder I write this:
> 
>    pending_frame.read_register ("register-name")
> 
> This is translated internally into a call to `value_of_register',
> which in turn becomes a call to `value_of_register_lazy'.
> 
> Usually this isn't a problem, `value_of_register_lazy' requires the
> next frame (more inner) to have a valid frame_id, which will be the
> case (if we're sniffing frame #1, then frame #0 will have had its
> frame-id figured out).
> 
> Unfortunately if frame #0 is inline within frame #1, then the frame-id
> for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
> frame #0 requires us to first compute the frame-id for frame #1.  As a
> result it is not same to call `value_of_register_lazy' before the
> frame-id of the current frame is known.
> 
> The solution I propose here is that `value_of_register' stops making a
> lazy register value (the step that requires the frame-id, and thus
> causes the problems), and instead calls `get_frame_register_value'.
> This avoids the need to calculate the frame-id and so avoids the
> problem.

I believe this situation is similar to the one I was investigating a 
while ago with the tailcall/inline frame breakage, and it is a bit messy.

The values returned from get_frame_register* functions (what you call 
now) and frame_unwind_register* functions (I believe this is what 
value_of_register_lazy ends up calling) may not be the same in all 
situations.

The difference comes from the fact that some targets have enough CFI 
information to compute a register's value according to DWARF rules 
(which frame_unwind_register* functions use), without requiring 
information from the previous frame.

Some other targets just assume "SAME_VALUE" and end up fetching the 
register from the previous frame, which requires a valid frame_id. In 
this last case, calling get_frame_register* will yield the same result, 
since it will fetch the data from the previous frame anyway.

I'd pay special attention to x86_64, which is one I noticed has enough 
CFI information in some cases, whereas AArch64 doesn't.

In summary, the functions are not equivalent and don't take the same 
paths all the time. The documentation is not too clear unfortunately, so 
there's room for falling into traps. :-)

An alternative solution would be to only call get_frame_register_value 
if the frame id is not yet computed, which is certain to drive us into 
an assertion in that case.

> 
> This bug has crept in because we allowed calls to the function
> `value_of_register_lazy' at a time when the frame-id of the frame
> passed to the function didn't have its frame-id calculated.  This is
> only ever a problem for inline frames.  To prevent bugs like this
> getting into GDB in the future I've added an assert to the function
> `value_of_register_lazy' to require the current frame have its id
> calculated.
> 
> If we are calling from outside of the frame sniffer then this will
> _always_ be true, so is not a problem.  If we are calling this
> function from within a frame sniffer then it will always trigger.
> 
> gdb/ChangeLog:
> 
> 	* findvar.c (value_of_register): Use get_frame_register_value
> 	rather than value_of_register_lazy.
> 	(value_of_register_lazy): Add new assert, and update comments.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/py-unwind-inline.c: New file.
> 	* gdb.python/py-unwind-inline.exp: New file.
> 	* gdb.python/py-unwind-inline.py: New file.
> ---
>   gdb/ChangeLog                                 |  6 ++
>   gdb/findvar.c                                 | 28 ++++++--
>   gdb/testsuite/ChangeLog                       |  6 ++
>   gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
>   gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
>   gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
>   6 files changed, 192 insertions(+), 5 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
>   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py
> 
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index c7cd31ce1a6..e5445b34930 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -263,16 +263,13 @@ struct value *
>   value_of_register (int regnum, struct frame_info *frame)
>   {
>     struct gdbarch *gdbarch = get_frame_arch (frame);
> -  struct value *reg_val;
>   
>     /* User registers lie completely outside of the range of normal
>        registers.  Catch them early so that the target never sees them.  */
>     if (regnum >= gdbarch_num_cooked_regs (gdbarch))
>       return value_of_user_reg (regnum, frame);
>   
> -  reg_val = value_of_register_lazy (frame, regnum);
> -  value_fetch_lazy (reg_val);
> -  return reg_val;
> +  return get_frame_register_value (frame, regnum);
>   }
>   
>   /* Return a `value' with the contents of (virtual or cooked) register
> @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
>   
>     next_frame = get_next_frame_sentinel_okay (frame);
>   
> -  /* We should have a valid next frame.  */
> +  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
> +     for the next frame will require that FRAME has a valid frame-id.
> +     Usually this is not a problem, however, if this function is ever
> +     called from a frame sniffer, the small window of time when not all
> +     frames have yet had their frame-id calculated, this function will
> +     trigger an assert within get_frame_id that a frame at a level > 0
> +     doesn't have a frame id.
> +
> +     Instead of waiting for an inline frame to reveal an invalid use of
> +     this function, we instead demand that FRAME must have had its frame-id
> +     calculated before we use this function.
> +
> +     The one time it is safe to call this function when FRAME does not yet
> +     have a frame id is when FRAME is at level 0, in this case the
> +     assertion in get_frame_id doesn't fire, and instead get_frame_id will
> +     correctly compute the frame id for us.  */
> +  gdb_assert (frame_relative_level (frame) == 0
> +	      || frame_id_computed_p (frame));
> +
> +  /* We should have a valid next frame too.  Given that FRAME is more
> +     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
> +     then this must always be true.  */
>     gdb_assert (frame_id_p (get_frame_id (next_frame)));
>   
>     reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> new file mode 100644
> index 00000000000..0cfe06dd273
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> @@ -0,0 +1,37 @@
> +/* Copyright 2019-2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +volatile int global_var;
> +
> +int  __attribute__ ((noinline))
> +bar ()
> +{
> +  return global_var;
> +}
> +
> +static inline int __attribute__ ((always_inline))
> +foo ()
> +{
> +  return bar ();
> +}
> +
> +int
> +main ()
> +{
> +  int ans;
> +  global_var = 0;
> +  ans = foo ();
> +  return ans;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> new file mode 100644
> index 00000000000..f7c65f6afc8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> @@ -0,0 +1,49 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This script tests GDB's handling of using a Python unwinder in the
> +# presence of inlined frames.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	  debug] } {
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +# The following tests require execution.
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +gdb_breakpoint "foo"
> +
> +gdb_test "source ${pyfile}" "Python script imported" \
> +        "import python scripts"
> +
> +gdb_continue_to_breakpoint "foo"
> +
> +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> +    "\\r\\n#0  foo \\(\\)"
> +    "\\r\\n#1  main \\(\\)"
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> new file mode 100644
> index 00000000000..7206a0b2830
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> @@ -0,0 +1,71 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# A dummy stack unwinder used for testing the Python unwinders when we
> +# have inline frames.  This unwinder will never claim any frames,
> +# instead, all it does it try to read all registers possible target
> +# registers as part of the frame sniffing process..
> +
> +import gdb
> +from gdb.unwinder import Unwinder
> +
> +apb_global = None
> +
> +class dummy_unwinder (Unwinder):
> +    """ A dummy unwinder that looks at a bunch of registers as part of
> +    the unwinding process."""
> +
> +    class frame_id (object):
> +        """ Basic frame id."""
> +
> +        def __init__ (self, sp, pc):
> +            """ Create the frame id."""
> +            self.sp = sp
> +            self.pc = pc
> +
> +    def __init__ (self):
> +        """Create the unwinder."""
> +        Unwinder.__init__ (self, "dummy stack unwinder")
> +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> +        self.regs = None
> +
> +    def get_regs (self, pending_frame):
> +        """Return a list of register names that should be read.  Only
> +        gathers the list once, then caches the result."""
> +        if (self.regs != None):
> +            return self.regs
> +
> +        # Collect the names of all registers to read.
> +        self.regs = list (pending_frame.architecture ()
> +                          .register_names ())
> +
> +        return self.regs
> +
> +    def __call__ (self, pending_frame):
> +        """Actually performs the unwind, or at least sniffs this frame
> +        to see if the unwinder should claim it, which is never does."""
> +        try:
> +            for r in (self.get_regs (pending_frame)):
> +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> +        except:
> +            print ("Dummy unwinder, exception")
> +            raise
> +
> +        return None
> +
> +# Register the ComRV stack unwinder.
> +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> +
> +print ("Python script imported")
> 

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

* Luis Machado <luis.machado@linaro.org> [2020-06-17 18:14:26 -0300]:

> On 6/17/20 2:38 PM, Andrew Burgess wrote:
> > I observed that when making use of a Python frame unwinder, if the
> > frame sniffing code accessed any registers within an inline frame then
> > GDB would crash with this error:
> > 
> >    gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
> > 
> > The problem is that, when in the Python unwinder I write this:
> > 
> >    pending_frame.read_register ("register-name")
> > 
> > This is translated internally into a call to `value_of_register',
> > which in turn becomes a call to `value_of_register_lazy'.
> > 
> > Usually this isn't a problem, `value_of_register_lazy' requires the
> > next frame (more inner) to have a valid frame_id, which will be the
> > case (if we're sniffing frame #1, then frame #0 will have had its
> > frame-id figured out).
> > 
> > Unfortunately if frame #0 is inline within frame #1, then the frame-id
> > for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
> > frame #0 requires us to first compute the frame-id for frame #1.  As a
> > result it is not same to call `value_of_register_lazy' before the
> > frame-id of the current frame is known.
> > 
> > The solution I propose here is that `value_of_register' stops making a
> > lazy register value (the step that requires the frame-id, and thus
> > causes the problems), and instead calls `get_frame_register_value'.
> > This avoids the need to calculate the frame-id and so avoids the
> > problem.
> 
> I believe this situation is similar to the one I was investigating a while
> ago with the tailcall/inline frame breakage, and it is a bit messy.
> 
> The values returned from get_frame_register* functions (what you call now)
> and frame_unwind_register* functions (I believe this is what
> value_of_register_lazy ends up calling) may not be the same in all
> situations.

I find that hard to believe given that:

  struct value *
  get_frame_register_value (struct frame_info *frame, int regnum)
  {
    return frame_unwind_register_value (frame->next, regnum);
  }

Previously, a call to value_of_register would result in:

* value_of_register - creates a lazy register value with a call to
  value_of_register_lazy, and then immediately resolves it with a call
  to value_fetch_lazy.

* value_fetch_lazy would call value_fetch_lazy_register, which would
  then call frame_unwind_register_value to unwind the register value.

Now we have:

* value_of_register calls get_frame_register_value.

* get_frame_register_value calls frame_unwind_register_value.

The two code paths have converged.

Additionally I don't see anywhere in value_fetch_lazy, or
value_fetch_lazy_register that GDB could muck around with the value
returned such that it might give us back a different value.

BUT, there is one difference.  Calling frame_unwind_register_value
might itself return a lazy value (for example a lazy memory value for
a register spilled to the stack), value_fetch_lazy ensures that these
are resolved.

This makes sense, after calling value_fetch_lazy I really should have
a non-lazy value.  However, I believe it should be fine if
value_of_register returns a lazy value.  That it didn't before does
worry me _slightly_, but until I see a good reason why it _must_
return a non-lazy value I'm inclined to take the position that
returning a lazy value is fine, and anyone who calls that function
should only be using the value API to get the value contents, and
anyone who does that will automatically resolve the lazy value at that
point.

> 
> The difference comes from the fact that some targets have enough CFI
> information to compute a register's value according to DWARF rules (which
> frame_unwind_register* functions use), without requiring information from
> the previous frame.
> 
> Some other targets just assume "SAME_VALUE" and end up fetching the register
> from the previous frame, which requires a valid frame_id. In this last case,
> calling get_frame_register* will yield the same result, since it will fetch
> the data from the previous frame anyway.
> 
> I'd pay special attention to x86_64, which is one I noticed has enough CFI
> information in some cases, whereas AArch64 doesn't.
> 
> In summary, the functions are not equivalent and don't take the same paths
> all the time. The documentation is not too clear unfortunately, so there's
> room for falling into traps. :-)

I'll try to track down the thread your talking about.  It sounds
... concerning ... that two APIs to read registers from the same frame
should return different values.  You kind of touched on _how_ that
might happen, but I don't feel I really understand _why_, so I'd
certainly be interested to learn more.


Thanks,
Andrew

> 
> An alternative solution would be to only call get_frame_register_value if
> the frame id is not yet computed, which is certain to drive us into an
> assertion in that case.
> 
> > 
> > This bug has crept in because we allowed calls to the function
> > `value_of_register_lazy' at a time when the frame-id of the frame
> > passed to the function didn't have its frame-id calculated.  This is
> > only ever a problem for inline frames.  To prevent bugs like this
> > getting into GDB in the future I've added an assert to the function
> > `value_of_register_lazy' to require the current frame have its id
> > calculated.
> > 
> > If we are calling from outside of the frame sniffer then this will
> > _always_ be true, so is not a problem.  If we are calling this
> > function from within a frame sniffer then it will always trigger.
> > 
> > gdb/ChangeLog:
> > 
> > 	* findvar.c (value_of_register): Use get_frame_register_value
> > 	rather than value_of_register_lazy.
> > 	(value_of_register_lazy): Add new assert, and update comments.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* gdb.python/py-unwind-inline.c: New file.
> > 	* gdb.python/py-unwind-inline.exp: New file.
> > 	* gdb.python/py-unwind-inline.py: New file.
> > ---
> >   gdb/ChangeLog                                 |  6 ++
> >   gdb/findvar.c                                 | 28 ++++++--
> >   gdb/testsuite/ChangeLog                       |  6 ++
> >   gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
> >   gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
> >   gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
> >   6 files changed, 192 insertions(+), 5 deletions(-)
> >   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
> >   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
> >   create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py
> > 
> > diff --git a/gdb/findvar.c b/gdb/findvar.c
> > index c7cd31ce1a6..e5445b34930 100644
> > --- a/gdb/findvar.c
> > +++ b/gdb/findvar.c
> > @@ -263,16 +263,13 @@ struct value *
> >   value_of_register (int regnum, struct frame_info *frame)
> >   {
> >     struct gdbarch *gdbarch = get_frame_arch (frame);
> > -  struct value *reg_val;
> >     /* User registers lie completely outside of the range of normal
> >        registers.  Catch them early so that the target never sees them.  */
> >     if (regnum >= gdbarch_num_cooked_regs (gdbarch))
> >       return value_of_user_reg (regnum, frame);
> > -  reg_val = value_of_register_lazy (frame, regnum);
> > -  value_fetch_lazy (reg_val);
> > -  return reg_val;
> > +  return get_frame_register_value (frame, regnum);
> >   }
> >   /* Return a `value' with the contents of (virtual or cooked) register
> > @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
> >     next_frame = get_next_frame_sentinel_okay (frame);
> > -  /* We should have a valid next frame.  */
> > +  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
> > +     for the next frame will require that FRAME has a valid frame-id.
> > +     Usually this is not a problem, however, if this function is ever
> > +     called from a frame sniffer, the small window of time when not all
> > +     frames have yet had their frame-id calculated, this function will
> > +     trigger an assert within get_frame_id that a frame at a level > 0
> > +     doesn't have a frame id.
> > +
> > +     Instead of waiting for an inline frame to reveal an invalid use of
> > +     this function, we instead demand that FRAME must have had its frame-id
> > +     calculated before we use this function.
> > +
> > +     The one time it is safe to call this function when FRAME does not yet
> > +     have a frame id is when FRAME is at level 0, in this case the
> > +     assertion in get_frame_id doesn't fire, and instead get_frame_id will
> > +     correctly compute the frame id for us.  */
> > +  gdb_assert (frame_relative_level (frame) == 0
> > +	      || frame_id_computed_p (frame));
> > +
> > +  /* We should have a valid next frame too.  Given that FRAME is more
> > +     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
> > +     then this must always be true.  */
> >     gdb_assert (frame_id_p (get_frame_id (next_frame)));
> >     reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > new file mode 100644
> > index 00000000000..0cfe06dd273
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > @@ -0,0 +1,37 @@
> > +/* Copyright 2019-2020 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > +
> > +volatile int global_var;
> > +
> > +int  __attribute__ ((noinline))
> > +bar ()
> > +{
> > +  return global_var;
> > +}
> > +
> > +static inline int __attribute__ ((always_inline))
> > +foo ()
> > +{
> > +  return bar ();
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  int ans;
> > +  global_var = 0;
> > +  ans = foo ();
> > +  return ans;
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > new file mode 100644
> > index 00000000000..f7c65f6afc8
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > @@ -0,0 +1,49 @@
> > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# This script tests GDB's handling of using a Python unwinder in the
> > +# presence of inlined frames.
> > +
> > +load_lib gdb-python.exp
> > +
> > +standard_testfile
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > +	  debug] } {
> > +    return -1
> > +}
> > +
> > +# Skip all tests if Python scripting is not enabled.
> > +if { [skip_python_tests] } { continue }
> > +
> > +# The following tests require execution.
> > +if ![runto_main] then {
> > +    fail "can't run to main"
> > +    return 0
> > +}
> > +
> > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> > +
> > +gdb_breakpoint "foo"
> > +
> > +gdb_test "source ${pyfile}" "Python script imported" \
> > +        "import python scripts"
> > +
> > +gdb_continue_to_breakpoint "foo"
> > +
> > +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> > +    "\\r\\n#0  foo \\(\\)"
> > +    "\\r\\n#1  main \\(\\)"
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > new file mode 100644
> > index 00000000000..7206a0b2830
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > @@ -0,0 +1,71 @@
> > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# A dummy stack unwinder used for testing the Python unwinders when we
> > +# have inline frames.  This unwinder will never claim any frames,
> > +# instead, all it does it try to read all registers possible target
> > +# registers as part of the frame sniffing process..
> > +
> > +import gdb
> > +from gdb.unwinder import Unwinder
> > +
> > +apb_global = None
> > +
> > +class dummy_unwinder (Unwinder):
> > +    """ A dummy unwinder that looks at a bunch of registers as part of
> > +    the unwinding process."""
> > +
> > +    class frame_id (object):
> > +        """ Basic frame id."""
> > +
> > +        def __init__ (self, sp, pc):
> > +            """ Create the frame id."""
> > +            self.sp = sp
> > +            self.pc = pc
> > +
> > +    def __init__ (self):
> > +        """Create the unwinder."""
> > +        Unwinder.__init__ (self, "dummy stack unwinder")
> > +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> > +        self.regs = None
> > +
> > +    def get_regs (self, pending_frame):
> > +        """Return a list of register names that should be read.  Only
> > +        gathers the list once, then caches the result."""
> > +        if (self.regs != None):
> > +            return self.regs
> > +
> > +        # Collect the names of all registers to read.
> > +        self.regs = list (pending_frame.architecture ()
> > +                          .register_names ())
> > +
> > +        return self.regs
> > +
> > +    def __call__ (self, pending_frame):
> > +        """Actually performs the unwind, or at least sniffs this frame
> > +        to see if the unwinder should claim it, which is never does."""
> > +        try:
> > +            for r in (self.get_regs (pending_frame)):
> > +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> > +        except:
> > +            print ("Dummy unwinder, exception")
> > +            raise
> > +
> > +        return None
> > +
> > +# Register the ComRV stack unwinder.
> > +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> > +
> > +print ("Python script imported")
> > 

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Luis Machado]

On 6/18/20 5:25 AM, Andrew Burgess wrote:
> * Luis Machado <luis.machado@linaro.org> [2020-06-17 18:14:26 -0300]:
> 
>> On 6/17/20 2:38 PM, Andrew Burgess wrote:
>>> I observed that when making use of a Python frame unwinder, if the
>>> frame sniffing code accessed any registers within an inline frame then
>>> GDB would crash with this error:
>>>
>>>     gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
>>>
>>> The problem is that, when in the Python unwinder I write this:
>>>
>>>     pending_frame.read_register ("register-name")
>>>
>>> This is translated internally into a call to `value_of_register',
>>> which in turn becomes a call to `value_of_register_lazy'.
>>>
>>> Usually this isn't a problem, `value_of_register_lazy' requires the
>>> next frame (more inner) to have a valid frame_id, which will be the
>>> case (if we're sniffing frame #1, then frame #0 will have had its
>>> frame-id figured out).
>>>
>>> Unfortunately if frame #0 is inline within frame #1, then the frame-id
>>> for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
>>> frame #0 requires us to first compute the frame-id for frame #1.  As a
>>> result it is not same to call `value_of_register_lazy' before the
>>> frame-id of the current frame is known.
>>>
>>> The solution I propose here is that `value_of_register' stops making a
>>> lazy register value (the step that requires the frame-id, and thus
>>> causes the problems), and instead calls `get_frame_register_value'.
>>> This avoids the need to calculate the frame-id and so avoids the
>>> problem.
>>
>> I believe this situation is similar to the one I was investigating a while
>> ago with the tailcall/inline frame breakage, and it is a bit messy.
>>
>> The values returned from get_frame_register* functions (what you call now)
>> and frame_unwind_register* functions (I believe this is what
>> value_of_register_lazy ends up calling) may not be the same in all
>> situations.
> 
> I find that hard to believe given that:
> 
>    struct value *
>    get_frame_register_value (struct frame_info *frame, int regnum)
>    {
>      return frame_unwind_register_value (frame->next, regnum);
>    }
> 
> Previously, a call to value_of_register would result in:
> 
> * value_of_register - creates a lazy register value with a call to
>    value_of_register_lazy, and then immediately resolves it with a call
>    to value_fetch_lazy.
> 
> * value_fetch_lazy would call value_fetch_lazy_register, which would
>    then call frame_unwind_register_value to unwind the register value.
> 
> Now we have:
> 
> * value_of_register calls get_frame_register_value.
> 
> * get_frame_register_value calls frame_unwind_register_value.

Looking at the code again, I agree that they do converge. The difference 
is mostly that get_frame_register* starts right off the bat by passing 
the next frame id, whereas value_of_register gets this from 
value_of_register_lazy. So you're dealing with unwinding from the next 
frame.

Take, for example, default_unwind_pc () (it calls frame_unwind_register* 
for the current frame), which is what I was dealing with at the time. It 
will try to unwind PC from the current frame first. Only then GDB will 
ask the next frame for PC. So there is a subtle difference compared to 
get_frame_register*, which calls frame_unwind_register for the next frame.

The regressions commit 5939967b355ba6a940887d19847b7893a4506067 created 
(defaulted to get_frame_register) for x86_64 were fixed by 
991a3e2e9944a4b3a27bd989ac03c18285bd545d (added the conditional block).

It may be the case that value_of_register has always fetched things from 
the next frame, without considering the current frame's idea of what the 
register value is. We may not be seeing failures because we are not 
exercising this situation.	

In any case, I just thought I'd mention this, as it is a bit of a trap 
when you're dealing with the unwinding code.

> 
> The two code paths have converged.
> 
> Additionally I don't see anywhere in value_fetch_lazy, or
> value_fetch_lazy_register that GDB could muck around with the value
> returned such that it might give us back a different value.
> 
> BUT, there is one difference.  Calling frame_unwind_register_value
> might itself return a lazy value (for example a lazy memory value for
> a register spilled to the stack), value_fetch_lazy ensures that these
> are resolved.
> 
> This makes sense, after calling value_fetch_lazy I really should have
> a non-lazy value.  However, I believe it should be fine if
> value_of_register returns a lazy value.  That it didn't before does
> worry me _slightly_, but until I see a good reason why it _must_
> return a non-lazy value I'm inclined to take the position that
> returning a lazy value is fine, and anyone who calls that function
> should only be using the value API to get the value contents, and
> anyone who does that will automatically resolve the lazy value at that
> point.
> 
>>
>> The difference comes from the fact that some targets have enough CFI
>> information to compute a register's value according to DWARF rules (which
>> frame_unwind_register* functions use), without requiring information from
>> the previous frame.
>>
>> Some other targets just assume "SAME_VALUE" and end up fetching the register
>> from the previous frame, which requires a valid frame_id. In this last case,
>> calling get_frame_register* will yield the same result, since it will fetch
>> the data from the previous frame anyway.
>>
>> I'd pay special attention to x86_64, which is one I noticed has enough CFI
>> information in some cases, whereas AArch64 doesn't.
>>
>> In summary, the functions are not equivalent and don't take the same paths
>> all the time. The documentation is not too clear unfortunately, so there's
>> room for falling into traps. :-)
> 
> I'll try to track down the thread your talking about.  It sounds
> ... concerning ... that two APIs to read registers from the same frame
> should return different values.  You kind of touched on _how_ that
> might happen, but I don't feel I really understand _why_, so I'd
> certainly be interested to learn more.
> 
> 
> Thanks,
> Andrew
> 
>>
>> An alternative solution would be to only call get_frame_register_value if
>> the frame id is not yet computed, which is certain to drive us into an
>> assertion in that case.
>>
>>>
>>> This bug has crept in because we allowed calls to the function
>>> `value_of_register_lazy' at a time when the frame-id of the frame
>>> passed to the function didn't have its frame-id calculated.  This is
>>> only ever a problem for inline frames.  To prevent bugs like this
>>> getting into GDB in the future I've added an assert to the function
>>> `value_of_register_lazy' to require the current frame have its id
>>> calculated.
>>>
>>> If we are calling from outside of the frame sniffer then this will
>>> _always_ be true, so is not a problem.  If we are calling this
>>> function from within a frame sniffer then it will always trigger.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* findvar.c (value_of_register): Use get_frame_register_value
>>> 	rather than value_of_register_lazy.
>>> 	(value_of_register_lazy): Add new assert, and update comments.
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> 	* gdb.python/py-unwind-inline.c: New file.
>>> 	* gdb.python/py-unwind-inline.exp: New file.
>>> 	* gdb.python/py-unwind-inline.py: New file.
>>> ---
>>>    gdb/ChangeLog                                 |  6 ++
>>>    gdb/findvar.c                                 | 28 ++++++--
>>>    gdb/testsuite/ChangeLog                       |  6 ++
>>>    gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
>>>    gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
>>>    gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
>>>    6 files changed, 192 insertions(+), 5 deletions(-)
>>>    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
>>>    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
>>>    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py
>>>
>>> diff --git a/gdb/findvar.c b/gdb/findvar.c
>>> index c7cd31ce1a6..e5445b34930 100644
>>> --- a/gdb/findvar.c
>>> +++ b/gdb/findvar.c
>>> @@ -263,16 +263,13 @@ struct value *
>>>    value_of_register (int regnum, struct frame_info *frame)
>>>    {
>>>      struct gdbarch *gdbarch = get_frame_arch (frame);
>>> -  struct value *reg_val;
>>>      /* User registers lie completely outside of the range of normal
>>>         registers.  Catch them early so that the target never sees them.  */
>>>      if (regnum >= gdbarch_num_cooked_regs (gdbarch))
>>>        return value_of_user_reg (regnum, frame);
>>> -  reg_val = value_of_register_lazy (frame, regnum);
>>> -  value_fetch_lazy (reg_val);
>>> -  return reg_val;
>>> +  return get_frame_register_value (frame, regnum);
>>>    }
>>>    /* Return a `value' with the contents of (virtual or cooked) register
>>> @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
>>>      next_frame = get_next_frame_sentinel_okay (frame);
>>> -  /* We should have a valid next frame.  */
>>> +  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
>>> +     for the next frame will require that FRAME has a valid frame-id.
>>> +     Usually this is not a problem, however, if this function is ever
>>> +     called from a frame sniffer, the small window of time when not all
>>> +     frames have yet had their frame-id calculated, this function will
>>> +     trigger an assert within get_frame_id that a frame at a level > 0
>>> +     doesn't have a frame id.
>>> +
>>> +     Instead of waiting for an inline frame to reveal an invalid use of
>>> +     this function, we instead demand that FRAME must have had its frame-id
>>> +     calculated before we use this function.
>>> +
>>> +     The one time it is safe to call this function when FRAME does not yet
>>> +     have a frame id is when FRAME is at level 0, in this case the
>>> +     assertion in get_frame_id doesn't fire, and instead get_frame_id will
>>> +     correctly compute the frame id for us.  */
>>> +  gdb_assert (frame_relative_level (frame) == 0
>>> +	      || frame_id_computed_p (frame));
>>> +
>>> +  /* We should have a valid next frame too.  Given that FRAME is more
>>> +     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
>>> +     then this must always be true.  */
>>>      gdb_assert (frame_id_p (get_frame_id (next_frame)));
>>>      reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
>>> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
>>> new file mode 100644
>>> index 00000000000..0cfe06dd273
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
>>> @@ -0,0 +1,37 @@
>>> +/* Copyright 2019-2020 Free Software Foundation, Inc.
>>> +
>>> +   This program is free software; you can redistribute it and/or modify
>>> +   it under the terms of the GNU General Public License as published by
>>> +   the Free Software Foundation; either version 3 of the License, or
>>> +   (at your option) any later version.
>>> +
>>> +   This program is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +   GNU General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU General Public License
>>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>> +
>>> +volatile int global_var;
>>> +
>>> +int  __attribute__ ((noinline))
>>> +bar ()
>>> +{
>>> +  return global_var;
>>> +}
>>> +
>>> +static inline int __attribute__ ((always_inline))
>>> +foo ()
>>> +{
>>> +  return bar ();
>>> +}
>>> +
>>> +int
>>> +main ()
>>> +{
>>> +  int ans;
>>> +  global_var = 0;
>>> +  ans = foo ();
>>> +  return ans;
>>> +}
>>> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
>>> new file mode 100644
>>> index 00000000000..f7c65f6afc8
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
>>> @@ -0,0 +1,49 @@
>>> +# Copyright (C) 2020 Free Software Foundation, Inc.
>>> +
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>> +# This script tests GDB's handling of using a Python unwinder in the
>>> +# presence of inlined frames.
>>> +
>>> +load_lib gdb-python.exp
>>> +
>>> +standard_testfile
>>> +
>>> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
>>> +	  debug] } {
>>> +    return -1
>>> +}
>>> +
>>> +# Skip all tests if Python scripting is not enabled.
>>> +if { [skip_python_tests] } { continue }
>>> +
>>> +# The following tests require execution.
>>> +if ![runto_main] then {
>>> +    fail "can't run to main"
>>> +    return 0
>>> +}
>>> +
>>> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
>>> +
>>> +gdb_breakpoint "foo"
>>> +
>>> +gdb_test "source ${pyfile}" "Python script imported" \
>>> +        "import python scripts"
>>> +
>>> +gdb_continue_to_breakpoint "foo"
>>> +
>>> +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
>>> +    "\\r\\n#0  foo \\(\\)"
>>> +    "\\r\\n#1  main \\(\\)"
>>> +}
>>> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
>>> new file mode 100644
>>> index 00000000000..7206a0b2830
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
>>> @@ -0,0 +1,71 @@
>>> +# Copyright (C) 2020 Free Software Foundation, Inc.
>>> +
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +
>>> +# A dummy stack unwinder used for testing the Python unwinders when we
>>> +# have inline frames.  This unwinder will never claim any frames,
>>> +# instead, all it does it try to read all registers possible target
>>> +# registers as part of the frame sniffing process..
>>> +
>>> +import gdb
>>> +from gdb.unwinder import Unwinder
>>> +
>>> +apb_global = None
>>> +
>>> +class dummy_unwinder (Unwinder):
>>> +    """ A dummy unwinder that looks at a bunch of registers as part of
>>> +    the unwinding process."""
>>> +
>>> +    class frame_id (object):
>>> +        """ Basic frame id."""
>>> +
>>> +        def __init__ (self, sp, pc):
>>> +            """ Create the frame id."""
>>> +            self.sp = sp
>>> +            self.pc = pc
>>> +
>>> +    def __init__ (self):
>>> +        """Create the unwinder."""
>>> +        Unwinder.__init__ (self, "dummy stack unwinder")
>>> +        self.void_ptr_t = gdb.lookup_type("void").pointer()
>>> +        self.regs = None
>>> +
>>> +    def get_regs (self, pending_frame):
>>> +        """Return a list of register names that should be read.  Only
>>> +        gathers the list once, then caches the result."""
>>> +        if (self.regs != None):
>>> +            return self.regs
>>> +
>>> +        # Collect the names of all registers to read.
>>> +        self.regs = list (pending_frame.architecture ()
>>> +                          .register_names ())
>>> +
>>> +        return self.regs
>>> +
>>> +    def __call__ (self, pending_frame):
>>> +        """Actually performs the unwind, or at least sniffs this frame
>>> +        to see if the unwinder should claim it, which is never does."""
>>> +        try:
>>> +            for r in (self.get_regs (pending_frame)):
>>> +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
>>> +        except:
>>> +            print ("Dummy unwinder, exception")
>>> +            raise
>>> +
>>> +        return None
>>> +
>>> +# Register the ComRV stack unwinder.
>>> +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
>>> +
>>> +print ("Python script imported")
>>>

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

* Luis Machado <luis.machado@linaro.org> [2020-06-18 07:29:53 -0300]:

> On 6/18/20 5:25 AM, Andrew Burgess wrote:
> > * Luis Machado <luis.machado@linaro.org> [2020-06-17 18:14:26 -0300]:
> > 
> > > On 6/17/20 2:38 PM, Andrew Burgess wrote:
> > > > I observed that when making use of a Python frame unwinder, if the
> > > > frame sniffing code accessed any registers within an inline frame then
> > > > GDB would crash with this error:
> > > > 
> > > >     gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
> > > > 
> > > > The problem is that, when in the Python unwinder I write this:
> > > > 
> > > >     pending_frame.read_register ("register-name")
> > > > 
> > > > This is translated internally into a call to `value_of_register',
> > > > which in turn becomes a call to `value_of_register_lazy'.
> > > > 
> > > > Usually this isn't a problem, `value_of_register_lazy' requires the
> > > > next frame (more inner) to have a valid frame_id, which will be the
> > > > case (if we're sniffing frame #1, then frame #0 will have had its
> > > > frame-id figured out).
> > > > 
> > > > Unfortunately if frame #0 is inline within frame #1, then the frame-id
> > > > for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for
> > > > frame #0 requires us to first compute the frame-id for frame #1.  As a
> > > > result it is not same to call `value_of_register_lazy' before the
> > > > frame-id of the current frame is known.
> > > > 
> > > > The solution I propose here is that `value_of_register' stops making a
> > > > lazy register value (the step that requires the frame-id, and thus
> > > > causes the problems), and instead calls `get_frame_register_value'.
> > > > This avoids the need to calculate the frame-id and so avoids the
> > > > problem.
> > > 
> > > I believe this situation is similar to the one I was investigating a while
> > > ago with the tailcall/inline frame breakage, and it is a bit messy.
> > > 
> > > The values returned from get_frame_register* functions (what you call now)
> > > and frame_unwind_register* functions (I believe this is what
> > > value_of_register_lazy ends up calling) may not be the same in all
> > > situations.
> > 
> > I find that hard to believe given that:
> > 
> >    struct value *
> >    get_frame_register_value (struct frame_info *frame, int regnum)
> >    {
> >      return frame_unwind_register_value (frame->next, regnum);
> >    }
> > 
> > Previously, a call to value_of_register would result in:
> > 
> > * value_of_register - creates a lazy register value with a call to
> >    value_of_register_lazy, and then immediately resolves it with a call
> >    to value_fetch_lazy.
> > 
> > * value_fetch_lazy would call value_fetch_lazy_register, which would
> >    then call frame_unwind_register_value to unwind the register value.
> > 
> > Now we have:
> > 
> > * value_of_register calls get_frame_register_value.
> > 
> > * get_frame_register_value calls frame_unwind_register_value.
> 
> Looking at the code again, I agree that they do converge. The difference is
> mostly that get_frame_register* starts right off the bat by passing the next
> frame id, whereas value_of_register gets this from value_of_register_lazy.
> So you're dealing with unwinding from the next frame.
> 
> Take, for example, default_unwind_pc () (it calls frame_unwind_register* for
> the current frame), which is what I was dealing with at the time. It will
> try to unwind PC from the current frame first. Only then GDB will ask the
> next frame for PC. So there is a subtle difference compared to
> get_frame_register*, which calls frame_unwind_register for the next frame.
> 
> The regressions commit 5939967b355ba6a940887d19847b7893a4506067 created
> (defaulted to get_frame_register) for x86_64 were fixed by
> 991a3e2e9944a4b3a27bd989ac03c18285bd545d (added the conditional block).
> 
> It may be the case that value_of_register has always fetched things from the
> next frame, without considering the current frame's idea of what the
> register value is. We may not be seeing failures because we are not
> exercising this situation.

Just in case you'd not spotted I followed up to the thread for your
original patch 5939967b355ba6a940887d19847b7893a4506067.

Thanks,
Andrew


> 
> In any case, I just thought I'd mention this, as it is a bit of a trap when
> you're dealing with the unwinding code.
> 
> > 
> > The two code paths have converged.
> > 
> > Additionally I don't see anywhere in value_fetch_lazy, or
> > value_fetch_lazy_register that GDB could muck around with the value
> > returned such that it might give us back a different value.
> > 
> > BUT, there is one difference.  Calling frame_unwind_register_value
> > might itself return a lazy value (for example a lazy memory value for
> > a register spilled to the stack), value_fetch_lazy ensures that these
> > are resolved.
> > 
> > This makes sense, after calling value_fetch_lazy I really should have
> > a non-lazy value.  However, I believe it should be fine if
> > value_of_register returns a lazy value.  That it didn't before does
> > worry me _slightly_, but until I see a good reason why it _must_
> > return a non-lazy value I'm inclined to take the position that
> > returning a lazy value is fine, and anyone who calls that function
> > should only be using the value API to get the value contents, and
> > anyone who does that will automatically resolve the lazy value at that
> > point.
> > 
> > > 
> > > The difference comes from the fact that some targets have enough CFI
> > > information to compute a register's value according to DWARF rules (which
> > > frame_unwind_register* functions use), without requiring information from
> > > the previous frame.
> > > 
> > > Some other targets just assume "SAME_VALUE" and end up fetching the register
> > > from the previous frame, which requires a valid frame_id. In this last case,
> > > calling get_frame_register* will yield the same result, since it will fetch
> > > the data from the previous frame anyway.
> > > 
> > > I'd pay special attention to x86_64, which is one I noticed has enough CFI
> > > information in some cases, whereas AArch64 doesn't.
> > > 
> > > In summary, the functions are not equivalent and don't take the same paths
> > > all the time. The documentation is not too clear unfortunately, so there's
> > > room for falling into traps. :-)
> > 
> > I'll try to track down the thread your talking about.  It sounds
> > ... concerning ... that two APIs to read registers from the same frame
> > should return different values.  You kind of touched on _how_ that
> > might happen, but I don't feel I really understand _why_, so I'd
> > certainly be interested to learn more.
> > 
> > 
> > Thanks,
> > Andrew
> > 
> > > 
> > > An alternative solution would be to only call get_frame_register_value if
> > > the frame id is not yet computed, which is certain to drive us into an
> > > assertion in that case.
> > > 
> > > > 
> > > > This bug has crept in because we allowed calls to the function
> > > > `value_of_register_lazy' at a time when the frame-id of the frame
> > > > passed to the function didn't have its frame-id calculated.  This is
> > > > only ever a problem for inline frames.  To prevent bugs like this
> > > > getting into GDB in the future I've added an assert to the function
> > > > `value_of_register_lazy' to require the current frame have its id
> > > > calculated.
> > > > 
> > > > If we are calling from outside of the frame sniffer then this will
> > > > _always_ be true, so is not a problem.  If we are calling this
> > > > function from within a frame sniffer then it will always trigger.
> > > > 
> > > > gdb/ChangeLog:
> > > > 
> > > > 	* findvar.c (value_of_register): Use get_frame_register_value
> > > > 	rather than value_of_register_lazy.
> > > > 	(value_of_register_lazy): Add new assert, and update comments.
> > > > 
> > > > gdb/testsuite/ChangeLog:
> > > > 
> > > > 	* gdb.python/py-unwind-inline.c: New file.
> > > > 	* gdb.python/py-unwind-inline.exp: New file.
> > > > 	* gdb.python/py-unwind-inline.py: New file.
> > > > ---
> > > >    gdb/ChangeLog                                 |  6 ++
> > > >    gdb/findvar.c                                 | 28 ++++++--
> > > >    gdb/testsuite/ChangeLog                       |  6 ++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
> > > >    6 files changed, 192 insertions(+), 5 deletions(-)
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py
> > > > 
> > > > diff --git a/gdb/findvar.c b/gdb/findvar.c
> > > > index c7cd31ce1a6..e5445b34930 100644
> > > > --- a/gdb/findvar.c
> > > > +++ b/gdb/findvar.c
> > > > @@ -263,16 +263,13 @@ struct value *
> > > >    value_of_register (int regnum, struct frame_info *frame)
> > > >    {
> > > >      struct gdbarch *gdbarch = get_frame_arch (frame);
> > > > -  struct value *reg_val;
> > > >      /* User registers lie completely outside of the range of normal
> > > >         registers.  Catch them early so that the target never sees them.  */
> > > >      if (regnum >= gdbarch_num_cooked_regs (gdbarch))
> > > >        return value_of_user_reg (regnum, frame);
> > > > -  reg_val = value_of_register_lazy (frame, regnum);
> > > > -  value_fetch_lazy (reg_val);
> > > > -  return reg_val;
> > > > +  return get_frame_register_value (frame, regnum);
> > > >    }
> > > >    /* Return a `value' with the contents of (virtual or cooked) register
> > > > @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
> > > >      next_frame = get_next_frame_sentinel_okay (frame);
> > > > -  /* We should have a valid next frame.  */
> > > > +  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
> > > > +     for the next frame will require that FRAME has a valid frame-id.
> > > > +     Usually this is not a problem, however, if this function is ever
> > > > +     called from a frame sniffer, the small window of time when not all
> > > > +     frames have yet had their frame-id calculated, this function will
> > > > +     trigger an assert within get_frame_id that a frame at a level > 0
> > > > +     doesn't have a frame id.
> > > > +
> > > > +     Instead of waiting for an inline frame to reveal an invalid use of
> > > > +     this function, we instead demand that FRAME must have had its frame-id
> > > > +     calculated before we use this function.
> > > > +
> > > > +     The one time it is safe to call this function when FRAME does not yet
> > > > +     have a frame id is when FRAME is at level 0, in this case the
> > > > +     assertion in get_frame_id doesn't fire, and instead get_frame_id will
> > > > +     correctly compute the frame id for us.  */
> > > > +  gdb_assert (frame_relative_level (frame) == 0
> > > > +	      || frame_id_computed_p (frame));
> > > > +
> > > > +  /* We should have a valid next frame too.  Given that FRAME is more
> > > > +     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
> > > > +     then this must always be true.  */
> > > >      gdb_assert (frame_id_p (get_frame_id (next_frame)));
> > > >      reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
> > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > > > new file mode 100644
> > > > index 00000000000..0cfe06dd273
> > > > --- /dev/null
> > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > > > @@ -0,0 +1,37 @@
> > > > +/* Copyright 2019-2020 Free Software Foundation, Inc.
> > > > +
> > > > +   This program is free software; you can redistribute it and/or modify
> > > > +   it under the terms of the GNU General Public License as published by
> > > > +   the Free Software Foundation; either version 3 of the License, or
> > > > +   (at your option) any later version.
> > > > +
> > > > +   This program is distributed in the hope that it will be useful,
> > > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +   GNU General Public License for more details.
> > > > +
> > > > +   You should have received a copy of the GNU General Public License
> > > > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > > > +
> > > > +volatile int global_var;
> > > > +
> > > > +int  __attribute__ ((noinline))
> > > > +bar ()
> > > > +{
> > > > +  return global_var;
> > > > +}
> > > > +
> > > > +static inline int __attribute__ ((always_inline))
> > > > +foo ()
> > > > +{
> > > > +  return bar ();
> > > > +}
> > > > +
> > > > +int
> > > > +main ()
> > > > +{
> > > > +  int ans;
> > > > +  global_var = 0;
> > > > +  ans = foo ();
> > > > +  return ans;
> > > > +}
> > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > > > new file mode 100644
> > > > index 00000000000..f7c65f6afc8
> > > > --- /dev/null
> > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > > > @@ -0,0 +1,49 @@
> > > > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > > > +
> > > > +# This program is free software; you can redistribute it and/or modify
> > > > +# it under the terms of the GNU General Public License as published by
> > > > +# the Free Software Foundation; either version 3 of the License, or
> > > > +# (at your option) any later version.
> > > > +#
> > > > +# This program is distributed in the hope that it will be useful,
> > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +# GNU General Public License for more details.
> > > > +#
> > > > +# You should have received a copy of the GNU General Public License
> > > > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > > +
> > > > +# This script tests GDB's handling of using a Python unwinder in the
> > > > +# presence of inlined frames.
> > > > +
> > > > +load_lib gdb-python.exp
> > > > +
> > > > +standard_testfile
> > > > +
> > > > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > > > +	  debug] } {
> > > > +    return -1
> > > > +}
> > > > +
> > > > +# Skip all tests if Python scripting is not enabled.
> > > > +if { [skip_python_tests] } { continue }
> > > > +
> > > > +# The following tests require execution.
> > > > +if ![runto_main] then {
> > > > +    fail "can't run to main"
> > > > +    return 0
> > > > +}
> > > > +
> > > > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> > > > +
> > > > +gdb_breakpoint "foo"
> > > > +
> > > > +gdb_test "source ${pyfile}" "Python script imported" \
> > > > +        "import python scripts"
> > > > +
> > > > +gdb_continue_to_breakpoint "foo"
> > > > +
> > > > +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> > > > +    "\\r\\n#0  foo \\(\\)"
> > > > +    "\\r\\n#1  main \\(\\)"
> > > > +}
> > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > > > new file mode 100644
> > > > index 00000000000..7206a0b2830
> > > > --- /dev/null
> > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > > > @@ -0,0 +1,71 @@
> > > > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > > > +
> > > > +# This program is free software; you can redistribute it and/or modify
> > > > +# it under the terms of the GNU General Public License as published by
> > > > +# the Free Software Foundation; either version 3 of the License, or
> > > > +# (at your option) any later version.
> > > > +#
> > > > +# This program is distributed in the hope that it will be useful,
> > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +# GNU General Public License for more details.
> > > > +#
> > > > +# You should have received a copy of the GNU General Public License
> > > > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > > +
> > > > +# A dummy stack unwinder used for testing the Python unwinders when we
> > > > +# have inline frames.  This unwinder will never claim any frames,
> > > > +# instead, all it does it try to read all registers possible target
> > > > +# registers as part of the frame sniffing process..
> > > > +
> > > > +import gdb
> > > > +from gdb.unwinder import Unwinder
> > > > +
> > > > +apb_global = None
> > > > +
> > > > +class dummy_unwinder (Unwinder):
> > > > +    """ A dummy unwinder that looks at a bunch of registers as part of
> > > > +    the unwinding process."""
> > > > +
> > > > +    class frame_id (object):
> > > > +        """ Basic frame id."""
> > > > +
> > > > +        def __init__ (self, sp, pc):
> > > > +            """ Create the frame id."""
> > > > +            self.sp = sp
> > > > +            self.pc = pc
> > > > +
> > > > +    def __init__ (self):
> > > > +        """Create the unwinder."""
> > > > +        Unwinder.__init__ (self, "dummy stack unwinder")
> > > > +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> > > > +        self.regs = None
> > > > +
> > > > +    def get_regs (self, pending_frame):
> > > > +        """Return a list of register names that should be read.  Only
> > > > +        gathers the list once, then caches the result."""
> > > > +        if (self.regs != None):
> > > > +            return self.regs
> > > > +
> > > > +        # Collect the names of all registers to read.
> > > > +        self.regs = list (pending_frame.architecture ()
> > > > +                          .register_names ())
> > > > +
> > > > +        return self.regs
> > > > +
> > > > +    def __call__ (self, pending_frame):
> > > > +        """Actually performs the unwind, or at least sniffs this frame
> > > > +        to see if the unwinder should claim it, which is never does."""
> > > > +        try:
> > > > +            for r in (self.get_regs (pending_frame)):
> > > > +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> > > > +        except:
> > > > +            print ("Dummy unwinder, exception")
> > > > +            raise
> > > > +
> > > > +        return None
> > > > +
> > > > +# Register the ComRV stack unwinder.
> > > > +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> > > > +
> > > > +print ("Python script imported")
> > > > 

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* findvar.c (value_of_register): Use get_frame_register_value
Andrew> 	rather than value_of_register_lazy.
Andrew> 	(value_of_register_lazy): Add new assert, and update comments.

I suspect this is PR python/22748.

I can't really comment on the correctness of the change.
This isn't an area I know well.

Tom

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

After feedback from Luis, and then discussion here:

  https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html

this is a completely new version of this patch that addresses the
original issue with Python unwinders, as well as addressing the issues
brought up in the discussion above.

Thanks,
Andrew

---

commit c66679e65c2f247a75d84a0166b07fed352879e1
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Mon Jun 8 11:36:13 2020 +0100

    gdb: Python unwinders, inline frames, and tail-call frames
    
    This started with me running into the bug described in python/22748,
    in summary, if the frame sniffing code accessed any registers within
    an inline frame then GDB would crash with this error:
    
      gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
    
    The problem is that, when in the Python unwinder I write this:
    
      pending_frame.read_register ("register-name")
    
    This is translated internally into a call to `value_of_register',
    which in turn becomes a call to `value_of_register_lazy'.
    
    Usually this isn't a problem, `value_of_register_lazy' requires the
    next frame (more inner) to have a valid frame_id, which will be the
    case (if we're sniffing frame #1, then frame #0 will have had its
    frame-id figured out).
    
    Unfortunately if frame #0 is inline within frame #1, then the frame-id
    for frame #0 can't be computed until we have the frame-id for #1.  As
    a result we can't create a lazy register for frame #1 when frame #0 is
    inline.
    
    Initially I proposed a solution inline with that proposed in bugzilla,
    changing value_of_register to avoid creating a lazy register value.
    However, when this was discussed on the mailing list I got this reply:
    
      https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html
    
    Which led me to look at these two patches:
    
      [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html
      [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html
    
    When I considered patches [1] and [2] I saw that all of the issues
    being addressed here were related, and that there was a single
    solution that could address all of these issues.
    
    First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which
    shows that [1] and [2] regress the inline tail-call unwinder, the
    reason for this is that these two patches replace a call to
    gdbarch_unwind_pc with a call to get_frame_register, however, this is
    not correct.  The previous call to gdbarch_unwind_pc takes THIS_FRAME
    and returns the $pc value in the previous frame.  In contrast
    get_frame_register takes THIS_FRAME and returns the value of the $pc
    in THIS_FRAME; these calls are not equivalent.
    
    The reason these patches appear (or do) fix the regressions listed in
    [1] is that the tail call sniffer depends on identifying the address
    of a caller and a callee, GDB then looks for a tail-call sequence that
    takes us from the caller address to the callee, if such a series is
    found then tail-call frames are added.
    
    The bug that was being hit, and which was address in patch [1] is that
    in order to find the address of the caller, GDB ended up creating a
    lazy register value for an inline frame with to frame-id.  The
    solution in patch [1] is to instead take the address of the callee and
    treat this as the address of the caller.  Getting the address of the
    callee works, but we then end up looking for a tail-call series from
    the callee to the callee, which obviously doesn't return any sane
    results, so we don't insert any tail call frames.
    
    The original patch [1] did cause some breakage, so patch [2] undid
    patch [1] in all cases except those where we had an inline frame with
    no frame-id.  It just so happens that there were no tests that fitted
    this description _and_ which required tail-call frames to be
    successfully spotted, as a result patch [2] appeared to work.
    
    The new test inline-frame-tailcall.exp, exposes the flaw in patch [2].
    
    This commit undoes patch [1] and [2], and replaces them with a new
    solution, which is also different to the solution proposed in the
    python/22748 bug report.
    
    In this solution I propose that we introduce some special case logic
    to value_of_register_lazy.  To understand what this logic is we must
    first look at how inline frames unwind registers, this is very simple,
    they do this:
    
      static struct value *
      inline_frame_prev_register (struct frame_info *this_frame,
                                  void **this_cache, int regnum)
      {
        return get_frame_register_value (this_frame, regnum);
      }
    
    And remember:
    
      struct value *
      get_frame_register_value (struct frame_info *frame, int regnum)
      {
        return frame_unwind_register_value (frame->next, regnum);
      }
    
    So in all cases, unwinding a register in an inline frame just asks the
    next frame to unwind the register, this makes sense, as an inline
    frame doesn't really exist, when we unwind a register in an inline
    frame, we're really just asking the next frame for the value of the
    register in the previous, non-inline frame.
    
    So, if we assume that we only get into the missing frame-id situation
    when we try to unwind a register from an inline frame during the frame
    sniffing process, then we can change value_of_register_lazy to not
    create lazy register values for an inline frame.
    
    Imagine this stack setup, where #1 is inline within #2.
    
      #3 -> #2 -> #1 -> #0
            \______/
             inline
    
    Now when trying to figure out the frame-id for #1, we need to compute
    the frame-id for #2.  If the frame sniffer for #2 causes a lazy
    register read in #2, either due to a Python Unwinder, or for the
    tail-call sniffer, then we call value_of_register_lazy passing in
    frame #2.
    
    In value_of_register_lazy, we grab the next frame, which is #1, and we
    used to then ask for the frame-id of #1, which was not computed, and
    this was our bug.
    
    Now, I propose we spot that #1 is an inline frame, and so lookup the
    next frame of #1, which is #0.  As #0 is not inline it will have a
    valid frame-id, and so we create a lazy register value using #0 as the
    next-frame-id.  This will give us the exact same result we had
    previously (thanks to the code we inspected above).
    
    Encoding into value_of_register_lazy the knowledge that reading an
    inline frame register will always just forward to the next frame
    feels.... not ideal, but this seems like the cleanest solution to this
    recursive frame-id computation/sniffing issue that appears to crop
    up.
    
    The following two commits are fully reverted with this commit, these
    correspond to patches [1] and [2] respectively:
    
      commit 5939967b355ba6a940887d19847b7893a4506067
      Date:   Tue Apr 14 17:26:22 2020 -0300
    
          Fix inline frame unwinding breakage
    
      commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
      Date:   Sat Apr 25 00:32:44 2020 -0300
    
          Fix remaining inline/tailcall unwinding breakage for x86_64
    
    gdb/ChangeLog:
    
            PR python/22748
            * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove
            special handling for inline frames.
            * findvar.c (value_of_register_lazy): Skip inline frames when
            creating lazy register values.
            * frame.c (frame_id_computed_p): Delete definition.
            * frame.h (frame_id_computed_p): Delete declaration.
    
    gdb/testsuite/ChangeLog:
    
            PR python/22748
            * gdb.opt/inline-frame-tailcall.c: New file.
            * gdb.opt/inline-frame-tailcall.exp: New file.
            * gdb.python/py-unwind-inline.c: New file.
            * gdb.python/py-unwind-inline.exp: New file.
            * gdb.python/py-unwind-inline.py: New file.

diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
index 16dba2b201a..2d219f13f9d 100644
--- a/gdb/dwarf2/frame-tailcall.c
+++ b/gdb/dwarf2/frame-tailcall.c
@@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
 
       prev_gdbarch = frame_unwind_arch (this_frame);
 
-      /* The dwarf2 tailcall sniffer runs early, at the end of populating the
-	 dwarf2 frame cache for the current frame.  If there exists inline
-	 frames inner (next) to the current frame, there is a good possibility
-	 of that inline frame not having a computed frame id yet.
-
-	 This is because computing such a frame id requires us to walk through
-	 the frame chain until we find the first normal frame after the inline
-	 frame and then compute the normal frame's id first.
-
-	 Some architectures' compilers generate enough register location
-	 information for a dwarf unwinder to fetch PC without relying on inner
-	 frames (x86_64 for example).  In this case the PC is retrieved
-	 according to dwarf rules.
-
-	 But others generate less strict dwarf data for which assumptions are
-	 made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as
-	 DWARF2_FRAME_REG_SAME_VALUE).  For such cases, GDB may attempt to
-	 create lazy values for registers, and those lazy values must be
-	 created with a valid frame id, but we potentially have no valid id.
-
-	 So, to avoid breakage, if we see a dangerous situation with inline
-	 frames without a computed id, use safer functions to retrieve the
-	 current frame's PC.  Otherwise use the provided dwarf rules.  */
-      frame_info *next_frame = get_next_frame (this_frame);
-
       /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
-      if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME
-	  && !frame_id_computed_p (next_frame))
-	{
-	  /* The next frame is an inline frame and its frame id has not been
-	     computed yet.  */
-	  get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
-			      (gdb_byte *) &prev_pc);
-	  prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
-	}
-      else
-	prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
+      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
 
       /* call_site_find_chain can throw an exception.  */
       chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index c7cd31ce1a6..7e9dab567f6 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 
   next_frame = get_next_frame_sentinel_okay (frame);
 
+  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
+     happen if we end up trying to unwind a register as part of the frame
+     sniffer.  The only time that we get here without a valid frame-id is
+     if NEXT_FRAME is an inline frame.  If this is the case then we can
+     avoid getting into trouble here by skipping past the inline frames.  */
+  while (get_frame_type (next_frame) == INLINE_FRAME)
+    next_frame = get_next_frame_sentinel_okay (next_frame);
+
   /* We should have a valid next frame.  */
   gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
diff --git a/gdb/frame.c b/gdb/frame.c
index ff27b9f00e9..ac1016b083f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr)
   return id;
 }
 
-bool
-frame_id_computed_p (struct frame_info *frame)
-{
-  gdb_assert (frame != nullptr);
-
-  return frame->this_id.p != 0;
-}
-
 int
 frame_id_p (struct frame_id l)
 {
diff --git a/gdb/frame.h b/gdb/frame.h
index e835d49f9ca..cfc15022ed5 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -236,10 +236,6 @@ extern struct frame_id
    as the special identifier address are set to indicate wild cards.  */
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
-/* Returns true if FRAME's id has been computed.
-   Returns false otherwise.  */
-extern bool frame_id_computed_p (struct frame_info *frame);
-
 /* Returns non-zero when L is a valid frame (a valid frame has a
    non-zero .base).  The outermost frame is valid even without an
    ID.  */
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
new file mode 100644
index 00000000000..0cfe06dd273
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
@@ -0,0 +1,37 @@
+/* Copyright 2019-2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+volatile int global_var;
+
+int  __attribute__ ((noinline))
+bar ()
+{
+  return global_var;
+}
+
+static inline int __attribute__ ((always_inline))
+foo ()
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  int ans;
+  global_var = 0;
+  ans = foo ();
+  return ans;
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
new file mode 100644
index 00000000000..f7c65f6afc8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
@@ -0,0 +1,49 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This script tests GDB's handling of using a Python unwinder in the
+# presence of inlined frames.
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  debug] } {
+    return -1
+}
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+# The following tests require execution.
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+
+gdb_breakpoint "foo"
+
+gdb_test "source ${pyfile}" "Python script imported" \
+        "import python scripts"
+
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
+    "\\r\\n#0  foo \\(\\)"
+    "\\r\\n#1  main \\(\\)"
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
new file mode 100644
index 00000000000..7206a0b2830
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
@@ -0,0 +1,71 @@
+# Copyright (C) 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# A dummy stack unwinder used for testing the Python unwinders when we
+# have inline frames.  This unwinder will never claim any frames,
+# instead, all it does it try to read all registers possible target
+# registers as part of the frame sniffing process..
+
+import gdb
+from gdb.unwinder import Unwinder
+
+apb_global = None
+
+class dummy_unwinder (Unwinder):
+    """ A dummy unwinder that looks at a bunch of registers as part of
+    the unwinding process."""
+
+    class frame_id (object):
+        """ Basic frame id."""
+
+        def __init__ (self, sp, pc):
+            """ Create the frame id."""
+            self.sp = sp
+            self.pc = pc
+
+    def __init__ (self):
+        """Create the unwinder."""
+        Unwinder.__init__ (self, "dummy stack unwinder")
+        self.void_ptr_t = gdb.lookup_type("void").pointer()
+        self.regs = None
+
+    def get_regs (self, pending_frame):
+        """Return a list of register names that should be read.  Only
+        gathers the list once, then caches the result."""
+        if (self.regs != None):
+            return self.regs
+
+        # Collect the names of all registers to read.
+        self.regs = list (pending_frame.architecture ()
+                          .register_names ())
+
+        return self.regs
+
+    def __call__ (self, pending_frame):
+        """Actually performs the unwind, or at least sniffs this frame
+        to see if the unwinder should claim it, which is never does."""
+        try:
+            for r in (self.get_regs (pending_frame)):
+                v = pending_frame.read_register (r).cast (self.void_ptr_t)
+        except:
+            print ("Dummy unwinder, exception")
+            raise
+
+        return None
+
+# Register the ComRV stack unwinder.
+gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
+
+print ("Python script imported")

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Luis Machado]

Hi Andrew,

On 6/22/20 12:47 PM, Andrew Burgess wrote:
> After feedback from Luis, and then discussion here:
> 
>    https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html
> 
> this is a completely new version of this patch that addresses the
> original issue with Python unwinders, as well as addressing the issues
> brought up in the discussion above.
> 
> Thanks,
> Andrew

Thanks for the very detailed analysis. The situation is clearer now.

I gave this a try on aarch64-linux and it looks good. I think 
special-casing things for inline frames is a good compromise, though it 
looks slightly out of place on value_of-register_lazy.

Thinking about it further, I was wondering if there were other cases of 
attempting to manipulate things on frames without having a frame id set 
yet. I suppose the inline frames will always have to ask a non-inline 
frame for register information, so we may be safe for most (if not all) 
cases.

> 
> ---
> 
> commit c66679e65c2f247a75d84a0166b07fed352879e1
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Mon Jun 8 11:36:13 2020 +0100
> 
>      gdb: Python unwinders, inline frames, and tail-call frames
>      
>      This started with me running into the bug described in python/22748,
>      in summary, if the frame sniffing code accessed any registers within
>      an inline frame then GDB would crash with this error:
>      
>        gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
>      
>      The problem is that, when in the Python unwinder I write this:
>      
>        pending_frame.read_register ("register-name")
>      
>      This is translated internally into a call to `value_of_register',
>      which in turn becomes a call to `value_of_register_lazy'.
>      
>      Usually this isn't a problem, `value_of_register_lazy' requires the
>      next frame (more inner) to have a valid frame_id, which will be the
>      case (if we're sniffing frame #1, then frame #0 will have had its
>      frame-id figured out).
>      
>      Unfortunately if frame #0 is inline within frame #1, then the frame-id
>      for frame #0 can't be computed until we have the frame-id for #1.  As
>      a result we can't create a lazy register for frame #1 when frame #0 is
>      inline.
>      
>      Initially I proposed a solution inline with that proposed in bugzilla,
>      changing value_of_register to avoid creating a lazy register value.
>      However, when this was discussed on the mailing list I got this reply:
>      
>        https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html
>      
>      Which led me to look at these two patches:
>      
>        [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html
>        [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html
>      
>      When I considered patches [1] and [2] I saw that all of the issues
>      being addressed here were related, and that there was a single
>      solution that could address all of these issues.
>      
>      First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which
>      shows that [1] and [2] regress the inline tail-call unwinder, the
>      reason for this is that these two patches replace a call to
>      gdbarch_unwind_pc with a call to get_frame_register, however, this is
>      not correct.  The previous call to gdbarch_unwind_pc takes THIS_FRAME
>      and returns the $pc value in the previous frame.  In contrast
>      get_frame_register takes THIS_FRAME and returns the value of the $pc
>      in THIS_FRAME; these calls are not equivalent.
>      
>      The reason these patches appear (or do) fix the regressions listed in
>      [1] is that the tail call sniffer depends on identifying the address
>      of a caller and a callee, GDB then looks for a tail-call sequence that
>      takes us from the caller address to the callee, if such a series is
>      found then tail-call frames are added.
>      
>      The bug that was being hit, and which was address in patch [1] is that
>      in order to find the address of the caller, GDB ended up creating a
>      lazy register value for an inline frame with to frame-id.  The
>      solution in patch [1] is to instead take the address of the callee and
>      treat this as the address of the caller.  Getting the address of the
>      callee works, but we then end up looking for a tail-call series from
>      the callee to the callee, which obviously doesn't return any sane
>      results, so we don't insert any tail call frames.
>      
>      The original patch [1] did cause some breakage, so patch [2] undid
>      patch [1] in all cases except those where we had an inline frame with
>      no frame-id.  It just so happens that there were no tests that fitted
>      this description _and_ which required tail-call frames to be
>      successfully spotted, as a result patch [2] appeared to work.
>      
>      The new test inline-frame-tailcall.exp, exposes the flaw in patch [2].
>      
>      This commit undoes patch [1] and [2], and replaces them with a new
>      solution, which is also different to the solution proposed in the
>      python/22748 bug report.
>      
>      In this solution I propose that we introduce some special case logic
>      to value_of_register_lazy.  To understand what this logic is we must
>      first look at how inline frames unwind registers, this is very simple,
>      they do this:
>      
>        static struct value *
>        inline_frame_prev_register (struct frame_info *this_frame,
>                                    void **this_cache, int regnum)
>        {
>          return get_frame_register_value (this_frame, regnum);
>        }
>      
>      And remember:
>      
>        struct value *
>        get_frame_register_value (struct frame_info *frame, int regnum)
>        {
>          return frame_unwind_register_value (frame->next, regnum);
>        }
>      
>      So in all cases, unwinding a register in an inline frame just asks the
>      next frame to unwind the register, this makes sense, as an inline
>      frame doesn't really exist, when we unwind a register in an inline
>      frame, we're really just asking the next frame for the value of the
>      register in the previous, non-inline frame.
>      
>      So, if we assume that we only get into the missing frame-id situation
>      when we try to unwind a register from an inline frame during the frame
>      sniffing process, then we can change value_of_register_lazy to not
>      create lazy register values for an inline frame.
>      
>      Imagine this stack setup, where #1 is inline within #2.
>      
>        #3 -> #2 -> #1 -> #0
>              \______/
>               inline
>      
>      Now when trying to figure out the frame-id for #1, we need to compute
>      the frame-id for #2.  If the frame sniffer for #2 causes a lazy
>      register read in #2, either due to a Python Unwinder, or for the
>      tail-call sniffer, then we call value_of_register_lazy passing in
>      frame #2.
>      
>      In value_of_register_lazy, we grab the next frame, which is #1, and we
>      used to then ask for the frame-id of #1, which was not computed, and
>      this was our bug.
>      
>      Now, I propose we spot that #1 is an inline frame, and so lookup the
>      next frame of #1, which is #0.  As #0 is not inline it will have a
>      valid frame-id, and so we create a lazy register value using #0 as the
>      next-frame-id.  This will give us the exact same result we had
>      previously (thanks to the code we inspected above).
>      
>      Encoding into value_of_register_lazy the knowledge that reading an
>      inline frame register will always just forward to the next frame
>      feels.... not ideal, but this seems like the cleanest solution to this
>      recursive frame-id computation/sniffing issue that appears to crop
>      up.
>      
>      The following two commits are fully reverted with this commit, these
>      correspond to patches [1] and [2] respectively:
>      
>        commit 5939967b355ba6a940887d19847b7893a4506067
>        Date:   Tue Apr 14 17:26:22 2020 -0300
>      
>            Fix inline frame unwinding breakage
>      
>        commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
>        Date:   Sat Apr 25 00:32:44 2020 -0300
>      
>            Fix remaining inline/tailcall unwinding breakage for x86_64
>      
>      gdb/ChangeLog:
>      
>              PR python/22748
>              * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove
>              special handling for inline frames.
>              * findvar.c (value_of_register_lazy): Skip inline frames when
>              creating lazy register values.
>              * frame.c (frame_id_computed_p): Delete definition.
>              * frame.h (frame_id_computed_p): Delete declaration.
>      
>      gdb/testsuite/ChangeLog:
>      
>              PR python/22748
>              * gdb.opt/inline-frame-tailcall.c: New file.
>              * gdb.opt/inline-frame-tailcall.exp: New file.
>              * gdb.python/py-unwind-inline.c: New file.
>              * gdb.python/py-unwind-inline.exp: New file.
>              * gdb.python/py-unwind-inline.py: New file.
> 
> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index 16dba2b201a..2d219f13f9d 100644
> --- a/gdb/dwarf2/frame-tailcall.c
> +++ b/gdb/dwarf2/frame-tailcall.c
> @@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
>   
>         prev_gdbarch = frame_unwind_arch (this_frame);
>   
> -      /* The dwarf2 tailcall sniffer runs early, at the end of populating the
> -	 dwarf2 frame cache for the current frame.  If there exists inline
> -	 frames inner (next) to the current frame, there is a good possibility
> -	 of that inline frame not having a computed frame id yet.
> -
> -	 This is because computing such a frame id requires us to walk through
> -	 the frame chain until we find the first normal frame after the inline
> -	 frame and then compute the normal frame's id first.
> -
> -	 Some architectures' compilers generate enough register location
> -	 information for a dwarf unwinder to fetch PC without relying on inner
> -	 frames (x86_64 for example).  In this case the PC is retrieved
> -	 according to dwarf rules.
> -
> -	 But others generate less strict dwarf data for which assumptions are
> -	 made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as
> -	 DWARF2_FRAME_REG_SAME_VALUE).  For such cases, GDB may attempt to
> -	 create lazy values for registers, and those lazy values must be
> -	 created with a valid frame id, but we potentially have no valid id.
> -
> -	 So, to avoid breakage, if we see a dangerous situation with inline
> -	 frames without a computed id, use safer functions to retrieve the
> -	 current frame's PC.  Otherwise use the provided dwarf rules.  */
> -      frame_info *next_frame = get_next_frame (this_frame);
> -
>         /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
> -      if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME
> -	  && !frame_id_computed_p (next_frame))
> -	{
> -	  /* The next frame is an inline frame and its frame id has not been
> -	     computed yet.  */
> -	  get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
> -			      (gdb_byte *) &prev_pc);
> -	  prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
> -	}
> -      else
> -	prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
> +      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
>   
>         /* call_site_find_chain can throw an exception.  */
>         chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index c7cd31ce1a6..7e9dab567f6 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
>   
>     next_frame = get_next_frame_sentinel_okay (frame);
>   
> +  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
> +     happen if we end up trying to unwind a register as part of the frame
> +     sniffer.  The only time that we get here without a valid frame-id is
> +     if NEXT_FRAME is an inline frame.  If this is the case then we can
> +     avoid getting into trouble here by skipping past the inline frames.  */
> +  while (get_frame_type (next_frame) == INLINE_FRAME)
> +    next_frame = get_next_frame_sentinel_okay (next_frame);
> +
>     /* We should have a valid next frame.  */
>     gdb_assert (frame_id_p (get_frame_id (next_frame)));
>   
> diff --git a/gdb/frame.c b/gdb/frame.c
> index ff27b9f00e9..ac1016b083f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr)
>     return id;
>   }
>   
> -bool
> -frame_id_computed_p (struct frame_info *frame)
> -{
> -  gdb_assert (frame != nullptr);
> -
> -  return frame->this_id.p != 0;
> -}
> -
>   int
>   frame_id_p (struct frame_id l)
>   {
> diff --git a/gdb/frame.h b/gdb/frame.h
> index e835d49f9ca..cfc15022ed5 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -236,10 +236,6 @@ extern struct frame_id
>      as the special identifier address are set to indicate wild cards.  */
>   extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>   
> -/* Returns true if FRAME's id has been computed.
> -   Returns false otherwise.  */
> -extern bool frame_id_computed_p (struct frame_info *frame);
> -
>   /* Returns non-zero when L is a valid frame (a valid frame has a
>      non-zero .base).  The outermost frame is valid even without an
>      ID.  */
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> new file mode 100644
> index 00000000000..0cfe06dd273
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> @@ -0,0 +1,37 @@
> +/* Copyright 2019-2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +volatile int global_var;
> +
> +int  __attribute__ ((noinline))
> +bar ()
> +{
> +  return global_var;
> +}
> +
> +static inline int __attribute__ ((always_inline))
> +foo ()
> +{
> +  return bar ();
> +}
> +
> +int
> +main ()
> +{
> +  int ans;
> +  global_var = 0;
> +  ans = foo ();
> +  return ans;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> new file mode 100644
> index 00000000000..f7c65f6afc8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> @@ -0,0 +1,49 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This script tests GDB's handling of using a Python unwinder in the
> +# presence of inlined frames.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	  debug] } {
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +# The following tests require execution.
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +gdb_breakpoint "foo"
> +
> +gdb_test "source ${pyfile}" "Python script imported" \
> +        "import python scripts"
> +
> +gdb_continue_to_breakpoint "foo"
> +
> +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> +    "\\r\\n#0  foo \\(\\)"
> +    "\\r\\n#1  main \\(\\)"
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> new file mode 100644
> index 00000000000..7206a0b2830
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> @@ -0,0 +1,71 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# A dummy stack unwinder used for testing the Python unwinders when we
> +# have inline frames.  This unwinder will never claim any frames,
> +# instead, all it does it try to read all registers possible target
> +# registers as part of the frame sniffing process..
> +
> +import gdb
> +from gdb.unwinder import Unwinder
> +
> +apb_global = None
> +
> +class dummy_unwinder (Unwinder):
> +    """ A dummy unwinder that looks at a bunch of registers as part of
> +    the unwinding process."""
> +
> +    class frame_id (object):
> +        """ Basic frame id."""
> +
> +        def __init__ (self, sp, pc):
> +            """ Create the frame id."""
> +            self.sp = sp
> +            self.pc = pc
> +
> +    def __init__ (self):
> +        """Create the unwinder."""
> +        Unwinder.__init__ (self, "dummy stack unwinder")
> +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> +        self.regs = None
> +
> +    def get_regs (self, pending_frame):
> +        """Return a list of register names that should be read.  Only
> +        gathers the list once, then caches the result."""
> +        if (self.regs != None):
> +            return self.regs
> +
> +        # Collect the names of all registers to read.
> +        self.regs = list (pending_frame.architecture ()
> +                          .register_names ())
> +
> +        return self.regs
> +
> +    def __call__ (self, pending_frame):
> +        """Actually performs the unwind, or at least sniffs this frame
> +        to see if the unwinder should claim it, which is never does."""
> +        try:
> +            for r in (self.get_regs (pending_frame)):
> +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> +        except:
> +            print ("Dummy unwinder, exception")
> +            raise
> +
> +        return None
> +
> +# Register the ComRV stack unwinder.
> +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> +
> +print ("Python script imported")
> 

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

Ping!

Unless anyone has any feedback I plan to push this series in the next
couple of days.

Thanks,
Andrew


* Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-22 16:47:23 +0100]:

> After feedback from Luis, and then discussion here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html
> 
> this is a completely new version of this patch that addresses the
> original issue with Python unwinders, as well as addressing the issues
> brought up in the discussion above.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit c66679e65c2f247a75d84a0166b07fed352879e1
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Mon Jun 8 11:36:13 2020 +0100
> 
>     gdb: Python unwinders, inline frames, and tail-call frames
>     
>     This started with me running into the bug described in python/22748,
>     in summary, if the frame sniffing code accessed any registers within
>     an inline frame then GDB would crash with this error:
>     
>       gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
>     
>     The problem is that, when in the Python unwinder I write this:
>     
>       pending_frame.read_register ("register-name")
>     
>     This is translated internally into a call to `value_of_register',
>     which in turn becomes a call to `value_of_register_lazy'.
>     
>     Usually this isn't a problem, `value_of_register_lazy' requires the
>     next frame (more inner) to have a valid frame_id, which will be the
>     case (if we're sniffing frame #1, then frame #0 will have had its
>     frame-id figured out).
>     
>     Unfortunately if frame #0 is inline within frame #1, then the frame-id
>     for frame #0 can't be computed until we have the frame-id for #1.  As
>     a result we can't create a lazy register for frame #1 when frame #0 is
>     inline.
>     
>     Initially I proposed a solution inline with that proposed in bugzilla,
>     changing value_of_register to avoid creating a lazy register value.
>     However, when this was discussed on the mailing list I got this reply:
>     
>       https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html
>     
>     Which led me to look at these two patches:
>     
>       [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html
>       [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html
>     
>     When I considered patches [1] and [2] I saw that all of the issues
>     being addressed here were related, and that there was a single
>     solution that could address all of these issues.
>     
>     First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which
>     shows that [1] and [2] regress the inline tail-call unwinder, the
>     reason for this is that these two patches replace a call to
>     gdbarch_unwind_pc with a call to get_frame_register, however, this is
>     not correct.  The previous call to gdbarch_unwind_pc takes THIS_FRAME
>     and returns the $pc value in the previous frame.  In contrast
>     get_frame_register takes THIS_FRAME and returns the value of the $pc
>     in THIS_FRAME; these calls are not equivalent.
>     
>     The reason these patches appear (or do) fix the regressions listed in
>     [1] is that the tail call sniffer depends on identifying the address
>     of a caller and a callee, GDB then looks for a tail-call sequence that
>     takes us from the caller address to the callee, if such a series is
>     found then tail-call frames are added.
>     
>     The bug that was being hit, and which was address in patch [1] is that
>     in order to find the address of the caller, GDB ended up creating a
>     lazy register value for an inline frame with to frame-id.  The
>     solution in patch [1] is to instead take the address of the callee and
>     treat this as the address of the caller.  Getting the address of the
>     callee works, but we then end up looking for a tail-call series from
>     the callee to the callee, which obviously doesn't return any sane
>     results, so we don't insert any tail call frames.
>     
>     The original patch [1] did cause some breakage, so patch [2] undid
>     patch [1] in all cases except those where we had an inline frame with
>     no frame-id.  It just so happens that there were no tests that fitted
>     this description _and_ which required tail-call frames to be
>     successfully spotted, as a result patch [2] appeared to work.
>     
>     The new test inline-frame-tailcall.exp, exposes the flaw in patch [2].
>     
>     This commit undoes patch [1] and [2], and replaces them with a new
>     solution, which is also different to the solution proposed in the
>     python/22748 bug report.
>     
>     In this solution I propose that we introduce some special case logic
>     to value_of_register_lazy.  To understand what this logic is we must
>     first look at how inline frames unwind registers, this is very simple,
>     they do this:
>     
>       static struct value *
>       inline_frame_prev_register (struct frame_info *this_frame,
>                                   void **this_cache, int regnum)
>       {
>         return get_frame_register_value (this_frame, regnum);
>       }
>     
>     And remember:
>     
>       struct value *
>       get_frame_register_value (struct frame_info *frame, int regnum)
>       {
>         return frame_unwind_register_value (frame->next, regnum);
>       }
>     
>     So in all cases, unwinding a register in an inline frame just asks the
>     next frame to unwind the register, this makes sense, as an inline
>     frame doesn't really exist, when we unwind a register in an inline
>     frame, we're really just asking the next frame for the value of the
>     register in the previous, non-inline frame.
>     
>     So, if we assume that we only get into the missing frame-id situation
>     when we try to unwind a register from an inline frame during the frame
>     sniffing process, then we can change value_of_register_lazy to not
>     create lazy register values for an inline frame.
>     
>     Imagine this stack setup, where #1 is inline within #2.
>     
>       #3 -> #2 -> #1 -> #0
>             \______/
>              inline
>     
>     Now when trying to figure out the frame-id for #1, we need to compute
>     the frame-id for #2.  If the frame sniffer for #2 causes a lazy
>     register read in #2, either due to a Python Unwinder, or for the
>     tail-call sniffer, then we call value_of_register_lazy passing in
>     frame #2.
>     
>     In value_of_register_lazy, we grab the next frame, which is #1, and we
>     used to then ask for the frame-id of #1, which was not computed, and
>     this was our bug.
>     
>     Now, I propose we spot that #1 is an inline frame, and so lookup the
>     next frame of #1, which is #0.  As #0 is not inline it will have a
>     valid frame-id, and so we create a lazy register value using #0 as the
>     next-frame-id.  This will give us the exact same result we had
>     previously (thanks to the code we inspected above).
>     
>     Encoding into value_of_register_lazy the knowledge that reading an
>     inline frame register will always just forward to the next frame
>     feels.... not ideal, but this seems like the cleanest solution to this
>     recursive frame-id computation/sniffing issue that appears to crop
>     up.
>     
>     The following two commits are fully reverted with this commit, these
>     correspond to patches [1] and [2] respectively:
>     
>       commit 5939967b355ba6a940887d19847b7893a4506067
>       Date:   Tue Apr 14 17:26:22 2020 -0300
>     
>           Fix inline frame unwinding breakage
>     
>       commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
>       Date:   Sat Apr 25 00:32:44 2020 -0300
>     
>           Fix remaining inline/tailcall unwinding breakage for x86_64
>     
>     gdb/ChangeLog:
>     
>             PR python/22748
>             * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove
>             special handling for inline frames.
>             * findvar.c (value_of_register_lazy): Skip inline frames when
>             creating lazy register values.
>             * frame.c (frame_id_computed_p): Delete definition.
>             * frame.h (frame_id_computed_p): Delete declaration.
>     
>     gdb/testsuite/ChangeLog:
>     
>             PR python/22748
>             * gdb.opt/inline-frame-tailcall.c: New file.
>             * gdb.opt/inline-frame-tailcall.exp: New file.
>             * gdb.python/py-unwind-inline.c: New file.
>             * gdb.python/py-unwind-inline.exp: New file.
>             * gdb.python/py-unwind-inline.py: New file.
> 
> diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> index 16dba2b201a..2d219f13f9d 100644
> --- a/gdb/dwarf2/frame-tailcall.c
> +++ b/gdb/dwarf2/frame-tailcall.c
> @@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
>  
>        prev_gdbarch = frame_unwind_arch (this_frame);
>  
> -      /* The dwarf2 tailcall sniffer runs early, at the end of populating the
> -	 dwarf2 frame cache for the current frame.  If there exists inline
> -	 frames inner (next) to the current frame, there is a good possibility
> -	 of that inline frame not having a computed frame id yet.
> -
> -	 This is because computing such a frame id requires us to walk through
> -	 the frame chain until we find the first normal frame after the inline
> -	 frame and then compute the normal frame's id first.
> -
> -	 Some architectures' compilers generate enough register location
> -	 information for a dwarf unwinder to fetch PC without relying on inner
> -	 frames (x86_64 for example).  In this case the PC is retrieved
> -	 according to dwarf rules.
> -
> -	 But others generate less strict dwarf data for which assumptions are
> -	 made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as
> -	 DWARF2_FRAME_REG_SAME_VALUE).  For such cases, GDB may attempt to
> -	 create lazy values for registers, and those lazy values must be
> -	 created with a valid frame id, but we potentially have no valid id.
> -
> -	 So, to avoid breakage, if we see a dangerous situation with inline
> -	 frames without a computed id, use safer functions to retrieve the
> -	 current frame's PC.  Otherwise use the provided dwarf rules.  */
> -      frame_info *next_frame = get_next_frame (this_frame);
> -
>        /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
> -      if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME
> -	  && !frame_id_computed_p (next_frame))
> -	{
> -	  /* The next frame is an inline frame and its frame id has not been
> -	     computed yet.  */
> -	  get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
> -			      (gdb_byte *) &prev_pc);
> -	  prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
> -	}
> -      else
> -	prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
> +      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
>  
>        /* call_site_find_chain can throw an exception.  */
>        chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index c7cd31ce1a6..7e9dab567f6 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
>  
>    next_frame = get_next_frame_sentinel_okay (frame);
>  
> +  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
> +     happen if we end up trying to unwind a register as part of the frame
> +     sniffer.  The only time that we get here without a valid frame-id is
> +     if NEXT_FRAME is an inline frame.  If this is the case then we can
> +     avoid getting into trouble here by skipping past the inline frames.  */
> +  while (get_frame_type (next_frame) == INLINE_FRAME)
> +    next_frame = get_next_frame_sentinel_okay (next_frame);
> +
>    /* We should have a valid next frame.  */
>    gdb_assert (frame_id_p (get_frame_id (next_frame)));
>  
> diff --git a/gdb/frame.c b/gdb/frame.c
> index ff27b9f00e9..ac1016b083f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr)
>    return id;
>  }
>  
> -bool
> -frame_id_computed_p (struct frame_info *frame)
> -{
> -  gdb_assert (frame != nullptr);
> -
> -  return frame->this_id.p != 0;
> -}
> -
>  int
>  frame_id_p (struct frame_id l)
>  {
> diff --git a/gdb/frame.h b/gdb/frame.h
> index e835d49f9ca..cfc15022ed5 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -236,10 +236,6 @@ extern struct frame_id
>     as the special identifier address are set to indicate wild cards.  */
>  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>  
> -/* Returns true if FRAME's id has been computed.
> -   Returns false otherwise.  */
> -extern bool frame_id_computed_p (struct frame_info *frame);
> -
>  /* Returns non-zero when L is a valid frame (a valid frame has a
>     non-zero .base).  The outermost frame is valid even without an
>     ID.  */
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> new file mode 100644
> index 00000000000..0cfe06dd273
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> @@ -0,0 +1,37 @@
> +/* Copyright 2019-2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +volatile int global_var;
> +
> +int  __attribute__ ((noinline))
> +bar ()
> +{
> +  return global_var;
> +}
> +
> +static inline int __attribute__ ((always_inline))
> +foo ()
> +{
> +  return bar ();
> +}
> +
> +int
> +main ()
> +{
> +  int ans;
> +  global_var = 0;
> +  ans = foo ();
> +  return ans;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> new file mode 100644
> index 00000000000..f7c65f6afc8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> @@ -0,0 +1,49 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This script tests GDB's handling of using a Python unwinder in the
> +# presence of inlined frames.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	  debug] } {
> +    return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +# The following tests require execution.
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +gdb_breakpoint "foo"
> +
> +gdb_test "source ${pyfile}" "Python script imported" \
> +        "import python scripts"
> +
> +gdb_continue_to_breakpoint "foo"
> +
> +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> +    "\\r\\n#0  foo \\(\\)"
> +    "\\r\\n#1  main \\(\\)"
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> new file mode 100644
> index 00000000000..7206a0b2830
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> @@ -0,0 +1,71 @@
> +# Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# A dummy stack unwinder used for testing the Python unwinders when we
> +# have inline frames.  This unwinder will never claim any frames,
> +# instead, all it does it try to read all registers possible target
> +# registers as part of the frame sniffing process..
> +
> +import gdb
> +from gdb.unwinder import Unwinder
> +
> +apb_global = None
> +
> +class dummy_unwinder (Unwinder):
> +    """ A dummy unwinder that looks at a bunch of registers as part of
> +    the unwinding process."""
> +
> +    class frame_id (object):
> +        """ Basic frame id."""
> +
> +        def __init__ (self, sp, pc):
> +            """ Create the frame id."""
> +            self.sp = sp
> +            self.pc = pc
> +
> +    def __init__ (self):
> +        """Create the unwinder."""
> +        Unwinder.__init__ (self, "dummy stack unwinder")
> +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> +        self.regs = None
> +
> +    def get_regs (self, pending_frame):
> +        """Return a list of register names that should be read.  Only
> +        gathers the list once, then caches the result."""
> +        if (self.regs != None):
> +            return self.regs
> +
> +        # Collect the names of all registers to read.
> +        self.regs = list (pending_frame.architecture ()
> +                          .register_names ())
> +
> +        return self.regs
> +
> +    def __call__ (self, pending_frame):
> +        """Actually performs the unwind, or at least sniffs this frame
> +        to see if the unwinder should claim it, which is never does."""
> +        try:
> +            for r in (self.get_regs (pending_frame)):
> +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> +        except:
> +            print ("Dummy unwinder, exception")
> +            raise
> +
> +        return None
> +
> +# Register the ComRV stack unwinder.
> +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> +
> +print ("Python script imported")

Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-07-02 22:07:03 +0100]:

> Ping!
> 
> Unless anyone has any feedback I plan to push this series in the next
> couple of days.

I've now pushed this series.

Thanks,
Andrew

> 
> Thanks,
> Andrew
> 
> 
> * Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-22 16:47:23 +0100]:
> 
> > After feedback from Luis, and then discussion here:
> > 
> >   https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html
> > 
> > this is a completely new version of this patch that addresses the
> > original issue with Python unwinders, as well as addressing the issues
> > brought up in the discussion above.
> > 
> > Thanks,
> > Andrew
> > 
> > ---
> > 
> > commit c66679e65c2f247a75d84a0166b07fed352879e1
> > Author: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date:   Mon Jun 8 11:36:13 2020 +0100
> > 
> >     gdb: Python unwinders, inline frames, and tail-call frames
> >     
> >     This started with me running into the bug described in python/22748,
> >     in summary, if the frame sniffing code accessed any registers within
> >     an inline frame then GDB would crash with this error:
> >     
> >       gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed.
> >     
> >     The problem is that, when in the Python unwinder I write this:
> >     
> >       pending_frame.read_register ("register-name")
> >     
> >     This is translated internally into a call to `value_of_register',
> >     which in turn becomes a call to `value_of_register_lazy'.
> >     
> >     Usually this isn't a problem, `value_of_register_lazy' requires the
> >     next frame (more inner) to have a valid frame_id, which will be the
> >     case (if we're sniffing frame #1, then frame #0 will have had its
> >     frame-id figured out).
> >     
> >     Unfortunately if frame #0 is inline within frame #1, then the frame-id
> >     for frame #0 can't be computed until we have the frame-id for #1.  As
> >     a result we can't create a lazy register for frame #1 when frame #0 is
> >     inline.
> >     
> >     Initially I proposed a solution inline with that proposed in bugzilla,
> >     changing value_of_register to avoid creating a lazy register value.
> >     However, when this was discussed on the mailing list I got this reply:
> >     
> >       https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html
> >     
> >     Which led me to look at these two patches:
> >     
> >       [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html
> >       [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html
> >     
> >     When I considered patches [1] and [2] I saw that all of the issues
> >     being addressed here were related, and that there was a single
> >     solution that could address all of these issues.
> >     
> >     First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which
> >     shows that [1] and [2] regress the inline tail-call unwinder, the
> >     reason for this is that these two patches replace a call to
> >     gdbarch_unwind_pc with a call to get_frame_register, however, this is
> >     not correct.  The previous call to gdbarch_unwind_pc takes THIS_FRAME
> >     and returns the $pc value in the previous frame.  In contrast
> >     get_frame_register takes THIS_FRAME and returns the value of the $pc
> >     in THIS_FRAME; these calls are not equivalent.
> >     
> >     The reason these patches appear (or do) fix the regressions listed in
> >     [1] is that the tail call sniffer depends on identifying the address
> >     of a caller and a callee, GDB then looks for a tail-call sequence that
> >     takes us from the caller address to the callee, if such a series is
> >     found then tail-call frames are added.
> >     
> >     The bug that was being hit, and which was address in patch [1] is that
> >     in order to find the address of the caller, GDB ended up creating a
> >     lazy register value for an inline frame with to frame-id.  The
> >     solution in patch [1] is to instead take the address of the callee and
> >     treat this as the address of the caller.  Getting the address of the
> >     callee works, but we then end up looking for a tail-call series from
> >     the callee to the callee, which obviously doesn't return any sane
> >     results, so we don't insert any tail call frames.
> >     
> >     The original patch [1] did cause some breakage, so patch [2] undid
> >     patch [1] in all cases except those where we had an inline frame with
> >     no frame-id.  It just so happens that there were no tests that fitted
> >     this description _and_ which required tail-call frames to be
> >     successfully spotted, as a result patch [2] appeared to work.
> >     
> >     The new test inline-frame-tailcall.exp, exposes the flaw in patch [2].
> >     
> >     This commit undoes patch [1] and [2], and replaces them with a new
> >     solution, which is also different to the solution proposed in the
> >     python/22748 bug report.
> >     
> >     In this solution I propose that we introduce some special case logic
> >     to value_of_register_lazy.  To understand what this logic is we must
> >     first look at how inline frames unwind registers, this is very simple,
> >     they do this:
> >     
> >       static struct value *
> >       inline_frame_prev_register (struct frame_info *this_frame,
> >                                   void **this_cache, int regnum)
> >       {
> >         return get_frame_register_value (this_frame, regnum);
> >       }
> >     
> >     And remember:
> >     
> >       struct value *
> >       get_frame_register_value (struct frame_info *frame, int regnum)
> >       {
> >         return frame_unwind_register_value (frame->next, regnum);
> >       }
> >     
> >     So in all cases, unwinding a register in an inline frame just asks the
> >     next frame to unwind the register, this makes sense, as an inline
> >     frame doesn't really exist, when we unwind a register in an inline
> >     frame, we're really just asking the next frame for the value of the
> >     register in the previous, non-inline frame.
> >     
> >     So, if we assume that we only get into the missing frame-id situation
> >     when we try to unwind a register from an inline frame during the frame
> >     sniffing process, then we can change value_of_register_lazy to not
> >     create lazy register values for an inline frame.
> >     
> >     Imagine this stack setup, where #1 is inline within #2.
> >     
> >       #3 -> #2 -> #1 -> #0
> >             \______/
> >              inline
> >     
> >     Now when trying to figure out the frame-id for #1, we need to compute
> >     the frame-id for #2.  If the frame sniffer for #2 causes a lazy
> >     register read in #2, either due to a Python Unwinder, or for the
> >     tail-call sniffer, then we call value_of_register_lazy passing in
> >     frame #2.
> >     
> >     In value_of_register_lazy, we grab the next frame, which is #1, and we
> >     used to then ask for the frame-id of #1, which was not computed, and
> >     this was our bug.
> >     
> >     Now, I propose we spot that #1 is an inline frame, and so lookup the
> >     next frame of #1, which is #0.  As #0 is not inline it will have a
> >     valid frame-id, and so we create a lazy register value using #0 as the
> >     next-frame-id.  This will give us the exact same result we had
> >     previously (thanks to the code we inspected above).
> >     
> >     Encoding into value_of_register_lazy the knowledge that reading an
> >     inline frame register will always just forward to the next frame
> >     feels.... not ideal, but this seems like the cleanest solution to this
> >     recursive frame-id computation/sniffing issue that appears to crop
> >     up.
> >     
> >     The following two commits are fully reverted with this commit, these
> >     correspond to patches [1] and [2] respectively:
> >     
> >       commit 5939967b355ba6a940887d19847b7893a4506067
> >       Date:   Tue Apr 14 17:26:22 2020 -0300
> >     
> >           Fix inline frame unwinding breakage
> >     
> >       commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d
> >       Date:   Sat Apr 25 00:32:44 2020 -0300
> >     
> >           Fix remaining inline/tailcall unwinding breakage for x86_64
> >     
> >     gdb/ChangeLog:
> >     
> >             PR python/22748
> >             * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove
> >             special handling for inline frames.
> >             * findvar.c (value_of_register_lazy): Skip inline frames when
> >             creating lazy register values.
> >             * frame.c (frame_id_computed_p): Delete definition.
> >             * frame.h (frame_id_computed_p): Delete declaration.
> >     
> >     gdb/testsuite/ChangeLog:
> >     
> >             PR python/22748
> >             * gdb.opt/inline-frame-tailcall.c: New file.
> >             * gdb.opt/inline-frame-tailcall.exp: New file.
> >             * gdb.python/py-unwind-inline.c: New file.
> >             * gdb.python/py-unwind-inline.exp: New file.
> >             * gdb.python/py-unwind-inline.py: New file.
> > 
> > diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c
> > index 16dba2b201a..2d219f13f9d 100644
> > --- a/gdb/dwarf2/frame-tailcall.c
> > +++ b/gdb/dwarf2/frame-tailcall.c
> > @@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame,
> >  
> >        prev_gdbarch = frame_unwind_arch (this_frame);
> >  
> > -      /* The dwarf2 tailcall sniffer runs early, at the end of populating the
> > -	 dwarf2 frame cache for the current frame.  If there exists inline
> > -	 frames inner (next) to the current frame, there is a good possibility
> > -	 of that inline frame not having a computed frame id yet.
> > -
> > -	 This is because computing such a frame id requires us to walk through
> > -	 the frame chain until we find the first normal frame after the inline
> > -	 frame and then compute the normal frame's id first.
> > -
> > -	 Some architectures' compilers generate enough register location
> > -	 information for a dwarf unwinder to fetch PC without relying on inner
> > -	 frames (x86_64 for example).  In this case the PC is retrieved
> > -	 according to dwarf rules.
> > -
> > -	 But others generate less strict dwarf data for which assumptions are
> > -	 made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as
> > -	 DWARF2_FRAME_REG_SAME_VALUE).  For such cases, GDB may attempt to
> > -	 create lazy values for registers, and those lazy values must be
> > -	 created with a valid frame id, but we potentially have no valid id.
> > -
> > -	 So, to avoid breakage, if we see a dangerous situation with inline
> > -	 frames without a computed id, use safer functions to retrieve the
> > -	 current frame's PC.  Otherwise use the provided dwarf rules.  */
> > -      frame_info *next_frame = get_next_frame (this_frame);
> > -
> >        /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p.  */
> > -      if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME
> > -	  && !frame_id_computed_p (next_frame))
> > -	{
> > -	  /* The next frame is an inline frame and its frame id has not been
> > -	     computed yet.  */
> > -	  get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch),
> > -			      (gdb_byte *) &prev_pc);
> > -	  prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc);
> > -	}
> > -      else
> > -	prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
> > +      prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame);
> >  
> >        /* call_site_find_chain can throw an exception.  */
> >        chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc);
> > diff --git a/gdb/findvar.c b/gdb/findvar.c
> > index c7cd31ce1a6..7e9dab567f6 100644
> > --- a/gdb/findvar.c
> > +++ b/gdb/findvar.c
> > @@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
> >  
> >    next_frame = get_next_frame_sentinel_okay (frame);
> >  
> > +  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
> > +     happen if we end up trying to unwind a register as part of the frame
> > +     sniffer.  The only time that we get here without a valid frame-id is
> > +     if NEXT_FRAME is an inline frame.  If this is the case then we can
> > +     avoid getting into trouble here by skipping past the inline frames.  */
> > +  while (get_frame_type (next_frame) == INLINE_FRAME)
> > +    next_frame = get_next_frame_sentinel_okay (next_frame);
> > +
> >    /* We should have a valid next frame.  */
> >    gdb_assert (frame_id_p (get_frame_id (next_frame)));
> >  
> > diff --git a/gdb/frame.c b/gdb/frame.c
> > index ff27b9f00e9..ac1016b083f 100644
> > --- a/gdb/frame.c
> > +++ b/gdb/frame.c
> > @@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr)
> >    return id;
> >  }
> >  
> > -bool
> > -frame_id_computed_p (struct frame_info *frame)
> > -{
> > -  gdb_assert (frame != nullptr);
> > -
> > -  return frame->this_id.p != 0;
> > -}
> > -
> >  int
> >  frame_id_p (struct frame_id l)
> >  {
> > diff --git a/gdb/frame.h b/gdb/frame.h
> > index e835d49f9ca..cfc15022ed5 100644
> > --- a/gdb/frame.h
> > +++ b/gdb/frame.h
> > @@ -236,10 +236,6 @@ extern struct frame_id
> >     as the special identifier address are set to indicate wild cards.  */
> >  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
> >  
> > -/* Returns true if FRAME's id has been computed.
> > -   Returns false otherwise.  */
> > -extern bool frame_id_computed_p (struct frame_info *frame);
> > -
> >  /* Returns non-zero when L is a valid frame (a valid frame has a
> >     non-zero .base).  The outermost frame is valid even without an
> >     ID.  */
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > new file mode 100644
> > index 00000000000..0cfe06dd273
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c
> > @@ -0,0 +1,37 @@
> > +/* Copyright 2019-2020 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > +
> > +volatile int global_var;
> > +
> > +int  __attribute__ ((noinline))
> > +bar ()
> > +{
> > +  return global_var;
> > +}
> > +
> > +static inline int __attribute__ ((always_inline))
> > +foo ()
> > +{
> > +  return bar ();
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  int ans;
> > +  global_var = 0;
> > +  ans = foo ();
> > +  return ans;
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > new file mode 100644
> > index 00000000000..f7c65f6afc8
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp
> > @@ -0,0 +1,49 @@
> > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# This script tests GDB's handling of using a Python unwinder in the
> > +# presence of inlined frames.
> > +
> > +load_lib gdb-python.exp
> > +
> > +standard_testfile
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> > +	  debug] } {
> > +    return -1
> > +}
> > +
> > +# Skip all tests if Python scripting is not enabled.
> > +if { [skip_python_tests] } { continue }
> > +
> > +# The following tests require execution.
> > +if ![runto_main] then {
> > +    fail "can't run to main"
> > +    return 0
> > +}
> > +
> > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> > +
> > +gdb_breakpoint "foo"
> > +
> > +gdb_test "source ${pyfile}" "Python script imported" \
> > +        "import python scripts"
> > +
> > +gdb_continue_to_breakpoint "foo"
> > +
> > +gdb_test_sequence "backtrace"  "backtrace with dummy unwinder" {
> > +    "\\r\\n#0  foo \\(\\)"
> > +    "\\r\\n#1  main \\(\\)"
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > new file mode 100644
> > index 00000000000..7206a0b2830
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py
> > @@ -0,0 +1,71 @@
> > +# Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# A dummy stack unwinder used for testing the Python unwinders when we
> > +# have inline frames.  This unwinder will never claim any frames,
> > +# instead, all it does it try to read all registers possible target
> > +# registers as part of the frame sniffing process..
> > +
> > +import gdb
> > +from gdb.unwinder import Unwinder
> > +
> > +apb_global = None
> > +
> > +class dummy_unwinder (Unwinder):
> > +    """ A dummy unwinder that looks at a bunch of registers as part of
> > +    the unwinding process."""
> > +
> > +    class frame_id (object):
> > +        """ Basic frame id."""
> > +
> > +        def __init__ (self, sp, pc):
> > +            """ Create the frame id."""
> > +            self.sp = sp
> > +            self.pc = pc
> > +
> > +    def __init__ (self):
> > +        """Create the unwinder."""
> > +        Unwinder.__init__ (self, "dummy stack unwinder")
> > +        self.void_ptr_t = gdb.lookup_type("void").pointer()
> > +        self.regs = None
> > +
> > +    def get_regs (self, pending_frame):
> > +        """Return a list of register names that should be read.  Only
> > +        gathers the list once, then caches the result."""
> > +        if (self.regs != None):
> > +            return self.regs
> > +
> > +        # Collect the names of all registers to read.
> > +        self.regs = list (pending_frame.architecture ()
> > +                          .register_names ())
> > +
> > +        return self.regs
> > +
> > +    def __call__ (self, pending_frame):
> > +        """Actually performs the unwind, or at least sniffs this frame
> > +        to see if the unwinder should claim it, which is never does."""
> > +        try:
> > +            for r in (self.get_regs (pending_frame)):
> > +                v = pending_frame.read_register (r).cast (self.void_ptr_t)
> > +        except:
> > +            print ("Dummy unwinder, exception")
> > +            raise
> > +
> > +        return None
> > +
> > +# Register the ComRV stack unwinder.
> > +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True)
> > +
> > +print ("Python script imported")