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

login
register
mail settings
Submitter Pedro Alves
Date Aug. 11, 2017, 4:47 p.m.
Message ID <742591a2-7d2e-f1fb-6cbf-174a27ed02ff@redhat.com>
Download mbox | patch
Permalink /patch/22103/
State New
Headers show

Comments

Pedro Alves - Aug. 11, 2017, 4:47 p.m.
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(-)
Yao Qi - Aug. 17, 2017, 3:27 p.m.
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.

Patch

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