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

[Daniel Jacobowitz <drow at false dot org>]

On Sun, Aug 20, 2006 at 04:27:49PM +0200, Mark Kettenis wrote:
> > 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.

Right.  I don't think we need to make this generic check work
everywhere; what I'd like to do instead would be to allow the prologue
unwinder to return its own "this is why I think unwinding is done"
reason.  The check doesn't really apply to SPARC in the same form.
However, a frameless normal frame where the next frame is also a normal
frame has the same problem, and could use the same unwind stop reason.

I see that gdb doesn't use dwarf2 unwind tables on sparc.  Does GCC
generate them?  How does GCC deal with the extra +8?  It looks like
it doesn't; it uses %o7 to handle unwinding and just looks at the
location of the call site.

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

We do have some similar code, but it can't readily be reused here.  I
want the frame prev_register to keep returning the non-transitive
values; that's how we generate a useful "info frame" which says which
registers were saved in this frame, instead of which registers were
saved in any 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.

Sure.  It's simple: look for calls to frame_register and
frame_register_unwind in the tdep files (alpha-tdep.c and
alpha-mdebug-tdep.c), and compare them to the more common
version which uses frame_unwind_register (I hate that naming...).
The alpha version has the next frame fill in *lvalp and
always recurses.  The i386 version has the current frame fill in *lvalp
and only recurses if we need the value.

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

Could you give me an example of how to make it clearer?  Here's what
it looks like at the moment, IIRC (mocked up, I don't have easy access
to the test right now):

#0  0xffffe410 in __kernel_vsyscall ()
#1  0xb7e441cb in poll () from /lib/tls/i686/cmov/libc.so.6
#2  0x0810c713 in gdb_do_one_event ()
Backtrace stopped: frame did not save the PC

And:

(gdb) i frame 2
Stack frame at 0xbf8842c0:
 eip = 0x810c713 in gdb_do_one_event; saved eip 0x8109a43
 Stops backtrace: frame did not save the PC
 called by frame at 0xbf8842f0
 Arglist at 0xbf8842b8, args:

Maybe "current frame" would be clearer?

-- 
Daniel Jacobowitz
CodeSourcery