Re: [RFA] epilogue unwinder for i386 (reverse 1/2)

Mark Kettenis
2009-07-12T11:12:36Z

> Date: Sat, 11 Jul 2009 11:11:35 -0700
> From: Michael Snyder <msnyder@vmware.com>
>
> Mark, Daniel, is this OK now?

Not quite. You fullfilled my request for using lower case for the
instruction mneonics, only to add more uppercase ones in the new
comment :(. Oh, anc for consistency please use %reg syntax instead of
REG.

You know what; go ahead and commit this; I'll clean up afterwards.

Mark

> Michael Snyder wrote:
> > Mark Kettenis wrote:
> >>> Date: Fri, 03 Jul 2009 17:34:59 -0700
> >>> From: Michael Snyder <msnyder@vmware.com>
> >>>
> >>> Michael Snyder wrote:
> >>>> This comes out of a discussion with Daniel, about how gcc
> >>>> does not generate the right dwarf info to allow correct
> >>>> frame unwinding in function epilogues, causing frame_unwind
> >>>> to return bad results.
> >>>>
> >>>> It's necessary for reverse-step, which will frequently step
> >>>> backward to the return instruction of a function. But it also
> >>>> provides an improvement for forward debugging, in that now,
> >>>> without this change, if you STEPI until you are at the return
> >>>> instruction, you will get a bad backtrace.
> >>>>
> >>>> The infrun changes that take advantage of this patch will follow
> >>>> separately.
> >>>>
> >>>> Michael
> >>> Oops, the patch wasn't meant to have that "#if 0" in it...
> >>> corrected patch below.
> >> Still has the #if 0 in there.
> >
> > Sorry. ;-(
> >> I also think you should add a comment about the specific ordering of
> >> this unwinder. It has to come before the dwarf2 unwinder because GCC
> >> doesn't provide proper CFI for the epilogue, right?
> >
> > Right. Since the others are similarly order-dependent, I will
> > expand on their comments as well.
> >
> >
> >>> + if (target_read_memory (pc, &insn, 1) != 0)
> >>> + return 0; /* Can't read memory at pc. */
> >> For consistency's sake, can you drop the != 0 here?
> >
> > OK.
> >
> >>> + if (insn != 0xc3) /* RET */
> >>> + return 0;
> >> Please use lowercase for instruction mnemonics.
> >
> > OK. Revised patch attached.
> >
> >
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > 2009-07-03 Michael Snyder <msnyder@vmware.com>
> >
> > * i386-tdep.c: Add a frame unwinder for function epilogues.
> > (i386_in_function_epilogue_p): New function.
> > (i386_epilogue_frame_sniffer): New function.
> > (i386_epilogue_frame_cache): New function.
> > (i386_epilogue_frame_this_id): New function.
> > (i386_epilogue_frame_unwind): New struct frame_unwind.
> > (i386_gdbarch_init): Hook the new unwinder.
> >
> > Index: i386-tdep.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> > retrieving revision 1.280
> > diff -u -p -r1.280 i386-tdep.c
> > --- i386-tdep.c 2 Jul 2009 17:25:54 -0000 1.280
> > +++ i386-tdep.c 5 Jul 2009 20:56:08 -0000
> > @@ -1487,6 +1487,89 @@ static const struct frame_unwind i386_fr
> > NULL,
> > default_frame_sniffer
> > };
> > +
> > +/* Normal frames, but in a function epilogue. */
> > +
> > +/* The epilogue is defined here as the RET instruction, which will
> > + follow any instruction such as LEAVE or POP EBP that destroys the
> > + function's stack frame. */
> > +
> > +static int
> > +i386_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> > +{
> > + gdb_byte insn;
> > +
> > + if (target_read_memory (pc, &insn, 1))
> > + return 0; /* Can't read memory at pc. */
> > +
> > + if (insn != 0xc3) /* 'ret' instruction. */
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > +static int
> > +i386_epilogue_frame_sniffer (const struct frame_unwind *self,
> > + struct frame_info *this_frame,
> > + void **this_prologue_cache)
> > +{
> > + if (frame_relative_level (this_frame) == 0)
> > + return i386_in_function_epilogue_p (get_frame_arch (this_frame),
> > + get_frame_pc (this_frame));
> > + else
> > + return 0;
> > +}
> > +
> > +static struct i386_frame_cache *
> > +i386_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
> > +{
> > + struct gdbarch *gdbarch = get_frame_arch (this_frame);
> > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > + struct i386_frame_cache *cache;
> > + gdb_byte buf[4];
> > +
> > + if (*this_cache)
> > + return *this_cache;
> > +
> > + cache = i386_alloc_frame_cache ();
> > + *this_cache = cache;
> > +
> > + /* Cache base will be ESP plus cache->sp_offset (-4). */
> > + get_frame_register (this_frame, I386_ESP_REGNUM, buf);
> > + cache->base = extract_unsigned_integer (buf, 4,
> > + byte_order) + cache->sp_offset;
> > +
> > + /* Cache pc will be the frame func. */
> > + cache->pc = get_frame_pc (this_frame);
> > +
> > + /* The saved ESP will be at cache->base plus 8. */
> > + cache->saved_sp = cache->base + 8;
> > +
> > + /* The saved EIP will be at cache->base plus 4. */
> > + cache->saved_regs[I386_EIP_REGNUM] = cache->base + 4;
> > +
> > + return cache;
> > +}
> > +
> > +static void
> > +i386_epilogue_frame_this_id (struct frame_info *this_frame,
> > + void **this_cache,
> > + struct frame_id *this_id)
> > +{
> > + struct i386_frame_cache *cache = i386_epilogue_frame_cache (this_frame,
> > + this_cache);
> > +
> > + (*this_id) = frame_id_build (cache->base + 8, cache->pc);
> > +}
> > +
> > +static const struct frame_unwind i386_epilogue_frame_unwind =
> > +{
> > + NORMAL_FRAME,
> > + i386_epilogue_frame_this_id,
> > + i386_frame_prev_register,
> > + NULL,
> > + i386_epilogue_frame_sniffer
> > +};
> > \f
> >
> > /* Signal trampolines. */
> > @@ -5329,7 +5412,15 @@ i386_gdbarch_init (struct gdbarch_info i
> > /* Helper for function argument information. */
> > set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument);
> >
> > - /* Hook in the DWARF CFI frame unwinder. */
> > + /* Hook the function epilogue frame unwinder. This unwinder is
> > + appended to the list first, so that it supercedes the Dwarf
> > + unwinder in function epilogues (where the Dwarf unwinder
> > + currently fails). */
> > + frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
> > +
> > + /* Hook in the DWARF CFI frame unwinder. This unwinder is appended
> > + to the list before the prologue-based unwinders, so that Dwarf
> > + CFI info will be used if it is available. */
> > dwarf2_append_unwinders (gdbarch);
> >
> > frame_base_set_default (gdbarch, &i386_frame_base);
> > @@ -5337,6 +5428,7 @@ i386_gdbarch_init (struct gdbarch_info i
> > /* Hook in ABI-specific overrides, if they have been registered. */
> > gdbarch_init_osabi (info, gdbarch);
> >
> > + /* Hook in the legacy prologue-based unwinders last (fallback). */
> > frame_unwind_append_unwinder (gdbarch, &i386_sigtramp_frame_unwind);
> > frame_unwind_append_unwinder (gdbarch, &i386_frame_unwind);
> >
>
>