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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> 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@.
> 
> 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.

Yeah, we're probably never going to be able to rule this case out
completely for the prologues-scanning unwinders.

> 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 doesn't fly for architectures where the return address is
computed though, like on sparc.

> 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.

Indeed on sparc unwinding PC_REGNUM will always end up with bot_lval.

> 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.

Don't we have that code already for evaluating variables?  Can't that
code be re-used?

> 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.

> 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?

I think the error message should be clear about which frame did not
save the PC.

Oh, and this patch doesn't really depend on your "backtrace stop
reasons" patch does it?  The code could just call error().

Mark

> 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.