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

[Michael Snyder]

Mark, Daniel, is this OK now?

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
> +};
>  

>  
>  /* 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);
>