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
