[c++] Avoid C++ FE creating !public weak variables

[c++] Avoid C++ FE creating !public weak variables

[Jan Hubicka]

Hi,
C++ frontend sometimes call make_decl_one_only on a decl and later changes the
mind and in constrain_visibility attempts to make declaration local again.gq
This is not done really properly keeping WEAK or COMMON flag set that later confuse
cgraph and LTO code since it is quite wrong to have private WEAK variables. 

Bootstrapped/regtested x86_64-linux, OK for the C++ part?

	* decl2.c (constrain_visibility): Clear WEAK and COMMON flags.

	* ipa.c (function_and_variable_visibility): Veirfy that WEAK or COMMON
	imply PUBLIC or EXTERNAL.

Index: cp/decl2.c
===================================================================
*** cp/decl2.c	(revision 154095)
--- cp/decl2.c	(working copy)
*************** constrain_visibility (tree decl, int vis
*** 1883,1888 ****
--- 1883,1890 ----
        if (!DECL_EXTERN_C_P (decl))
  	{
  	  TREE_PUBLIC (decl) = 0;
+ 	  DECL_WEAK (decl) = 0;
+ 	  DECL_COMMON (decl) = 0;
  	  DECL_COMDAT_GROUP (decl) = NULL_TREE;
  	  DECL_INTERFACE_KNOWN (decl) = 1;
  	  if (DECL_LANG_SPECIFIC (decl))
Index: ipa.c
===================================================================
*** ipa.c	(revision 154108)
--- ipa.c	(working copy)
*************** function_and_variable_visibility (bool w
*** 292,297 ****
--- 292,298 ----
  
    for (node = cgraph_nodes; node; node = node->next)
      {
+       gcc_assert (!DECL_WEAK (node->decl) || TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node->decl));
        if (cgraph_externally_visible_p (node, whole_program))
          {
  	  gcc_assert (!node->global.inlined_to);
*************** function_and_variable_visibility (bool w
*** 316,321 ****
--- 317,324 ----
      {
        if (!vnode->finalized)
          continue;
+       gcc_assert ((!DECL_WEAK (vnode->decl) || DECL_COMMON (vnode->decl))
+       		  || TREE_PUBLIC (vnode->decl) || DECL_EXTERNAL (node->decl));
        if (vnode->needed
  	  && (DECL_COMDAT (vnode->decl) || TREE_PUBLIC (vnode->decl))
  	  && (!whole_program

Re: [c++] Avoid C++ FE creating !public weak variables

[Mark Mitchell]

Jan Hubicka wrote:

> Bootstrapped/regtested x86_64-linux, OK for the C++ part?
> 
> 	* decl2.c (constrain_visibility): Clear WEAK and COMMON flags.

OK.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [c++] Avoid C++ FE creating !public weak variables

[Jan Hubicka]

Thanks!

With some further sanity checking I noticed that with -fno-weak we also tend to
create DECL_COMDAT !TREE_PUBLIC functions.  This is caused by comdat_linkage function
that simply drops to this path when no weak support is there.
Is this expected behaviour or shall we also clear COMDAT flag here?
Some of cgraph code assumes that COMDAT decls are public, I can fix it on that side too.

Honza

Re: [c++] Avoid C++ FE creating !public weak variables

[Jason Merrill]

On 11/12/2009 02:38 PM, Jan Hubicka wrote:
> With some further sanity checking I noticed that with -fno-weak we also tend to
> create DECL_COMDAT !TREE_PUBLIC functions.  This is caused by comdat_linkage function
> that simply drops to this path when no weak support is there.
> Is this expected behaviour or shall we also clear COMDAT flag here?
> Some of cgraph code assumes that COMDAT decls are public, I can fix it on that side too.

I'm not sure what the best answer is for vague-linkage functions that we 
can't actually share between translation units because of target 
limitations.  There is some code in the front end that expects 
DECL_COMDAT to be set on these decls, though it could be changed to use 
a different test.

Jason

Re: [c++] Avoid C++ FE creating !public weak variables

[Jason Merrill]

On 11/12/2009 02:38 PM, Jan Hubicka wrote:
> With some further sanity checking I noticed that with -fno-weak we also tend to
> create DECL_COMDAT !TREE_PUBLIC functions.  This is caused by comdat_linkage function
> that simply drops to this path when no weak support is there.
> Is this expected behaviour or shall we also clear COMDAT flag here?
> Some of cgraph code assumes that COMDAT decls are public, I can fix it on that side too.

I'm not sure what the best answer is for vague-linkage functions that we 
can't actually share between translation units because of target 
limitations.  There is some code in the front end that expects 
DECL_COMDAT to be set on these decls, though it could be changed to use 
a different test.

Jason

Re: [c++] Avoid C++ FE creating !public weak variables

[Jan Hubicka]

> On 11/12/2009 02:38 PM, Jan Hubicka wrote:
>> With some further sanity checking I noticed that with -fno-weak we also tend to
>> create DECL_COMDAT !TREE_PUBLIC functions.  This is caused by comdat_linkage function
>> that simply drops to this path when no weak support is there.
>> Is this expected behaviour or shall we also clear COMDAT flag here?
>> Some of cgraph code assumes that COMDAT decls are public, I can fix it on that side too.
>
> I'm not sure what the best answer is for vague-linkage functions that we  
> can't actually share between translation units because of target  
> limitations.  There is some code in the front end that expects  
> DECL_COMDAT to be set on these decls, though it could be changed to use  
> a different test.

This is moderately ugly from backend perspective to have random flag set that
make no real difference.  If it would help cp FE, we can also just clear this
flag in the visibility pass where we mangle the flags anyway.

Honza
>
> Jason

Re: [c++] Avoid C++ FE creating !public weak variables

[Jason Merrill]

On 11/12/2009 03:50 PM, Jan Hubicka wrote:
> This is moderately ugly from backend perspective to have random flag set that
> make no real difference.  If it would help cp FE, we can also just clear this
> flag in the visibility pass where we mangle the flags anyway.

That sounds reasonable.

It would be nice to rewrite all the linkage code in the front end; it 
could be simplified a lot now that we're always unit-at-a-time.

Jason

Re: [c++] Avoid C++ FE creating !public weak variables

[Jan Hubicka]

> On 11/12/2009 03:50 PM, Jan Hubicka wrote:
>> This is moderately ugly from backend perspective to have random flag set that
>> make no real difference.  If it would help cp FE, we can also just clear this
>> flag in the visibility pass where we mangle the flags anyway.
>
> That sounds reasonable.
>
> It would be nice to rewrite all the linkage code in the front end; it  
> could be simplified a lot now that we're always unit-at-a-time.

Hi,
this is what I comitted to avoid the ICE. Once we have CP FE cleaned up, we can drop this.

Honza

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 154165)
+++ ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2009-11-13  Jan Hubicka  <jh@suse.cz>
+
+	* ipa.c (function_and_variable_visibility): Clear COMDAT on functions
+	when to PUBLIC is set.
+
 2009-11-13  Jakub Jelinek  <jakub@redhat.com>
 
 	PR middle-end/42029
Index: ipa.c
===================================================================
--- ipa.c	(revision 154165)
+++ ipa.c	(working copy)
@@ -292,6 +292,12 @@ function_and_variable_visibility (bool w
 
   for (node = cgraph_nodes; node; node = node->next)
     {
+      /* C++ FE on lack of COMDAT support create local COMDAT functions
+	 (that ought to be shared but can not due to object format
+	 limitations).  It is neccesary to keep the flag to make rest of C++ FE
+	 happy.  Clear the flag here to avoid confusion in middle-end.  */
+      if (DECL_COMDAT (node->decl) && !TREE_PUBLIC (node->decl))
+        DECL_COMDAT (node->decl) = 0;
       gcc_assert ((!DECL_WEAK (node->decl) && !DECL_COMDAT (node->decl))
       	          || TREE_PUBLIC (node->decl) || DECL_EXTERNAL (node->decl));
       if (cgraph_externally_visible_p (node, whole_program))