Re: getrlimit compatibility issues

[Richard Biener <rguenther at suse dot de>]

On Tue, 1 Sep 2015, Jan Hubicka wrote:

> > > Yep, this looks like a resonable direction. It will break the one declaration
> > > rule in a more wild sense than current frontends does so, because if a builtin
> > > win as a prevailing declaration, we end up with no merging at all.
> > > I wonder if we don't want to always prevail to first non-builtin variant?
> > 
> > I think we already try.  But this can be tricky as seen by the following
> > which is needed to finally fix LTO bootstrap on trunk ...
> > 
> > Basically there are some function decls that do not go through
> > lto-symtab handling because they are not in any CUs cgraph but
> > only exist because they are refered to from BLOCK abstract origins
> > (and thus also LTRANS boundary streaming doesn't force them
> > into the cgraph).  For example during LTO bootstrap I see
> > this from inlining of split functions where both split parts and
> > the original function are optimized away before stream-out.
> 
> Yep, I remember poking around this when I finally stopped my attempts
> to get abstract origins working with LTO :(
> > 
> > We hit
> > 
> > static void
> > gen_inlined_subroutine_die (tree stmt, dw_die_ref context_die)
> > {
> > ...
> >   /* Make sure any inlined functions are known to be inlineable.  */
> >   gcc_checking_assert (DECL_ABSTRACT_P (decl)
> >                        || cgraph_function_possibly_inlined_p (decl));
> > 
> > a lot without the following fixes.  I suppose we _can_ merge
> > function decls which just differ in DECL_POSSIBLY_INLINED if
> > we make sure to "merge" the flag during tree merging.  But
> > that's for a followup, below is for correctness.
> 
> How this is going to work with early debug? If we merge one function to
> another, won't we move the references inside blocks to references to another
> block tree that is possibly incompatible with one we expect?

Hmm, indeed, if we merge the abstract instance FUNCTION_DECL we end
up adjusting the abstract origin DIE reference accordingly but as
we don't merge BLOCKs the actual BLOCK abstract origins would refer
to the "original" early debug abstract instance.  Not sure if that
will cause problems for the debug consumer - the abstract instances
should be the same (literally, unless you play tricks with #ifdefs
in different uses...).

Note that the current patch-set still clears BLOCK_ABSTRACT_ORIGINS
apart from directly referencing FUNCTION_DECL origins for the
inline outer scope.  One step at a time - streaming all block
abstract origins might work now though, didn't check yet.

> > 
> > We were also missing to compare DECL_ABSTRACT_ORIGIN during
> > tree merging (not sure if that caused any issue, I just run
> > into the clone abstract origin issue above and saw that).
> > 
> > LTO bootstrapped and tested on x86_64-unknown-linux-gnu, I'll throw
> > it on regular bootstrap & test and then plan to commit it unless
> > you object that lto-symtab.c hunk...
> > 
> > Richard.
> > 
> > 2015-08-31  Richard Biener  <rguenther@suse.de>
> > 
> > 	lto/
> > 	* lto.c (compare_tree_sccs_1): Compare DECL_ABSTRACT_ORIGIN.
> > 	* lto-symtab.c (lto_symtab_merge): Merge DECL_POSSIBLY_INLINED flag.
> > 	(lto_symtab_prevailing_decl): Do not replace a decl that didn't
> > 	participate in merging with something else.
> > 
> > Index: gcc/lto/lto.c
> > ===================================================================
> > --- gcc/lto/lto.c	(revision 227339)
> > +++ gcc/lto/lto.c	(working copy)
> > @@ -1305,6 +1305,7 @@ compare_tree_sccs_1 (tree t1, tree t2, t
> >        compare_tree_edges (DECL_SIZE (t1), DECL_SIZE (t2));
> >        compare_tree_edges (DECL_SIZE_UNIT (t1), DECL_SIZE_UNIT (t2));
> >        compare_tree_edges (DECL_ATTRIBUTES (t1), DECL_ATTRIBUTES (t2));
> > +      compare_tree_edges (DECL_ABSTRACT_ORIGIN (t1), DECL_ABSTRACT_ORIGIN (t2));
> >        if ((code == VAR_DECL
> >  	   || code == PARM_DECL)
> >  	  && DECL_HAS_VALUE_EXPR_P (t1))
> > Index: gcc/lto/lto-symtab.c
> > ===================================================================
> > --- gcc/lto/lto-symtab.c	(revision 227339)
> > +++ gcc/lto/lto-symtab.c	(working copy)
> > @@ -312,6 +312,11 @@ lto_symtab_merge (symtab_node *prevailin
> >  
> >    if (TREE_CODE (decl) == FUNCTION_DECL)
> >      {
> > +      /* Merge decl state in both directions, we may still end up using
> > +	 the new decl.  */
> > +      DECL_POSSIBLY_INLINED (prevailing_decl) |= DECL_POSSIBLY_INLINED (decl);
> > +      DECL_POSSIBLY_INLINED (decl) |= DECL_POSSIBLY_INLINED (prevailing_decl);
> > +
> >        if (warn_type_compatibility_p (TREE_TYPE (prevailing_decl),
> >  			             TREE_TYPE (decl)))
> >  	return false;
> > @@ -798,6 +803,18 @@ lto_symtab_prevailing_decl (tree decl)
> >    if (TREE_CODE (decl) == FUNCTION_DECL && DECL_ABSTRACT_P (decl))
> >      return decl;
> >  
> > +  /* When decl did not participate in symbol resolution leave it alone.
> > +     This can happen when we streamed the decl as abstract origin
> > +     from the block tree of inlining a partially inlined function.
> > +     If all, the split function and the original function end up
> > +     optimized away early we do not put the abstract origin into the
> > +     ltrans boundary and we'll end up ICEing in
> > +     dwarf2out.c:gen_inlined_subroutine_die because we eventually
> > +     replace a decl with DECL_POSSIBLY_INLINED set with one without.  */
> > +  if (TREE_CODE (decl) == FUNCTION_DECL
> > +      && ! cgraph_node::get (decl))
> > +    return decl;
> 
> So here you care about not merging DECLs that are not part of symtab with
> decls that are part of symtab? I have bit hard time to understand why that
> helps.

It helps because otherwise we end up replacing a DECL_POSSIBLY_INLINED
decl (in the block abstract origins) with a similar decl _not_
DECL_POSSIBLY_INLINED.  I could of course simply transfer the bit here
but that looked fishy.  It's all about that assert in dwarf2out.c...

> The rest of patch makes sense to me modulo the possible issues with moving
> ECL_ABSTRACT_ORIGIN reference to an incompatible blob of dwarf info comming
> from other instance of the inlined function from other translation unit.
> 
> I see you added DECL_ABSTRACT_ORIGIN comparer, but that won't make block numbers
> to match. 

Who cares about block numbers?  Note the abstract origin comparer was
for the clone abstract origins we have.

Richard.