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

Mark Kettenis
2009-07-05T12:36:08Z

> 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.

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?

Further comments inline below.

> 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 4 Jul 2009 00:37:12 -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) != 0)
> + return 0; /* Can't read memory at pc. */

For consistency's sake, can you drop the != 0 here?

> + if (insn != 0xc3) /* RET */
> + return 0;

Please use lowercase for instruction mnemonics.