From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30118 invoked by alias); 14 Nov 2014 10:34:13 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 30012 invoked by uid 89); 14 Nov 2014 10:34:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.98.4 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-ob0-f179.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=wvqOk0I1mLczyJG1tBrg0bvLJ9NrMim/WHXOEbWLEYY=; b=Zyh3jtEKEOwBFHiGnqp6OsMDJrZKe3A3T852hHhpGk99MLGYsU62g5Dx7VOIDxl0OI tbMYmPWyMjnPNxZMscU2U4LYZzIL1nZjaQvX3G2TEKEaWLtlCAaP+N+v8XS23izV65h8 HnPS90depcyw6DgJXFca+QS6DogtepdPwNUnUtOYE1R40zTKYUljzLtPWWK+DIItwkN+ ZNcCko7/4z1GIEehEWJYVQ8v8dKVn4IFDGbVdr9SVWLiAZRm6lMf3xSu4lVqYuU5t8ND 0AszlGCqBZNGr+xtef/lowMATibEKA4Kwe4IeaXkvh9SANOqlcyIpUxs3OAj8FhsQGhn QKfQ== MIME-Version: 1.0 X-Received: by 10.202.227.73 with SMTP id a70mr6679584oih.59.1415961249212; Fri, 14 Nov 2014 02:34:09 -0800 (PST) In-Reply-To: <54655894.8070602@redhat.com> References: <1415816209-34483-1-git-send-email-dmalcolm@redhat.com> <1415829895.2209.66.camel@surprise> <1415892495.2209.120.camel@surprise> <54655894.8070602@redhat.com> Date: Wed, 01 Jan 2014 00:00:00 -0000 Message-ID: Subject: Re: [PATCH] Add a way to mark regions of code which assume that the GC won't run From: Richard Biener To: Jeff Law Cc: David Malcolm , GCC Patches , jit@gcc.gnu.org Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2014-q4/txt/msg00145.txt.bz2 On Fri, Nov 14, 2014 at 2:19 AM, Jeff Law 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