RFC: one approach to fixing PR 14100

RFC: one approach to fixing PR 14100

[Tom Tromey]

PR 14100 concerns a way to crash gdb by C-c during a 'bt'.

The way this happens is that dwarf2_frame_cache initializes the frame's
prologue_cache.  Then, it continues to do some more work, including
(indirectly) reading target memory.

Then, target_read invokes QUIT, throwing an exception.
The cleanups are run, and eventually we get to
frame_cleanup_after_sniffer, which asserts that prologue_cache==NULL.

This fix assumes that what dwarf2_frame_cache is doing is not
unreasonable, and simply clears the prologue_cache field.

I am not sure whether this is really correct.

Another approach would be to change dwarf2_frame_cache to set the
prologue_cache at the end of its work rather than at the beginning.
Then, I suppose, we'd have to document this restriction and audit all
the other sniffers.

Thoughts?

This built and regtested on x86-64 Fedora 16.
Also I tried it by hand and couldn't reproduce the crash.

Tom

	* frame.c (frame_cleanup_after_sniffer): Remove assert.
	Clear frame's prologue_cache.
---
 gdb/frame.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index e012f2d..edb379c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2392,8 +2392,11 @@ frame_cleanup_after_sniffer (void *arg)
   struct frame_info *frame = arg;
 
   /* The sniffer should not allocate a prologue cache if it did not
-     match this frame.  */
-  gdb_assert (frame->prologue_cache == NULL);
+     match this frame.  We used to assert that prologue_cache was NULL
+     here -- however, that ran afoul of code paths where the
+     prologue_cache was set by the sniffer, but some later processing
+     called QUIT.  */
+  frame->prologue_cache = NULL;
 
   /* No sniffer should extend the frame chain; sniff based on what is
      already certain.  */
-- 
1.7.7.6

Re: RFC: one approach to fixing PR 14100

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Another approach would be to change dwarf2_frame_cache to set the
Tom> prologue_cache at the end of its work rather than at the beginning.
Tom> Then, I suppose, we'd have to document this restriction and audit all
Tom> the other sniffers.

Here is a different fix for PR 14100.  It applies on top of my earlier
cleanup fix patch.

I read through all the frame sniffers in gdb.  Of them, only the DWARF
sniffer can potentially set the prologue cache and then be
interrupted.

So, this patch fixes the DWARF sniffer (indirectly, by fixing
dwarf2_frame_cache); and then makes this requirement more clear in
frame-unwind.h.

On the whole I think I prefer this one.

Tom

	* dwarf2-frame.c (dwarf2_frame_cache): Set *this_cache at
	return only.
	* frame-unwind.h (frame_sniffer_ftype): Document prologue
	cache initialization constraint.
---
 gdb/dwarf2-frame.c |    3 ++-
 gdb/frame-unwind.h |    4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 741a103..b716a63 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1016,7 +1016,6 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   /* Allocate a new cache.  */
   cache = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_cache);
   cache->reg = FRAME_OBSTACK_CALLOC (num_regs, struct dwarf2_frame_state_reg);
-  *this_cache = cache;
 
   /* Allocate and initialize the frame state.  */
   fs = XZALLOC (struct dwarf2_frame_state);
@@ -1111,6 +1110,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 	{
 	  cache->unavailable_retaddr = 1;
 	  do_cleanups (old_chain);
+	  *this_cache = cache;
 	  return cache;
 	}
 
@@ -1226,6 +1226,7 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
 				 (entry_cfa_sp_offset_p
 				  ? &entry_cfa_sp_offset : NULL));
 
+  *this_cache = cache;
   return cache;
 }
 
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index f82d763..aa58640 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -44,7 +44,9 @@ struct value;
 
 /* Given THIS frame, take a whiff of its registers (namely
    the PC and attributes) and if SELF is the applicable unwinder,
-   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE.  */
+   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE; but
+   only if returning 1.  Initializing THIS_PROLOGUE_CACHE in other
+   cases (0 return, or exception) is invalid.  */
 
 typedef int (frame_sniffer_ftype) (const struct frame_unwind *self,
 				   struct frame_info *this_frame,
-- 
1.7.7.6

Re: RFC: one approach to fixing PR 14100

[Jan Kratochvil]

On Fri, 03 Aug 2012 18:02:59 +0200, Tom Tromey wrote:
> 	* dwarf2-frame.c (dwarf2_frame_cache): Set *this_cache at
> 	return only.
> 	* frame-unwind.h (frame_sniffer_ftype): Document prologue
> 	cache initialization constraint.

FYI I got now the assertion from ppc64 gdb.threads/watchpoint-fork.exp.
That is without this patch:

#5  0x00000000103e071c in internal_error (file=0x1065c308 "frame.c", line=2396, string=0x1065c2e8 "%s: Assertion `%s' failed.") at utils.c:882
#6  0x00000000103f12f4 in frame_cleanup_after_sniffer (arg=0x100106162f0) at frame.c:2396
#7  0x00000000101dcfc4 in do_my_cleanups (pmy_chain=0x1077cee8, old_chain=0x1060c5f0) at cleanups.c:155
#8  0x00000000101dd0b4 in do_cleanups (old_chain=0x1060c5f0) at cleanups.c:177
#9  0x0000000010279cf0 in throw_exception (exception=...) at exceptions.c:227
#10 0x00000000103f1fe4 in frame_unwind_find_by_frame (this_frame=0x100106162f0, this_cache=0x10010616308) at frame-unwind.c:123
(gdb) p ex
$2 = {reason = RETURN_ERROR, error = GENERIC_ERROR, message = 0x10010a00d50 "reading register r31 (#31): No such process."}

But with this patch it gets endless crashing loop, even on x86*, this is from
ppc64.  This patch could not be x86* regression tested.

#43574 0x00000000103539a0 in dwarf2_frame_cache (this_frame=0x10012c9fc50, this_cache=0x10012c9fc68) at dwarf2-frame.c:1225
#43575 0x0000000010353bd0 in dwarf2_frame_prev_register (this_frame=0x10012c9fc50, this_cache=0x10012c9fc68, regnum=64) at dwarf2-frame.c:1270
#43576 0x00000000103ed22c in frame_unwind_register_value (frame=0x10012c9fc50, regnum=64) at frame.c:952
#43577 0x00000000103ecbd8 in frame_register_unwind (frame=0x10012c9fc50, regnum=64, optimizedp=0xfffe35daa88, unavailablep=0xfffe35daa8c, lvalp= 0xfffe35daa94, addrp=0xfffe35daa98, realnump=0xfffe35daa90, bufferp=0xfffe35dab48 "") at frame.c:858
#43578 0x00000000103ecfd0 in frame_unwind_register (frame=0x10012c9fc50, regnum=64, buf=0xfffe35dab48 "") at frame.c:912
#43579 0x00000000103ed748 in frame_unwind_register_unsigned (frame=0x10012c9fc50, regnum=64) at frame.c:1024
#43580 0x000000001007605c in rs6000_unwind_pc (gdbarch=0x10012cbbc90, next_frame=0x10012c9fc50) at rs6000-tdep.c:3111
#43581 0x00000000102981f4 in gdbarch_unwind_pc (gdbarch=0x10012cbbc90, next_frame=0x10012c9fc50) at gdbarch.c:2839
#43582 0x000000001035848c in dwarf2_tailcall_sniffer_first (this_frame=0x10012c9fc50, tailcall_cachep=0x10012c9fd40, entry_cfa_sp_offsetp=0xfffe35dae28) at dwarf2-frame-tailcall.c:387
#43583 0x00000000103539a0 in dwarf2_frame_cache (this_frame=0x10012c9fc50, this_cache=0x10012c9fc68) at dwarf2-frame.c:1225
#43584 0x00000000103542e0 in dwarf2_frame_sniffer (self=0x1064e840, this_frame=0x10012c9fc50, this_cache=0x10012c9fc68) at dwarf2-frame.c:1403
#43585 0x00000000103f1f88 in frame_unwind_find_by_frame (this_frame=0x10012c9fc50, this_cache=0x10012c9fc68) at frame-unwind.c:112

This is because dwarf2_tailcall_sniffer_first at the end of dwarf2_frame_cache
needs to have THIS_FRAME already finalized, therefore with the cache, as it
needs to access the previous (unwound) frame.


Thanks,
Jan

Re: RFC: one approach to fixing PR 14100

[Tom Tromey]

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

Jan> #5 0x00000000103e071c in internal_error (file=0x1065c308 "frame.c",
Jan> line=2396, string=0x1065c2e8 "%s: Assertion `%s' failed.") at
Jan> utils.c:882
Jan> #6 0x00000000103f12f4 in frame_cleanup_after_sniffer
Jan> (arg=0x100106162f0) at frame.c:2396

It's more useful to see where it was originally thrown; or to see what
sniffer is failing.

Jan> This patch could not be x86* regression tested.

It wasn't.

Jan> This is because dwarf2_tailcall_sniffer_first at the end of
Jan> dwarf2_frame_cache needs to have THIS_FRAME already finalized,
Jan> therefore with the cache, as it needs to access the previous
Jan> (unwound) frame.

Then we need a cleanup instead.
I'll look at it on Monday.

Tom

Re: RFC: one approach to fixing PR 14100

[Jan Kratochvil]

On Sat, 04 Aug 2012 03:58:36 +0200, Tom Tromey wrote:
> It's more useful to see where it was originally thrown; or to see what
> sniffer is failing.

I looked at it interactively, it was dwarf2-frame.c.


> Then we need a cleanup instead.

Yes.


Thanks,
Jan

Re: RFC: one approach to fixing PR 14100

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Then we need a cleanup instead.
Tom> I'll look at it on Monday.

Here it is.
I regression-tested it on x86-64 Fedora 16.

Tom

    	* dwarf2-frame.c (clear_pointer_cleanup): New function.
    	(dwarf2_frame_cache): Use it.
    	* frame-unwind.h (frame_sniffer_ftype): Document prologue
    	cache initialization constraint.

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 741a103..986aaea 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -994,10 +994,20 @@ struct dwarf2_frame_cache
   void *tailcall_cache;
 };
 
+/* A cleanup that sets a pointer to NULL.  */
+
+static void
+clear_pointer_cleanup (void *arg)
+{
+  void **ptr = arg;
+
+  *ptr = NULL;
+}
+
 static struct dwarf2_frame_cache *
 dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 {
-  struct cleanup *old_chain;
+  struct cleanup *reset_cache_cleanup, *old_chain;
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   const int num_regs = gdbarch_num_regs (gdbarch)
 		       + gdbarch_num_pseudo_regs (gdbarch);
@@ -1017,6 +1027,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   cache = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_cache);
   cache->reg = FRAME_OBSTACK_CALLOC (num_regs, struct dwarf2_frame_state_reg);
   *this_cache = cache;
+  reset_cache_cleanup = make_cleanup (clear_pointer_cleanup, this_cache);
 
   /* Allocate and initialize the frame state.  */
   fs = XZALLOC (struct dwarf2_frame_state);
@@ -1111,6 +1122,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 	{
 	  cache->unavailable_retaddr = 1;
 	  do_cleanups (old_chain);
+	  discard_cleanups (reset_cache_cleanup);
 	  return cache;
 	}
 
@@ -1226,6 +1238,7 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
 				 (entry_cfa_sp_offset_p
 				  ? &entry_cfa_sp_offset : NULL));
 
+  discard_cleanups (reset_cache_cleanup);
   return cache;
 }
 
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index f82d763..aa58640 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -44,7 +44,9 @@ struct value;
 
 /* Given THIS frame, take a whiff of its registers (namely
    the PC and attributes) and if SELF is the applicable unwinder,
-   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE.  */
+   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE; but
+   only if returning 1.  Initializing THIS_PROLOGUE_CACHE in other
+   cases (0 return, or exception) is invalid.  */
 
 typedef int (frame_sniffer_ftype) (const struct frame_unwind *self,
 				   struct frame_info *this_frame,

Re: RFC: one approach to fixing PR 14100

[Jan Kratochvil]

On Mon, 06 Aug 2012 17:30:59 +0200, Tom Tromey wrote:
>     	* dwarf2-frame.c (clear_pointer_cleanup): New function.
>     	(dwarf2_frame_cache): Use it.
>     	* frame-unwind.h (frame_sniffer_ftype): Document prologue
>     	cache initialization constraint.

This patch therefore looks OK to me.


Thanks,
Jan

Re: RFC: one approach to fixing PR 14100

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Mon, 06 Aug 2012 09:30:59 -0600
> 
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> Then we need a cleanup instead.
> Tom> I'll look at it on Monday.
> 
> Here it is.
> I regression-tested it on x86-64 Fedora 16.
> 
> Tom
> 
>     	* dwarf2-frame.c (clear_pointer_cleanup): New function.
>     	(dwarf2_frame_cache): Use it.
>     	* frame-unwind.h (frame_sniffer_ftype): Document prologue
>     	cache initialization constraint.

Sorry, but I really think you're working around a problem in the
tailcall sniffer here.  The tailcall sniffer seems to violate several
of the design principles of the frame unwinder framework.  It should
be fixed instead.

> diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
> index 741a103..986aaea 100644
> --- a/gdb/dwarf2-frame.c
> +++ b/gdb/dwarf2-frame.c
> @@ -994,10 +994,20 @@ struct dwarf2_frame_cache
>    void *tailcall_cache;
>  };
>  
> +/* A cleanup that sets a pointer to NULL.  */
> +
> +static void
> +clear_pointer_cleanup (void *arg)
> +{
> +  void **ptr = arg;
> +
> +  *ptr = NULL;
> +}
> +
>  static struct dwarf2_frame_cache *
>  dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
>  {
> -  struct cleanup *old_chain;
> +  struct cleanup *reset_cache_cleanup, *old_chain;
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    const int num_regs = gdbarch_num_regs (gdbarch)
>  		       + gdbarch_num_pseudo_regs (gdbarch);
> @@ -1017,6 +1027,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
>    cache = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_cache);
>    cache->reg = FRAME_OBSTACK_CALLOC (num_regs, struct dwarf2_frame_state_reg);
>    *this_cache = cache;
> +  reset_cache_cleanup = make_cleanup (clear_pointer_cleanup, this_cache);
>  
>    /* Allocate and initialize the frame state.  */
>    fs = XZALLOC (struct dwarf2_frame_state);
> @@ -1111,6 +1122,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
>  	{
>  	  cache->unavailable_retaddr = 1;
>  	  do_cleanups (old_chain);
> +	  discard_cleanups (reset_cache_cleanup);
>  	  return cache;
>  	}
>  
> @@ -1226,6 +1238,7 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
>  				 (entry_cfa_sp_offset_p
>  				  ? &entry_cfa_sp_offset : NULL));
>  
> +  discard_cleanups (reset_cache_cleanup);
>    return cache;
>  }
>  
> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
> index f82d763..aa58640 100644
> --- a/gdb/frame-unwind.h
> +++ b/gdb/frame-unwind.h
> @@ -44,7 +44,9 @@ struct value;
>  
>  /* Given THIS frame, take a whiff of its registers (namely
>     the PC and attributes) and if SELF is the applicable unwinder,
> -   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE.  */
> +   return non-zero.  Possibly also initialize THIS_PROLOGUE_CACHE; but
> +   only if returning 1.  Initializing THIS_PROLOGUE_CACHE in other
> +   cases (0 return, or exception) is invalid.  */
>  
>  typedef int (frame_sniffer_ftype) (const struct frame_unwind *self,
>  				   struct frame_info *this_frame,
> 

Re: RFC: one approach to fixing PR 14100

[Jan Kratochvil]

On Thu, 09 Aug 2012 11:42:28 +0200, Mark Kettenis wrote:
> Sorry, but I really think you're working around a problem in the
> tailcall sniffer here.  The tailcall sniffer seems to violate several
> of the design principles of the frame unwinder framework.  It should
> be fixed instead.

Could you be more specific? The tailcall sniffer is unusual that it needs to
unwind frame (PC) X+1 from frame X and then insert arbitrary number of frames
in between.  So it needs to do "preliminary unwind" first.

I have tried various ways while coding it and I found the current
implementation to fit most the GDB codebase.


Thanks,
Jan

Re: RFC: one approach to fixing PR 14100

[Tom Tromey]

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

Mark> Sorry, but I really think you're working around a problem in the
Mark> tailcall sniffer here.  The tailcall sniffer seems to violate several
Mark> of the design principles of the frame unwinder framework.  It should
Mark> be fixed instead.

Jan> Could you be more specific?

I also don't know what design principles it is violating, but I'd like
to.

Also, FWIW, there's at least one other sniffer that resets the cache to
NULL upon deciding to give up.  I can find it again if it matters.

Tom

Re: RFC: one approach to fixing PR 14100

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Thu, 02 Aug 2012 10:11:47 -0600
> 
> PR 14100 concerns a way to crash gdb by C-c during a 'bt'.
> 
> The way this happens is that dwarf2_frame_cache initializes the frame's
> prologue_cache.  Then, it continues to do some more work, including
> (indirectly) reading target memory.
> 
> Then, target_read invokes QUIT, throwing an exception.
> The cleanups are run, and eventually we get to
> frame_cleanup_after_sniffer, which asserts that prologue_cache==NULL.
> 
> This fix assumes that what dwarf2_frame_cache is doing is not
> unreasonable, and simply clears the prologue_cache field.
> 
> I am not sure whether this is really correct.
> 
> Another approach would be to change dwarf2_frame_cache to set the
> prologue_cache at the end of its work rather than at the beginning.
> Then, I suppose, we'd have to document this restriction and audit all
> the other sniffers.
> 
> Thoughts?

I'd say the gdb_assert() *is* the documentation of the restriction.
So I'd say the 2nd approach is the correct approach.  Setting
prologue_cache at the very end certainly used to be the style that
most frame cache routines used.

> 	* frame.c (frame_cleanup_after_sniffer): Remove assert.
> 	Clear frame's prologue_cache.
> ---
>  gdb/frame.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index e012f2d..edb379c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2392,8 +2392,11 @@ frame_cleanup_after_sniffer (void *arg)
>    struct frame_info *frame = arg;
>  
>    /* The sniffer should not allocate a prologue cache if it did not
> -     match this frame.  */
> -  gdb_assert (frame->prologue_cache == NULL);
> +     match this frame.  We used to assert that prologue_cache was NULL
> +     here -- however, that ran afoul of code paths where the
> +     prologue_cache was set by the sniffer, but some later processing
> +     called QUIT.  */
> +  frame->prologue_cache = NULL;

Not a big fan of these comments that try to document history.  Better
to just state the reason for setting it to NULL.