[patch, rfc, 6?] Enable identical frame sanity check

[patch, rfc, 6?] Enable identical frame sanity check

[Andrew Cagney]

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

Hello,

This patch enables the `is the prev frame identical to this frame' check 
in get_prev_frame().  I forgot to enable it when I switched the frame ID 
code from using the frame's PC to using the frame's function.  Noticed 
this when reviewing Daniel's Arm frame update.

It should probably go straight into the mainline (tested on i386 and d10v).

Thoughts on 6?

Andrew

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

2003-07-02  Andrew Cagney  <cagney@redhat.com>

	* frame.c (get_prev_frame): Enable check for identical frames.
	Update comments.  Update error messages.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.128
diff -u -r1.128 frame.c
--- frame.c	29 Jun 2003 13:27:26 -0000	1.128
+++ frame.c	2 Jul 2003 18:22:38 -0000
@@ -1933,25 +1933,21 @@
     }
 
   /* Check that this frame's ID isn't inner to (younger, below, next)
-     the next frame.  This happens when frame unwind goes backwards.
-     Since the sentinel frame isn't valid, don't apply this if this
-     frame is entier the inner-most or sentinel frame.  */
+     the next frame.  This happens when a frame unwind goes backwards.
+     Since the sentinel frame doesn't really exist, don't compare the
+     inner-most against that sentinel.  */
   if (this_frame->level > 0
       && frame_id_inner (get_frame_id (this_frame),
 			 get_frame_id (this_frame->next)))
-    error ("This frame inner-to next frame (corrupt stack?)");
+    error ("Previous frame inner to this frame (corrupt stack?)");
 
-  /* Check that this and the next frame are different.  If they are
-     not, there is most likely a stack cycle.  As with the inner-than
-     test, avoid the inner-most and sentinel frames.  */
-  /* FIXME: cagney/2003-03-17: Can't yet enable this this check. The
-     frame_id_eq() method doesn't yet use function addresses when
-     comparing frame IDs.  */
-  if (0
-      && this_frame->level > 0
+  /* Check that this and the next frame are not identical.  If they
+     are, there is most likely a stack cycle.  As with the inner-than
+     test above, avoid comparing the inner-most and sentinel frames.  */
+  if (this_frame->level > 0
       && frame_id_eq (get_frame_id (this_frame),
 		      get_frame_id (this_frame->next)))
-    error ("This frame identical to next frame (corrupt stack?)");
+    error ("Previous frame identical to this frame (corrupt stack?)");
 
   /* Allocate the new frame but do not wire it in to the frame chain.
      Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Daniel Jacobowitz]

On Wed, Jul 02, 2003 at 02:27:39PM -0400, Andrew Cagney wrote:
> Hello,
> 
> This patch enables the `is the prev frame identical to this frame' check 
> in get_prev_frame().  I forgot to enable it when I switched the frame ID 
> code from using the frame's PC to using the frame's function.  Noticed 
> this when reviewing Daniel's Arm frame update.
> 
> It should probably go straight into the mainline (tested on i386 and d10v).

d10v, AVR, and soon ARM all include this check as a silent stop
condition rather than an error... perhaps that has some significance.
I know it's hit at least on ARM.  I'd have to dumb down the prologue
analyzer if I wanted it not to be, or else figure out where an
inside_entry_func check has gone completely missing in the new frame
code.  Just putting it unwind_this_id didn't work (don't remember why
not).


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Andrew Cagney]

> On Wed, Jul 02, 2003 at 02:27:39PM -0400, Andrew Cagney wrote:
> 
>> Hello,
>> 
>> This patch enables the `is the prev frame identical to this frame' check 
>> in get_prev_frame().  I forgot to enable it when I switched the frame ID 
>> code from using the frame's PC to using the frame's function.  Noticed 
>> this when reviewing Daniel's Arm frame update.
>> 
>> It should probably go straight into the mainline (tested on i386 and d10v).
> 
> 
> d10v, AVR, and soon ARM all include this check as a silent stop
> condition rather than an error... perhaps that has some significance.

Ulgh! `prior art' :-)

> I know it's hit at least on ARM.

It hit the d10v as well, I'd have not otherwize added the test.

> I'd have to dumb down the prologue
> analyzer if I wanted it not to be, or else figure out where an
> inside_entry_func check has gone completely missing in the new frame
> code.  Just putting it unwind_this_id didn't work (don't remember why
> not).

Sounds like it should be treated like the stack bottom?

The other choice is to treat it as undefined and accept the warning.

Andrew

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Daniel Jacobowitz]

On Thu, Jul 03, 2003 at 11:27:31AM -0400, Andrew Cagney wrote:
> >On Wed, Jul 02, 2003 at 02:27:39PM -0400, Andrew Cagney wrote:
> >
> >>Hello,
> >>
> >>This patch enables the `is the prev frame identical to this frame' check 
> >>in get_prev_frame().  I forgot to enable it when I switched the frame ID 
> >>code from using the frame's PC to using the frame's function.  Noticed 
> >>this when reviewing Daniel's Arm frame update.
> >>
> >>It should probably go straight into the mainline (tested on i386 and 
> >>d10v).
> >
> >
> >d10v, AVR, and soon ARM all include this check as a silent stop
> >condition rather than an error... perhaps that has some significance.
> 
> Ulgh! `prior art' :-)
> 
> >I know it's hit at least on ARM.
> 
> It hit the d10v as well, I'd have not otherwize added the test.
> 
> >I'd have to dumb down the prologue
> >analyzer if I wanted it not to be, or else figure out where an
> >inside_entry_func check has gone completely missing in the new frame
> >code.  Just putting it unwind_this_id didn't work (don't remember why
> >not).
> 
> Sounds like it should be treated like the stack bottom?
> 
> The other choice is to treat it as undefined and accept the warning.

Both make sense to me.  Probably the warning is better.  I think we
should figure out why inside_entry_func isn't stopping the backtrace
before this even becomes an issue, though...

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Andrew Cagney]

>> The other choice is to treat it as undefined and accept the warning.
> 
> 
> Both make sense to me.  Probably the warning is better.  I think we
> should figure out why inside_entry_func isn't stopping the backtrace
> before this even becomes an issue, though...

See my other post, inside_main_func() is broken when there are only 
minimal symbols.

BTW, the inside_entry_func test is disabled pending evidence supporting 
its need.  With inside_main_func fixed, there's still no evidence.

Andrew

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Andrew Cagney]

> On Wed, Jul 02, 2003 at 02:27:39PM -0400, Andrew Cagney wrote:
> 
> Hello,
> 
> This patch enables the `is the prev frame identical to this frame' check in get_prev_frame().  I forgot to enable it when I switched the frame ID code from using the frame's PC to using the frame's function.  Noticed this when reviewing Daniel's Arm frame update.
> 
> It should probably go straight into the mainline (tested on i386 and d10v).
> 
> 
> d10v, AVR, and soon ARM all include this check as a silent stop
> condition rather than an error... perhaps that has some significance.
> 
> Ulgh! `prior art' :-)
> 
> I know it's hit at least on ARM.
> 
> It hit the d10v as well, I'd have not otherwize added the test.

>> I'd have to dumb down the prologue
>> analyzer if I wanted it not to be, or else figure out where an
>> inside_entry_func check has gone completely missing in the new frame
>> code.  Just putting it unwind_this_id didn't work (don't remember why
>> not).

> 
> Sounds like it should be treated like the stack bottom?
> 
> The other choice is to treat it as undefined and accept the warning.

Hmm, I think this is all really a GDB (or testsuite?) bug in asm-source. 
  The backtrace should have stopped at main but instead (at least for 
the d10v) it kept going.

If someone tells GDB to backtrace past main then well, I think, all bets 
should be off.

Andrew

Re: [patch, rfc, 6?] Enable identical frame sanity check

[Andrew Cagney]

> 
> 2003-07-02  Andrew Cagney  <cagney@redhat.com>
> 
> 	* frame.c (get_prev_frame): Enable check for identical frames.
> 	Update comments.  Update error messages.

I've checked this bit in.  This might, but shouldn't disturb new unwinders.

Next is a patch to fix inside_main_func so that it handles object files 
with only minimal symbols.

Andrew