Re: [PATCH] Add a way to mark regions of code which assume that the GC won't run

[Richard Biener <richard dot guenther at gmail dot com>]

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).

Richard.

>
> OK for trunk assuming the bootstrap&regrtest pass?
>
> Thanks
> Dave