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

[David Malcolm]

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&regrtest pass?

Thanks
Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-a-RAII-class-for-marking-scopes-that-assume-that.patch
Type: text/x-patch
Size: 5873 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/jit/attachments/20140101/7eeceaa5/attachment.bin>