Re: [patch/rfc/rfa*] Only compare against dummy frame tos when saved

[Andrew Cagney <ac131313@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]

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



[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2826 bytes --]

2002-09-25  Andrew Cagney  <ac131313@redhat.com>

	* blockframe.c (generic_find_dummy_frame): Rewrite.  Only test
	against TOP when TOP was explictly set.
	(generic_push_dummy_frame): Set TOP to zero.

Index: blockframe.c
===================================================================
RCS file: /cvs/src/src/gdb/blockframe.c,v
retrieving revision 1.40
diff -u -r1.40 blockframe.c
--- blockframe.c	17 Sep 2002 20:42:01 -0000	1.40
+++ blockframe.c	26 Sep 2002 16:19:30 -0000
@@ -1151,8 +1151,8 @@
 
 /* Function: find_dummy_frame(pc, fp, sp)
 
-   Search the stack of dummy frames for one matching the given PC, FP
-   and SP.  Unlike PC_IN_CALL_DUMMY, this function doesn't need to
+   Search the stack of dummy frames for one matching the given PC and
+   FP/SP.  Unlike PC_IN_CALL_DUMMY, this function doesn't need to
    adjust for DECR_PC_AFTER_BREAK.  This is because it is only legal
    to call this function after the PC has been adjusted.  */
 
@@ -1163,12 +1163,37 @@
 
   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 */
+    {
+      /* Does the PC fall within the dummy frame's breakpoint
+         instruction.  If not, discard this one.  */
+      if (!(pc >= dummyframe->call_lo && pc < dummyframe->call_hi))
+	continue;
+      /* Does the FP match?  */
+      if (dummyframe->top != 0)
+	{
+	  /* If the target architecture explicitly saved the
+	     top-of-stack before the inferior function call, assume
+	     that that same architecture will always pass in an FP
+	     (frame base) value that eactly matches that saved TOS.
+	     Don't check the saved SP and SP as they can lead to false
+	     hits.  */
+	  if (fp != dummyframe->top)
+	    continue;
+	}
+      else
+	{
+	  /* An older target that hasn't explicitly or implicitly
+             saved the dummy frame's top-of-stack.  Try matching the
+             FP against the saved SP and FP.  NOTE: If you're trying
+             to fix a problem with GDB not correctly finding a dummy
+             frame, check the comments that go with FRAME_ALIGN() and
+             SAVE_DUMMY_FRAME_TOS().  */
+	  if (fp != dummyframe->fp && fp != dummyframe->sp)
+	    continue;
+	}
+      /* The FP matches this dummy frame.  */
       return dummyframe->regcache;
+    }
 
   return 0;
 }
@@ -1265,7 +1290,7 @@
 
   dummy_frame->pc = read_pc ();
   dummy_frame->sp = read_sp ();
-  dummy_frame->top = dummy_frame->sp;
+  dummy_frame->top = 0;
   dummy_frame->fp = fp;
   regcache_cpy (dummy_frame->regcache, current_regcache);
   dummy_frame->next = dummy_frame_stack;