On Fri, Nov 14, 2014 at 2:19 AM, Jeff Law <l...@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
