info frame ADDR internal error

[Pedro Alves <pedro at codesourcery dot com>]

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

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