On Wed, Nov 12, 2014 at 11:04 PM, David Malcolm <dmalc...@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®rtest pass?
>
> Thanks
> Dave
