[rfc, frame] Always check for unsaved PC

[Daniel Jacobowitz <drow at false dot org>]

This patch, combined with my previous two frame patches for today,
fixes the infinite backtrace Olav Zarges reported on gdb@.

Infinite backtraces are a funny phenomenon, which I've seen on a lot of
platforms in the time I've been working on GDB.  Consider a function
prologue which creates a stack frame, but does not store the PC into
the frame - for instance, because the compiler knows it will never
return.  Then it calls some other function, clobbering the return
register.  A hapless prologue unwinder may report "the stack frame size
is 200 bytes, and the saved PC still lives in LR" not realizing that
LR's been clobbered.  If there were no stack frame, this would trigger
the duplicate frame ID check, but there is; the function remains the
same but the CFA moves continually down, and the frame repeats
endlessly.

Here's a simple way to detect this.  Suppose we have two normal frames
in a row.  Unwind PC_REGNUM from both of them.  Two functions having
the same return address isn't a problem.  But two functions sharing a
stack slot for the return address is a big problem; that never happens
normally, since the first one would pop it.  So this indicates that the
older frame didn't actually save it.

This isn't a perfect check, since it relies on PC_REGNUM, as opposed to
gdbarch_unwind_pc and frame_unwind's prev_pc, but it's still safe; if
the PC_REGNUM does not map onto a real and saved register, then it
won't end up with lval_memory.

There's one wart in this patch.  frame_register_unwind doesn't tell us
where a register was ultimately stored; it recurses for the register
value, but fills in *optimizedp, *lvalp, *realnump, *addrp for where
_this frame_ saved it.  Which is reasonable and useful behavior.  So,
we need to iterate ourselves in order to find the final location.

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.

Performance-wise: for frames past the current frame, this causes us to
do an additional two register unwinds.  That's not too expensive, and
we're usually backtracing anyway by that point.  I couldn't measure a
performance impact.  I don't think we really cache this information,
but if this code ever becomes a performance issue somehow, that's
how it should be fixed: per-frame unwound register caching.

Any thoughts on this patch?

-- 
Daniel Jacobowitz
CodeSourcery

2006-08-19  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.

---
 gdb/frame.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/frame.h |    4 +++
 2 files changed, 71 insertions(+)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2006-08-19 11:33:33.000000000 -0400
+++ src/gdb/frame.c	2006-08-19 11:38:43.000000000 -0400
@@ -1023,6 +1023,34 @@ 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)
+{
+  while (this_frame->level >= 0)
+    {
+      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.
 
@@ -1077,6 +1105,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
@@ -1623,6 +1687,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: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2006-08-19 11:29:33.000000000 -0400
+++ src/gdb/frame.h	2006-08-19 11:35:41.000000000 -0400
@@ -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.  */