Re: [rfc] Unwind the ARM CPSR

[Daniel Jacobowitz <drow at false dot org>]

Hi Mark, and thanks for the comments.  I've been working on
this again (all day...), so it's fresh in my mind.

On Wed, Oct 17, 2007 at 12:01:03AM +0200, Mark Kettenis wrote:
> > I can see why you'd like some feedback. This (amazing piece of) code
> > has always been a bit difficult for me to grasp in its entirety...
> > I hope others will take some time to think about it too, because
> > I don't feel sufficiently proficient to provide a confident answer.
> > 
> > FWIW, I don't see how we could end up breaking something with your
> > approach. As it turns out, there is a comment that says about
> > the prev_register method:
> > 
> >    Why not pass in THIS_FRAME?  By passing in NEXT frame and THIS
> >    cache, the supplied parameters are consistent with the sibling
> >    function THIS_ID.
> > 
> > So it sounds like the author wasn't seeing any issue with that
> > either.
> 
> I wouldn't be so sure.  Andrew Cagney put quite a bit of thought in
> the interface, and while it sometimes appears to be strange on first
> sight, it has always proven to be the right choice.

I agree that he put a lot of thought into it, but some of his
decisions were forced by the path to get GDB from where it was then
(ew) to where it is now (much nicer).  I looked into the history of
this today.

The original version of frame-unwind.h (excluding development
branches) was on 2003-01-18, and passed the current frame.  It was
changed to pass the next frame on 2003-03-17 as the result
of Andrew's offbyone branch.  Here's the patch:

  http://sourceware.org/ml/gdb-patches/2003-03/msg00339.html

The problem was a mismatch between which cache the this_id function
was passed and which cache the prev_register function for the same
frame was passed.  I rediscovered this problem today and I can
see why it was such a pain to fix at the time - it rather
ruined my evening.

But the most interesting thing I found in the archives was this:

  http://sourceware.org/ml/gdb-patches/2003-03/msg00365.html

That, a couple of days later, was the first step in not calculating
the frame ID until we needed it.  Before that happened, Andrew's fix
for the off-by-one bug was the only workable solution: the previous
frame wasn't usable until after its ID had been calculated.  After the
frame ID was calculated lazily, though, getting the previous frame
became a cheap operation and we could postpone ID checks until we
needed them.

So the problem that Andrew originally solved by passing the NEXT frame
to the unwinder is gone now.

Just for kicks I took a look at Frysk, whose unwinder he's working on
as we speak.  A Frame object has a getFrameIdentifier method
(equivalent to ->this_id taking this_frame), and a getRegister method.
I can't tell if that's prev_register taking this_frame or my_register
taking this_frame.

> I think it is a very bad idea.  It will take ages before all targets
> are converted, and having two functions to do the same thing will be
> even more confusing.

Yes, that's true; it will be confusing.  I've taken the opportunity to
work on a related project at the same time though and so far I like
the shape of the result:

typedef struct value * (frame_prev_register_value_ftype)
  (struct frame_info *this_frame, void **this_prologue_cache,
   int prev_regnum);

The mental exercise of having only the NEXT frame prevents some stupid
things.  But it also prevents some smart things, and it makes
unwinders (in my personal opinion only, of course) much harder to
write.  This is probably the subtlest part of GDB; I like anything
that makes it more obvious how it works.

I'll have a concrete example finished in another day or so, and
then I'll post a proper RFC for the frame changes.

On Wed, Oct 17, 2007 at 12:20:49AM +0200, Mark Kettenis wrote:
> > To handle that, I added a new DWARF2_FRAME_REG_FN which supplies a
> > default unwinder.  For the PC, I originally added
> > DWARF2_FRAME_REG_RA_MASK instead to mask out the low bit, but
> > that wasn't enough for the CPSR.  And I discovered that
> > there is no easy way, given the arguments to an unwinder's
> > prev_register method, to request the unwound value of another
> > register from the same frame!  That's why the patch below had
> > to make dwarf2_frame_prev_register global, and pass its opaque
> > THIS_CACHE to the function supplied by DWARF2_FRAME_REG_FN.
> 
> Two remarks:
> 
> 1. Isn't this function always going to return a plain value?  That
>    means we can probably simplify the register.

Not pass all those extra arguments, you mean?  Yes, maybe we can.  It
did always return a plain value in my case and we could change it
later if more generality was needed.

> 2. Perhaps the function should get passed the unwinder struct, such
>    that it can use the function pointer.

Yeah, that's a nice alternative to making dwarf2_frame_prev_register
global.  It means we need to pass the frame, the unwinder, and the
cache, though - that still suggests to me that there is a simpler and
righter answer.

> > During a prev_register method there are three frames of interest.  The
> > NEXT frame is passed; the CURRENT frame's cache is passed and some
> > knowledge of the CURRENT frame is implicit in the called function; and
> > the PREVIOUS frame's register value is calculated.  Given a frame
> > there are functions to get its register values or the values of the
> > previous frame; since we start with NEXT that means we can use generic
> > frame interfaces to get only NEXT or CURRENT registers.  But to
> > calculate PREVIOUS's CPSR we need PREVIOUS's LR so we have to call
> > dwarf2_frame_prev_register directly.
> 
> I think this means that making the unwinder reconstruct CPSR may not
> be the right approach.  Isn't it better to determine thumbness based
> on the unwound LR?  That should work for all frames, except the
> topmost.  But for the topmost frame we should be able to trust CPSR.

I implemented that first, actually.  In that case I need to check the
types (get_frame_type) of various frames, since the frame before a
signal frame or dummy frame are also "topmost".  And I ended up with
some assumptions that violated the abstraction boundaries of the
unwinder, e.g., that signal and dummy frames would always have a saved
CPSR.

It seemed very inelegant to me; the right "unwound" value of the CPSR
depends on how we unwound, so why not have the unwinder supply it?

> I also see that you treat unwinding the PC specially in the unwinders,
> to strip off bits.  But isn't that why we have the unwind_pc gdbarch
> method?

The failing testcase that started me down this rabbit hole was
return.exp, which uses frame_pop.  The gdbarch unwind_pc method is
always used when core GDB wants a PC or resume address, but it's not
used when we want to restore the value of a register (which happens to
be register number PC_REGNUM).  And something needs to clean up the
saved CPSR value when popping, for the same reason - you need
interworking (mixed ARM and Thumb) for that to cause a failure
though so the current testsuite never triggers it.

-- 
Daniel Jacobowitz
CodeSourcery