[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