Re: [rfc, frame] Always check for unsaved PC

[Daniel Jacobowitz <drow@false.org>]

On Sun, Aug 20, 2006 at 04:27:49PM +0200, Mark Kettenis wrote:
> > Date: Sat, 19 Aug 2006 12:11:39 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > This patch, combined with my previous two frame patches for today,
> > fixes the infinite backtrace Olav Zarges reported on gdb@.

Actually, the second patch (the most controversial of the bunch) wasn't
necessary for that specific problem.  So until someday I have time to
revisit that one, let's just consider this one on its own...

There wasn't any objection in the followup discussion to this patch.
Accordingly, I have committed it, along with a small fix (Michael
Snyder noticed that I had the wrong expectations for get_next_frame's
return value, it never returns the sentinel frame).

> > There's an associated FIXME; I discovered that three targets don't do
> > it this way.  That doesn't break this patch, but it's inconsistent, and
> > does a bit of extra computation.  So if there's general agreement that
> > this patch is a good idea, I'll go through and fix them, and try to
> > document more clearly that you aren't supposed to do it that way.
> > Then I can remove the FIXME.
> 
> I'll have a go at alpha if you don't mind.

That refered to the FIXME above frame_register_unwind_location.  Mark,
would you still like to do this, or shall I do all three affected
targets?  It should be fairly mechanical.

-- 
Daniel Jacobowitz
CodeSourcery

2006-11-10  Daniel Jacobowitz  <dan@codesourcery.com>

	* frame.c (frame_register_unwind_location): New function.
	(get_prev_frame_1): Check for UNWIND_NO_SAVED_PC.
	(frame_stop_reason_string): Handle UNWIND_NO_SAVED_PC.
	* frame.h (enum unwind_stop_reason): Add UNWIND_NO_SAVED_PC.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.214
diff -u -p -r1.214 frame.c
--- frame.c	18 Oct 2006 19:52:05 -0000	1.214
+++ frame.c	10 Nov 2006 20:06:05 -0000
@@ -1023,6 +1023,36 @@ reinit_frame_cache (void)
     }
 }
 
+/* Find where a register is saved (in memory or another register).
+   The result of frame_register_unwind is just where it is saved
+   relative to this particular frame.
+
+   FIXME: alpha, m32c, and h8300 actually do the transitive operation
+   themselves.  */
+
+static void
+frame_register_unwind_location (struct frame_info *this_frame, int regnum,
+				int *optimizedp, enum lval_type *lvalp,
+				CORE_ADDR *addrp, int *realnump)
+{
+  gdb_assert (this_frame == NULL || this_frame->level >= 0);
+
+  while (this_frame != NULL)
+    {
+      frame_register_unwind (this_frame, regnum, optimizedp, lvalp,
+			     addrp, realnump, NULL);
+
+      if (*optimizedp)
+	break;
+
+      if (*lvalp != lval_register)
+	break;
+
+      regnum = *realnump;
+      this_frame = get_next_frame (this_frame);
+    }
+}
+
 /* Return a "struct frame_info" corresponding to the frame that called
    THIS_FRAME.  Returns NULL if there is no such frame.
 
@@ -1111,6 +1141,42 @@ get_prev_frame_1 (struct frame_info *thi
       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
+     functions can't share a register save slot for the PC.  This can
+     happen when the prologue analyzer finds a stack adjustment, but
+     no PC save.  This check does assume that the "PC register" is
+     roughly a traditional PC, even if the gdbarch_unwind_pc method
+     frobs it.  */
+  if (this_frame->level > 0
+      && get_frame_type (this_frame) == NORMAL_FRAME
+      && get_frame_type (this_frame->next) == NORMAL_FRAME)
+    {
+      int optimized, realnum;
+      enum lval_type lval, nlval;
+      CORE_ADDR addr, naddr;
+
+      frame_register_unwind_location (this_frame, PC_REGNUM, &optimized,
+				      &lval, &addr, &realnum);
+      frame_register_unwind_location (get_next_frame (this_frame), PC_REGNUM,
+				      &optimized, &nlval, &naddr, &realnum);
+
+      if (lval == lval_memory && lval == nlval && addr == naddr)
+	{
+	  if (frame_debug)
+	    {
+	      fprintf_unfiltered (gdb_stdlog, "-> ");
+	      fprint_frame (gdb_stdlog, NULL);
+	      fprintf_unfiltered (gdb_stdlog, " // no saved PC }\n");
+	    }
+
+	  this_frame->stop_reason = UNWIND_NO_SAVED_PC;
+	  this_frame->prev = NULL;
+	  return NULL;
+	}
+    }
+
   /* Allocate the new frame but do not wire it in to the frame chain.
      Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along
      frame->next to pull some fancy tricks (of course such code is, by
@@ -1611,6 +1677,9 @@ frame_stop_reason_string (enum unwind_st
     case UNWIND_SAME_ID:
       return _("previous frame identical to this frame (corrupt stack?)");
 
+    case UNWIND_NO_SAVED_PC:
+      return _("frame did not save the PC");
+
     case UNWIND_NO_REASON:
     case UNWIND_FIRST_ERROR:
     default:
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.149
diff -u -p -r1.149 frame.h
--- frame.h	18 Oct 2006 19:52:05 -0000	1.149
+++ frame.h	10 Nov 2006 20:06:05 -0000
@@ -428,6 +428,10 @@ enum unwind_stop_reason
        this is a sign of unwinder failure.  It could also indicate
        stack corruption.  */
     UNWIND_SAME_ID,
+
+    /* The frame unwinder didn't find any saved PC, but we needed
+       one to unwind further.  */
+    UNWIND_NO_SAVED_PC,
   };
 
 /* Return the reason why we can't unwind past this frame.  */