> > 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> ()) > modref_summaries (symtab); > > ggc_alloc (as opposed to ggc_alloc_no_dtor) uses need_finalization_p<T> > and "sees" that the class has a nontrivial dtor, and hence it passes > finalize<T> 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 >
