[PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)

[Pedro Alves <palves@redhat.com>]

On 11/19/2013 08:24 PM, Tom Tromey wrote:
> Pedro> I don't think so, because get_prev_frame_1 would not link in
> Pedro> the dup frame.  The loop in question would never see it.
> 
> Pedro> Hmm, I think one of us is missing something.
> 
> Haha, yeah, that usually means me :-)

Haha, I wish.  :-)

> 
> No worries.  I think I understand this bit now.
> 
> Pedro> So the bad loop can only ever happen (outside the unwinder code)
> Pedro> if we ever let outselves get in the dup frame_id situation:
> 
>>> #4  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> #5  0x0000007fb7f0956c in clone () from /lib64/libc.so.6
>>> Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> 
> Pedro> At least, I'm not seeing any other way.
> 
> Yes, I see now.

OK, here's the same in patch form.

> 
> Really not looking forward to writing the test.

Yeah, me neither. :-P

---------
Subject: Don't let two frames with the same id end up in the frame chain.

The UNWIND_SAME_ID check is done between THIS_FRAME and the next
frame.  But at this point, it's already too late -- we ended up with
two frames with the same ID in the frame chain.  Each frame having its
own ID is an invariant assumed throughout GDB.  So this patch applies
the UNWIND_SAME_ID detection earlier, right after the previous frame
is unwond, discarding the dup frame if a cycle is detected.

gdb/
2013-11-20  Pedro Alves  <palves@redhat.com>

	* frame.c (get_prev_frame_1): Do the UNWIND_SAME_ID check between
	this frame and the new previous frame, not between this frame and
	the next frame.
---

 gdb/frame.c |   43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 63f20d5..535a5a6 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1666,6 +1666,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
 {
   struct frame_id this_id;
   struct gdbarch *gdbarch;
+  struct frame_info *prev_frame;
 
   gdb_assert (this_frame != NULL);
   gdbarch = get_frame_arch (this_frame);
@@ -1767,22 +1768,6 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  /* Check that this and the next frame are not identical.  If they
-     are, there is most likely a stack cycle.  As with the inner-than
-     test above, avoid comparing the inner-most and sentinel frames.  */
-  if (this_frame->level > 0
-      && frame_id_eq (this_id, get_frame_id (this_frame->next)))
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
-	}
-      this_frame->stop_reason = UNWIND_SAME_ID;
-      return NULL;
-    }
-
   /* Check that this and the next frame do not unwind the PC register
      to the same memory location.  If they do, then even though they
      have different frame IDs, the new frame will be bogus; two
@@ -1830,7 +1815,31 @@ get_prev_frame_1 (struct frame_info *this_frame)
 	}
     }
 
-  return get_prev_frame_raw (this_frame);
+  prev_frame = get_prev_frame_raw (this_frame);
+
+  /* Check that this and the prev frame are not identical.  If they
+     are, there is most likely a stack cycle.  Unlike the tests above,
+     we do this right after creating the prev frame, to avoid ever
+     ending up with two frames with the same id in the frame
+     chain.  */
+  if (prev_frame != NULL
+      && frame_id_eq (get_frame_id (prev_frame),
+		      get_frame_id (this_frame)))
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+	}
+      this_frame->stop_reason = UNWIND_SAME_ID;
+      /* Unlink.  */
+      prev_frame->next = NULL;
+      this_frame->prev = NULL;
+      return NULL;
+    }
+
+  return prev_frame;
 }
 
 /* Construct a new "struct frame_info" and link it previous to