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

[Pedro Alves <palves at redhat dot 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(-)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f8e6522..89e574b 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -919,6 +919,8 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
 
 struct dwarf2_frame_cache
 {
+  explicit dwarf2_frame_cache (frame_info *frame);
+
   /* DWARF Call Frame Address.  */
   CORE_ADDR cfa;
 
@@ -960,24 +962,17 @@ struct dwarf2_frame_cache
   int entry_cfa_sp_offset_p;
 };
 
-static struct dwarf2_frame_cache *
-dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
+dwarf2_frame_cache::dwarf2_frame_cache (struct frame_info *this_frame)
 {
+  struct dwarf2_frame_cache *cache = this;
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   const int num_regs = gdbarch_num_regs (gdbarch)
 		       + gdbarch_num_pseudo_regs (gdbarch);
-  struct dwarf2_frame_cache *cache;
   struct dwarf2_fde *fde;
   CORE_ADDR entry_pc;
   const gdb_byte *instr;
 
-  if (*this_cache)
-    return (struct dwarf2_frame_cache *) *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;
 
   /* Unwind the PC.
 
@@ -1066,7 +1061,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
       if (ex.error == NOT_AVAILABLE_ERROR)
 	{
 	  cache->unavailable_retaddr = 1;
-	  return cache;
+	  return;
 	}
 
       throw_exception (ex);
@@ -1170,16 +1165,13 @@ incomplete CFI data; unspecified registers (e.g., %s) at %s"),
   if (fs.retaddr_column < fs.regs.num_regs
       && fs.regs.reg[fs.retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
     cache->undefined_retaddr = 1;
-
-  return cache;
 }
 
 static enum unwind_stop_reason
 dwarf2_frame_unwind_stop_reason (struct frame_info *this_frame,
 				 void **this_cache)
 {
-  struct dwarf2_frame_cache *cache
-    = dwarf2_frame_cache (this_frame, this_cache);
+  auto *cache = get_frame_cache<dwarf2_frame_cache> (this_frame, this_cache);
 
   if (cache->unavailable_retaddr)
     return UNWIND_UNAVAILABLE;
@@ -1194,8 +1186,7 @@ static void
 dwarf2_frame_this_id (struct frame_info *this_frame, void **this_cache,
 		      struct frame_id *this_id)
 {
-  struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (this_frame, this_cache);
+  auto *cache = get_frame_cache<dwarf2_frame_cache> (this_frame, this_cache);
 
   if (cache->unavailable_retaddr)
     (*this_id) = frame_id_build_unavailable_stack (get_frame_func (this_frame));
@@ -1210,8 +1201,7 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
 			    int regnum)
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
-  struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (this_frame, this_cache);
+  auto *cache = get_frame_cache<dwarf2_frame_cache> (this_frame, this_cache);
   CORE_ADDR addr;
   int realnum;
 
@@ -1316,7 +1306,7 @@ dwarf2_frame_prev_register (struct frame_info *this_frame, void **this_cache,
 static void
 dwarf2_frame_dealloc_cache (struct frame_info *self, void *this_cache)
 {
-  struct dwarf2_frame_cache *cache = dwarf2_frame_cache (self, &this_cache);
+  auto *cache = get_frame_cache<dwarf2_frame_cache> (self, &this_cache);
 
   if (cache->tailcall_cache)
     dwarf2_tailcall_frame_unwind.dealloc_cache (self, cache->tailcall_cache);
@@ -1401,8 +1391,7 @@ dwarf2_append_unwinders (struct gdbarch *gdbarch)
 static CORE_ADDR
 dwarf2_frame_base_address (struct frame_info *this_frame, void **this_cache)
 {
-  struct dwarf2_frame_cache *cache =
-    dwarf2_frame_cache (this_frame, this_cache);
+  auto *cache = get_frame_cache<dwarf2_frame_cache> (this_frame, this_cache);
 
   return cache->cfa;
 }
diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
index 8226b6d..b2c65d0 100644
--- a/gdb/frame-unwind.h
+++ b/gdb/frame-unwind.h
@@ -29,6 +29,7 @@ struct regcache;
 struct value;
 
 #include "frame.h"		/* For enum frame_type.  */
+#include "gdb_obstack.h"
 
 /* The following unwind functions assume a chain of frames forming the
    sequence: (outer) prev <-> this <-> next (inner).  All the
@@ -220,4 +221,30 @@ struct value *frame_unwind_got_bytes (struct frame_info *frame, int regnum,
 struct value *frame_unwind_got_address (struct frame_info *frame, int regnum,
 					CORE_ADDR addr);
 
+template<typename Cache>
+Cache *
+get_frame_cache (struct frame_info *this_frame, void **this_cache)
+{
+  if (*this_cache)
+    return (Cache *) *this_cache;
+
+  /* Allocate a new cache.  */
+  Cache *cache = FRAME_OBSTACK_ZALLOC (Cache);
+
+  /* If an exception happens here, free everything on the obstack up
+     to CACHE.  */
+  scoped_obstack_freer obstack_freer (&frame_cache_obstack, cache);
+
+  /* Install CACHE in *THIS_CACHE, and set up to clear it if something
+     goes wrong.  */
+  scoped_restore restore_this_cache = make_scoped_restore (this_cache);
+  *this_cache = cache;
+
+  cache = new (cache) Cache (this_frame);
+
+  obstack_freer.dont_free ();
+  restore_this_cache.release ();
+  return cache;
+}
+
 #endif
diff --git a/gdb/frame.c b/gdb/frame.c
index 30e4aea..f4f7a5d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1552,7 +1552,7 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
    inferior is stopped.  Control variables for the frame cache should
    be local to this module.  */
 
-static struct obstack frame_cache_obstack;
+struct obstack frame_cache_obstack;
 
 void *
 frame_obstack_zalloc (unsigned long size)
diff --git a/gdb/frame.h b/gdb/frame.h
index 56cbd44..1694241 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -679,6 +679,8 @@ extern void *frame_obstack_zalloc (unsigned long size);
 #define FRAME_OBSTACK_CALLOC(NUMBER,TYPE) \
   ((TYPE *) frame_obstack_zalloc ((NUMBER) * sizeof (TYPE)))
 
+extern struct obstack frame_cache_obstack;
+
 /* Create a regcache, and copy the frame's registers into it.  */
 struct regcache *frame_save_as_regcache (struct frame_info *this_frame);
 
diff --git a/gdb/gdb_obstack.h b/gdb/gdb_obstack.h
index b241c82..b481396 100644
--- a/gdb/gdb_obstack.h
+++ b/gdb/gdb_obstack.h
@@ -78,4 +78,34 @@ struct auto_obstack : obstack
   { obstack_free (this, obstack_base (this)); }
 };
 
+/* RAII object that automatically frees all objects allocated in an
+   obstack starting at a specified object.  Since the obstack is a
+   stack of objects, freeing one object automatically frees all other
+   objects allocated more recently in the same obstack.  */
+class scoped_obstack_freer
+{
+public:
+  scoped_obstack_freer (struct obstack *obstack, void *from_obj)
+    ATTRIBUTE_NONNULL (2) ATTRIBUTE_NONNULL (3)
+    : m_obstack (obstack),
+      m_from_obj (from_obj)
+  {
+    gdb_assert (m_obstack != NULL);
+    gdb_assert (m_from_obj != NULL);
+  }
+
+  ~scoped_obstack_freer ()
+  {
+    if (m_from_obj != NULL)
+      obstack_free (m_obstack, m_from_obj);
+  }
+
+  /* Don't free objects.  */
+  void dont_free () { m_from_obj = NULL; }
+
+private:
+  struct obstack *m_obstack;
+  void *m_from_obj;
+};
+
 #endif
-- 
2.5.5