Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH] Fix some i386 unwinder inconcistencies
@ 2011-06-12 20:57 Mark Kettenis
  2011-06-13  2:32 ` Yao Qi
  2011-06-13 10:49 ` Regression: " Jan Kratochvil
  0 siblings, 2 replies; 16+ messages in thread
From: Mark Kettenis @ 2011-06-12 20:57 UTC (permalink / raw)
  To: gdb-patches

This diff fixes a few issues with the epilogue and stack tramp unwinders.

Committed.


2011-06-12  Mark Kettenis  <kettenis@gnu.org>
 
	* i386-tdep.c (i386_epilogue_frame_cache): Simplify code.  Call
	get_frame_func instead of get_frame_pc to determine the code
	address used to construct the frame ID.
	(i386_epilogue_frame_unwind_stop_reason): Fix coding style.
	(i386_epilogue_frame_this_id): Likewise.
	(i386_epilogue_frame_prev_register): New function.
	(i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
	(i386_stack_tramp_frame_sniffer): Fix coding style.
	(i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
	(i386_gdbarch_init): Fix comments.

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.333
diff -u -p -r1.333 i386-tdep.c
--- i386-tdep.c	12 Jun 2011 18:21:55 -0000	1.333
+++ i386-tdep.c	12 Jun 2011 20:39:01 -0000
@@ -1910,11 +1910,9 @@ i386_epilogue_frame_sniffer (const struc
 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);
   volatile struct gdb_exception ex;
   struct i386_frame_cache *cache;
-  gdb_byte buf[4];
+  CORE_ADDR sp;
 
   if (*this_cache)
     return *this_cache;
@@ -1924,18 +1922,14 @@ i386_epilogue_frame_cache (struct frame_
 
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
-      /* 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 = get_frame_func (this_frame);
 
-      /* Cache pc will be the frame func.  */
-      cache->pc = get_frame_pc (this_frame);
-
-      /* The saved %esp will be at cache->base plus 8.  */
+      /* At this point the stack looks as if we just entered the
+	 function, with the return address at the top of the
+	 stack.  */
+      sp = get_frame_register_unsigned (this_frame, I386_ESP_REGNUM);
+      cache->base = sp + cache->sp_offset;
       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;
 
       cache->base_p = 1;
@@ -1950,8 +1944,8 @@ static enum unwind_stop_reason
 i386_epilogue_frame_unwind_stop_reason (struct frame_info *this_frame,
 					void **this_cache)
 {
-  struct i386_frame_cache *cache
-    = i386_epilogue_frame_cache (this_frame, this_cache);
+  struct i386_frame_cache *cache =
+    i386_epilogue_frame_cache (this_frame, this_cache);
 
   if (!cache->base_p)
     return UNWIND_UNAVAILABLE;
@@ -1964,8 +1958,8 @@ i386_epilogue_frame_this_id (struct fram
 			     void **this_cache,
 			     struct frame_id *this_id)
 {
-  struct i386_frame_cache *cache = i386_epilogue_frame_cache (this_frame,
-							      this_cache);
+  struct i386_frame_cache *cache =
+    i386_epilogue_frame_cache (this_frame, this_cache);
 
   if (!cache->base_p)
     return;
@@ -1973,12 +1967,22 @@ i386_epilogue_frame_this_id (struct fram
   (*this_id) = frame_id_build (cache->base + 8, cache->pc);
 }
 
+static struct value *
+i386_epilogue_frame_prev_register (struct frame_info *this_frame,
+				   void **this_cache, int regnum)
+{
+  /* Make sure we've initialized the cache.  */
+  i386_epilogue_frame_cache (this_frame, this_cache);
+
+  return i386_frame_prev_register (this_frame, this_cache, regnum);
+}
+
 static const struct frame_unwind i386_epilogue_frame_unwind =
 {
   NORMAL_FRAME,
   i386_epilogue_frame_unwind_stop_reason,
   i386_epilogue_frame_this_id,
-  i386_frame_prev_register,
+  i386_epilogue_frame_prev_register,
   NULL, 
   i386_epilogue_frame_sniffer
 };
@@ -2045,8 +2049,8 @@ i386_in_stack_tramp_p (struct gdbarch *g
 
 static int
 i386_stack_tramp_frame_sniffer (const struct frame_unwind *self,
-			     struct frame_info *this_frame,
-			     void **this_prologue_cache)
+				struct frame_info *this_frame,
+				void **this_cache)
 {
   if (frame_relative_level (this_frame) == 0)
     return i386_in_stack_tramp_p (get_frame_arch (this_frame),
@@ -2060,7 +2064,7 @@ static const struct frame_unwind i386_st
   NORMAL_FRAME,
   i386_epilogue_frame_unwind_stop_reason,
   i386_epilogue_frame_this_id,
-  i386_frame_prev_register,
+  i386_epilogue_frame_prev_register,
   NULL, 
   i386_stack_tramp_frame_sniffer
 };
@@ -7311,13 +7315,13 @@ i386_gdbarch_init (struct gdbarch_info i
   set_gdbarch_fetch_pointer_argument (gdbarch, i386_fetch_pointer_argument);
 
   /* 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
+     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
+     to the list before the prologue-based unwinders, so that DWARF
      CFI info will be used if it is available.  */
   dwarf2_append_unwinders (gdbarch);
 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-12 20:57 [PATCH] Fix some i386 unwinder inconcistencies Mark Kettenis
@ 2011-06-13  2:32 ` Yao Qi
  2011-06-13 14:50   ` Mark Kettenis
  2011-06-13 10:49 ` Regression: " Jan Kratochvil
  1 sibling, 1 reply; 16+ messages in thread
From: Yao Qi @ 2011-06-13  2:32 UTC (permalink / raw)
  To: gdb-patches

On 06/13/2011 04:57 AM, Mark Kettenis wrote:
> 2011-06-12  Mark Kettenis  <kettenis@gnu.org>
>  
> 	* i386-tdep.c (i386_epilogue_frame_cache): Simplify code.  Call
> 	get_frame_func instead of get_frame_pc to determine the code
> 	address used to construct the frame ID.
> 	(i386_epilogue_frame_unwind_stop_reason): Fix coding style.
> 	(i386_epilogue_frame_this_id): Likewise.
> 	(i386_epilogue_frame_prev_register): New function.
> 	(i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
> 	(i386_stack_tramp_frame_sniffer): Fix coding style.
> 	(i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
> 	(i386_gdbarch_init): Fix comments.
> 

Looks like you commit two irrelevant changes (simplification and code
style/comment fix) together.  IMO, each commit should be a
self-contained, single-purpose change.  I don't know this rule applies
to GDB development or not.

> -      /* 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 = get_frame_func (this_frame);
>  
> -      /* Cache pc will be the frame func.  */
> -      cache->pc = get_frame_pc (this_frame);
> -
> -      /* The saved %esp will be at cache->base plus 8.  */

I am not sure why this comment is removed, which is still valid to
statement below "cache->saved_sp = cache->base + 8;", even it says
nothing more than the code.

> +      /* At this point the stack looks as if we just entered the
> +	 function, with the return address at the top of the
> +	 stack.  */
> +      sp = get_frame_register_unsigned (this_frame, I386_ESP_REGNUM);
> +      cache->base = sp + cache->sp_offset;
>        cache->saved_sp = cache->base + 8;
> -
> -      /* The saved %eip will be at cache->base plus 4.  */

Why this comment is removed?

-- 
Yao (齐尧)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-12 20:57 [PATCH] Fix some i386 unwinder inconcistencies Mark Kettenis
  2011-06-13  2:32 ` Yao Qi
@ 2011-06-13 10:49 ` Jan Kratochvil
  2011-06-13 15:37   ` Mark Kettenis
                     ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-13 10:49 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Sun, 12 Jun 2011 22:57:30 +0200, Mark Kettenis wrote:
> This diff fixes a few issues with the epilogue and stack tramp unwinders.
> 
> Committed.
> 
> 2011-06-12  Mark Kettenis  <kettenis@gnu.org>
>  
> 	* i386-tdep.c (i386_epilogue_frame_cache): Simplify code.  Call
> 	get_frame_func instead of get_frame_pc to determine the code
> 	address used to construct the frame ID.
> 	(i386_epilogue_frame_unwind_stop_reason): Fix coding style.
> 	(i386_epilogue_frame_this_id): Likewise.
> 	(i386_epilogue_frame_prev_register): New function.
> 	(i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
> 	(i386_stack_tramp_frame_sniffer): Fix coding style.
> 	(i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
> 	(i386_gdbarch_init): Fix comments.

On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
(but not for x86_64):
-PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
+FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
-XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
+XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
-XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
+XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)

due to: commit b304e1f3bb4fee38d829dbd85ea3c0f43399aa7c

This is a regression.

The regressions are all just instances of:

 finish
 Run till exit from #0  func () at ./gdb.base/watchpoint-cond-gone.c:26
-
-Watchpoint 3 deleted because the program has left the block in
-which its expression is valid.
+Error evaluating expression for watchpoint 3
+can't compute CFA for this frame
+Watchpoint 3 deleted.
 0x080483e0 in func () at ./gdb.base/watchpoint-cond-gone.c:28
 28     }
-(gdb) PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
+(gdb) FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint


Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-13  2:32 ` Yao Qi
@ 2011-06-13 14:50   ` Mark Kettenis
  0 siblings, 0 replies; 16+ messages in thread
From: Mark Kettenis @ 2011-06-13 14:50 UTC (permalink / raw)
  To: yao; +Cc: gdb-patches

> Date: Mon, 13 Jun 2011 10:31:59 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 06/13/2011 04:57 AM, Mark Kettenis wrote:
> > 2011-06-12  Mark Kettenis  <kettenis@gnu.org>
> >  
> > 	* i386-tdep.c (i386_epilogue_frame_cache): Simplify code.  Call
> > 	get_frame_func instead of get_frame_pc to determine the code
> > 	address used to construct the frame ID.
> > 	(i386_epilogue_frame_unwind_stop_reason): Fix coding style.
> > 	(i386_epilogue_frame_this_id): Likewise.
> > 	(i386_epilogue_frame_prev_register): New function.
> > 	(i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
> > 	(i386_stack_tramp_frame_sniffer): Fix coding style.
> > 	(i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
> > 	(i386_gdbarch_init): Fix comments.
> > 
> 
> Looks like you commit two irrelevant changes (simplification and code
> style/comment fix) together.  IMO, each commit should be a
> self-contained, single-purpose change.  I don't know this rule applies
> to GDB development or not.

It does.  And I should probably have seperated out the change to use
get_frame_func.  The rest of the changes are all part of a single
logical change to get rid of the inconcistencies that have crept into
this bit of the code.  There is such a thing as splitting a change up
into too many bits.

> > -      /* 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 = get_frame_func (this_frame);
> >  
> > -      /* Cache pc will be the frame func.  */
> > -      cache->pc = get_frame_pc (this_frame);
> > -
> > -      /* The saved %esp will be at cache->base plus 8.  */
> 
> I am not sure why this comment is removed, which is still valid to
> statement below "cache->saved_sp = cache->base + 8;", even it says
> nothing more than the code.

Exactly.  It says nothing more than the code itself.  That isn't
terribly helpful.

> > +      /* At this point the stack looks as if we just entered the
> > +	 function, with the return address at the top of the
> > +	 stack.  */
> > +      sp = get_frame_register_unsigned (this_frame, I386_ESP_REGNUM);
> > +      cache->base = sp + cache->sp_offset;
> >        cache->saved_sp = cache->base + 8;
> > -
> > -      /* The saved %eip will be at cache->base plus 4.  */
> 
> Why this comment is removed?

Because it didn't really say much more than what the code said either.
I replaced it with a comment that adds some useful information by
saying that the return address lives at the top of the stack.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-13 10:49 ` Regression: " Jan Kratochvil
@ 2011-06-13 15:37   ` Mark Kettenis
  2011-06-13 16:11     ` Jan Kratochvil
  2011-06-26  8:41   ` [patch 1/2] Code reformatting for patch 2/2 [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies] Jan Kratochvil
  2011-06-26  8:42   ` [patch 2/2] Disable epilogue unwinders on recent GCCs " Jan Kratochvil
  2 siblings, 1 reply; 16+ messages in thread
From: Mark Kettenis @ 2011-06-13 15:37 UTC (permalink / raw)
  To: jan.kratochvil; +Cc: gdb-patches

> Date: Mon, 13 Jun 2011 12:49:11 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Sun, 12 Jun 2011 22:57:30 +0200, Mark Kettenis wrote:
> > This diff fixes a few issues with the epilogue and stack tramp unwinders.
> > 
> > Committed.
> > 
> > 2011-06-12  Mark Kettenis  <kettenis@gnu.org>
> >  
> > 	* i386-tdep.c (i386_epilogue_frame_cache): Simplify code.  Call
> > 	get_frame_func instead of get_frame_pc to determine the code
> > 	address used to construct the frame ID.
> > 	(i386_epilogue_frame_unwind_stop_reason): Fix coding style.
> > 	(i386_epilogue_frame_this_id): Likewise.
> > 	(i386_epilogue_frame_prev_register): New function.
> > 	(i386_epilogue_frame_unwind): Use i386_epilogue_frame_prev_register.
> > 	(i386_stack_tramp_frame_sniffer): Fix coding style.
> > 	(i386_stack_tramp_frame_unwind): Use i386_epilogue_frame_prev_register.
> > 	(i386_gdbarch_init): Fix comments.
> 
> On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
> (but not for x86_64):
> -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint

Odd, that tests still passes for me on i386-unknown-openbsd4.9.

PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint

Something did change though.  Before my change:

  Run till exit from #0  func () at ../../../src/gdb/testsuite/gdb.base/watchpoint-cond-gone.c:26

  Watchpoint 3 deleted because the program has left the block in
  which its expression is valid.
  0x1c0006d0 in func () at ../../../src/gdb/testsuite/gdb.base/watchpoint-cond-gone.c:28
  28      }

So the watchpoint went out of scope before the function returned.  Whereas after my change:

  Run till exit from #0  func () at ../../../src/gdb/testsuite/gdb.base/watchpoint-cond-gone.c:26

  Watchpoint 3 deleted because the program has left the block in
  which its expression is valid.
  0x1c000707 in jumper ()

the watchpoint went out of scope when the function returned, as I believe it should.

> -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)

Ok, I'm seeing these as well.  Didn't classify these as a regression
since they went from XFAIL to XFAIL.  They seem to be related to the
fact that I changed get_frame_pc into get_frame_func.  That change is
correct though.  The frame ID returned by the epilogue unwinder should
be identical to the one returned by the "normal", so the code address
needs to be the start of the function and not the address of the 'ret'
instruction.

> The regressions are all just instances of:
> 
>  finish
>  Run till exit from #0  func () at ./gdb.base/watchpoint-cond-gone.c:26
> -
> -Watchpoint 3 deleted because the program has left the block in
> -which its expression is valid.
> +Error evaluating expression for watchpoint 3
> +can't compute CFA for this frame
> +Watchpoint 3 deleted.

I think this can be avoided by implementing the
in_function_epilogue_p() gdbarch method for i386/amd64.  In fact, that
method already seems to be implemented.  It just isn't registered.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-13 15:37   ` Mark Kettenis
@ 2011-06-13 16:11     ` Jan Kratochvil
  2011-06-13 19:10       ` Mark Kettenis
  0 siblings, 1 reply; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-13 16:11 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Mon, 13 Jun 2011 17:37:01 +0200, Mark Kettenis wrote:
> > -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> 
> Odd, that tests still passes for me on i386-unknown-openbsd4.9.

From the error message
> > +can't compute CFA for this frame
on Fedora I guess it may be because the compiler there does not produce the
optimized DWARF form:
    <35>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)


> PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> 
> Something did change though.  Before my change:

In both cases on Fedora it stops at the same place:
0x080483e0 in func () at ./gdb.base/watchpoint-cond-gone.c:28^M

Which seems correct to me as it is -O0 -g code, therefore without variable
tracking for instruction-precise DW_AT_location, therefore variables become
invalid by the `leave' instruction:

 80483df:       c9                      leave
 80483e0:       c3                      ret


> Something did change though.  Before my change:
> So the watchpoint went out of scope before the function returned.

Yes, because the frame got destroyed, the variable is no longer valid.


>   Whereas after my change:
> the watchpoint went out of scope when the function returned, as I believe it should.

I do not agree, when you stop at the `ret' instruction above the watchpoint
should be already deleted there because its watched variable is no longer
valid.


> > -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> > -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)
> 
> Ok, I'm seeing these as well.  Didn't classify these as a regression
> since they went from XFAIL to XFAIL.  They seem to be related to the
> fact that I changed get_frame_pc into get_frame_func.  That change is
> correct though.

If you believe it is correct then you should fix the testcase.
I find "(stopped at wrong place)"->"(unknown output after running)" to be
a regression.

But as there is printed:
+&"can't compute CFA for this frame\n"^M
I find the message as a regression on its own.


> I think this can be avoided by implementing the
> in_function_epilogue_p() gdbarch method for i386/amd64.  In fact, that
> method already seems to be implemented.  It just isn't registered.

After `leave' the local variables are no longer valid, they are already
located under $sp and can be overwritten by any interrupt that time, no GDB
unwinder can fix it.

Also for the epilogue unwinder you would need to somehow fix:
1441	  /* This restriction could be lifted if other unwinders are known to
1442	     compute the frame base in a way compatible with the DWARF
1443	     unwinder.  */
1444	  if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
1445	    error (_("can't compute CFA for this frame"));



Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-13 16:11     ` Jan Kratochvil
@ 2011-06-13 19:10       ` Mark Kettenis
  2011-06-13 20:46         ` Jan Kratochvil
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Kettenis @ 2011-06-13 19:10 UTC (permalink / raw)
  To: jan.kratochvil; +Cc: gdb-patches

> Date: Mon, 13 Jun 2011 18:11:14 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Mon, 13 Jun 2011 17:37:01 +0200, Mark Kettenis wrote:
> > > -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > > +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > 
> > Odd, that tests still passes for me on i386-unknown-openbsd4.9.
> 
> > PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > 
> > Something did change though.  Before my change:
> 
> In both cases on Fedora it stops at the same place:
> 0x080483e0 in func () at ./gdb.base/watchpoint-cond-gone.c:28^M

Are you sure?  If that's the case, there must be debug info that tells
GDB that the watchpoint goes out of scope.  Smells like there is a
flaw in the watchpoint code where it notices that the watchpoint goes
out of scope, but still tries to evaluate the watchpoint condition.

> Which seems correct to me as it is -O0 -g code, therefore without variable
> tracking for instruction-precise DW_AT_location, therefore variables become
> invalid by the `leave' instruction:
> 
>  80483df:       c9                      leave
>  80483e0:       c3                      ret
> 
> 
> > Something did change though.  Before my change:
> > So the watchpoint went out of scope before the function returned.
> 
> Yes, because the frame got destroyed, the variable is no longer valid.

True.

> >   Whereas after my change:
> > the watchpoint went out of scope when the function returned, as I believe it should.
> 
> I do not agree, when you stop at the `ret' instruction above the watchpoint
> should be already deleted there because its watched variable is no longer
> valid.

Frame IDs are nothing but a unique identifier for a particular
invocation of a function.  We're still inside that function when we're
at the 'ret' instruction, not in a different function, so the frame ID
should be the same.  This is how the frame unwinding code was
designed.  There is other code in GDB that depends on this.

> > > -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > > +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> > > -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > > +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)
> > 
> > Ok, I'm seeing these as well.  Didn't classify these as a regression
> > since they went from XFAIL to XFAIL.  They seem to be related to the
> > fact that I changed get_frame_pc into get_frame_func.  That change is
> > correct though.
> 
> If you believe it is correct then you should fix the testcase.
> I find "(stopped at wrong place)"->"(unknown output after running)" to be
> a regression.

Probably.  The MI tests are mostly pure magic to me though :(.

> > I think this can be avoided by implementing the
> > in_function_epilogue_p() gdbarch method for i386/amd64.  In fact, that
> > method already seems to be implemented.  It just isn't registered.
> 
> After `leave' the local variables are no longer valid, they are already
> located under $sp and can be overwritten by any interrupt that time, no GDB
> unwinder can fix it.

Right.  But it is not the job of the unwinder to fix this.

There should be debug info to tell us exactly when a certain variable
goes out of scope, and the breakpoint/watchpoint code should use it.
In absence of that debug info, assuming that the watchpoint goes out
of scope when the function returns, combined with the
in_function_epilogue_p() check will have to do the job.

> Also for the epilogue unwinder you would need to somehow fix:
> 1441	  /* This restriction could be lifted if other unwinders are known to
> 1442	     compute the frame base in a way compatible with the DWARF
> 1443	     unwinder.  */
> 1444	  if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> 1445	    error (_("can't compute CFA for this frame"));

All unwinders are supposed to return a frame base that is "compatible"
amongst unwinders, including the DWARF one.  Now that may be tricky if
compilers don't agree on what the frame base (CFA) is.  But we should
get this right for GCC, and that's all I care about.  If you'd ask me,
that check should be removed.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies
  2011-06-13 19:10       ` Mark Kettenis
@ 2011-06-13 20:46         ` Jan Kratochvil
  0 siblings, 0 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-13 20:46 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Mon, 13 Jun 2011 21:10:28 +0200, Mark Kettenis wrote:
> > In both cases on Fedora it stops at the same place:
> > 0x080483e0 in func () at ./gdb.base/watchpoint-cond-gone.c:28^M
> 
> Are you sure?  If that's the case, there must be debug info that tells
> GDB that the watchpoint goes out of scope.

It stops because the original frame changes and in watchpoint_check
within_current_scope becomes 0.  This is because code_addr is different but
your patch fixed code_addr so it no longer gets trapped.

You should also have provided a testcase showing a PASS->FAIL on the epilogue
code_addr fix.


> Smells like there is a
> flaw in the watchpoint code where it notices that the watchpoint goes
> out of scope, but still tries to evaluate the watchpoint condition.

BTW I do not say how many flaws are in GDB, there are many.  But so far the
functionality worked and now it does not.  So either we find a simple fix soon
enough or one should revert the patch.  Cross-comparison of various known
regressions get complicated.


> There should be debug info to tell us exactly when a certain variable
> goes out of scope, and the breakpoint/watchpoint code should use it.

For -O0 -g code the debug info is not perfect per instruction - this is why
for example the prologues need to be skipped.


> In absence of that debug info, assuming that the watchpoint goes out
> of scope when the function returns, combined with the
> in_function_epilogue_p() check will have to do the job.

Yes but you broke a functionality depending on existing bugs so you should
have also fixed these associated problems not visible before.


> > Also for the epilogue unwinder you would need to somehow fix:
> > 1441	  /* This restriction could be lifted if other unwinders are known to
> > 1442	     compute the frame base in a way compatible with the DWARF
> > 1443	     unwinder.  */
> > 1444	  if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> > 1445	    error (_("can't compute CFA for this frame"));
> 
> All unwinders are supposed to return a frame base that is "compatible"
> amongst unwinders, including the DWARF one.  Now that may be tricky if
> compilers don't agree on what the frame base (CFA) is.  But we should
> get this right for GCC, and that's all I care about.  If you'd ask me,
> that check should be removed.

I agree, CFA is computed for the same address in all unwinders I have seen so
far.


Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch 1/2] Code reformatting for patch 2/2  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-13 10:49 ` Regression: " Jan Kratochvil
  2011-06-13 15:37   ` Mark Kettenis
@ 2011-06-26  8:41   ` Jan Kratochvil
  2011-06-29 22:20     ` Jan Kratochvil
  2011-06-26  8:42   ` [patch 2/2] Disable epilogue unwinders on recent GCCs " Jan Kratochvil
  2 siblings, 1 reply; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-26  8:41 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Mon, 13 Jun 2011 12:49:11 +0200, Jan Kratochvil wrote:
> On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
> (but not for x86_64):
> -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)

Nothing interesting in this part, no functionality change intended.


Thanks,
Jan


gdb/
2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup - reformatting.
	* dwarf2read.c (producer_is_gcc_ge_4_0): Rename to ...
	(producer_is_gcc_ge_4): ... here, change the return value.
	(process_full_comp_unit): New variable gcc_4_minor, adjust the value
	interpretation.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4646,10 +4646,12 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
     }
 }
 
-/* Check for GCC >= 4.0.  */
+/* Check for GCC >= 4.x.  Return minor version (x) of 4.x in such case.  If it
+   is not GCC or it is GCC older than 4.x return -1.  If it is GCC 5.x or
+   higher return INT_MAX.  */
 
 static int
-producer_is_gcc_ge_4_0 (struct dwarf2_cu *cu)
+producer_is_gcc_ge_4 (struct dwarf2_cu *cu)
 {
   const char *cs;
   int major, minor;
@@ -4660,7 +4662,7 @@ producer_is_gcc_ge_4_0 (struct dwarf2_cu *cu)
 	 this case can also happen for -gdwarf-4 type units supported since
 	 gcc-4.5.  */
 
-      return 0;
+      return -1;
     }
 
   /* Skip any identifier after "GNU " - such as "C++" or "Java".  */
@@ -4669,7 +4671,7 @@ producer_is_gcc_ge_4_0 (struct dwarf2_cu *cu)
     {
       /* For non-GCC compilers expect their behavior is not compliant.  */
 
-      return 0;
+      return -1;
     }
   cs = &cu->producer[strlen ("GNU ")];
   while (*cs && !isdigit (*cs))
@@ -4678,10 +4680,14 @@ producer_is_gcc_ge_4_0 (struct dwarf2_cu *cu)
     {
       /* Not recognized as GCC.  */
 
-      return 0;
+      return -1;
     }
 
-  return major >= 4;
+  if (major < 4)
+    return -1;
+  if (major > 4)
+    return INT_MAX;
+  return minor;
 }
 
 /* Generate full symbol information for PST and CU, whose DIEs have
@@ -4725,6 +4731,8 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
 
   if (symtab != NULL)
     {
+      int gcc_4_minor = producer_is_gcc_ge_4 (cu);
+
       /* Set symtab language to language from DW_AT_language.  If the
 	 compilation is from a C file generated by language preprocessors, do
 	 not set the language if it was already deduced by start_subfile.  */
@@ -4741,7 +4749,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
 	 Still one can confuse GDB by using non-standard GCC compilation
 	 options - this waits on GCC PR other/32998 (-frecord-gcc-switches).
 	 */ 
-      if (cu->has_loclist && producer_is_gcc_ge_4_0 (cu))
+      if (cu->has_loclist && gcc_4_minor >= 0)
 	symtab->locations_valid = 1;
     }
 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-13 10:49 ` Regression: " Jan Kratochvil
  2011-06-13 15:37   ` Mark Kettenis
  2011-06-26  8:41   ` [patch 1/2] Code reformatting for patch 2/2 [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies] Jan Kratochvil
@ 2011-06-26  8:42   ` Jan Kratochvil
  2011-06-27  9:39     ` Mark Kettenis
  2011-06-28 19:56     ` Tom Tromey
  2 siblings, 2 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-26  8:42 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Mon, 13 Jun 2011 12:49:11 +0200, Jan Kratochvil wrote:
> On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
> (but not for x86_64):
> -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)

On recent GCCs:
	http://gcc.gnu.org/gcc-4.5/changes.html
	GCC now generates unwind info also for epilogues.

The epilogue unwinders are therefore no longer useful in common cases.
Moreover as they precede the DWARF unwinder they prevent
archer-jankratochvil-entryval (to be submitted these days) functionality at
those return instructions as entryval can provide more constructed parameters
debug info even at those PCs.  It is a bit offtopic now but it causes:


(gdb) bt
#0  d (i=70) at tailcall.c:6
       ^^^^ value provided by archer-jankratochvil-entryval
#1  0x00000000004004ba in c (i=7) at tailcall.c:8
    ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
#2  0x00000000004004d8 in b (i=5) at tailcall.c:12
    ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
#3  0x0000000000400384 in main () at tailcall.c:15
(gdb) disass
Dump of assembler code for function d:
   0x0000000000400490 <+0>:	callq  0x400480 <e>
   0x0000000000400495 <+5>:	mov    0x20046d(%rip),%edi        # 0x600908 <v>
   0x000000000040049b <+11>:	callq  0x400480 <e>
=> 0x00000000004004a0 <+16>:	xor    %eax,%eax
   0x00000000004004a2 <+18>:	retq   
End of assembler dump.
(gdb) bt
#0  d (i=70) at tailcall.c:6
#1  0x00000000004004ba in c (i=7) at tailcall.c:8
#2  0x00000000004004d8 in b (i=5) at tailcall.c:12
#3  0x0000000000400384 in main () at tailcall.c:15

with epilogue unwinder (=without this patch):

(gdb) stepi
0x00000000004004a2 in d (i=<optimized out>) at tailcall.c:6
6	/* line  6 */             return 0; }
(gdb) bt
#0  0x00000000004004a2 in d (i=<optimized out>) at tailcall.c:6
#1  0x0000000000400384 in main () at tailcall.c:15

without epilogue unwinder (=with this patch):

(gdb) stepi
0x00000000004004a2	6	/* line  6 */             return 0; }
(gdb) bt
#0  0x00000000004004a2 in d (i=70) at tailcall.c:6
#1  0x00000000004004ba in c (i=7) at tailcall.c:8
#2  0x00000000004004d8 in b (i=5) at tailcall.c:12
#3  0x0000000000400384 in main () at tailcall.c:15


I find questionable this detection uses DWARF DW_AT_producer.  Therefore on
binaries with .eh_frame but no (DWARF) debug info the detection fails and
epilogue unwinders will get into affect.  But they cause no harm in such case,
archer-jankratochvil-entryval cannot work without DWARF and the erroring code

1441      /* This restriction could be lifted if other unwinders are known to
1442         compute the frame base in a way compatible with the DWARF
1443         unwinder.  */
1444      if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
1445        error (_("can't compute CFA for this frame"));

is also not relevent when no DWARF is available.

No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu, it fixes the
regression reported above.  I will check it in some time if no comments
appear.


Thanks,
Jan


gdb/
2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Disable epilogue unwinders on recent GCCs.
	* amd64-tdep.c (amd64_in_function_epilogue_p): New variable symtab,
	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
	* dwarf2read.c (process_full_comp_unit): Initialize
	EPILOGUE_UNWIND_VALID.
	* i386-tdep.c (i386_in_function_epilogue_p): New variable symtab,
	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
	* symtab.h (struct symtab): New field epilogue_unwind_valid.

--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2219,6 +2219,11 @@ static int
 amd64_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
+  struct symtab *symtab;
+
+  symtab = find_pc_symtab (pc);
+  if (symtab && symtab->epilogue_unwind_valid)
+    return 0;
 
   if (target_read_memory (pc, &insn, 1))
     return 0;   /* Can't read memory at pc.  */
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4751,6 +4751,9 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
 	 */ 
       if (cu->has_loclist && gcc_4_minor >= 0)
 	symtab->locations_valid = 1;
+
+      if (gcc_4_minor >= 5)
+	symtab->epilogue_unwind_valid = 1;
     }
 
   if (dwarf2_per_objfile->using_index)
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1885,6 +1885,11 @@ static int
 i386_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
   gdb_byte insn;
+  struct symtab *symtab;
+
+  symtab = find_pc_symtab (pc);
+  if (symtab && symtab->epilogue_unwind_valid)
+    return 0;
 
   if (target_read_memory (pc, &insn, 1))
     return 0;	/* Can't read memory at pc.  */
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -779,6 +779,11 @@ struct symtab
 
   unsigned int locations_valid : 1;
 
+  /* DWARF unwinder for this CU is valid even for epilogues (PC at the return
+     instruction).  This is supported by GCC since 4.5.0.  */
+
+  unsigned int epilogue_unwind_valid : 1;
+
   /* The macro table for this symtab.  Like the blockvector, this
      may be shared between different symtabs --- and normally is for
      all the symtabs in a given compilation unit.  */


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-26  8:42   ` [patch 2/2] Disable epilogue unwinders on recent GCCs " Jan Kratochvil
@ 2011-06-27  9:39     ` Mark Kettenis
  2011-06-28 20:02       ` Tom Tromey
  2011-06-29 22:26       ` Jan Kratochvil
  2011-06-28 19:56     ` Tom Tromey
  1 sibling, 2 replies; 16+ messages in thread
From: Mark Kettenis @ 2011-06-27  9:39 UTC (permalink / raw)
  To: jan.kratochvil; +Cc: mark.kettenis, gdb-patches

> Date: Sun, 26 Jun 2011 10:41:41 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Mon, 13 Jun 2011 12:49:11 +0200, Jan Kratochvil wrote:
> > On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
> > (but not for x86_64):
> > -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> > -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)
> 
> On recent GCCs:
> 	http://gcc.gnu.org/gcc-4.5/changes.html
> 	GCC now generates unwind info also for epilogues.
> 
> The epilogue unwinders are therefore no longer useful in common cases.

Good news!  The epilogue unwinders will still be useful for debugging
code without debug information.

> Moreover as they precede the DWARF unwinder they prevent
> archer-jankratochvil-entryval (to be submitted these days) functionality at
> those return instructions as entryval can provide more constructed parameters
> debug info even at those PCs.  It is a bit offtopic now but it causes:
> 
> 
> (gdb) bt
> #0  d (i=70) at tailcall.c:6
>        ^^^^ value provided by archer-jankratochvil-entryval
> #1  0x00000000004004ba in c (i=7) at tailcall.c:8
>     ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
> #2  0x00000000004004d8 in b (i=5) at tailcall.c:12
>     ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
> #3  0x0000000000400384 in main () at tailcall.c:15
> (gdb) disass
> Dump of assembler code for function d:
>    0x0000000000400490 <+0>:	callq  0x400480 <e>
>    0x0000000000400495 <+5>:	mov    0x20046d(%rip),%edi        # 0x600908 <v>
>    0x000000000040049b <+11>:	callq  0x400480 <e>
> => 0x00000000004004a0 <+16>:	xor    %eax,%eax
>    0x00000000004004a2 <+18>:	retq   
> End of assembler dump.
> (gdb) bt
> #0  d (i=70) at tailcall.c:6
> #1  0x00000000004004ba in c (i=7) at tailcall.c:8
> #2  0x00000000004004d8 in b (i=5) at tailcall.c:12
> #3  0x0000000000400384 in main () at tailcall.c:15

Interesting.  Don't see how you can reliably do that, but I'm looking
forward to seeing your patch.  Tail-call optimizations often confuse
GDB users.

> I find questionable this detection uses DWARF DW_AT_producer.
> Therefore on binaries with .eh_frame but no (DWARF) debug info the
> detection fails and epilogue unwinders will get into affect.

I think it is an acceptable compromise.  People should expect some
lost functionality when they strip their binaries.  

> 1441      /* This restriction could be lifted if other unwinders are known to
> 1442         compute the frame base in a way compatible with the DWARF
> 1443         unwinder.  */
> 1444      if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> 1445        error (_("can't compute CFA for this frame"));

I still think, this code should be removed.  Tom, since you added that
bit, what's your take on that?

> No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu, it fixes the
> regression reported above.  I will check it in some time if no comments
> appear.

Implementation makes sense to me.  So go ahead once 1/2 is approved
(not really in my area).

> gdb/
> 2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Disable epilogue unwinders on recent GCCs.
> 	* amd64-tdep.c (amd64_in_function_epilogue_p): New variable symtab,
> 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> 	* dwarf2read.c (process_full_comp_unit): Initialize
> 	EPILOGUE_UNWIND_VALID.
> 	* i386-tdep.c (i386_in_function_epilogue_p): New variable symtab,
> 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> 	* symtab.h (struct symtab): New field epilogue_unwind_valid.
> 
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2219,6 +2219,11 @@ static int
>  amd64_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    gdb_byte insn;
> +  struct symtab *symtab;
> +
> +  symtab = find_pc_symtab (pc);
> +  if (symtab && symtab->epilogue_unwind_valid)
> +    return 0;
>  
>    if (target_read_memory (pc, &insn, 1))
>      return 0;   /* Can't read memory at pc.  */
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -4751,6 +4751,9 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
>  	 */ 
>        if (cu->has_loclist && gcc_4_minor >= 0)
>  	symtab->locations_valid = 1;
> +
> +      if (gcc_4_minor >= 5)
> +	symtab->epilogue_unwind_valid = 1;
>      }
>  
>    if (dwarf2_per_objfile->using_index)
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1885,6 +1885,11 @@ static int
>  i386_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    gdb_byte insn;
> +  struct symtab *symtab;
> +
> +  symtab = find_pc_symtab (pc);
> +  if (symtab && symtab->epilogue_unwind_valid)
> +    return 0;
>  
>    if (target_read_memory (pc, &insn, 1))
>      return 0;	/* Can't read memory at pc.  */
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -779,6 +779,11 @@ struct symtab
>  
>    unsigned int locations_valid : 1;
>  
> +  /* DWARF unwinder for this CU is valid even for epilogues (PC at the return
> +     instruction).  This is supported by GCC since 4.5.0.  */
> +
> +  unsigned int epilogue_unwind_valid : 1;
> +
>    /* The macro table for this symtab.  Like the blockvector, this
>       may be shared between different symtabs --- and normally is for
>       all the symtabs in a given compilation unit.  */
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-26  8:42   ` [patch 2/2] Disable epilogue unwinders on recent GCCs " Jan Kratochvil
  2011-06-27  9:39     ` Mark Kettenis
@ 2011-06-28 19:56     ` Tom Tromey
  1 sibling, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2011-06-28 19:56 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Mark Kettenis, gdb-patches

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I find questionable this detection uses DWARF DW_AT_producer.

Me too, but unless GCC emits some other new bit, I don't see how else we
could reasonably get this information.

Tom


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-27  9:39     ` Mark Kettenis
@ 2011-06-28 20:02       ` Tom Tromey
  2011-06-28 20:06         ` Jan Kratochvil
  2011-06-29 22:26       ` Jan Kratochvil
  1 sibling, 1 reply; 16+ messages in thread
From: Tom Tromey @ 2011-06-28 20:02 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: jan.kratochvil, gdb-patches

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

>> 1441      /* This restriction could be lifted if other unwinders are known to
>> 1442         compute the frame base in a way compatible with the DWARF
>> 1443         unwinder.  */
>> 1444      if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
>> 1445        error (_("can't compute CFA for this frame"));

Mark> I still think, this code should be removed.  Tom, since you added that
Mark> bit, what's your take on that?

When I wrote that I was under the impression that the different
unwinders computed the CFA differently.

If that is incorrect, and recent discussion indicates that it is, then I
think it is fine to drop this check.

Actually, the whole purpose of frame_unwinder_is and dwarf2_frame_cfa is
just to do this check.  All that stuff could be removed.

Tom


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-28 20:02       ` Tom Tromey
@ 2011-06-28 20:06         ` Jan Kratochvil
  0 siblings, 0 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-28 20:06 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Mark Kettenis, gdb-patches

On Tue, 28 Jun 2011 22:01:57 +0200, Tom Tromey wrote:
> >>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:
> 
> >> 1441      /* This restriction could be lifted if other unwinders are known to
> >> 1442         compute the frame base in a way compatible with the DWARF
> >> 1443         unwinder.  */
> >> 1444      if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> >> 1445        error (_("can't compute CFA for this frame"));
> 
> Mark> I still think, this code should be removed.  Tom, since you added that
> Mark> bit, what's your take on that?
[...]
> If that is incorrect, and recent discussion indicates that it is, then I
> think it is fine to drop this check.

I have checked that frame_id.stack comment still says:

               Watch out for all the legacy targets that still use the
     function pointer register or stack pointer register.  They are
     wrong.

But I do not have a proof by any target.  My opinion if that counts is to keep
the check there as it should not hurt anything now and I find error message
better than a wrong result.


Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 1/2] Code reformatting for patch 2/2  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-26  8:41   ` [patch 1/2] Code reformatting for patch 2/2 [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies] Jan Kratochvil
@ 2011-06-29 22:20     ` Jan Kratochvil
  0 siblings, 0 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-29 22:20 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Sun, 26 Jun 2011 10:41:10 +0200, Jan Kratochvil wrote:
> gdb/
> 2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Code cleanup - reformatting.
> 	* dwarf2read.c (producer_is_gcc_ge_4_0): Rename to ...
> 	(producer_is_gcc_ge_4): ... here, change the return value.
> 	(process_full_comp_unit): New variable gcc_4_minor, adjust the value
> 	interpretation.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-06/msg00172.html


Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch 2/2] Disable epilogue unwinders on recent GCCs  [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]
  2011-06-27  9:39     ` Mark Kettenis
  2011-06-28 20:02       ` Tom Tromey
@ 2011-06-29 22:26       ` Jan Kratochvil
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kratochvil @ 2011-06-29 22:26 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: gdb-patches

On Mon, 27 Jun 2011 11:38:43 +0200, Mark Kettenis wrote:
> Implementation makes sense to me.  So go ahead once 1/2 is approved
> (not really in my area).
> 
> > gdb/
> > 2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Disable epilogue unwinders on recent GCCs.
> > 	* amd64-tdep.c (amd64_in_function_epilogue_p): New variable symtab,
> > 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> > 	* dwarf2read.c (process_full_comp_unit): Initialize
> > 	EPILOGUE_UNWIND_VALID.
> > 	* i386-tdep.c (i386_in_function_epilogue_p): New variable symtab,
> > 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> > 	* symtab.h (struct symtab): New field epilogue_unwind_valid.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-06/msg00173.html

The -m32 regression got fixed now.


Thanks,
Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-06-29 22:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-12 20:57 [PATCH] Fix some i386 unwinder inconcistencies Mark Kettenis
2011-06-13  2:32 ` Yao Qi
2011-06-13 14:50   ` Mark Kettenis
2011-06-13 10:49 ` Regression: " Jan Kratochvil
2011-06-13 15:37   ` Mark Kettenis
2011-06-13 16:11     ` Jan Kratochvil
2011-06-13 19:10       ` Mark Kettenis
2011-06-13 20:46         ` Jan Kratochvil
2011-06-26  8:41   ` [patch 1/2] Code reformatting for patch 2/2 [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies] Jan Kratochvil
2011-06-29 22:20     ` Jan Kratochvil
2011-06-26  8:42   ` [patch 2/2] Disable epilogue unwinders on recent GCCs " Jan Kratochvil
2011-06-27  9:39     ` Mark Kettenis
2011-06-28 20:02       ` Tom Tromey
2011-06-28 20:06         ` Jan Kratochvil
2011-06-29 22:26       ` Jan Kratochvil
2011-06-28 19:56     ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox