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

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

[David Malcolm]

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)
 	{
-- 
1.8.5.3

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

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]

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

[-- Attachment #2: 0001-Add-a-RAII-class-for-marking-scopes-that-assume-that.patch --]
[-- Type: text/x-patch, Size: 5872 bytes --]

From b2ae5dfaabb515879c46c23b2aa62bbdd328c646 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
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&regrtest 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

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

[Richard Biener]

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

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

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 7908 bytes --]

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

[-- Attachment #2: 0001-Add-protect_gc-unprotect_gc-and-class-auto_protect_g.patch --]
[-- Type: text/x-patch, Size: 5739 bytes --]

From 9f998238e1a0f37fe6ec834e25c653b3e8149437 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@redhat.com>
Date: Wed, 12 Nov 2014 11:34:50 -0500
Subject: [PATCH] Add protect_gc/unprotect_gc and class auto_protect_gc

gcc/ChangeLog:
	* ggc-page.c (protect_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 (protect_gc_count): New variable.
	(protect_gc): New function.
	(unprotect_gc): New function.
	(class auto_protect_gc): New class for RAII-style matched calls
	to protect_gc and unprotect_gc.

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_protect_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              | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gcc/jit/jit-playback.c | 52 ++++++++++++++++++++++++++++++++++++---------
 gcc/jit/jit-playback.h |  3 +++
 4 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
index f55c4e9..69978a7 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 protect_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 calls to protect_gc and
+     instances of class auto_protect_gc).
+
+     If this assertion fails, then ggc_collect was called in a
+     region that assumed no GC could occur.  */
+  gcc_checking_assert (protect_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..da3e3e8 100644
--- a/gcc/ggc.h
+++ b/gcc/ggc.h
@@ -363,4 +363,61 @@ 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.  */
+
+/* This variable tracks the number of outstanding protect_gc calls.  */
+
+#if defined ENABLE_CHECKING
+extern int protect_gc_count;
+#endif
+
+/* 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 call to
+   unprotect_gc.  */
+
+static inline void
+protect_gc (void)
+{
+#if defined ENABLE_CHECKING
+  protect_gc_count++;
+#endif
+}
+
+/* Terminate a region in which ggc_collect is forbidden.  */
+
+static inline void
+unprotect_gc (void)
+{
+#if defined ENABLE_CHECKING
+    gcc_assert (protect_gc_count > 0);
+    protect_gc_count--;
 #endif
+}
+
+/* A RAII-style class for marking a scope that assumes GC doesn't happen,
+   so that you can put:
+
+      auto_protect_gc nogc;
+
+   in a scope and have ggc_collect fail with an assertion if it's
+   called.  */
+
+class auto_protect_gc
+{
+ public:
+  auto_protect_gc ()
+  {
+    protect_gc ();
+  }
+
+  ~auto_protect_gc ()
+  {
+    unprotect_gc ();
+  }
+};
+
+
+#endif /* #ifndef GCC_GGC_H */
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 285a3ef..1983682 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_protect_gc nogc;
+
   /* 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

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

[Jeff Law]

On 11/13/14 08:28, David Malcolm wrote:

>>> 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.
RAII is good.


>
> 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?).
I'd tend to want an assert so that any such code could be identified 
rather than allowing folks to be lazy.

As for whether or not to change behaviour of the GC system in release vs 
checked builds -- I don't think it's a big deal, at least not in this 
case.  If folks think it's a big deal, then just remove the 
ENABLE_CHECKING bits -- they're so little overhead compared to the 
actual GC system that I wouldn't worry about them at all.

I think the patch is fine as-is.

jeff

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

[Richard Biener]

On Fri, Nov 14, 2014 at 2:19 AM, Jeff Law <law@redhat.com> wrote:
> On 11/13/14 08:28, David Malcolm wrote:
>
>>>> 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.
>
> RAII is good.
>
>
>>
>> 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?).
>
> I'd tend to want an assert so that any such code could be identified rather
> than allowing folks to be lazy.

Well - we do have that scary TODO_do_not_ggc_collect.  It would be
nice to be able to call protect_gc () from ira.c and unprotect_gc ()
from the reload/lra pass and get rid of that TODO.

And yes, a ggc_collect () should be a no-op inside such region (maybe
set a flag, do_ggc_collect_at_unprotect_gc and do what it suggests?)

[un]protect_gc () should also nest properly.

> As for whether or not to change behaviour of the GC system in release vs
> checked builds -- I don't think it's a big deal, at least not in this case.
> If folks think it's a big deal, then just remove the ENABLE_CHECKING bits --
> they're so little overhead compared to the actual GC system that I wouldn't
> worry about them at all.
>
> I think the patch is fine as-is.

So yes, the patchis fine as-is but I'd like to see the above done - removal
of the TODO and making the thing really protect stuff.  Incrementally
I'd like to identify passes that are safe GC-wise, make the collector
thread-safe and push collection to a thread.

Thanks,
Richard.

> jeff

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

[Jeff Law]

On 11/14/14 03:34, Richard Biener wrote:

>
> Well - we do have that scary TODO_do_not_ggc_collect.  It would be
> nice to be able to call protect_gc () from ira.c and unprotect_gc ()
> from the reload/lra pass and get rid of that TODO.
Yea, can't argue with that.

>
> And yes, a ggc_collect () should be a no-op inside such region (maybe
> set a flag, do_ggc_collect_at_unprotect_gc and do what it suggests?)
Basically it's saying "please collect when you can" or a hint of that 
nature.  Given our structure today and the likely location of the 
unprotect calls, I doubt it's worth the effort right now.  But I can see 
it being a lot more useful in the future if we can ensure GC safety of 
some passes.


>
> So yes, the patchis fine as-is but I'd like to see the above done - removal
> of the TODO and making the thing really protect stuff.  Incrementally
> I'd like to identify passes that are safe GC-wise, make the collector
> thread-safe and push collection to a thread.
Yea, all makes sense to me.  Probably good for David's JIT work as well 
to move in this direction.

jeff