This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix comdat optimized same body virtual destructors (PR c++/42317)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]