Patchwork Fix PR59626, _FORTIFY_SOURCE wrappers and LTO

login
register
mail settings
Submitter Jan Hubicka
Date March 25, 2014, 12:07 a.m.
Message ID <20140325000713.GA22285@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/333202/
State New
Headers show

Comments

Jan Hubicka - March 25, 2014, 12:07 a.m.
> 
> Currently a _lot_ of packages fail to build with LTO because LTO
> messes up fortify wrappers by replacing the call to the alias with
> the symbol itself, making the wrapper look like infinitely
> recursing.
> 
> The following patch fixes this by dropping the bodies of
> DECL_EXTERN always-inline functions on the floor before IPA
> (and when doing LTO).  IMHO we should at some point do this
> unconditional on LTO as the early inliner is supposed to
> remove all references to always-inline functions.
> 
> I'm not 100% sure about the cgraph API use to drop the function
> body, but at least it seems to work ;)  I'm not sure if we want
> to restrict the set of functions to apply this even more than
> just those with always-inline and DECL_EXTERNAL set?
> 
> Now double-checking with a fortified LTO bootstrap.
> 
> Ok for trunk?
> 
> Thanks,
> Richard.
> 
> 2014-03-24  Richard Biener  <rguenther@suse.de>
> 
> 	PR lto/59626
> 	* passes.c (ipa_write_summaries): Drop function bodies of
> 	extern always-inline functions.
> 
> 	* gcc.dg/lto/pr59626_0.c: New testcase.
> 	* gcc.dg/lto/pr59626_1.c: Likewise.
> 
Hi,
do you see some problem with this approach? Unlike in dropping for ipa_write_summaries
it drops just after early optimizations and it is done unconditonally.
I see it kills every cross module inlining of extern inlines, but so does your patch.
This may make difference for C++ implicit extern inlines (keyed functions) where I can
imagine users both to declare them always inline and then call indirectly...
I do it unconditionally because I do not like much of idea of making the LTO and non-LTO
pipelines diverging even more.

Honza
Richard Guenther - March 25, 2014, 8:54 a.m.
On Tue, 25 Mar 2014, Jan Hubicka wrote:

> > 
> > Currently a _lot_ of packages fail to build with LTO because LTO
> > messes up fortify wrappers by replacing the call to the alias with
> > the symbol itself, making the wrapper look like infinitely
> > recursing.
> > 
> > The following patch fixes this by dropping the bodies of
> > DECL_EXTERN always-inline functions on the floor before IPA
> > (and when doing LTO).  IMHO we should at some point do this
> > unconditional on LTO as the early inliner is supposed to
> > remove all references to always-inline functions.
> > 
> > I'm not 100% sure about the cgraph API use to drop the function
> > body, but at least it seems to work ;)  I'm not sure if we want
> > to restrict the set of functions to apply this even more than
> > just those with always-inline and DECL_EXTERNAL set?
> > 
> > Now double-checking with a fortified LTO bootstrap.
> > 
> > Ok for trunk?
> > 
> > Thanks,
> > Richard.
> > 
> > 2014-03-24  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR lto/59626
> > 	* passes.c (ipa_write_summaries): Drop function bodies of
> > 	extern always-inline functions.
> > 
> > 	* gcc.dg/lto/pr59626_0.c: New testcase.
> > 	* gcc.dg/lto/pr59626_1.c: Likewise.
> > 
> Hi,
> do you see some problem with this approach? Unlike in dropping for ipa_write_summaries
> it drops just after early optimizations and it is done unconditonally.
> I see it kills every cross module inlining of extern inlines, but so does your patch.
> This may make difference for C++ implicit extern inlines (keyed functions) where I can
> imagine users both to declare them always inline and then call indirectly...
> I do it unconditionally because I do not like much of idea of making the LTO and non-LTO
> pipelines diverging even more.

Well - it doesn't fix the testcase for me.

Richard.

> Honza
> 
> Index: ipa.c
> ===================================================================
> --- ipa.c	(revision 208798)
> +++ ipa.c	(working copy)
> @@ -139,7 +139,10 @@ process_references (struct ipa_ref_list
>  
>        if (node->definition && !node->in_other_partition
>  	  && ((!DECL_EXTERNAL (node->decl) || node->alias)
> -	      || (before_inlining_p
> +	      || (((before_inlining_p
> +		    && (cgraph_state < CGRAPH_STATE_IPA_SSA
> +		        || !lookup_attribute ("always_inline",
> +					      DECL_ATTRIBUTES (node->decl)))))
>  		  /* We use variable constructors during late complation for
>  		     constant folding.  Keep references alive so partitioning
>  		     knows about potential references.  */
> @@ -191,7 +194,10 @@ walk_polymorphic_call_targets (pointer_s
>  	  /* Prior inlining, keep alive bodies of possible targets for
>  	     devirtualization.  */
>  	   if (n->definition
> -	       && before_inlining_p)
> +	       && (before_inlining_p
> +		   && (cgraph_state < CGRAPH_STATE_IPA_SSA
> +		       || !lookup_attribute ("always_inline",
> +					     DECL_ATTRIBUTES (n->decl)))))
>  	     pointer_set_insert (reachable, n);
>  
>  	  /* Even after inlining we want to keep the possible targets in the
> 
>
Jan Hubicka - March 25, 2014, 7:01 p.m.
> > > 	PR lto/59626
> > > 	* passes.c (ipa_write_summaries): Drop function bodies of
> > > 	extern always-inline functions.
> > > 
> > > 	* gcc.dg/lto/pr59626_0.c: New testcase.
> > > 	* gcc.dg/lto/pr59626_1.c: Likewise.
> > > 
> > Hi,
> > do you see some problem with this approach? Unlike in dropping for ipa_write_summaries
> > it drops just after early optimizations and it is done unconditonally.
> > I see it kills every cross module inlining of extern inlines, but so does your patch.
> > This may make difference for C++ implicit extern inlines (keyed functions) where I can
> > imagine users both to declare them always inline and then call indirectly...
> > I do it unconditionally because I do not like much of idea of making the LTO and non-LTO
> > pipelines diverging even more.
> 
> Well - it doesn't fix the testcase for me.

A fair complaint ;) I just check that it removes the body of always_inline.  I will check
why it doesn't help.  We probably also need to drop the attribute as you do in ipa-profile.

Honza

Patch

Index: ipa.c
===================================================================
--- ipa.c	(revision 208798)
+++ ipa.c	(working copy)
@@ -139,7 +139,10 @@  process_references (struct ipa_ref_list
 
       if (node->definition && !node->in_other_partition
 	  && ((!DECL_EXTERNAL (node->decl) || node->alias)
-	      || (before_inlining_p
+	      || (((before_inlining_p
+		    && (cgraph_state < CGRAPH_STATE_IPA_SSA
+		        || !lookup_attribute ("always_inline",
+					      DECL_ATTRIBUTES (node->decl)))))
 		  /* We use variable constructors during late complation for
 		     constant folding.  Keep references alive so partitioning
 		     knows about potential references.  */
@@ -191,7 +194,10 @@  walk_polymorphic_call_targets (pointer_s
 	  /* Prior inlining, keep alive bodies of possible targets for
 	     devirtualization.  */
 	   if (n->definition
-	       && before_inlining_p)
+	       && (before_inlining_p
+		   && (cgraph_state < CGRAPH_STATE_IPA_SSA
+		       || !lookup_attribute ("always_inline",
+					     DECL_ATTRIBUTES (n->decl)))))
 	     pointer_set_insert (reachable, n);
 
 	  /* Even after inlining we want to keep the possible targets in the