[PATCH] PR backtrace/9786

[PATCH] PR backtrace/9786

[Nick Roberts]

This patch prevents the assertion error reported in:

http://sourceware.org/bugzilla/show_bug.cgi?id=9786

Output from "info frame" after connecting to a remote target now looks like:

    (gdb) info frame
    Stack level 0, frame at 0x0:
     eip = 0xb7f7d810; saved eip none
     Outermost frame: unwinder did not report frame ID
     Arglist at unknown address.
     Locals at unknown address, Previous frame's sp in esp

With native targets "info frame" works as before and there are no regressions
in the testsuite.  I don't understand the last line of output but I think
this is better than having Gdb throw an assertion error.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2009-02-23  Nick Roberts  <nickrob@snap.net.nz>

	PR backtrace/9786
	* stack.c (frame_info): Avoid assertion error when there
        is no saved pc.


--- stack.c	12 Feb 2009 19:33:27 +1300	1.185
+++ stack.c	23 Feb 2009 00:09:02 +1300	
@@ -894,6 +894,7 @@ frame_info (char *addr_exp, int from_tty
   int selected_frame_p;
   struct gdbarch *gdbarch;
   struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
+  struct value *value;
 
   fi = parse_frame_specification_1 (addr_exp, "No stack.", &selected_frame_p);
   gdbarch = get_frame_arch (fi);
@@ -976,7 +977,12 @@ frame_info (char *addr_exp, int from_tty
   puts_filtered ("; ");
   wrap_here ("    ");
   printf_filtered ("saved %s ", pc_regname);
-  fputs_filtered (paddress (frame_pc_unwind (fi)), gdb_stdout);
+  value = frame_unwind_register_value (fi, gdbarch_pc_regnum (gdbarch));
+  if (VALUE_LVAL (value)  == lval_register
+      && !(frame_id_p (VALUE_FRAME_ID (value))))
+    printf_filtered ("none");
+  else
+    fputs_filtered (paddress (frame_pc_unwind (fi)), gdb_stdout);
   printf_filtered ("\n");
 
   if (calling_frame_info == NULL)

Re: [PATCH] PR backtrace/9786

[Joel Brobecker]

The problem was reported as follow: Doing an "info frame" right after
connecting to a remote target started through the gdbserver causes
a failed assertion:

    (gdb) target remote localhost:4444
    Remote debugging using localhost:4444
    0xb7fda810 in ?? () from /lib/ld-linux.so.2
    (gdb) info frame
    Stack level 0, frame at 0x0:
     eip = 0xb7fda810; saved eip 
    findvar.c:299: internal-error: value_of_register_lazy: Assertion `frame_id_p
    (get_frame_id (frame))' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

The problem occurs when trying to print the frame's return address
(saved eip ...). Of course, since the program has just been spawned
but hasn't started running yet, there is no return address...

The fix suggested by Nick is the following:

> @@ -976,7 +977,12 @@ frame_info (char *addr_exp, int from_tty
>    puts_filtered ("; ");
>    wrap_here ("    ");
>    printf_filtered ("saved %s ", pc_regname);
> -  fputs_filtered (paddress (frame_pc_unwind (fi)), gdb_stdout);
> +  value = frame_unwind_register_value (fi, gdbarch_pc_regnum (gdbarch));
> +  if (VALUE_LVAL (value)  == lval_register
> +      && !(frame_id_p (VALUE_FRAME_ID (value))))
> +    printf_filtered ("none");
> +  else
> +    fputs_filtered (paddress (frame_pc_unwind (fi)), gdb_stdout);
>    printf_filtered ("\n");

I wonder if it wouldn't be better to do a "get_prev_frame" and
check that this frame is valid, and if valid, then get the PC
from this prev_frame.

What do others think?

-- 
Joel

Re: [PATCH] PR backtrace/9786

[Pedro Alves]

Nick Roberts wrote:
>     (gdb) target remote localhost:4444
>     Remote debugging using localhost:4444
>     0xb7fda810 in ?? () from /lib/ld-linux.so.2
>     (gdb) info frame
>     Stack level 0, frame at 0x0:

I'm guessing that i386 unwinder is noticing
that ebp is 0 and deciding this is the outermost frame.  The amd64
unwinder seems to handle this a bit different, and will probably end up
thinking that this is a frameless function.  i386-tdep.c has this:

i386_frame_cache ()
{
(...)
  get_frame_register (this_frame, I386_EBP_REGNUM, buf);
  cache->base = extract_unsigned_integer (buf, 4);
  if (cache->base == 0)
    return cache;
(...)


>      eip = 0xb7fda810; saved eip 
>     findvar.c:299: internal-error: value_of_register_lazy: Assertion `frame_id_p
>     (get_frame_id (frame))' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)

On Monday 23 February 2009 00:16:51, Joel Brobecker wrote:
> What do others think?

This internal error sounds suspiciously related to the fact that
there's no current way to distinguish a null/invalid frame id,
from an outermost frame id...  We should really fix that sometime.

-- 
Pedro Alves

Re: [PATCH] PR backtrace/9786

[Daniel Jacobowitz]

On Mon, Feb 23, 2009 at 12:48:52AM +0000, Pedro Alves wrote:
> This internal error sounds suspiciously related to the fact that
> there's no current way to distinguish a null/invalid frame id,
> from an outermost frame id...  We should really fix that sometime.

I'd been holding off on this because there were all sorts of
less-hacky ways to do it, but maybe it's time for the hack:
replace the one null value with two magic null values.  Use a
different one to represent the outermost frame than to represent no
frame.

This will still have some problems (all "outermost frames" will be
treated as equal) and require an audit of calls to frame_id_p (the
outermost frame should satisfy frame_id_p, that's the whole point, so
a different test will be needed to stop the backtrace).  But it should
be a lot easier than an overhaul.

-- 
Daniel Jacobowitz
CodeSourcery

[PATCH] PR backtrace/9786

[Nick Roberts]

 > This patch prevents the assertion error reported in:
 > 
 > http://sourceware.org/bugzilla/show_bug.cgi?id=9786


The patch I submitted was incomplete and needed the removal of the assertion
in value_of_register_lazy, I think.

There was a discussion about better ways of fixing this but this regression
is still present in 32 bit x86 (64 bit seemss to be OK and finds the
register values in cache somehow).

Current behaviour breaks remote debugging when using GDB in Emacs as it
currently uses "info frame".  If this behaviour can't be fully resolved
before the release could a less than perfect patch be applied to prevent
the assertion error?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] PR backtrace/9786

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick> Current behaviour breaks remote debugging when using GDB in Emacs as it
Nick> currently uses "info frame".  If this behaviour can't be fully resolved
Nick> before the release could a less than perfect patch be applied to prevent
Nick> the assertion error?

I don't know anything about this patch, so I can't really answer
that... but could you add this to the wiki as a release blocker?
I think that's the best way to ensure that some decision is made
before 7.0.

Tom

Re: [PATCH] PR backtrace/9786

[Nick Roberts]

 > I don't know anything about this patch, so I can't really answer
 > that... but could you add this to the wiki as a release blocker?
 > I think that's the best way to ensure that some decision is made
 > before 7.0.

OK, thanks.  I have done that.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] PR backtrace/9786

[Joel Brobecker]

> Current behaviour breaks remote debugging when using GDB in Emacs as it
> currently uses "info frame".  If this behaviour can't be fully resolved
> before the release could a less than perfect patch be applied to prevent
> the assertion error?

This is on the list of blocking issues for 7.0, I think. We'll figure
out something. The question remains who "we" will be - I'm going to be
pretty swamped until September...

-- 
Joel

Re: [PATCH] PR backtrace/9786

[Daniel Jacobowitz]

On Tue, Jun 23, 2009 at 11:38:53AM -0700, Joel Brobecker wrote:
> > Current behaviour breaks remote debugging when using GDB in Emacs as it
> > currently uses "info frame".  If this behaviour can't be fully resolved
> > before the release could a less than perfect patch be applied to prevent
> > the assertion error?
> 
> This is on the list of blocking issues for 7.0, I think. We'll figure
> out something. The question remains who "we" will be - I'm going to be
> pretty swamped until September...

I have a patch which, if I haven't gotten turned around, fixes this.
I can post it after I get inlining merged.

I'll warn you right off, though, you won't like it.  Consider it a
starting point for discussion of the right solution :-)

-- 
Daniel Jacobowitz
CodeSourcery