From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id B0B2B3857C4C for ; Wed, 23 Sep 2020 08:31:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B0B2B3857C4C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=hubicka@kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id D94342829A0; Wed, 23 Sep 2020 10:31:01 +0200 (CEST) Date: Wed, 23 Sep 2020 10:31:01 +0200 From: Jan Hubicka To: David Malcolm Cc: gcc-patches@gcc.gnu.org Subject: Re: Issue with ggc_delete and finalizers (was Re: New modref/ipa_modref optimization passes) Message-ID: <20200923083101.GF84756@kam.mff.cuni.cz> References: <20200920173043.GD6758@kam.mff.cuni.cz> <9597b8e20d5b50ad4e501d4506e578107474d55f.camel@redhat.com> <20200922064545.GF91738@kam.mff.cuni.cz> <20200922070731.GA15864@kam.mff.cuni.cz> <6a8bf0b30fff9059afc2c5f7af13b863e3c93dad.camel@redhat.com> <20200922183912.GE49266@kam.mff.cuni.cz> <705d525d56d62aaff4b56f250b5a755948de740b.camel@redhat.com> <45ebbb6c3227dc592823e96e69d82dcd53322aea.camel@redhat.com> <20200922202431.GC84756@kam.mff.cuni.cz> <76365a6d87d9b25f6c29bd1c170fe7b0f460fc09.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <76365a6d87d9b25f6c29bd1c170fe7b0f460fc09.camel@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Sep 2020 08:31:06 -0000 > > Summarizing what's going on: > > We have a use-after-ggc_delete happening with the finalizers code. > > analyze_function has: > > summaries = new (ggc_alloc ()) > modref_summaries (symtab); > > ggc_alloc (as opposed to ggc_alloc_no_dtor) uses need_finalization_p > and "sees" that the class has a nontrivial dtor, and hence it passes > finalize to ggc_internal_alloc as the "f" callback. > > Within ggc_internal_alloc we have: > > if (f) > add_finalizer (result, f, s, n); > > and so that callback is registered within G.finalizers - but there's > nothing stored in the object itself to track that finalizer. > > Later, in ipa_modref_c_finalize, gcc_delete is called on the > mod_summaries object, but the finalizer is still registered in > G.finalizers with its address. > > Later, a GC happens, and if the bit for "marked" on that old > modref_summaries object happens to be cleared (with whatever that > memory is now being used for, if anything), the finalizer callback is > called, and ~modref_summaries is called with its "this" pointing at > random bits, and we have a segfault. > > This seems like a big "gotcha" in ggc_delete: it doesn't remove any > finalizer for the object, instead leaving it as a timebomb to happen in > some future collection. > > Should ggc_delete remove the finalizer? It would have to scan the > G.finalizer vecs to find them - O(N). Alternatively, perhaps we could > stash some kind of reference to the finalizer in memory near the object > (perhaps allocating a header). I think the difference between ggc_free and ggc_delete should be just like the difference between free and delete, that is, ggc_delete should call finalizer. I think the mistake is that ggc_delete would work well with ggc_alloc_no_dtor, but the patch uses ggc_alloc. I guess we do not see the problem in normal compilation since ggc_delete is used in the patch 3 times for objects with no destructor and once for object with a destructor but only at the end of compilation when ggc is not run w/o libjit. > > Or we could put a big warning on ggc_delete saying not to use it on > ggc_alloc-ed objects with dtors. I do not think it is reasonable for ggc_delete to walk all finalizers, so perhaps we just want a sanity check with --enable-checking that things are not mixed up? That is, maintain on-side hash of finalizers introduced by ggc_alloc and look it up in ggc_delete, assert on a spot with a comment saying how things are intended to work? > > I'm not sure what the best approach here is. > > There were only 4 places in the source tree where ggc_delete were used > before the patch, which added 4 places; maybe we should just remove > those new ggc_deletes and set the pointers to NULL and let them be > cleared as regular garbage? For modref we really need to release things explicitly since it runs at WPA time and I am trying to maintain the fact that WPA do not need ggc_collect runs: they have large memory footprint and it is easy to explicitly manage all memory used by symtab/optimization summaries. Of course I can reorganize the code to not have a destructor as well (which is not very ++-sy). In fact since the templates have hand written markers anyway, I was htinking of moving them off ggc memory and just walk to discover the tree pointers in them. Honza > > Dave >