From patchwork Wed Nov 12 22:04:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 410134 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B7F481400AB for ; Thu, 13 Nov 2014 09:09:31 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version; q=dns; s=default; b=vazaI4HAxE7qc9Lm dVgwFE2ibAJvbrBUlHF80jzgE2sHVr5ROXfl97cMPMn1FPBmaZFqMvJ1mQqRZxvB BQiN0uJNK8V4tPzIq2dmOwm9fWI1R2XRmKplse2DNXcQhf4uTWx73YFd1U4Yjsku 014ZLbg3EaGLiynm5OcDT2CkubQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:cc:date:in-reply-to:references :content-type:mime-version; s=default; bh=svtSy8HQFAb8PtpRaf6zz3 dtOoU=; b=V3z36GGQp34HwO8PHZm6TYh4d4XPHsgXH39UyWnKIA1RsGtPVF5G+r DPJndmxi0sWbTS9k7ObPHk91yX/65yXCbADyOhWvpk4b99eW+h1lNXodUOGaPMye meTASfB6q+BtRI+hY28C8rMEKNC5+d1UxOoBvS+J83ILIrRZn8HEI= Received: (qmail 29127 invoked by alias); 12 Nov 2014 22:09:23 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 29107 invoked by uid 89); 12 Nov 2014 22:09:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 12 Nov 2014 22:09:21 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sACM9Ipo004665 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 12 Nov 2014 17:09:19 -0500 Received: from [10.3.236.104] (vpn-236-104.phx2.redhat.com [10.3.236.104]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sACM9H9s026746; Wed, 12 Nov 2014 17:09:17 -0500 Message-ID: <1415829895.2209.66.camel@surprise> Subject: Re: [PATCH] Add a way to mark regions of code which assume that the GC won't run From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: jit@gcc.gnu.org Date: Wed, 12 Nov 2014 17:04:55 -0500 In-Reply-To: <1415816209-34483-1-git-send-email-dmalcolm@redhat.com> References: <1415816209-34483-1-git-send-email-dmalcolm@redhat.com> Mime-Version: 1.0 X-IsSubscribed: yes On Wed, 2014-11-12 at 13:16 -0500, David Malcolm wrote: > We make assumptions in the codebase about when the GC can run, and when > it can't (e.g. within numerous passes) but these aren't captured in a way > that's verifiable. > > This patch introduces macros GGC_{BEGIN|END}_ASSERT_NO_COLLECTIONS for > marking regions of code where we assume that a GC can't happen, together > with an assert within ggc_collect to verify that we're not within such > a region. > > This lets us both > (A) document code regions where a GC must not happen and > (B) verify in a checked build that these assumptions hold > > The patch also adds an example of using the macros, to the JIT. > > It all compiles away in a release build. > > I'm not fond of the names of the macros, but I can't think of better ones > (suggestions?) > > Successfully bootstrapped and regrtested on x86_64-unknown-linux-gnu. > > OK for trunk? > > gcc/ChangeLog: > * ggc-page.c (ggc_count_of_collection_blocking_assertions): New > variable. > (ggc_collect): Assert that we're not within code regions that are > assuming that the GC isn't going to run. > * ggc.h (GGC_BEGIN_ASSERT_NO_COLLECTIONS): New macro. > (GGC_END_ASSERT_NO_COLLECTIONS): New macro. > (ggc_count_of_collection_blocking_assertions): New variable. > > gcc/jit/ChangeLog: > * jit-playback.c (gcc::jit::playback::context::replay): Add > uses of GGC_{BEGIN|END}_ASSERT_NO_COLLECTIONS to clearly > delimit the region of code in which the GC must not run. > --- > gcc/ggc-page.c | 13 +++++++++++++ > gcc/ggc.h | 39 ++++++++++++++++++++++++++++++++++++++- > gcc/jit/jit-playback.c | 10 +++++++++- > 3 files changed, 60 insertions(+), 2 deletions(-) > > diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c > index f55c4e9..fd185e4 100644 > --- a/gcc/ggc-page.c > +++ b/gcc/ggc-page.c > @@ -2151,11 +2151,24 @@ validate_free_objects (void) > #define validate_free_objects() > #endif > > +#ifdef ENABLE_CHECKING > +int ggc_count_of_collection_blocking_assertions; > +#endif > + > /* Top level mark-and-sweep routine. */ > > void > ggc_collect (void) > { > + /* Ensure that we don't try to garbage-collect in a code region that > + assumes the GC won't run (as guarded by > + GGC_BEGIN_ASSERT_NO_COLLECTIONS). > + > + If this assertion fails, then either ggc_collect was called in a > + region that assumed no GC could occur, or we don't have matching pairs > + of GGC_{BEGIN|END}_ASSERT_NO_COLLECTIONS macros. */ > + gcc_checking_assert (ggc_count_of_collection_blocking_assertions == 0); > + > /* Avoid frequent unnecessary work by skipping collection if the > total allocations haven't expanded much since the last > collection. */ > diff --git a/gcc/ggc.h b/gcc/ggc.h > index dc21520..827a8a5 100644 > --- a/gcc/ggc.h > +++ b/gcc/ggc.h > @@ -363,4 +363,41 @@ gt_pch_nx (unsigned int) > { > } > > -#endif > +/* We don't attempt to track references to GC-allocated entities > + that are on the stack, so the garbage collectior can only run at > + specific times. > + > + The following machinery is available for recording assumptions about > + code regions in which the GC is expected *not* to collect. */ > + > +#if defined ENABLE_CHECKING > + > +/* Begin a region in which it's a bug for a ggc_collect to occur. > + Such regions can be nested. > + Each such region must be terminated with a matching > + GGC_END_ASSERT_NO_COLLECTIONS. */ > + > +#define GGC_BEGIN_ASSERT_NO_COLLECTIONS() \ > + do { ggc_count_of_collection_blocking_assertions++; } while (0) > + > +/* Terminate a region in which ggc_collect is forbidden. */ > + > +#define GGC_END_ASSERT_NO_COLLECTIONS() \ > + do { \ > + gcc_assert (ggc_count_of_collection_blocking_assertions > 0); \ > + ggc_count_of_collection_blocking_assertions--; \ > + } while (0) > + > +/* How many such assertions are active? */ > + > +extern int ggc_count_of_collection_blocking_assertions; > + > +#else /* not defined ENABLE_CHECKING */ > + > +/* Do no checking in a release build. */ > +#define GGC_BEGIN_ASSERT_NO_COLLECTIONS() > +#define GGC_END_ASSERT_NO_COLLECTIONS() > + > +#endif /* not defined ENABLE_CHECKING */ > + > +#endif /* #ifndef GCC_GGC_H */ > diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c > index 285a3ef..2998631 100644 > --- a/gcc/jit/jit-playback.c > +++ b/gcc/jit/jit-playback.c > @@ -1723,6 +1723,8 @@ void > playback::context:: > replay () > { > + GGC_BEGIN_ASSERT_NO_COLLECTIONS (); > + > /* Adapted from c-common.c:c_common_nodes_and_builtins. */ > tree array_domain_type = build_index_type (size_int (200)); > m_char_array_type_node > @@ -1745,7 +1747,11 @@ replay () > > timevar_pop (TV_JIT_REPLAY); > > - if (!errors_occurred ()) > + if (errors_occurred ()) > + { > + GGC_END_ASSERT_NO_COLLECTIONS (); > + } > + else > { > int i; > function *func; > @@ -1761,6 +1767,8 @@ replay () > > /* No GC can have happened yet. */ > > + GGC_END_ASSERT_NO_COLLECTIONS (); > + > /* Postprocess the functions. This could trigger GC. */ > FOR_EACH_VEC_ELT (m_functions, i, func) > { It was pointed out to me on IRC that I could instead use RAII for this, so here's an alternative version of the patch that puts it in a class, so that you can put: auto_assert_no_gc no_gc_here; into a scope to get the assertion failure if someone uses ggc_collect somewhere inside. OK for trunk assuming the bootstrap®rtest pass? Thanks Dave From b2ae5dfaabb515879c46c23b2aa62bbdd328c646 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Wed, 12 Nov 2014 11:34:50 -0500 Subject: [PATCH] Add a RAII class for marking scopes that assume that the GC won't run We make assumptions in the codebase about when the GC can run, and when it can't (e.g. within numerous passes) but these aren't captured in a way that's verifiable. This patch introduces a new RAII-style class auto_assert_no_gc, for marking regions of code where we assume that a GC can't happen, together with an assert within ggc_collect to verify that we're not within such a region. This lets us both (A) document code regions where a GC must not happen and (B) verify in a checked build that these assumptions hold The patch also adds an example of using the class, to the JIT. It all compiles away in a release build. Bootstrap®rtest in progress. OK for trunk assuming it passes? gcc/ChangeLog: * ggc-page.c (auto_assert_no_gc::count): New variable. (ggc_collect): Assert that we're not within code regions that are assuming that the GC isn't going to run. * ggc.h (class auto_assert_no_gc): New class for RAII-style assertions that ggc_collect is not called. gcc/jit/ChangeLog: * jit-playback.c (gcc::jit::playback::context::replay): Split out into... (gcc::jit::playback::context::replay_no_gc): ...new method, adding an instance of auto_assert_no_gc to mark that ggc_collect must not be called within here, and... (gcc::jit::playback::context::replay_could_gc): ...new method. * jit-playback.h (gcc::jit::playback::context::replay_no_gc): New method. (gcc::jit::playback::context::replay_could_gc): New method. --- gcc/ggc-page.c | 12 ++++++++++++ gcc/ggc.h | 36 ++++++++++++++++++++++++++++++++++ gcc/jit/jit-playback.c | 52 ++++++++++++++++++++++++++++++++++++++++---------- gcc/jit/jit-playback.h | 3 +++ 4 files changed, 93 insertions(+), 10 deletions(-) diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c index f55c4e9..2916192 100644 --- a/gcc/ggc-page.c +++ b/gcc/ggc-page.c @@ -2151,11 +2151,23 @@ validate_free_objects (void) #define validate_free_objects() #endif +#ifdef ENABLE_CHECKING +int auto_assert_no_gc::count = 0; +#endif + /* Top level mark-and-sweep routine. */ void ggc_collect (void) { + /* Ensure that we don't try to garbage-collect in a code region that + assumes the GC won't run (as guarded by instances of + class auto_assert_no_gc). + + If this assertion fails, then ggc_collect was called in a + region that assumed no GC could occur. */ + gcc_checking_assert (auto_assert_no_gc::count == 0); + /* Avoid frequent unnecessary work by skipping collection if the total allocations haven't expanded much since the last collection. */ diff --git a/gcc/ggc.h b/gcc/ggc.h index dc21520..1422208 100644 --- a/gcc/ggc.h +++ b/gcc/ggc.h @@ -363,4 +363,40 @@ gt_pch_nx (unsigned int) { } +/* We don't attempt to track references to GC-allocated entities + that are on the stack, so the garbage collectior can only run at + specific times. + + A RAII-style class for marking a scope that assumes GC doesn't happen, + so that you can put: + + auto_assert_no_gc no_gc_here; + + in a scope and have ggc_collect fail with an assertion if it's + called. */ + +class auto_assert_no_gc +{ + public: + auto_assert_no_gc () + { +#if defined ENABLE_CHECKING + count++; #endif + } + + ~auto_assert_no_gc () + { +#if defined ENABLE_CHECKING + gcc_assert (count > 0); + count--; +#endif + } + + private: + static int count; + friend void ggc_collect (void); +}; + + +#endif /* #ifndef GCC_GGC_H */ diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c index 285a3ef..9db01d3 100644 --- a/gcc/jit/jit-playback.c +++ b/gcc/jit/jit-playback.c @@ -1715,14 +1715,34 @@ compile () /* Top-level hook for playing back a recording context. - This plays back m_recording_ctxt, and, if no errors - occurred builds statement lists for and then postprocesses - every function in the result. */ + This is split into the part that assumes the GC can't run, + and the part where ggc_collect could be called. */ void playback::context:: replay () { + replay_no_gc (); + + if (errors_occurred ()) + return; + + replay_could_gc (); +} + +/* The first half of playback::context::replay, which assumes the GC can't + run. + + This plays back m_recording_ctxt, and, if no errors + occurred builds statement lists for every function in the + result. */ + +void +playback::context:: +replay_no_gc () +{ + auto_assert_no_gc no_gc_here; + /* Adapted from c-common.c:c_common_nodes_and_builtins. */ tree array_domain_type = build_index_type (size_int (200)); m_char_array_type_node @@ -1759,14 +1779,26 @@ replay () FOR_EACH_VEC_ELT (m_functions, i, func) func->build_stmt_list (); - /* No GC can have happened yet. */ + } - /* Postprocess the functions. This could trigger GC. */ - FOR_EACH_VEC_ELT (m_functions, i, func) - { - gcc_assert (func); - func->postprocess (); - } + /* No GC can have happened yet. */ +} + +/* The second half of playback::context::replay, in which ggc_collect can + be called. It postprocesses every function. */ + +void +playback::context:: +replay_could_gc () +{ + int i; + function *func; + + /* Postprocess the functions. This could trigger GC. */ + FOR_EACH_VEC_ELT (m_functions, i, func) + { + gcc_assert (func); + func->postprocess (); } } diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h index dcb19bf..63458bc 100644 --- a/gcc/jit/jit-playback.h +++ b/gcc/jit/jit-playback.h @@ -209,6 +209,9 @@ public: } private: + void replay_no_gc (); + void replay_could_gc (); + void dump_generated_code (); rvalue * -- 1.8.5.3