From patchwork Fri Aug 11 16:47:16 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 22103 Received: (qmail 98817 invoked by alias); 11 Aug 2017 16:47:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 97954 invoked by uid 89); 11 Aug 2017 16:47:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 Aug 2017 16:47:19 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD1D03D3F89; Fri, 11 Aug 2017 16:47:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AD1D03D3F89 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id A7D6A60C16; Fri, 11 Aug 2017 16:47:17 +0000 (UTC) Subject: Re: [PATCH 4/9] Reset *THIS_CACHE in frame_unwind_try_unwinder in case of exception To: Yao Qi , gdb-patches@sourceware.org References: <1501539715-8049-1-git-send-email-yao.qi@linaro.org> <1501539715-8049-5-git-send-email-yao.qi@linaro.org> From: Pedro Alves Message-ID: <742591a2-7d2e-f1fb-6cbf-174a27ed02ff@redhat.com> Date: Fri, 11 Aug 2017 17:47:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1501539715-8049-5-git-send-email-yao.qi@linaro.org> 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 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 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 (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 (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 (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 (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 (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 +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