[PATCH] Add a way to mark regions of code which assume that the GC won't run
David Malcolm
dmalcolm@redhat.com
Thu Nov 13 15:39:00 GMT 2014
On Thu, 2014-11-13 at 11:22 +0100, Richard Biener wrote:
> On Wed, Nov 12, 2014 at 11:04 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > 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.
>
> I think rather than "assert-no-gc-here" its name should reflect that
> the caller wants to protect a region from GC (just as if we had
> a thread-safe collector). Thus better name it 'protect_gc'?
> I'd add explicit protect_gc () / unprotect_gc () calls to the RAII
> interface as well - see TODO_do_not_ggc_collect which is probably
> hard to reflect with RAII (the TODO prevents a collection between
> the current and the next pass).
Thanks.
Here's an updated patch that adds protect_gc / unprotect_gc inline fns
to ggc.h, and renames the RAII class to "auto_protect_gc", calling them.
My original intention here was an assertion i.e. something that merely
adds checking to a non-release build, which is what the patch does - I
use it to mark a routine in the JIT that is written with the assumption
that nothing in it can lead to a gcc_collect call (and for which
currently it can't).
However, "protect" to me suggests that this could instead affect the
behavior of ggc_collect, making it immediately return, rather than
merely being an assertion that it wasn't called.
That approach would make ggc_collect safe in such a region, rather than
the attached patch's approach of making it be an assertion failure
(though either approach is better than the status quo of having
unpredictable heap corruption if somehow there is a ggc_collect call in
such a region).
Is this OK for trunk as-is (assuming usual testing), or would you prefer
the "ggc_collect bails out if we're in a protected region" behavior?
(in which case the ENABLE_CHECKING bits of it needs to go away - we
don't want differences between a release vs checked build, especially in
GC, right?).
Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-protect_gc-unprotect_gc-and-class-auto_protect_g.patch
Type: text/x-patch
Size: 5740 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20141113/f9c4dec8/attachment.bin>
More information about the Gcc-patches
mailing list