Re: RFC: one approach to fixing PR 14100

[Mark Kettenis <mark.kettenis@xs4all.nl>]

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