This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix comdat optimized same body virtual destructors (PR c++/42317)
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Jan Hubicka <hubicka at ucw dot cz>
- Cc: Jason Merrill <jason at redhat dot com>, Richard Guenther <rguenther at suse dot de>, gcc-patches at gcc dot gnu dot org
- Date: Tue, 8 Dec 2009 10:47:20 -0500
- Subject: Re: [PATCH] Fix comdat optimized same body virtual destructors (PR c++/42317)
- References: <20091207182839.GR22813@hs20-bc2-1.build.redhat.com> <20091208153519.GA21863@kam.mff.cuni.cz>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
On Tue, Dec 08, 2009 at 04:35:20PM +0100, Jan Hubicka wrote:
> I planned to have comdat groups mixing varpool and cgraph nodes for this.
> Are we never combining variables and functions into single chunk?
Never currently.
> > + /* If we mark !DECL_EXTERNAL one of the symbols in some comdat
> > + group, we need to mark all symbols in the same comdat group
> > + that way. */
>
> I can not approve C++ bits, but I was more thinking of using
> DECL_COMDAT_GROUP in cgraph code + simple assembler name driven hash to
> make cgraph discover this without frontend assistance. I am happy with
> both sollutions (and I don't know if mine works in all cases)
I agree it would be nicer to initialize same_comdat_group automatically
based on DECL_COMDAT_GROUP, but if it is used just in this case in 4.5
a hash table for that might be too expensive.
I believe it shouldn't be really hard to change it to do this automatically
if needed for 4.6 by looking at DECL_COMDAT_GROUP, but IMHO we really want
to have those links in cgraph_node, because querying a hashtable all the
time would be too expensive.
> > @@ -1113,7 +1129,23 @@ cgraph_mark_functions_to_output (void)
> > || (e && node->reachable))
> > && !TREE_ASM_WRITTEN (decl)
> > && !DECL_EXTERNAL (decl))
> > - node->process = 1;
> > + {
> > + node->process = 1;
> > + if (node->same_comdat_group)
> > + {
> > + struct cgraph_node *next;
> > + for (next = node->same_comdat_group;
> > + next != node;
> > + next = next->same_comdat_group)
> > + next->process = 1;
> > + }
> > + }
> > + else if (node->same_comdat_group)
> > + {
> > +#ifdef ENABLE_CHECKING
> > + check_same_comdat_groups = true;
> > +#endif
> > + }
>
> Is this needed? The code should mark everything in callgraph to output
> (i.e. not attempt to solve the reachability problem expecting that unreachable
> nodes are removed earlier). So we should end up outputting everything anyway.
Unfortunately yes, because sometimes the deleting dtors which are only
marked as reachable because complete/base dtor is don't have any callers.
As node->process is only set if node->needed || (e && node->reachable) and
node->needed is often false and for the deleting dtor e is NULL (no
callers), the deleting dtor wouldn't be marked for processing and instead
triggered assertion failure (or the more verbose ENABLE_CHECKING error).
> I believe you will also need to teach cgraph_can_remove_if_node_direct_calls_p
> to get right that node can stay available only because other node in same
> group exists.
I'll look at that function tonight.
Jakub