Re: [PATCH 4/9] Reset *THIS_CACHE in frame_unwind_try_unwinder in case of exception

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

> If we always do this on error, then why not do it in:
>
>  static void
>  frame_cleanup_after_sniffer (void *arg)
>  {
>    struct frame_info *frame = (struct frame_info *) arg;
>
>    /* The sniffer should not allocate a prologue cache if it did not
>       match this frame.  */
>    gdb_assert (frame->prologue_cache == NULL);
>
> with the comment adjusted?  I.e., replace the assertion with
> frame->prologue_cache = NULL;

Each frame sniffer still has to de-allocate frame->prologue_cache if the
unwinder isn't applicable.  My patch touches the patch on
error/exception, and the assert is to check each unwinder's sniffer has
to release frame->prologue_cache on no-error return.

>
> Still, there's something in the whole try-unwind mechanism that
> isn't handled 100% nicely, that makes me wonder whether this
> central this_cache clearing here is the right approach.
>
> Ant that is the fact that we clear *this_cache, but the dwarf2_frame_cache
> object is left in the frame chain obstack.   Sure, it won't usually

We don't allocate dwarf2_frame_cache in dwarf2_frame_sniffer, so
dwarf2_frame_cache object won't be left in obstack.  However,

 - dwarf2_frame_cache created in dwarf2_frame_{unwinder_stop_reason,
   this_id, prev_register} may be left in obstack in case of exception.

 - other unwinders' sniffer may allocate frame cache, and its frame
   cache may be left on obstack in case of exception.

> be a problem, the memory will be reclaimed at some point later, but
> it still feels like a design hole.  It looks to me like we should
> just wipe everything at was added to the obstack if the unwinder

Such design hole exists for several years.  If we want to fix this
design hole, as you did, we should somehow call obstack_free.  On the
contrary, this shows the benefit of this patch series, that is, we
centralize the exception handling, so we can call obstack_free when
exception is thrown in unwinder apis (see patch #5).  Otherwise (we
catch exceptions in each unwinder) we have to call obstack_free in each
unwinder.

> fails.  I gave it a try, see prototype patch below, to see if something
> would break.  It didn't -- the patch passes regtesting on x86-64,
> at least.  I made get_frame_cache a template since all unwinders
> would do exactly the same.
>
> A simpler, though maybe not as nice approach could
> be to call obstack_free in frame_cleanup_after_sniffer instead:
>
>    if (frame->prologue_cache != NULL)
>      {
>         // assume 
>         obstack_free (&frame_cache_obstack, frame->prologue_cache);
> 	frame->prologue_cache = NULL;
>      }

Your patch get_frame_cache can guarantee the obstack space can be
released when an exception is thrown during initialization.  We need it
for every unwinder.  The simpler approach above looks better, because we
can do it after every unwinder apis call.

-- 
Yao (齐尧)