[PATCH 0/4] Prevent more recursion into python based unwinders

[PATCH 0/4] Prevent more recursion into python based unwinders

[Kevin Buettner]

Pedro pushed a patch earlier this month which avoids recursion into a
python based unwinder in some cases.  The exact case that his patch
fixed was when the unwinder attempted to call
gdb.PendingFrame.read_register with either an actual or pseudo
register.

For reference, Pedro's patch is here:

https://sourceware.org/ml/gdb-patches/2016-08/msg00239.html

I recently posted a test case which exhibits problems with recursion
in two other cases: 1) when gdb.parse_and_eval is called, and 2)
when gdb.PendingFrame.read_register is called with a "user" register
like "pc" on x86_64.

My test case (extension) can be found here:

https://sourceware.org/ml/gdb-patches/2016-09/msg00368.html

This patch set fixes those two cases of recursion and adjusts one
test, py-unwind-maint.exp, which was showing failures / regressions
after my changes.

I wish to thank Pedro for his collaboration on these patches.  He
suggested several of the key ideas that went into them.  (That said,
he hasn't seen them yet, so if I botched things, the fault is mine.)

Kevin

[PATCH 1/4] Distinguish sentinel frame from null frame

[Kevin Buettner]

Distinguish sentinel frame from null frame.

This patch replaces the `current_frame' static global in frame.c with
`sentinel_frame'.  It also makes the sentinel frame id unique and
different from the null frame.

By itself, there is not much point to this patch, but it makes
the code cleaner for the VALUE_FRAME_ID changes in another patch.
Since we now allow "navigation" to the sentinel frame, it removes
the necessity of adding special cases to other parts of GDB.

Note that a new function, get_next_frame_sentinel_okay, is introduced
in this patch.  It will be used by the VALUE_FRAME_ID changes that
I've made.

Thanks to Pedro Alves for this suggestion.

gdb/ChangeLog:
    
	* frame.h (enum frame_id_stack_status): Add FID_STACK_SENTINEL.
	(struct frame_id): Increase number of bits required for storing
	stack status to 3 from 2.
	(sentinel_frame_id): New declaration.
	(get_next_frame_sentinel_okay): Declare.
	(frame_find_by_id_sentinel_okay): Declare.
	* frame.c (current_frame): Rename this static global to...
	(sentinel_frame): ...this static global, which has also been
	moved an earlier location in the file.
	(fprint_frame_id): Add case for sentinel frame id.
	(get_frame_id): Return early for sentinel frame.
	(sentinel_frame_id): Define.
	(frame_find_by_id): Add case for sentinel_frame_id.
	(create_sentinel_frame): Use sentinel_frame_id for this_id.value
	instead of null_frame_id.
	(get_current_frame): Add local declaration for `current_frame'.
	Remove local declaration for `sentinel_frame.'  Add new_local,
	new_sentinel_p, and use it for knowing when to call get_frame_id().
	(get_next_frame_sentinel_okay): New function.
	(reinit_frame_cache): Use `sentinel_frame' in place of
	`current_frame'.
---
 gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++-------------------
 gdb/frame.h |  12 +++++++-
 2 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index d3f3056..3ca8ab2 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -43,6 +43,17 @@
 #include "hashtab.h"
 #include "valprint.h"
 
+/* The sentinel frame is the innermost frame.  It generally does not
+   have any stack associated with it.  Register values may be directly
+   determined by inspection with no unwinding necessary.
+
+   The sentinel frame is constructed so that the next frame is a pointer
+   to the sentinel frame itself.
+
+   The current frame can be found at sentinel_frame->prev.  */
+
+static struct frame_info *sentinel_frame;
+
 static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame);
 static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason);
 
@@ -65,7 +76,7 @@ enum cached_copy_status
 
 /* We keep a cache of stack frames, each of which is a "struct
    frame_info".  The innermost one gets allocated (in
-   wait_for_inferior) each time the inferior stops; current_frame
+   wait_for_inferior) each time the inferior stops; sentinel_frame
    points to it.  Additional frames get allocated (in get_prev_frame)
    as needed, and are chained through the next and prev fields.  Any
    time that the frame cache becomes invalid (most notably when we
@@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
     fprintf_unfiltered (file, "!stack");
   else if (id.stack_status == FID_STACK_UNAVAILABLE)
     fprintf_unfiltered (file, "stack=<unavailable>");
+  else if (id.stack_status == FID_STACK_SENTINEL)
+    fprintf_unfiltered (file, "stack=<sentinel>");
   else
     fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
   fprintf_unfiltered (file, ",");
@@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi)
   if (fi == NULL)
     return null_frame_id;
 
+  if (fi == sentinel_frame)
+    return sentinel_frame_id;
+
   if (!fi->this_id.p)
     {
       int stashed;
@@ -562,6 +578,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
 }
 
 const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
+const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 };
 const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 };
 
 struct frame_id
@@ -797,6 +814,10 @@ frame_find_by_id (struct frame_id id)
   if (!frame_id_p (id))
     return NULL;
 
+  /* Check for the sentinel frame.  */
+  if (frame_id_eq (id, sentinel_frame_id))
+    return sentinel_frame;
+
   /* Try using the frame stash first.  Finding it there removes the need
      to perform the search by looping over all frames, which can be very
      CPU-intensive if the number of frames is very high (the loop is O(n)
@@ -1474,10 +1495,9 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
   /* Link this frame back to itself.  The frame is self referential
      (the unwound PC is the same as the pc), so make it so.  */
   frame->next = frame;
-  /* Make the sentinel frame's ID valid, but invalid.  That way all
-     comparisons with it should fail.  */
+  /* The sentinel frame has a special ID.  */
   frame->this_id.p = 1;
-  frame->this_id.value = null_frame_id;
+  frame->this_id.value = sentinel_frame_id;
   if (frame_debug)
     {
       fprintf_unfiltered (gdb_stdlog, "{ create_sentinel_frame (...) -> ");
@@ -1487,10 +1507,6 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
   return frame;
 }
 
-/* Info about the innermost stack frame (contents of FP register).  */
-
-static struct frame_info *current_frame;
-
 /* Cache for frame addresses already read by gdb.  Valid only while
    inferior is stopped.  Control variables for the frame cache should
    be local to this module.  */
@@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
 struct frame_info *
 get_current_frame (void)
 {
+  struct frame_info *current_frame;
+  int new_sentinel_p = 0;
+
   /* First check, and report, the lack of registers.  Having GDB
      report "No stack!" or "No memory" when the target doesn't even
      have registers is very confusing.  Besides, "printcmd.exp"
@@ -1526,30 +1545,37 @@ get_current_frame (void)
   if (get_traceframe_number () < 0)
     validate_registers_access ();
 
-  if (current_frame == NULL)
+  if (sentinel_frame == NULL)
     {
-      int stashed;
-      struct frame_info *sentinel_frame =
+      sentinel_frame =
 	create_sentinel_frame (current_program_space, get_current_regcache ());
-
-      /* Set the current frame before computing the frame id, to avoid
-	 recursion inside compute_frame_id, in case the frame's
-	 unwinder decides to do a symbol lookup (which depends on the
-	 selected frame's block).
-
-	 This call must always succeed.  In particular, nothing inside
-	 get_prev_frame_always_1 should try to unwind from the
-	 sentinel frame, because that could fail/throw, and we always
-	 want to leave with the current frame created and linked in --
-	 we should never end up with the sentinel frame as outermost
-	 frame.  */
-      current_frame = get_prev_frame_always_1 (sentinel_frame);
-      gdb_assert (current_frame != NULL);
-
-      /* No need to compute the frame id yet.  That'll be done lazily
-	 from inside get_frame_id instead.  */
+      new_sentinel_p = 1;
     }
 
+  /* Set the current frame before computing the frame id, to avoid
+     recursion inside compute_frame_id, in case the frame's
+     unwinder decides to do a symbol lookup (which depends on the
+     selected frame's block).
+
+     This call must always succeed.  In particular, nothing inside
+     get_prev_frame_always_1 should try to unwind from the
+     sentinel frame, because that could fail/throw, and we always
+     want to leave with the current frame created and linked in --
+     we should never end up with the sentinel frame as outermost
+     frame.  */
+  current_frame = get_prev_frame_always_1 (sentinel_frame);
+  gdb_assert (current_frame != NULL);
+
+  /* The call to get_frame_id, below, is not necessary when the stack is
+     well behaved.  But when it's not well behaved, we want to ensure
+     that the frame id for the current frame is known (stashed) prior to
+     finding frame id values for any outer frames.
+
+     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
+     in the "backtrace from stop_frame" test when we don't do this here.  */
+  if (new_sentinel_p)
+    get_frame_id (current_frame);
+
   return current_frame;
 }
 
@@ -1729,6 +1755,19 @@ get_next_frame (struct frame_info *this_frame)
     return NULL;
 }
 
+/* Return the frame that THIS_FRAME calls.  If THIS_FRAME is the innermost
+   frame, we return the sentinel frame.  Thus, NULL will never be returned.  */
+
+struct frame_info *
+get_next_frame_sentinel_okay (struct frame_info *this_frame)
+{
+  gdb_assert (this_frame != NULL);
+
+  /* This always works, since the sentinel frame refers to itself via the
+     ``next'' field.  */
+  return this_frame->next;
+}
+
 /* Observer for the target_changed event.  */
 
 static void
@@ -1745,7 +1784,7 @@ reinit_frame_cache (void)
   struct frame_info *fi;
 
   /* Tear down all frame caches.  */
-  for (fi = current_frame; fi != NULL; fi = fi->prev)
+  for (fi = sentinel_frame; fi != NULL; fi = fi->prev)
     {
       if (fi->prologue_cache && fi->unwind->dealloc_cache)
 	fi->unwind->dealloc_cache (fi, fi->prologue_cache);
@@ -1757,10 +1796,10 @@ reinit_frame_cache (void)
   obstack_free (&frame_cache_obstack, 0);
   obstack_init (&frame_cache_obstack);
 
-  if (current_frame != NULL)
+  if (sentinel_frame != NULL)
     annotate_frames_invalid ();
 
-  current_frame = NULL;		/* Invalidate cache */
+  sentinel_frame = NULL;		/* Invalidate cache */
   select_frame (NULL);
   frame_stash_invalidate ();
   if (frame_debug)
diff --git a/gdb/frame.h b/gdb/frame.h
index 5f21bb8..849a83d 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -89,6 +89,9 @@ enum frame_id_stack_status
   /* Stack address is valid, and is found in the stack_addr field.  */
   FID_STACK_VALID = 1,
 
+  /* Sentinel frame.  Stack may or may not be valid.  */
+  FID_STACK_SENTINEL = 2,
+
   /* Stack address is unavailable.  I.e., there's a valid stack, but
      we don't know where it is (because memory or registers we'd
      compute it from were not collected).  */
@@ -149,7 +152,7 @@ struct frame_id
   CORE_ADDR special_addr;
 
   /* Flags to indicate the above fields have valid contents.  */
-  ENUM_BITFIELD(frame_id_stack_status) stack_status : 2;
+  ENUM_BITFIELD(frame_id_stack_status) stack_status : 3;
   unsigned int code_addr_p : 1;
   unsigned int special_addr_p : 1;
 
@@ -165,6 +168,9 @@ struct frame_id
 /* For convenience.  All fields are zero.  This means "there is no frame".  */
 extern const struct frame_id null_frame_id;
 
+/* Sentinel frame.  */
+extern const struct frame_id sentinel_frame_id;
+
 /* This means "there is no frame ID, but there is a frame".  It should be
    replaced by best-effort frame IDs for the outermost frame, somehow.
    The implementation is only special_addr_p set.  */
@@ -309,6 +315,10 @@ extern void select_frame (struct frame_info *);
 extern struct frame_info *get_prev_frame (struct frame_info *);
 extern struct frame_info *get_next_frame (struct frame_info *);
 
+/* Like get_next_frame(), but allows return of the sentinel frame.  NULL
+   is never returned.  */
+extern struct frame_info *get_next_frame_sentinel_okay (struct frame_info *);
+
 /* Return a "struct frame_info" corresponding to the frame that called
    THIS_FRAME.  Returns NULL if there is no such frame.
 

[PATCH 2/4] Tweak meaning of VALUE_FRAME_ID

[Kevin Buettner]

Tweak meaning of VALUE_FRAME_ID.

The VALUE_FRAME_ID macro provides access to a member in struct value
that's used to hold the frame id that's used when determining a
register's value or when assigning to a register.  The underlying
member has a long and obscure name.  I won't refer to it here, but
will simply refer to VALUE_FRAME_ID as if it's the struct value member
instead of being a convenient macro.

At the moment, without this patch in place, VALUE_FRAME_ID is set in
value_of_register_lazy() and several other locations to hold the frame
id of the frame passed to those functions.

VALUE_FRAME_ID is used in the lval_register case of
value_fetch_lazy().  To fetch the register's value, it calls
get_frame_register_value() which, in turn, calls
frame_unwind_register_value() with frame->next.

A python based unwinder may wish to determine the value of a register
or evaluate an expression containing a register.  When it does this,
value_fetch_lazy() will be called under some circumstances.  It will
attempt to determine the frame id associated with the frame passed to
it.  In so doing, it will end up back in the frame sniffer of the very
same python unwinder that's attempting to learn the value of a
register as part of the sniffing operation.  This recursion is not
desirable.

As noted above, when value_fetch_lazy() wants to fetch a register's
value, it does so (indirectly) by unwinding from frame->next.

With this in mind, a solution suggests itself:  Change VALUE_FRAME_ID
to hold the frame id associated with the next frame.  Then, when it
comes time to obtain the value associated with the register, we can
simply unwind from the frame corresponding to the frame id stored in
VALUE_FRAME_ID.  This neatly avoids the python unwinder recursion
problem by changing when the "next" operation occurs.  Instead of the
"next" operation occuring when the register value is fetched, it
occurs earlier on when assigning a frame id to VALUE_FRAME_ID.
(Thanks to Pedro for this suggestion.)

This patch implements this idea.

It builds on the patch "Distinguish sentinel frame from null frame".
Without that work in place, it's necessary to check for null_id at
several places and then obtain the sentinel frame.

In the course of developing this patch, I found that the lval_register
case in value_assign() required some special care.  It was passing the
frame associated with VALUE_FRAME_ID (for the value being assigned) to
put_frame_register_bytes().  But put_frame_register_bytes() calls
put_frame_register(), which calls frame_register, which does
frame_register_unwind() on frame->next.  I briefly considered adjusting
frame_register_unwind to work on the frame passed to it instead of
frame->next, but this would have required more extensive changes
throughout more of GDB.  Instead, I opted for changing value_assign()
so that frame->prev is passed to put_frame_register_bytes().

gdb/ChangeLog:

	    * findvar.c (value_of_register_lazy): VALUE_FRAME_ID for
	    register value now refers to the next frame.
	    (default_value_from_register): Likewise.
	    (value_from_register): Likewise.
	    * frame_unwind.c (frame_unwind_got_optimized): Likewise.
	    * sentinel-frame.c (sentinel_frame_prev_register): Likewise.
	    * valops.c (value_assign): Obtain `prev' frame for passing
	    to put_frame_register_bytes().
	    * value.c (value_fetch_lazy): Call frame_unwind_register_value()
	    instead of get_frame_register_value().
---
 gdb/findvar.c        | 22 ++++++++++++++++++----
 gdb/frame-unwind.c   |  2 +-
 gdb/sentinel-frame.c |  2 +-
 gdb/valops.c         | 11 +++++++++++
 gdb/value.c          |  2 +-
 5 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 6e28a29..4d4ae49 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -283,17 +283,23 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct value *reg_val;
+  struct frame_info *next_frame;
 
   gdb_assert (regnum < (gdbarch_num_regs (gdbarch)
 			+ gdbarch_num_pseudo_regs (gdbarch)));
 
-  /* We should have a valid (i.e. non-sentinel) frame.  */
-  gdb_assert (frame_id_p (get_frame_id (frame)));
+  gdb_assert (frame != NULL);
+
+  next_frame = get_next_frame_sentinel_okay (frame);
+
+  /* We should have a valid next frame.  */
+  gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
   reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
   VALUE_LVAL (reg_val) = lval_register;
   VALUE_REGNUM (reg_val) = regnum;
-  VALUE_FRAME_ID (reg_val) = get_frame_id (frame);
+  VALUE_FRAME_ID (reg_val) = get_frame_id (next_frame);
+
   return reg_val;
 }
 
@@ -815,8 +821,16 @@ default_value_from_register (struct gdbarch *gdbarch, struct type *type,
 {
   int len = TYPE_LENGTH (type);
   struct value *value = allocate_value (type);
+  struct frame_info *frame;
 
   VALUE_LVAL (value) = lval_register;
+  frame = frame_find_by_id (frame_id);
+
+  if (frame == NULL)
+    frame_id = null_frame_id;
+  else
+    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+
   VALUE_FRAME_ID (value) = frame_id;
   VALUE_REGNUM (value) = regnum;
 
@@ -902,7 +916,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
          including the location.  */
       v = allocate_value (type);
       VALUE_LVAL (v) = lval_register;
-      VALUE_FRAME_ID (v) = get_frame_id (frame);
+      VALUE_FRAME_ID (v) = get_frame_id (get_next_frame_sentinel_okay (frame));
       VALUE_REGNUM (v) = regnum;
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
 				      value_contents_raw (v), &optim,
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 187e6c2..0dd98a2 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -210,7 +210,7 @@ frame_unwind_got_optimized (struct frame_info *frame, int regnum)
   mark_value_bytes_optimized_out (val, 0, TYPE_LENGTH (type));
   VALUE_LVAL (val) = lval_register;
   VALUE_REGNUM (val) = regnum;
-  VALUE_FRAME_ID (val) = get_frame_id (frame);
+  VALUE_FRAME_ID (val) = get_frame_id (get_next_frame_sentinel_okay (frame));
   return val;
 }
 
diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
index 6cd1bc3..515650c 100644
--- a/gdb/sentinel-frame.c
+++ b/gdb/sentinel-frame.c
@@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame,
   struct value *value;
 
   value = regcache_cooked_read_value (cache->regcache, regnum);
-  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
+  VALUE_FRAME_ID (value) = get_frame_id (get_next_frame_sentinel_okay (this_frame));
 
   return value;
 }
diff --git a/gdb/valops.c b/gdb/valops.c
index 40392e8..c3305fa 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1114,6 +1114,17 @@ value_assign (struct value *toval, struct value *fromval)
 
 	/* Figure out which frame this is in currently.  */
 	frame = frame_find_by_id (VALUE_FRAME_ID (toval));
+
+	/* value_of_register_lazy() stores what amounts to frame->next
+	   in VALUE_FRAME_ID.  But our eventual call to
+	   put_frame_register_bytes() will do its own next.  So, to
+	   make things work out, we must pass it frame->prev instead
+	   of just frame.  Otherwise, it'll (essentially) be
+	   frame->next->next.  */
+
+	if (frame != NULL)
+	  frame = get_prev_frame (frame);
+
 	value_reg = VALUE_REGNUM (toval);
 
 	if (!frame)
diff --git a/gdb/value.c b/gdb/value.c
index b825aec..92afc45 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -4004,7 +4004,7 @@ value_fetch_lazy (struct value *val)
 	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
 						   regnum, type));
 
-	  new_val = get_frame_register_value (frame, regnum);
+	  new_val = frame_unwind_register_value (frame, regnum);
 
 	  /* If we get another lazy lval_register value, it means the
 	     register is found by reading it from the next frame.

[PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers

[Kevin Buettner]

Make gdb.PendingFrame.read_register handle "user" registers.

The C function, pending_framepy_read_register(), which implements
the python interface gdb.PendingFrame.read_register does not handle
the so called "user" registers like "pc".  An assertion error is
triggered due to the user registers having numbers larger than or
equal to gdbarch_num_regs(gdbarch).

With the VALUE_FRAME_ID tweak in place, the call to
get_frame_register_value() can simply be replaced by a call to
value_of_register(), which handles both real registers as well as the
user registers.

gdb/ChangeLog:
    
	* python/py-unwind.c (pending_framepy_read_register): Use
	value_of_register() instead of get_frame_register_value().
---
 gdb/python/py-unwind.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index cc685ae..6740034 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -412,7 +412,12 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
 
   TRY
     {
-      val = get_frame_register_value (pending_frame->frame_info, regnum);
+      /* Fetch the value associated with a register, whether it's
+	 a real register or a so called "user" register, like "pc",
+	 which maps to a real register.  In the past,
+	 get_frame_register_value() was used here, which did not
+	 handle the user register case.  */
+      val = value_of_register (regnum, pending_frame->frame_info);
       if (val == NULL)
         PyErr_Format (PyExc_ValueError,
                       "Cannot read register %d from frame.",

[PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import

[Kevin Buettner]

py-unwind-maint.exp: Allow unwinders to be called during python import

In his commit of "Fix PR19927:  Avoid unwinder recursion if sniffer
uses calls parse_and_eval", Pedro updated the py-unwind-maint.exp
test to account for different behavior in GDB caused by his changes
to frame.c.

This patch changes py-unwind-maint.exp so that either behavior is
acceptable.

Regardless of which path is taken, the number of PASSes and the
names of the tests are the same.

gdb/testsuite/ChangeLog:
    
    	* gdb.python/py-unwind-maint.exp: Adjust tests to allow for
    	unwinders to be called as a side effect of "source" and "disable
    	unwinder" commands.  If these side effects don't occur, detect
    	that behavior and run slightly different tests which are also
    	considered to be correct behavior.
---
 gdb/testsuite/gdb.python/py-unwind-maint.exp | 59 ++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-unwind-maint.exp b/gdb/testsuite/gdb.python/py-unwind-maint.exp
index 1253057..91b1fdf 100644
--- a/gdb/testsuite/gdb.python/py-unwind-maint.exp
+++ b/gdb/testsuite/gdb.python/py-unwind-maint.exp
@@ -34,13 +34,49 @@ if ![runto_main ] then {
     return -1
 }
 
-gdb_test "source ${pyfile}" "Python script imported" \
-    "import python scripts"
 
-gdb_test_sequence "frame" "All unwinders enabled" {
-    "py_unwind_maint_ps_unwinder called"
-    "global_unwinder called"
-    "#0  main"
+send_gdb "source ${pyfile}\n"
+gdb_expect {
+    -re "Python script imported\r\n$gdb_prompt $" {
+	set unwinder_called_on_import false
+    }
+    -re ".*$gdb_prompt $" {
+	set unwinder_called_on_import true
+    }
+    timeout {
+      fail "Can't source python script"
+      return -1
+    }
+}
+
+clean_restart ${testfile}
+
+if ![runto_main ] then {
+    fail "Can't run to main"
+    return -1
+}
+
+if { $unwinder_called_on_import } {
+    gdb_test_sequence "source ${pyfile}" "import python scripts" {
+	"Python script imported"
+	"py_unwind_maint_ps_unwinder called"
+	"global_unwinder called"
+    }
+    # The unwinders were called above.  We keep the name of the
+    # test the same so that it matches the case below and so that
+    # we have no greater or fewer passes regardless which path
+    # is taken.
+    gdb_test_sequence "frame" "All unwinders enabled" {
+	"#0  main"
+    }
+} else {
+    gdb_test "source ${pyfile}" "Python script imported" \
+	"import python scripts"
+    gdb_test_sequence "frame" "All unwinders enabled" {
+	"py_unwind_maint_ps_unwinder called"
+	"global_unwinder called"
+	"#0  main"
+    }
 }
 
 gdb_test_sequence "info unwinder" "Show all unwinders" {
@@ -57,8 +93,15 @@ gdb_test_sequence "continue" "Unwinders called" {
     "global_unwinder called"
 }
 
-gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" {
-    "1 unwinder disabled"
+if { $unwinder_called_on_import } {
+    gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" {
+	"1 unwinder disabled"
+	"py_unwind_maint_ps_unwinder called"
+    }
+} else {
+    gdb_test_sequence "disable unwinder global .*" "Unwinder disabled" {
+	"1 unwinder disabled"
+    }
 }
 
 gdb_test_sequence "info unwinder" "Show with global unwinder disabled" {

Re: [PATCH 1/4] Distinguish sentinel frame from null frame

[Pedro Alves]

Hi Kevin,

Finally looking at this.

On 09/28/2016 09:49 AM, Kevin Buettner wrote:

> ---
>  gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++-------------------
>  gdb/frame.h |  12 +++++++-
>  2 files changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d3f3056..3ca8ab2 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -43,6 +43,17 @@
>  #include "hashtab.h"
>  #include "valprint.h"
>  
> +/* The sentinel frame is the innermost frame.  It generally does not
> +   have any stack associated with it.  Register values may be directly
> +   determined by inspection with no unwinding necessary.

I think this comment is confusing.  If you look at comments in
sentinel-frame.h|c, you'll find it calling the sentinel 
frame the one past the inner-most instead of being _the_ innermost:

[...]
/* Implement the sentinel frame.  The sentinel frame terminates the
   inner most end of the frame chain.  If unwound, it returns the
   information need to construct an inner-most frame.  */
[...]
/* Pump prime the sentinel frame's cache.  Since this needs the
   REGCACHE provide that here.  */

  /* The sentinel frame is used as a starting point for creating the
     previous (inner most) frame.  That frame's THIS_ID method will be
     called to determine the inner most frame's ID.  Not this one.  */
[...]

> It generally does not have any stack associated with it. 

Right, it should not ever be possible to ask the sentinel
frame for its stack pointer, because we'd have to unwind that
from frame level -2.  :-)

> Register values may be directly determined by inspection with
> no unwinding necessary.

This is true of the current frame.  The reason we have a sentinel
frame is that the unwind machinery always gets the THIS_FRAME's
registers by unwinding them from the next frame.  In order to
keep that working without a special case for frame #0, we stuff
the sentinel frame before (at frame level -1).  That's why the
only useful thing sentinel-frame.c does is implement prev_register.


> +
> +   The sentinel frame is constructed so that the next frame is a pointer
> +   to the sentinel frame itself.
> +
> +   The current frame can be found at sentinel_frame->prev.  */
> +
> +static struct frame_info *sentinel_frame;


> @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
>      fprintf_unfiltered (file, "!stack");
>    else if (id.stack_status == FID_STACK_UNAVAILABLE)
>      fprintf_unfiltered (file, "stack=<unavailable>");
> +  else if (id.stack_status == FID_STACK_SENTINEL)
> +    fprintf_unfiltered (file, "stack=<sentinel>");
>    else
>      fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
>    fprintf_unfiltered (file, ",");
> @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi)
>    if (fi == NULL)
>      return null_frame_id;
>  
> +  if (fi == sentinel_frame)
> +    return sentinel_frame_id;
> +

Why do you need this?  When the sentinel frame is created, it's
given a frame id already:

  /* The sentinel frame has a special ID.  */
  frame->this_id.p = 1;
  frame->this_id.value = sentinel_frame_id;

>    if (!fi->this_id.p)
>      {
>        int stashed;


> -/* Info about the innermost stack frame (contents of FP register).  */
> -
> -static struct frame_info *current_frame;
> -
>  /* Cache for frame addresses already read by gdb.  Valid only while
>     inferior is stopped.  Control variables for the frame cache should
>     be local to this module.  */
> @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
>  struct frame_info *
>  get_current_frame (void)
>  {
> +  struct frame_info *current_frame;
> +  int new_sentinel_p = 0;
> +  /* Set the current frame before computing the frame id, to avoid
> +     recursion inside compute_frame_id, in case the frame's
> +     unwinder decides to do a symbol lookup (which depends on the
> +     selected frame's block).
> +
> +     This call must always succeed.  In particular, nothing inside
> +     get_prev_frame_always_1 should try to unwind from the
> +     sentinel frame, because that could fail/throw, and we always
> +     want to leave with the current frame created and linked in --
> +     we should never end up with the sentinel frame as outermost
> +     frame.  */
> +  current_frame = get_prev_frame_always_1 (sentinel_frame);
> +  gdb_assert (current_frame != NULL);
> +
> +  /* The call to get_frame_id, below, is not necessary when the stack is
> +     well behaved.  But when it's not well behaved, we want to ensure
> +     that the frame id for the current frame is known (stashed) prior to
> +     finding frame id values for any outer frames.
> +
> +     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
> +     in the "backtrace from stop_frame" test when we don't do this here.  */
> +  if (new_sentinel_p)
> +    get_frame_id (current_frame);

I don't understand this.  I applied patch #1 locally with this bit
removed, and I don't see said internal error.

Oh, this only happens when the following patches are applied.

IMO, it's better to move this hunk to the patch that actually
creates the requirement for it, so that we have the whole
logical change in one patch.  Can you elaborate more on
why the test causes the internal "backtrace from stop_frame"
error without this here?

> +
>    return current_frame;
>  }
>  
> @@ -1729,6 +1755,19 @@ get_next_frame (struct frame_info *this_frame)
>      return NULL;
>  }
>  
> +/* Return the frame that THIS_FRAME calls.  If THIS_FRAME is the innermost
> +   frame, we return the sentinel frame.  Thus, NULL will never be returned.  */

This comment is a ambiguous too -- is innermost here talking about the sentinel
frame or the real innermost frame (current frame) ?

> +
> +struct frame_info *
> +get_next_frame_sentinel_okay (struct frame_info *this_frame)
> +{
> +  gdb_assert (this_frame != NULL);
> +
> +  /* This always works, since the sentinel frame refers to itself via the
> +     ``next'' field.  */
> +  return this_frame->next;
> +}


Thanks,
Pedro Alves

Re: [PATCH 2/4] Tweak meaning of VALUE_FRAME_ID

[Pedro Alves]

On 09/28/2016 09:52 AM, Kevin Buettner wrote:
> Tweak meaning of VALUE_FRAME_ID.
> 
> The VALUE_FRAME_ID macro provides access to a member in struct value
> that's used to hold the frame id that's used when determining a
> register's value or when assigning to a register.  The underlying
> member has a long and obscure name.  I won't refer to it here, but
> will simply refer to VALUE_FRAME_ID as if it's the struct value member
> instead of being a convenient macro.
> 
> At the moment, without this patch in place, VALUE_FRAME_ID is set in
> value_of_register_lazy() and several other locations to hold the frame
> id of the frame passed to those functions.
> 
> VALUE_FRAME_ID is used in the lval_register case of
> value_fetch_lazy().  To fetch the register's value, it calls
> get_frame_register_value() which, in turn, calls
> frame_unwind_register_value() with frame->next.
> 
> A python based unwinder may wish to determine the value of a register
> or evaluate an expression containing a register.  When it does this,
> value_fetch_lazy() will be called under some circumstances.  It will
> attempt to determine the frame id associated with the frame passed to
> it.  In so doing, it will end up back in the frame sniffer of the very
> same python unwinder that's attempting to learn the value of a
> register as part of the sniffing operation.  This recursion is not
> desirable.
> 
> As noted above, when value_fetch_lazy() wants to fetch a register's
> value, it does so (indirectly) by unwinding from frame->next.
> 
> With this in mind, a solution suggests itself:  Change VALUE_FRAME_ID
> to hold the frame id associated with the next frame.  Then, when it
> comes time to obtain the value associated with the register, we can
> simply unwind from the frame corresponding to the frame id stored in
> VALUE_FRAME_ID.  This neatly avoids the python unwinder recursion
> problem by changing when the "next" operation occurs.  Instead of the
> "next" operation occuring when the register value is fetched, it
> occurs earlier on when assigning a frame id to VALUE_FRAME_ID.
> (Thanks to Pedro for this suggestion.)
> 

At the very least, The VALUE_FRAME_ID macro's documentation and
the value->frame_id's documentation must be updated to reflect
the new meaning.  Ideally, we'd rename these to VALUE_NEXT_FRAME_ID
and value->frame_id.  But that'd probably be best done as a
separate follow up patch.

> diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
> index 6cd1bc3..515650c 100644
> --- a/gdb/sentinel-frame.c
> +++ b/gdb/sentinel-frame.c
> @@ -51,7 +51,7 @@ sentinel_frame_prev_register (struct frame_info *this_frame,
>    struct value *value;
>  
>    value = regcache_cooked_read_value (cache->regcache, regnum);
> -  VALUE_FRAME_ID (value) = get_frame_id (this_frame);
> +  VALUE_FRAME_ID (value) = get_frame_id (get_next_frame_sentinel_okay (this_frame));

This looks a bit odd.  I think it only works because getting the
next of the sentinel frame returns back the sentinel frame again.

How about we write directly:

  VALUE_FRAME_ID (value) = sentinel_frame_id;

With that, maybe we could make it an internal error to
call get_next_frame_sentinel_okay on the sentinel frame?

Otherwise this look OK to me, but I'd like to understand
better the need for get_frame_id call in the other patch.
Maybe there's something else that we should be doing.

Thanks,
Pedro Alves

Re: [PATCH 3/4] Make gdb.PendingFrame.read_register handle "user" registers

[Pedro Alves]

On 09/28/2016 09:56 AM, Kevin Buettner wrote:
> Make gdb.PendingFrame.read_register handle "user" registers.
> 
> The C function, pending_framepy_read_register(), which implements
> the python interface gdb.PendingFrame.read_register does not handle
> the so called "user" registers like "pc".  An assertion error is
> triggered due to the user registers having numbers larger than or
> equal to gdbarch_num_regs(gdbarch).
> 
> With the VALUE_FRAME_ID tweak in place, the call to
> get_frame_register_value() can simply be replaced by a call to
> value_of_register(), which handles both real registers as well as the
> user registers.
> 
> gdb/ChangeLog:
>     
> 	* python/py-unwind.c (pending_framepy_read_register): Use
> 	value_of_register() instead of get_frame_register_value().

OK.

Thanks,
Pedro Alves

Re: [PATCH 4/4] py-unwind-maint.exp: Allow unwinders to be called during python import

[Pedro Alves]

On 09/28/2016 09:59 AM, Kevin Buettner wrote:
> py-unwind-maint.exp: Allow unwinders to be called during python import

Looks like you have the subject here again.

> 
> In his commit of "Fix PR19927:  Avoid unwinder recursion if sniffer
> uses calls parse_and_eval", Pedro updated the py-unwind-maint.exp
> test to account for different behavior in GDB caused by his changes
> to frame.c.
> 
> This patch changes py-unwind-maint.exp so that either behavior is
> acceptable.
> 
> Regardless of which path is taken, the number of PASSes and the
> names of the tests are the same.

I'd like to understand better the need for this.

BTW, should probably use gdb_test_multiple instead
of send_gdb/gdb_expect.

Thanks,
Pedro Alves

Re: [PATCH 1/4] Distinguish sentinel frame from null frame

[Kevin Buettner]

On Wed, 12 Oct 2016 14:07:13 +0100
Pedro Alves <palves@redhat.com> wrote:

> > @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
> >      fprintf_unfiltered (file, "!stack");
> >    else if (id.stack_status == FID_STACK_UNAVAILABLE)
> >      fprintf_unfiltered (file, "stack=<unavailable>");
> > +  else if (id.stack_status == FID_STACK_SENTINEL)
> > +    fprintf_unfiltered (file, "stack=<sentinel>");
> >    else
> >      fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
> >    fprintf_unfiltered (file, ",");
> > @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi)
> >    if (fi == NULL)
> >      return null_frame_id;
> >  
> > +  if (fi == sentinel_frame)
> > +    return sentinel_frame_id;
> > +  
> 
> Why do you need this?  When the sentinel frame is created, it's
> given a frame id already:

You're right - this isn't needed.  I've removed it from a new
version of the patch which I'm preparing.

> >  /* Cache for frame addresses already read by gdb.  Valid only while
> >     inferior is stopped.  Control variables for the frame cache should
> >     be local to this module.  */
> > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
> >  struct frame_info *
> >  get_current_frame (void)
> >  {
> > +  struct frame_info *current_frame;
> > +  int new_sentinel_p = 0;
> > +  /* Set the current frame before computing the frame id, to avoid
> > +     recursion inside compute_frame_id, in case the frame's
> > +     unwinder decides to do a symbol lookup (which depends on the
> > +     selected frame's block).
> > +
> > +     This call must always succeed.  In particular, nothing inside
> > +     get_prev_frame_always_1 should try to unwind from the
> > +     sentinel frame, because that could fail/throw, and we always
> > +     want to leave with the current frame created and linked in --
> > +     we should never end up with the sentinel frame as outermost
> > +     frame.  */
> > +  current_frame = get_prev_frame_always_1 (sentinel_frame);
> > +  gdb_assert (current_frame != NULL);
> > +
> > +  /* The call to get_frame_id, below, is not necessary when the stack is
> > +     well behaved.  But when it's not well behaved, we want to ensure
> > +     that the frame id for the current frame is known (stashed) prior to
> > +     finding frame id values for any outer frames.
> > +
> > +     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
> > +     in the "backtrace from stop_frame" test when we don't do this here.  */
> > +  if (new_sentinel_p)
> > +    get_frame_id (current_frame);  
> 
> I don't understand this.  I applied patch #1 locally with this bit
> removed, and I don't see said internal error.
> 
> Oh, this only happens when the following patches are applied.
> 
> IMO, it's better to move this hunk to the patch that actually
> creates the requirement for it, so that we have the whole
> logical change in one patch.  Can you elaborate more on
> why the test causes the internal "backtrace from stop_frame"
> error without this here?

Here's the portion of the log file showing the failure/internal error:

(gdb) break stop_frame
Breakpoint 1 at 0x40059a: file dw2-dup-frame.c, line 22.
(gdb) run 
Starting program: /mesquite2/sourceware-git/mesquite-native-python-unwind-2-split/bld/gdb/testsuite/outputs/gdb.dwarf2/dw2-dup-frame/dw2-dup-frame 

Breakpoint 1, stop_frame () at dw2-dup-frame.c:22
22	}
(gdb) bt
/ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.dwarf2/dw2-dup-frame.exp: backtrace from stop_frame (GDB internal error)

Here's a partial backtrace from the internal error, showing the frames
which I think are relevant, plus several extra to provide context:

#0  internal_error (
    file=0x932b98 "/ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c", line=544, 
    fmt=0x932b20 "%s: Assertion `%s' failed.")
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/common/errors.c:54
#1  0x000000000072207e in get_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:544
#2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390
#3  0x00000000004ef5be in bootstrap_python_frame_filters (frame=0xe5a760, 
    frame_low=0, frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1453
#4  0x00000000004ef7a9 in gdbpy_apply_frame_filter (
    extlang=0x8857e0 <extension_language_python>, frame=0xe5a760, flags=7, 
    args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-framefilter.c:1548
#5  0x00000000005f2c5a in apply_ext_lang_frame_filter (frame=0xe5a760, 
    flags=7, args_type=CLI_SCALAR_VALUES, out=0xf6def0, frame_low=0, 
    frame_high=-1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/extension.c:572
#6  0x00000000005ea896 in backtrace_command_1 (count_exp=0x0, show_locals=0, 
    no_filters=0, from_tty=1)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/stack.c:1834

Examination of the code in frame_info_to_frame_object(), which is in
python/py-frame.c, is key to understanding this problem:

      if (get_prev_frame (frame) == NULL
	  && get_frame_unwind_stop_reason (frame) != UNWIND_NO_REASON
	  && get_next_frame (frame) != NULL)
	{
	  frame_obj->frame_id = get_frame_id (get_next_frame (frame));
	  frame_obj->frame_id_is_next = 1;
	}
      else
	{
	  frame_obj->frame_id = get_frame_id (frame);
	  frame_obj->frame_id_is_next = 0;
	}

I will first note that the frame id for frame has not been computed yet.  (This
was verified by placing a breakpoint on compute_frame_id().)

The call to get_prev_frame() causes the the frame id to (eventually) be
computed for the previous frame.  Here's a backtrace showing how we
get there:

#0  compute_frame_id (fi=0x10e2810)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496
#1  0x0000000000724a67 in get_prev_frame_if_no_cycle (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:1871
#2  0x0000000000725136 in get_prev_frame_always_1 (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2045
#3  0x000000000072516b in get_prev_frame_always (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2061
#4  0x000000000072570f in get_prev_frame (this_frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:2303
#5  0x00000000004eb471 in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:381

For this particular case, we end up in the else clause of the code above
which calls get_frame_id (frame).  It's at this point that the frame id
for frame is computed.  Again, here's a backtrace:

#0  compute_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:496
#1  0x000000000072203d in get_frame_id (fi=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/frame.c:539
#2  0x00000000004eb50d in frame_info_to_frame_object (frame=0xe5a760)
    at /ironwood1/sourceware-git/mesquite-native-python-unwind-2-split/bld/../../binutils-gdb/gdb/python/py-frame.c:390

The test in question, dw2-dup-frame.exp, deliberately creates a broken
(cyclic) stack.  So, in this instance, the frame id for the prev
`frame' will be the same as that for `frame'.  But that particular
frame id ended up in the stash during the previous frame operation. 
When, just a few lines later, we compute the frame id for `frame', the
id in question is already in the stash, thus triggering the assertion
failure.

An alternate "fix" (or perhaps band-aid) for this problem is as
follows:

diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 6bdac08..a1d305e 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -375,6 +375,10 @@ frame_info_to_frame_object (struct frame_info *frame)
   TRY
     {
 
+      /* Avoid case where the frame id for previous frame is computed
+        before that for the frame under consideration.  */
+      (void) get_frame_id (frame);
+
       /* Try to get the previous frame, to determine if this is the last frame
 	 in a corrupt stack.  If so, we need to store the frame_id of the next
 	 frame and not of this one (which is possibly invalid).  */

I'm not especially fond of this solution though.  If it's necessary to
create frame ids in a certain sequence, this ordering should be
imposed elsewhere.  Also, I've caught just one case here.  There may
also be similar problems lurking which we haven't caught yet.

I hope you that you (or someone else) can suggest a better solution...

Kevin

Re: [PATCH 1/4] Distinguish sentinel frame from null frame

[Kevin Buettner]

On Wed, 26 Oct 2016 00:21:53 -0700
Kevin Buettner <kevin@buettner.to> wrote:

> > >  /* Cache for frame addresses already read by gdb.  Valid only while
> > >     inferior is stopped.  Control variables for the frame cache should
> > >     be local to this module.  */
> > > @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
> > >  struct frame_info *
> > >  get_current_frame (void)
> > >  {
> > > +  struct frame_info *current_frame;
> > > +  int new_sentinel_p = 0;
> > > +  /* Set the current frame before computing the frame id, to avoid
> > > +     recursion inside compute_frame_id, in case the frame's
> > > +     unwinder decides to do a symbol lookup (which depends on the
> > > +     selected frame's block).
> > > +
> > > +     This call must always succeed.  In particular, nothing inside
> > > +     get_prev_frame_always_1 should try to unwind from the
> > > +     sentinel frame, because that could fail/throw, and we always
> > > +     want to leave with the current frame created and linked in --
> > > +     we should never end up with the sentinel frame as outermost
> > > +     frame.  */
> > > +  current_frame = get_prev_frame_always_1 (sentinel_frame);
> > > +  gdb_assert (current_frame != NULL);
> > > +
> > > +  /* The call to get_frame_id, below, is not necessary when the stack is
> > > +     well behaved.  But when it's not well behaved, we want to ensure
> > > +     that the frame id for the current frame is known (stashed) prior to
> > > +     finding frame id values for any outer frames.
> > > +
> > > +     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
> > > +     in the "backtrace from stop_frame" test when we don't do this here.  */
> > > +  if (new_sentinel_p)
> > > +    get_frame_id (current_frame);    
[...]
> An alternate "fix" (or perhaps band-aid) for this problem is as
> follows:
> 
> diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
> index 6bdac08..a1d305e 100644
> --- a/gdb/python/py-frame.c
> +++ b/gdb/python/py-frame.c
> @@ -375,6 +375,10 @@ frame_info_to_frame_object (struct frame_info *frame)
>    TRY
>      {
>  
> +      /* Avoid case where the frame id for previous frame is computed
> +        before that for the frame under consideration.  */
> +      (void) get_frame_id (frame);
> +
>        /* Try to get the previous frame, to determine if this is the last frame
>  	 in a corrupt stack.  If so, we need to store the frame_id of the next
>  	 frame and not of this one (which is possibly invalid).  */
> 
> I'm not especially fond of this solution though.  If it's necessary to
> create frame ids in a certain sequence, this ordering should be
> imposed elsewhere.  Also, I've caught just one case here.  There may
> also be similar problems lurking which we haven't caught yet.
> 
> I hope you that you (or someone else) can suggest a better solution...

FYI, I found a solution that I like better.  I plan to submit this
patch - or something like it - as part of a new patch series for
preventing recursion in python based unwinders.

So...  no need to review it now, but (I hope that) there's also no
need to spend time looking for the "better solution" requested in my
earlier post.

diff --git a/gdb/frame.c b/gdb/frame.c
index bc42674..b19e782 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2217,6 +2217,18 @@ get_prev_frame (struct frame_info *this_frame)
      something should be calling get_selected_frame() or
      get_current_frame().  */
   gdb_assert (this_frame != NULL);
+  
+  /* If this_frame is the current frame, then compute and stash
+     its frame id prior to fetching and computing the frame id of the
+     previous frame.  Otherwise, the cycle detection code in
+     get_prev_frame_if_no_cycle() will not work correctly.  When
+     get_frame_id() is called later on, an assertion error will
+     be triggered in the event of a cycle between the current
+     frame and its previous frame.  */
+     
+  if (this_frame->level == 0)
+    get_frame_id (this_frame);
+
   frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc);
 
   /* tausq/2004-12-07: Dummy frames are skipped because it doesn't make much