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

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

[Michael Snyder]

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

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



[-- Attachment #2: epilogue.txt --]
[-- Type: text/plain, Size: 3491 bytes --]

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:16:03 -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.  */
+
+  if (insn != 0xc3)	/* RET */
+    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.  */
@@ -5328,7 +5411,10 @@ i386_gdbarch_init (struct gdbarch_info i
 
   /* Helper for function argument information.  */
   set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument);
-
+#if 0
+  /* Hook the function epilogue frame unwinder.  */
+  frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
+#endif
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_append_unwinders (gdbarch);
 

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

[Michael Snyder]

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

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.



[-- Attachment #2: epilogue.txt --]
[-- Type: text/plain, Size: 3491 bytes --]

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.  */
+
+  if (insn != 0xc3)	/* RET */
+    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.  */
@@ -5328,7 +5411,10 @@ i386_gdbarch_init (struct gdbarch_info i
 
   /* Helper for function argument information.  */
   set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument);
-
+#if 0
+  /* Hook the function epilogue frame unwinder.  */
+  frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
+#endif
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_append_unwinders (gdbarch);
 

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

[Hui Zhu]

I tried this patch with ubuntu i386.  Everything is OK.

Thanks,
Hui

On Sat, Jul 4, 2009 at 08:34, Michael Snyder<msnyder@vmware.com> wrote:
> 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.
>
>
>
> 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.  */
> +
> +  if (insn != 0xc3)    /* RET */
> +    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.  */
> @@ -5328,7 +5411,10 @@ i386_gdbarch_init (struct gdbarch_info i
>
>   /* Helper for function argument information.  */
>   set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument);
> -
> +#if 0
> +  /* Hook the function epilogue frame unwinder.  */
> +  frame_unwind_append_unwinder (gdbarch, &i386_epilogue_frame_unwind);
> +#endif
>   /* Hook in the DWARF CFI frame unwinder.  */
>   dwarf2_append_unwinders (gdbarch);
>
>
>

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

[Mark Kettenis]

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

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

[Daniel Jacobowitz]

On Sun, Jul 05, 2009 at 02:35:43PM +0200, Mark Kettenis wrote:
> 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 - I would like to have a way to suppress this unwinder, maybe
based on the producer string like other recognized dwarf2-frame
quirks, but we can worry about that later.  I hope it will be
unnecessary with GCC 4.5.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Sun, Jul 05, 2009 at 02:35:43PM +0200, Mark Kettenis wrote:
>> 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 - I would like to have a way to suppress this unwinder, maybe
> based on the producer string like other recognized dwarf2-frame
> quirks, but we can worry about that later.  I hope it will be
> unnecessary with GCC 4.5.

I would like that too -- maybe you can point me at an example?

I'm thinking that even if GCC 4.5 fixes the issue, people will
continue to use older GCCs for a while.

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

[Michael Snyder]

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

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.




[-- Attachment #2: epilogue.txt --]
[-- Type: text/plain, Size: 4289 bytes --]

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

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

[Daniel Jacobowitz]

On Sun, Jul 05, 2009 at 01:43:09PM -0700, Michael Snyder wrote:
> I would like that too -- maybe you can point me at an example?
> 
> I'm thinking that even if GCC 4.5 fixes the issue, people will
> continue to use older GCCs for a while.

Sorry, unfinished thought on my part - I meant to say that someday,
I'd like to add a way to detect the fixed GCC and disable this.  There
isn't a similar example yet.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [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
> +};
>  \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);
>  

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

[Mark Kettenis]

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

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

[Michael Snyder]

Mark Kettenis wrote:
>> 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.

Sorry!  Fixed.

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

That's great of you.  Hope I've already taken care of it.

Committing.

Daniel, a heads-up -- this seems to break your test "i386-signal.exp".

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

[drow]

On Sun, Jul 12, 2009 at 10:42:54AM -0700, Michael Snyder wrote:
> Daniel, a heads-up -- this seems to break your test "i386-signal.exp".

When checked in, it's our test.  I don't think it's appropriate to
commit a patch which causes testsuite regressions.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Michael Snyder]

drow@false.org wrote:
> On Sun, Jul 12, 2009 at 10:42:54AM -0700, Michael Snyder wrote:
>> Daniel, a heads-up -- this seems to break your test "i386-signal.exp".
> 
> When checked in, it's our test.  I don't think it's appropriate to
> commit a patch which causes testsuite regressions.

You're right, bad judgement call on my part.
I'll take it out again until this is resolved.

The issue is, the test contains a hand-coded artificial signal
frame/handler consisting of a single "ret" instruction.

My new epilogue unwinder recognizes the "ret", so it lays
claim to the frame before the dwarf unwinder has a chance.

It still does a proper backtrace, but of course it doesn't
show the <signal handler> tag, it just shows the function name.

What do you think?  Could we maybe add a nop here or something?
I don't understand the details well enough to speculate.

Michael

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

[Daniel Jacobowitz]

On Mon, Jul 13, 2009 at 12:30:19PM -0700, Michael Snyder wrote:
> You're right, bad judgement call on my part.
> I'll take it out again until this is resolved.

Thanks.  Or we can just fix the test, since it sounds straightforward.

> The issue is, the test contains a hand-coded artificial signal
> frame/handler consisting of a single "ret" instruction.
> 
> My new epilogue unwinder recognizes the "ret", so it lays
> claim to the frame before the dwarf unwinder has a chance.
> 
> It still does a proper backtrace, but of course it doesn't
> show the <signal handler> tag, it just shows the function name.
> 
> What do you think?  Could we maybe add a nop here or something?
> I don't understand the details well enough to speculate.

That ought to work, with a comment about what it's there for.
Just don't push/pop to change the stack layout.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Mark Kettenis]

> Date: Mon, 13 Jul 2009 12:30:19 -0700
> From: Michael Snyder <msnyder@vmware.com>
> 
> What do you think?  Could we maybe add a nop here or something?
> I don't understand the details well enough to speculate.

That may be acceptable.  Signal trampolines should end with a
sigreturn system call or something equivalent.  However that is
impractical for the test.

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

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Mon, Jul 13, 2009 at 12:30:19PM -0700, Michael Snyder wrote:
>> You're right, bad judgement call on my part.
>> I'll take it out again until this is resolved.
> 
> Thanks.  Or we can just fix the test, since it sounds straightforward.
> 
>> The issue is, the test contains a hand-coded artificial signal
>> frame/handler consisting of a single "ret" instruction.
>>
>> My new epilogue unwinder recognizes the "ret", so it lays
>> claim to the frame before the dwarf unwinder has a chance.
>>
>> It still does a proper backtrace, but of course it doesn't
>> show the <signal handler> tag, it just shows the function name.
>>
>> What do you think?  Could we maybe add a nop here or something?
>> I don't understand the details well enough to speculate.
> 
> That ought to work, with a comment about what it's there for.
> Just don't push/pop to change the stack layout.

Ah, thank goodness, it works.
Will submit separately.