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

From: Pedro Alves <palves@redhat.com>

  On 07/31/2017 11:21 PM, Yao Qi wrote:
> It is required that unwinder->sniffer should set *this_cache to NULL if
> the unwinder is not applicable or exception is thrown, so
> 78ac5f831692f70b841044961069e50d4ba6a76f adds clear_pointer_cleanup to set
> *this_cache to NULL in case of exception in order to fix PR 14100.
> https://sourceware.org/ml/gdb-patches/2012-08/msg00075.html

I suspect that the tailcall sniffer issues Mark was alluding to
in that thread were meanwhile fixed by:

 commit 1ec56e88aa9b052ab10b806d82fbdbc8d153d977
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Fri Nov 22 13:17:46 2013 +0000

    Eliminate dwarf2_frame_cache recursion, don't unwind from the dwarf2 sniffer (move dwarf2_tailcall_sniffer_first elsewhere).

However, Tom's 2nd approach still wouldn't work, because
dwarf2_frame_cache still has other kinds of (benign) recursion.

> This patch removes that clear_pointer_cleanup, and catch all exception in
> the caller of unwinder->sniffer.  In case of exception, reset *this_case.

> @@ -106,8 +106,11 @@ frame_unwind_try_unwinder (struct frame_info *this_frame, void **this_cache,
>      {
>        res = unwinder->sniffer (unwinder, this_frame, this_cache);
>      }
> -  CATCH (ex, RETURN_MASK_ERROR)
> +  CATCH (ex, RETURN_MASK_ALL)
>      {
> +      /* Catch all exceptions, caused by either interrupt or error.
> +	 Reset *THIS_CACHE.  */
> +      *this_cache = NULL;

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;

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

From a6964a47796b5f0b4e9c40a3afa110409c26f668 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 11 Aug 2017 16:27:21 +0100
Subject: [PATCH] frame-obstack

---
 gdb/dwarf2-frame.c | 31 ++++++++++---------------------
 gdb/frame-unwind.h | 27 +++++++++++++++++++++++++++
 gdb/frame.c        |  2 +-
 gdb/frame.h        |  2 ++
 gdb/gdb_obstack.h  | 30 ++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+), 22 deletions(-)

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

Commit Message

Comments

Patch