[IPA] Refactor decl localizing

This patch refactors the decl localizing that happens in 
function_and_variable_visibility.  It doesn't fix the bug I'm working on 
(that's next).

Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very 
similar, but not quite the same code for localizing a definition that 
it's determined need not be externally visible.  It looks to me that the 
not-quite-the-sameness is erroneous, and this patch refactors that code 
into a common subroutine. If the differences need to be maintained 
(slight differences in when unique_name is updated and whether 
resolution is set to LDPR_PREVAILING_DEF_IRONLY), I think a flag to the 
new function would be best, rather than keep the duplicated code.

booted & tested on x86_64-linux, ok?

nathan

Ping?

On 01/06/2017 04:25 PM, Nathan Sidwell wrote:
> This patch refactors the decl localizing that happens in
> function_and_variable_visibility.  It doesn't fix the bug I'm working on
> (that's next).
>
> Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very
> similar, but not quite the same code for localizing a definition that
> it's determined need not be externally visible.  It looks to me that the
> not-quite-the-sameness is erroneous, and this patch refactors that code
> into a common subroutine. If the differences need to be maintained
> (slight differences in when unique_name is updated and whether
> resolution is set to LDPR_PREVAILING_DEF_IRONLY), I think a flag to the
> new function would be best, rather than keep the duplicated code.
>
> booted & tested on x86_64-linux, ok?
>
> nathan

> This patch refactors the decl localizing that happens in
> function_and_variable_visibility.  It doesn't fix the bug I'm working on
> (that's next).
> 
> Both the FOR_EACH_FUNCTION and FOR_EACH_VARIABLE loops contain very similar,
> but not quite the same code for localizing a definition that it's determined
> need not be externally visible.  It looks to me that the
> not-quite-the-sameness is erroneous, and this patch refactors that code into
> a common subroutine. If the differences need to be maintained (slight
> differences in when unique_name is updated and whether resolution is set to
> LDPR_PREVAILING_DEF_IRONLY), I think a flag to the new function would be
> best, rather than keep the duplicated code.
> 
> booted & tested on x86_64-linux, ok?

OK,
the code has indeed grown into quite a mess over the years ;)

Thanks,
Honza
> 
> nathan
> -- 
> Nathan Sidwell

> 2017-01-06  Nathan Sidwell  <nathan@acm.org>
> 
> 	* ipa-visibility.c (localize_node): New function, broken out of ...
> 	(function_and_variable_visibility): Call it.
> 
> Index: ipa-visibility.c
> ===================================================================
> --- ipa-visibility.c	(revision 244159)
> +++ ipa-visibility.c	(working copy)
> @@ -529,6 +529,53 @@ optimize_weakref (symtab_node *node)
>    gcc_assert (node->alias);
>  }
>  
> +/* NODE is an externally visible definition, which we've discovered is
> +   not needed externally.  Make it local to this compilation.  */
> +
> +static void
> +localize_node (bool whole_program, symtab_node *node)
> +{
> +  gcc_assert (whole_program || in_lto_p || !TREE_PUBLIC (node->decl));
> +
> +  if (node->same_comdat_group && TREE_PUBLIC (node->decl))
> +    {
> +      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);
> +	}
> +
> +      /* Now everything's localized, the grouping has no meaning, and
> +	 will cause crashes if we keep it around.  */
> +      node->dissolve_same_comdat_group_list ();
> +    }
> +
> +  node->unique_name
> +    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
> +	 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> +	&& TREE_PUBLIC (node->decl)
> +	&& !flag_incremental_link);
> +
> +  if (TREE_PUBLIC (node->decl))
> +    node->set_comdat_group (NULL);
> +  if (DECL_COMDAT (node->decl) && !node->alias)
> +    node->set_section (NULL);
> +  if (!node->transparent_alias)
> +    {
> +      node->resolution = LDPR_PREVAILING_DEF_IRONLY;
> +      node->make_decl_local ();
> +    }
> +}
> +
>  /* Decide on visibility of all symbols.  */
>  
>  static unsigned int
> @@ -606,48 +653,7 @@ function_and_variable_visibility (bool w
>        if (!node->externally_visible
>  	  && node->definition && !node->weakref
>  	  && !DECL_EXTERNAL (node->decl))
> -	{
> -	  gcc_assert (whole_program || in_lto_p
> -		      || !TREE_PUBLIC (node->decl));
> -	  node->unique_name
> -	    |= ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
> -		 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> -		&& TREE_PUBLIC (node->decl)
> -		&& !flag_incremental_link);
> -	  node->resolution = LDPR_PREVAILING_DEF_IRONLY;
> -	  if (node->same_comdat_group && TREE_PUBLIC (node->decl))
> -	    {
> -	      symtab_node *next = node;
> -
> -	      /* Set all members of comdat group local.  */
> -	      for (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);
> -		}
> -	      /* cgraph_externally_visible_p has already checked all
> -	         other nodes in the group and they will all be made
> -	         local.  We need to dissolve the group at once so that
> -	         the predicate does not segfault though. */
> -	      node->dissolve_same_comdat_group_list ();
> -	    }
> -	  if (TREE_PUBLIC (node->decl))
> -	    node->set_comdat_group (NULL);
> -	  if (DECL_COMDAT (node->decl) && !node->alias)
> -	    node->set_section (NULL);
> -	  if (!node->transparent_alias)
> -	    node->make_decl_local ();
> -	}
> +	localize_node (whole_program, node);
>  
>        if (node->thunk.thunk_p
>  	  && !node->thunk.add_pointer_bounds_args
> @@ -757,49 +763,11 @@ function_and_variable_visibility (bool w
>        if (lookup_attribute ("no_reorder",
>  			    DECL_ATTRIBUTES (vnode->decl)))
>  	vnode->no_reorder = 1;
> +
>        if (!vnode->externally_visible
>  	  && !vnode->transparent_alias)
> -	{
> -	  gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));
> -	  vnode->unique_name |= ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY
> -			          || vnode->resolution
> -				      == LDPR_PREVAILING_DEF_IRONLY_EXP)
> -			         && TREE_PUBLIC (vnode->decl)
> -				 && !flag_incremental_link);
> -	  if (vnode->same_comdat_group && TREE_PUBLIC (vnode->decl))
> -	    {
> -	      symtab_node *next = vnode;
> +	localize_node (whole_program, vnode);
>  
> -	      /* Set all members of comdat group local.  */
> -	      if (vnode->same_comdat_group)
> -		for (next = vnode->same_comdat_group;
> -		     next != vnode;
> -		     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);
> -		    }
> -		}
> -	      vnode->dissolve_same_comdat_group_list ();
> -	    }
> -	  if (TREE_PUBLIC (vnode->decl))
> -	    vnode->set_comdat_group (NULL);
> -	  if (DECL_COMDAT (vnode->decl) && !vnode->alias)
> -	    vnode->set_section (NULL);
> -	  if (!vnode->transparent_alias)
> -	    {
> -	      vnode->make_decl_local ();
> -	      vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
> -	    }
> -	}
>        update_visibility_by_resolution_info (vnode);
>  
>        /* Update virtual tables to point to local aliases where possible.  */