> 
> I think this change will be okay for ia64 so long as the following
> FIXME comment in ia64-tdep.c is fixed:
> 
>   /* FIXME: This doesn't belong here!  Instead, SAVE_DUMMY_FRAME_TOS needs
>      to be defined to call generic_save_dummy_frame_tos().  But at the
>      time of this writing, SAVE_DUMMY_FRAME_TOS wasn't gdbarch'd, so
>      I chose to put this call here instead of using the old mechanisms. 
>      Once SAVE_DUMMY_FRAME_TOS is gdbarch'd, all we need to do is add the
>      line
> 
> 	set_gdbarch_save_dummy_frame_tos (gdbarch, generic_save_dummy_frame_tos);
> 
>      to ia64_gdbarch_init() and remove the line below. */
> 
> At the moment, ia64 depends upon ->top being set, but your change will
> disable the appropriate test because ia64-tdep.c doesn't (correctly) set
> the SAVE_DUMMY_FRAME_TOS method.
> 
> I'll try to find a moment or two to give it a try...

No need.  I skipped ``plan b'' and went straight onto ``plan c''.

The attached revised patch modifies generic_find_dummy_frame() so that, 
as before, it only tests TOP when it was set.  However, this time it 
uses the value of TOP instead of find_dummy_frame_p().  I think this is 
more robust since, as with the ia64, it allows an architecture to 
explicitly set TOS (rather than rely on hand_function_call() to set it).

I should note that, per the rs6000, this change is likely to flush out 
some latent architecture bugs and, as a consequence, cause regressions 
:-(  Sigh!

There is going to be a trade off here .... :-/

Andrew



The original description was:

> The generic code for finding a previously saved dummy frame, given the PC and ``frame'', looks like:
> 
>  static struct regcache *
> generic_find_dummy_frame (CORE_ADDR pc, CORE_ADDR fp)
> {
>   struct dummy_frame *dummyframe;
> 
>   for (dummyframe = dummy_frame_stack; dummyframe != NULL;
>        dummyframe = dummyframe->next)
>     if ((pc >= dummyframe->call_lo && pc < dummyframe->call_hi)
>         && (fp == dummyframe->fp
>             || fp == dummyframe->sp
>             || fp == dummyframe->top))
>       /* The frame in question lies between the saved fp and sp, inclusive */
>       return dummyframe->regcache;
> 
>   return 0;
> }
> 
> I think it is trying to handle two cases:
> 
> 1. Where the FP passed in is the dummy frames true base and should match SP of the previous frame or FP of that frame.
> 
> 2. Where the FP passed in is the frame base from the next inner most frame (returned by frame_chain()) it should match the saved top-of-stack value (using SAVE_DUMMY_FRAME_TOS()).
> 
> If a target is somehow capable of unwinding through the dummy frame back to its base then #1 is used.  If, however, it isn't possible to unwind back through a [generic] dummy frame, then targets use #2.  Case #2 is easy to spot because the frame_chain code starts with:
> 
>     if (PC_IN_CALL_DUMMY(frame_saved_pc(frame)...)
>       return frame->frame;
> 
> Unfortunatly, the test is too generous and a problem occures when using #2 while trying to unwind through frameless functions :-(
> 
>     0: innermost()  fp=30 top=40
>     1: <dummy frame> fp=20 top=30
>     2: frameless() fp=20 top=20
>     3: <dummy frame> fp=10 top=20
> 
> When trying to unwind 2:frameless(), GDB calls frame_saved_pc()=<dummy frame> and frame_chain()=20.  These two values are then used to find saved the dummy frame on the dummy_frame_stack.  The above test is so generous, though, that it incorrectly matches the first dummy frame (#1, 20 == 1:fp) instead of (#3 20 == 3:top).  This will send GDB into a backtrace loop #0, #1, #2, #1, #2, #1 #2 ....
> 
> The attached patch fixes this problem by being more careful about which of the above tests is used and when.  the ``fp == dummyframe->top'' test is used IFF top was saved.  The other tests IFF top wasn't saved.
> 
> This change does, however, have the potential to break break every target that sets save_dummy_frame_tos().  Going through the list of targets (and treating any with a frame_chain() like the above as ok):
> 
> cris-tdep.c:  ok
> ia64-tdep.c:  ok
> mcore-tdep.c:  *** I suspect it doesn't but should
> mn10300-tdep.c:  *** I suspect it doesn't but should
> rs6000-tdep.c:  ok
> s390-tdep.c:  ok
> xstormy16-tdep.c:  ok (I think).
> 
> I think, for the two *** cases, the code can be tweaked to have frame_chain() return frame->frame.
> 
> Thoughts?  Ok for the targets with maintainers?
> 
> I'll need this or something similar to get MIPS generic dummy frames working.
> 
> Oh, and I have a plan B:  Add an extra gdbarch flag that, when set, enables the more strict tests.
> 
> Andrew