[WPA] Comdat group splitting

honza,
this is the fix for the partitioned WPA bug I was tracking down.

We have base and complete dtors sharing a comdat group (one's an alias 
for the other).  The linker tells us the complete dtor is 
PREVAILING_DEF, as it's referenced from some other library.  The base 
dtor is UNKNOWN.

We therefore internalize the base dtor, making it PREVAILING_DEF_IRONLY 
and split the comdat group.  But the comdat group splitting also 
internalizes the complete dtor /and/ it does so inconsistently.

The bug manifested at runtime w/o link error as the complete dtor 
resolved to zero in the final link from wpa partitions that didn't 
contain its definition. (For extra fun, that was via a call to 
__cxa_at_exit registering a null function pointer, and getting the 
subsequent seg fault at program exit.)  When we created WPA partitions 
node->externally_visible was still set, so we thought the 
now-internalized complete dtor was still externally visible -- but 
varasm looks at the DECL itself and emits an internal one.  Plus the 
references to it were weak (& now hidden), so resolved to zero, rather 
than link error.  And the external library either had its own definition 
which then prevailed for it.  All rather 'ew'.

Anyway, this patch does 3 things
1) Moves the next->unique_name adjustment to before make_decl_local for 
members of the comdat group -- that matches the behaviour of the decl of 
interest itself.

2) For LDPR_PREVAILING_DEF members we don't make_decl_local, but instead 
clear DECL_COMDAT and DECL_WEAK.  Thus forcing this decl to be the 
prevailing decl in the final link

3) For decls we localize, we also clear node->externally_visible and 
node->force_by_abi.  That matches the behavior for the decl of interest 
too and will clue the wpa partitioning logic into knowing it needs to 
hidden-externalize the decl.

ok?

nathan

> honza,
> this is the fix for the partitioned WPA bug I was tracking down.
> 
> We have base and complete dtors sharing a comdat group (one's an alias for
> the other).  The linker tells us the complete dtor is PREVAILING_DEF, as
> it's referenced from some other library.  The base dtor is UNKNOWN.
> 
> We therefore internalize the base dtor, making it PREVAILING_DEF_IRONLY and
> split the comdat group.  But the comdat group splitting also internalizes
> the complete dtor /and/ it does so inconsistently.
> 
> The bug manifested at runtime w/o link error as the complete dtor resolved
> to zero in the final link from wpa partitions that didn't contain its
> definition. (For extra fun, that was via a call to __cxa_at_exit registering
> a null function pointer, and getting the subsequent seg fault at program
> exit.)  When we created WPA partitions node->externally_visible was still
> set, so we thought the now-internalized complete dtor was still externally
> visible -- but varasm looks at the DECL itself and emits an internal one.
> Plus the references to it were weak (& now hidden), so resolved to zero,
> rather than link error.  And the external library either had its own
> definition which then prevailed for it.  All rather 'ew'.
> 
> Anyway, this patch does 3 things
> 1) Moves the next->unique_name adjustment to before make_decl_local for
> members of the comdat group -- that matches the behaviour of the decl of
> interest itself.

That part is OK.
> 
> 2) For LDPR_PREVAILING_DEF members we don't make_decl_local, but instead
> clear DECL_COMDAT and DECL_WEAK.  Thus forcing this decl to be the
> prevailing decl in the final link
> 
> 3) For decls we localize, we also clear node->externally_visible and
> node->force_by_abi.  That matches the behavior for the decl of interest too
> and will clue the wpa partitioning logic into knowing it needs to
> hidden-externalize the decl.

So at the moment it works in a way
 1) we walk first symbol of the comdat and it is LDPR_PREVAILING_DEF and thus
    we set externall visible flag
 2) we walk second symbol of the comdat and it is LDPR_PREVAILING_DEF_IRONLY
    and thus we decide to privatize the whole comdat group, during this
    process we force the first symbol local and clear the externally_visible
    flag?

I think at a time we decide on external visibility of a symbol in a comdat
group, we need to check that the comdat group as a whole is not exporte (i.e.
no LDPR_PREVAILING_DEF_EXP or incremental linking).  Then we know we can dissolve
the comdat group (without actually affecting visibility) and then we can
handle each symbol independently.

Honza
> 
> nathan
> 
> -- 
> Nathan Sidwell

> 2017-01-18  Nathan Sidwell  <nathan@acm.org>
> 
> 	* ipa-visibility.c (localize_node): Set comdat's unique name
> 	before adjusting resolution. Make PREVAILING_DEF members strongly
> 	public. Set visibility to false for localized decls.
> 
> Index: ipa-visibility.c
> ===================================================================
> --- ipa-visibility.c	(revision 244546)
> +++ ipa-visibility.c	(working copy)
> @@ -542,16 +542,32 @@ localize_node (bool whole_program, symta
>        for (symtab_node *next = node->same_comdat_group;
>  	   next != node; next = next->same_comdat_group)
>  	{
> -	  next->set_comdat_group (NULL);
> -	  if (!next->alias)
> -	    next->set_section (NULL);
> -	  if (!next->transparent_alias)
> -	    next->make_decl_local ();
>  	  next->unique_name
>  	    |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>  		 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
>  		&& TREE_PUBLIC (next->decl)
>  		&& !flag_incremental_link);
> +
> +	  next->set_comdat_group (NULL);
> +	  if (!next->alias)
> +	    next->set_section (NULL);
> +	  if (next->transparent_alias)
> +	    /* Do nothing.  */;
> +	  else if (next->resolution == LDPR_PREVAILING_DEF)
> +	    {
> +	      /* Make this a strong defn, so the external
> +		 users don't mistakenly choose some other
> +		 instance.  */
> +	      DECL_COMDAT (next->decl) = false;
> +	      DECL_WEAK (next->decl) = false;
> +	    }
> +	  else
> +	    {
> +	      next->externally_visible = false;
> +	      next->forced_by_abi = false;
> +	      next->resolution = LDPR_PREVAILING_DEF_IRONLY;
> +	      next->make_decl_local ();
> +	    }
>  	}
>  
>        /* Now everything's localized, the grouping has no meaning, and

On 01/19/2017 10:26 AM, Jan Hubicka wrote:

>> 2) For LDPR_PREVAILING_DEF members we don't make_decl_local, but instead
>> clear DECL_COMDAT and DECL_WEAK.  Thus forcing this decl to be the
>> prevailing decl in the final link
>>
>> 3) For decls we localize, we also clear node->externally_visible and
>> node->force_by_abi.  That matches the behavior for the decl of interest too
>> and will clue the wpa partitioning logic into knowing it needs to
>> hidden-externalize the decl.
>
> So at the moment it works in a way
>  1) we walk first symbol of the comdat and it is LDPR_PREVAILING_DEF and thus
>     we set externall visible flag
>  2) we walk second symbol of the comdat and it is LDPR_PREVAILING_DEF_IRONLY
>     and thus we decide to privatize the whole comdat group, during this
>     process we force the first symbol local and clear the externally_visible
>     flag?

Nearly correct.  We attempt to do #2 but we fail to clear 
node->externally_visible (but we do update DECL_PUBLIC).  Thus the 
PREVAILING_DEF symbol is in an inconsistent state (which is what 
confused the partitioning logic).

> I think at a time we decide on external visibility of a symbol in a comdat
> group, we need to check that the comdat group as a whole is not exporte (i.e.
> no LDPR_PREVAILING_DEF_EXP or incremental linking).  Then we know we can dissolve
> the comdat group (without actually affecting visibility) and then we can
> handle each symbol independently.

Could be.  I did consider splitting this loop into two (which I think is 
an outcome of what you suggest) -- one that set each nodes 
external_visiblilty as desired and the a second loop that did the 
processing.

It also occurs to me that if we did that we'd need to process the 
vardecls in a similar order.  I.e. go from the current:

foreach FN
   determine visibility
   adjust decl/break comdat
for each VAR
   determine visibility
   adjust decl/break comdat

to
foreach FN
   determine visibility
foreach VAR
   determine visibility
foreach FN
   adjust decl/maybe break comdat
foreach VAR
   adjust decl/maybe break comdat

nathan