info frame ADDR internal error

info frame ADDR internal error

[Pedro Alves]

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

Hi,

(Since I had just touched create_new_frame, ...)

I was looking at this internal error:

 (top-gdb) bt
 #0  main (argc=1, argv=0x7fffca57f988) at ../../src/gdb/gdb.c:28

 (top-gdb) info frame 1
 Stack frame at 0x1:
  rip = 0x0; saved rip
 /build/buildd/gdb-6.8/gdb/dwarf2-frame.c:860: internal-error: dwarf2_frame_cache: Assertion `fde != NULL' failed.
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.
 Quit this debugging session? (y or n)

Which is mentioned in PR8864 and PR9458:

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

, and came up with the attached patch.  That internal error is happening
because when we sniff for an unwinder of this hacked up new frame,
we find the frame pc to be one thing:

 static int
 dwarf2_frame_sniffer (const struct frame_unwind *self,
 		      struct frame_info *this_frame, void **this_cache)
 {
   CORE_ADDR block_addr = get_frame_address_in_block (this_frame);
   struct dwarf2_fde *fde = dwarf2_frame_find_fde (&block_addr);
   if (!fde)
     return 0;

 ...
 <the rest of unwinding, cause prev_pc wasn't cached yet>
 #6  0x00000000005cb72d in frame_pc_unwind (this_frame=0x1719570) at ../../src/gdb/frame.c:480
 #7  0x00000000005cd707 in get_frame_pc (frame=0x17194e0) at ../../src/gdb/frame.c:1547
 #8  0x00000000005cd71e in get_frame_address_in_block (this_frame=0x17194e0) at ../../src/gdb/frame.c:1556
 #9  0x000000000057e1c3 in dwarf2_frame_sniffer (self=0x768580, this_frame=0x17194e0, this_cache=0x17194e8)
 ...

This gets us,

 (top-gdb) p /x block_addr
 $4 = 0x450a87

Since we do find an FDE for that address, we take the dwarf unwinder for this frame.

But, just after finding the unwinder, we tinker with next->prev_pc's value, so, this
assertion fails:

 910       /* Find the correct FDE.  */
 911       fde = dwarf2_frame_find_fde (&fs->pc);
 912       gdb_assert (fde != NULL);

 #0  internal_error (file=0x768109 "../../src/gdb/dwarf2-frame.c", line=912,
     string=0x768174 "%s: Assertion `%s' failed.") at ../../src/gdb/utils.c:972
 #1  0x000000000057d80d in dwarf2_frame_cache (this_frame=0x17194e0, this_cache=0x17194e8)
     at ../../src/gdb/dwarf2-frame.c:912
 #2  0x000000000057de1b in dwarf2_frame_prev_register (this_frame=0x17194e0, this_cache=0x17194e8, regnum=16)
     at ../../src/gdb/dwarf2-frame.c:1081
 #3  0x00000000005cbdce in frame_unwind_register_value (frame=0x17194e0, regnum=16) at ../../src/gdb/frame.c:671
 #4  0x00000000005cba5d in frame_register_unwind (frame=0x17194e0, regnum=16, optimizedp=0x7fff935b8f6c,
     lvalp=0x7fff935b8f64, addrp=0x7fff935b8f58, realnump=0x7fff935b8f68, bufferp=0x7fff935b8f90 "0��")
     at ../../src/gdb/frame.c:593
 #5  0x00000000005cbcd6 in frame_unwind_register (frame=0x17194e0, regnum=16, buf=0x7fff935b8f90 "0��")
     at ../../src/gdb/frame.c:639
 #6  0x00000000004643dd in i386_unwind_pc (gdbarch=0xbc1880, next_frame=0x17194e0) at ../../src/gdb/i386-tdep.c:1277
 #7  0x0000000000531483 in gdbarch_unwind_pc (gdbarch=0xbc1880, next_frame=0x17194e0) at ../../src/gdb/gdbarch.c:2372
 #8  0x00000000005cb72d in frame_pc_unwind (this_frame=0x17194e0) at ../../src/gdb/frame.c:480
 #9  0x000000000051d4e1 in frame_info (addr_exp=0xae11cb "1", from_tty=1) at ../../src/gdb/stack.c:979
 #10 0x000000000049de64 in do_cfunc (c=0xb1bc00, args=0xae11cb "1", from_tty=1) at ../../src/gdb/cli/cli-decode.c

So...  it looks to me that we should be hacking fi->next->prev_pc before looking
for FI's unwinder.

However, with that one handled, we trip on yet another assertion:

 struct value *
 value_of_register_lazy (struct frame_info *frame, int regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct value *reg_val;
 
   gdb_assert (regnum < (gdbarch_num_regs (gdbarch)
 			+ gdbarch_num_pseudo_regs (gdbarch)));
 
   /* We should have a valid (i.e. non-sentinel) frame.  */
   gdb_assert (frame_id_p (get_frame_id (frame)));
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 #0  internal_error (file=0x733492 "../../src/gdb/findvar.c", line=299, string=0x733507 "%s: Assertion `%s' failed.")
     at ../../src/gdb/utils.c:972
 #1  0x00000000004cf4bb in value_of_register_lazy (frame=0xb2a910, regnum=0) at ../../src/gdb/findvar.c:299
 #2  0x00000000005ce563 in frame_unwind_got_register (frame=0xb2a910, regnum=0, new_regnum=0)
     at ../../src/gdb/frame-unwind.c:143
 #3  0x00000000005dedd4 in amd64_frame_prev_register (this_frame=0xb2a910, this_cache=0xb2a918, regnum=0)
     at ../../src/gdb/amd64-tdep.c:1072
 #4  0x00000000005cbdd6 in frame_unwind_register_value (frame=0xb2a910, regnum=0) at ../../src/gdb/frame.c:671
 #5  0x00000000005cba65 in frame_register_unwind (frame=0xb2a910, regnum=0, optimizedp=0x7fffa756c09c,
     lvalp=0x7fffa756c0a0, addrp=0x7fffa756c020, realnump=0x7fffa756c098, bufferp=0x0) at ../../src/gdb/frame.c:593
 #6  0x000000000051da23 in frame_info (addr_exp=0xae11cb "1", from_tty=1) at ../../src/gdb/stack.c:1128


You'll notice that create_new_frame sets up the frame id, like so:

  fi->this_id.p = 1;
  fi->this_id.value.stack_addr = addr;

This misses setting fi->this_id.value.stack_addr_p, so it didn't matter
what was put in stack_addr ...

This frame id building came from inlining deprecated_update_frame_base_hack:

 http://sourceware.org/ml/gdb-patches/2009-01/msg00473.html

It looks like that hack assumed we already had 
fi->this_id.value.stack_addr_p set, since it was supposed to
*update* stack_addr.  I'm guessing this came from even before
we had frame ids.

I could just set stack_addr_p, or use frame_id_build_wild to fix
this, although, it looks suspicious to me to have a frame with a wildcard
frame id.  So, the next best thing seems to be to build the frame id
with both ADDR and the passed in PC.

At this point, I wondered: why not just let the unwinder build
the frame id itself on demand?  That is, just get rid of these lines:

 -  fi->this_id.p = 1;
 -  fi->this_id.value.stack_addr = addr;

?

The side effect is that this changes the behaviour of
create_new_frame, as this:

 (top-gdb) info frame 0x7fffffffe331
 Stack frame at 0x7fffffffe331:
  rip = 0x0; saved rip 0x7fffffffe408
  called by frame at 0x7fffffffe300
  Arglist at 0x7fffffffe2e8, args:
  Locals at 0x7fffffffe2e8, Previous frame's sp is 0x7fffffffe2f8
  Saved registers:
   rip at 0x7fffffffe2f0

becomes this:

 (top-gdb) info frame 0x7fffffffe331
 Stack frame at 0x7fffffffe2f8:
  rip = 0x0; saved rip 0x7fffffffe408
  called by frame at 0x7fffffffe300
  Arglist at 0x7fffffffe2e8, args:
  Locals at 0x7fffffffe2e8, Previous frame's sp is 0x7fffffffe2f8
  Saved registers:
   rip at 0x7fffffffe2f0

This is because this command uses get_frame_base, and that returns the
stack_addr stored in the frame id.

Then, I wonder, what is the real use case behind this
"info frame RANDOM_BASE_ADDR_NOT_IN_FRAME_CHAIN"?  Is it useful at all?

(<insert complain about the fact that ADDR is overloaded to be either
a frame relative level or a frame address --- why do we try to be smart
with these things instead of making the user specify distinct
switches?  Notice PR9458's How-To-Repeat description:

 " (1) Fire up GDB and attach it to a process with x number of frames.
   (2) info frame (x + 1) "

Sigh... />)

(I'll stop before going into the fact that "info frame ADDR" is equal
to "info frame ADDR 0".)

This create_new_frame method is so filled with hacks, that it looks to me
like it's wanting to just go away.  The only other caller it has, is in
mn10300-tdep.c --- any suggestions on how to eliminate that call?

So, ... the attached fixes the internal error, plus an obvious typo,
and has no regressions on x86_64-linux.

WDYT?

-- 
Pedro Alves

[-- Attachment #2: create_new_frame.diff --]
[-- Type: text/x-diff, Size: 2152 bytes --]

2009-01-25  Pedro Alves  <pedro@codesourcery.com>

	* frame.c (create_new_frame): Update the frame's cached PC before
	finding its unwinder.  Use frame_id_build to build the new frame's
	id.
	* stack.c (parse_frame_specification_1): Correct setting ``addrs''
	array values from the ``args'' array values.

---
 gdb/frame.c |   14 ++++++++------
 gdb/stack.c |    2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2009-01-25 16:26:08.000000000 +0000
+++ src/gdb/frame.c	2009-01-25 16:26:24.000000000 +0000
@@ -1106,17 +1106,19 @@ create_new_frame (CORE_ADDR addr, CORE_A
 
   fi->next = create_sentinel_frame (get_current_regcache ());
 
+  /* Set/update this frame's cached PC value, found in the next frame.
+     Do this before looking for this frame's unwinder.  A sniffer is
+     very likely to read this, and the corresponding unwinder is
+     entitled to rely that the PC doesn't magically change.  */
+  fi->next->prev_pc.value = pc;
+  fi->next->prev_pc.p = 1;
+
   /* Select/initialize both the unwind function and the frame's type
      based on the PC.  */
   fi->unwind = frame_unwind_find_by_frame (fi, &fi->prologue_cache);
 
   fi->this_id.p = 1;
-  fi->this_id.value.stack_addr = addr;
-  /* While we're at it, update this frame's cached PC value, found
-     in the next frame.  Oh for the day when "struct frame_info"
-     is opaque and this hack on hack can just go away.  */
-  fi->next->prev_pc.value = pc;
-  fi->next->prev_pc.p = 1;
+  fi->this_id.value = frame_id_build (addr, pc);
 
   if (frame_debug)
     {
Index: src/gdb/stack.c
===================================================================
--- src.orig/gdb/stack.c	2009-01-25 16:26:07.000000000 +0000
+++ src/gdb/stack.c	2009-01-25 16:26:24.000000000 +0000
@@ -832,7 +832,7 @@ parse_frame_specification_1 (const char 
   {
     int i;
     for (i = 0; i < numargs; i++)
-      addrs[i] = value_as_address (args[0]);
+      addrs[i] = value_as_address (args[i]);
   }
 
   /* Assume that the single arg[0] is an address, use that to identify

Re: info frame ADDR internal error

[Daniel Jacobowitz]

On Sun, Jan 25, 2009 at 04:35:29PM +0000, Pedro Alves wrote:
> So, ... the attached fixes the internal error, plus an obvious typo,
> and has no regressions on x86_64-linux.
> 
> WDYT?

I think this looks good.

-- 
Daniel Jacobowitz
CodeSourcery

Re: info frame ADDR internal error

[Pedro Alves]

:-(

Was going to commit the patch, but then I tried it on x86 (I was on x86-64 before) ...

 (top-gdb) bt
 #0  main (argc=1, argv=0xffffd524) at ../../src/gdb/gdb.c:28

 (top-gdb) info frame 1
 Stack frame at 0x1:
  eip = 0x0; saved eip 0xf7f26f59

 Breakpoint 1, internal_error (file=0x8314a4f "../../src/gdb/valops.c", line=662,
     string=0x8314a19 "%s: Assertion `%s' failed.") at ../../src/gdb/utils.c:972
 972       va_start (ap, string);

 (top-gdb) bt
 #0  internal_error (file=0x8314a4f "../../src/gdb/valops.c", line=662, string=0x8314a19 "%s: Assertion `%s' failed.")
     at ../../src/gdb/utils.c:972
 #1  0x08116c44 in value_fetch_lazy (val=0x883f960) at ../../src/gdb/valops.c:662
 #2  0x0810b766 in value_contents_all (value=0x883f960) at ../../src/gdb/value.c:375
 #3  0x081efd43 in frame_register_unwind (frame=0x8421250, regnum=5, optimizedp=0xffc759f4, lvalp=0xffc759e8,
     addrp=0xffc759f0, realnump=0xffc759ec, bufferp=0xffc75a40 "...") at ../../src/gdb/frame.c:603
 #4  0x081eff19 in frame_unwind_register (frame=0x8421250, regnum=5, buf="...") at ../../src/gdb/frame.c:639
 #5  0x081eff3d in get_frame_register (frame=0x84212e4, regnum=5, buf="...") at ../../src/gdb/frame.c:647
 #6  0x0809c2b7 in i386_frame_cache (this_frame=0x84212e4, this_cache=0x84212e8) at ../../src/gdb/i386-tdep.c:1306
 #7  0x0809c47e in i386_frame_this_id (this_frame=0x84212e4, this_cache=0x84212e8, this_id=0x8421304)
     at ../../src/gdb/i386-tdep.c:1369
 #8  0x081ef2cb in get_frame_id (fi=0x84212e4) at ../../src/gdb/frame.c:261
 #9  0x081f194d in get_frame_base (fi=0x84212e4) at ../../src/gdb/frame.c:1666
 #10 0x0814cd44 in frame_info (addr_exp=0x83f5133 "1", from_tty=1) at ../../src/gdb/stack.c:995
 #11 0x080d51b7 in do_cfunc (c=0x84187c0, args=0x83f5133 "1", from_tty=1) at ../../src/gdb/cli/cli-decode.c:67

Here's the offender:

 int
 value_fetch_lazy (struct value *val)
 {
   gdb_assert (value_lazy (val));
   allocate_value_contents (val);
   if (VALUE_LVAL (val) == lval_memory)
     {
       CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
       int length = TYPE_LENGTH (check_typedef (value_enclosing_type (val)));
 
       if (length)
 	read_memory (addr, value_contents_all_raw (val), length);
     }
   else if (VALUE_LVAL (val) == lval_register)
     {
       struct frame_info *frame;
       int regnum;
       struct type *type = check_typedef (value_type (val));
       struct value *new_val = val, *mark = value_mark ();
 
       /* Offsets are not supported here; lazy register values must
 	 refer to the entire register.  */
       gdb_assert (value_offset (val) == 0);
 
       while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
 	{
 	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
                  ^^^^^^^^^^^^^^^^
 	  regnum = VALUE_REGNUM (new_val);
 
 	  gdb_assert (frame != NULL);
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^

 (top-gdb) p VALUE_FRAME_ID (new_val)
 $1 = {stack_addr = 1, code_addr = 0, special_addr = 0, stack_addr_p = 1, code_addr_p = 1, special_addr_p = 0}

The new frame that create_new_frame created, isn't linked in the
regular ( current_frame->... ) frame chain, it lives in its own chain,
so this frame_find_by_id call isn't going to find it, unless you
get lucky.

This reinforces the bad things I was saying about create_new_frame.

Any suggestions on how this could be fixed?

Before we go farther, is this a valid use case at all?  Could
we rip this piece of parse_frame_specification_1 out instead?  It's been
broken for years now --- I can reproduce the original internal error
on gdb-6.0:

 >./gdb ./gdb
 GNU gdb 6.0
 Copyright 2003 Free Software Foundation, Inc.
 ...
 (top-gdb) b main
 Breakpoint 3 at 0x8077aa4: file ../../gdb-6.0/gdb/gdb.c, line 30.
 (top-gdb) r
 Starting program: /home/pedro/gdb/ancient/build-6.0/gdb/gdb
 Breakpoint 3, main (argc=1, argv=0xffd15254) at ../../gdb-6.0/gdb/gdb.c:30
 30        memset (&args, 0, sizeof args);
 (top-gdb) info frame 1
 Stack frame at 0x1:
  eip = 0x0; saved eip
 ../../gdb-6.0/gdb/dwarf2-frame.c:518: internal-error: dwarf2_frame_cache: Assertion `fde != NULL' failed.

Note that "info frame ADDR" and "frame ADDR" do look for a frame in
the regular chain that matches ADDR, before resorting to hacking up
a new one with create_new_frame.

-- 
Pedro Alves

Re: info frame ADDR internal error

[Pedro Alves]

On Sunday 25 January 2009 19:44:32, Pedro Alves wrote:
> :-(
> 
> Was going to commit the patch, but then I tried it on x86 (I was on x86-64 before) ...

I actually went ahead, and committed the patch.  It fixes some bogosity,
and uncovers the next problem.  This should help everyone else who
wants to look at the issue.

> 
>  (top-gdb) bt
>  #0  main (argc=1, argv=0xffffd524) at ../../src/gdb/gdb.c:28
> 
>  (top-gdb) info frame 1
>  Stack frame at 0x1:
>   eip = 0x0; saved eip 0xf7f26f59
> 
>  Breakpoint 1, internal_error (file=0x8314a4f "../../src/gdb/valops.c", line=662,
>      string=0x8314a19 "%s: Assertion `%s' failed.") at ../../src/gdb/utils.c:972
>  972       va_start (ap, string);
> 
>  (top-gdb) bt
>  #0  internal_error (file=0x8314a4f "../../src/gdb/valops.c", line=662, string=0x8314a19 "%s: Assertion `%s' failed.")
>      at ../../src/gdb/utils.c:972
>  #1  0x08116c44 in value_fetch_lazy (val=0x883f960) at ../../src/gdb/valops.c:662
>  #2  0x0810b766 in value_contents_all (value=0x883f960) at ../../src/gdb/value.c:375
>  #3  0x081efd43 in frame_register_unwind (frame=0x8421250, regnum=5, optimizedp=0xffc759f4, lvalp=0xffc759e8,
>      addrp=0xffc759f0, realnump=0xffc759ec, bufferp=0xffc75a40 "...") at ../../src/gdb/frame.c:603
>  #4  0x081eff19 in frame_unwind_register (frame=0x8421250, regnum=5, buf="...") at ../../src/gdb/frame.c:639
>  #5  0x081eff3d in get_frame_register (frame=0x84212e4, regnum=5, buf="...") at ../../src/gdb/frame.c:647
>  #6  0x0809c2b7 in i386_frame_cache (this_frame=0x84212e4, this_cache=0x84212e8) at ../../src/gdb/i386-tdep.c:1306
>  #7  0x0809c47e in i386_frame_this_id (this_frame=0x84212e4, this_cache=0x84212e8, this_id=0x8421304)
>      at ../../src/gdb/i386-tdep.c:1369
>  #8  0x081ef2cb in get_frame_id (fi=0x84212e4) at ../../src/gdb/frame.c:261
>  #9  0x081f194d in get_frame_base (fi=0x84212e4) at ../../src/gdb/frame.c:1666
>  #10 0x0814cd44 in frame_info (addr_exp=0x83f5133 "1", from_tty=1) at ../../src/gdb/stack.c:995
>  #11 0x080d51b7 in do_cfunc (c=0x84187c0, args=0x83f5133 "1", from_tty=1) at ../../src/gdb/cli/cli-decode.c:67
> 
> Here's the offender:
> 
>  int
>  value_fetch_lazy (struct value *val)
>  {
>    gdb_assert (value_lazy (val));
>    allocate_value_contents (val);
>    if (VALUE_LVAL (val) == lval_memory)
>      {
>        CORE_ADDR addr = VALUE_ADDRESS (val) + value_offset (val);
>        int length = TYPE_LENGTH (check_typedef (value_enclosing_type (val)));
>  
>        if (length)
>  	read_memory (addr, value_contents_all_raw (val), length);
>      }
>    else if (VALUE_LVAL (val) == lval_register)
>      {
>        struct frame_info *frame;
>        int regnum;
>        struct type *type = check_typedef (value_type (val));
>        struct value *new_val = val, *mark = value_mark ();
>  
>        /* Offsets are not supported here; lazy register values must
>  	 refer to the entire register.  */
>        gdb_assert (value_offset (val) == 0);
>  
>        while (VALUE_LVAL (new_val) == lval_register && value_lazy (new_val))
>  	{
>  	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
>                   ^^^^^^^^^^^^^^^^
>  	  regnum = VALUE_REGNUM (new_val);
>  
>  	  gdb_assert (frame != NULL);
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>  (top-gdb) p VALUE_FRAME_ID (new_val)
>  $1 = {stack_addr = 1, code_addr = 0, special_addr = 0, stack_addr_p = 1, code_addr_p = 1, special_addr_p = 0}
> 
> The new frame that create_new_frame created, isn't linked in the
> regular ( current_frame->... ) frame chain, it lives in its own chain,
> so this frame_find_by_id call isn't going to find it, unless you
> get lucky.
> 
> This reinforces the bad things I was saying about create_new_frame.
> 
> Any suggestions on how this could be fixed?
> 
> Before we go farther, is this a valid use case at all?  Could
> we rip this piece of parse_frame_specification_1 out instead?  It's been
> broken for years now --- I can reproduce the original internal error
> on gdb-6.0:
> 
>  >./gdb ./gdb
>  GNU gdb 6.0
>  Copyright 2003 Free Software Foundation, Inc.
>  ...
>  (top-gdb) b main
>  Breakpoint 3 at 0x8077aa4: file ../../gdb-6.0/gdb/gdb.c, line 30.
>  (top-gdb) r
>  Starting program: /home/pedro/gdb/ancient/build-6.0/gdb/gdb
>  Breakpoint 3, main (argc=1, argv=0xffd15254) at ../../gdb-6.0/gdb/gdb.c:30
>  30        memset (&args, 0, sizeof args);
>  (top-gdb) info frame 1
>  Stack frame at 0x1:
>   eip = 0x0; saved eip
>  ../../gdb-6.0/gdb/dwarf2-frame.c:518: internal-error: dwarf2_frame_cache: Assertion `fde != NULL' failed.
> 
> Note that "info frame ADDR" and "frame ADDR" do look for a frame in
> the regular chain that matches ADDR, before resorting to hacking up
> a new one with create_new_frame.
> 



-- 
Pedro Alves

Re: info frame ADDR internal error

[Daniel Jacobowitz]

On Sun, Jan 25, 2009 at 07:44:32PM +0000, Pedro Alves wrote:
> The new frame that create_new_frame created, isn't linked in the
> regular ( current_frame->... ) frame chain, it lives in its own chain,
> so this frame_find_by_id call isn't going to find it, unless you
> get lucky.
> 
> This reinforces the bad things I was saying about create_new_frame.
> 
> Any suggestions on how this could be fixed?

This is just gross.  But yes, I see a way we could "fix" it.  This
code is only reachable from select_frame and info frame.  In the
former case, if we create a new frame we could clear the old frame
chain and set both current and selected frame to the new one;
in the latter case we'd have to clear the frame chain after info
frame was done, and restore the saved selected frame.

I do think this is somewhat useful, although being able to set
registers while debugging a core file would eliminate a lot of the
use...

-- 
Daniel Jacobowitz
CodeSourcery