Re: [PATCH] PR rtl-optimization/32219: optimizer causes wrong code in pic/hidden/weak symbol checking

[Richard Henderson <rth at redhat dot com>]

On 02/07/2015 08:45 AM, H.J. Lu wrote:
> Here is an updated patch.

This is an annoying comment to differentiate 3 different versions, FYI.

> @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>        && (TREE_STATIC (exp) || DECL_EXTERNAL (exp)))
>      {
>        varpool_node *vnode = varpool_node::get (exp);
> -      if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition))
> -	resolved_locally = true;
> -      if (vnode
> -	  && resolution_to_local_definition_p (vnode->resolution))
> -	resolved_to_local_def = true;
> +      /* If not building shared library, common or initialized symbols
> +	 are also resolved locally, regardless they are weak or not if
> +	 weak_dominate is true.  */
> +      if (vnode)
> +	{
> +	  if ((weak_dominate && !shlib && vnode->definition)
> +	      || vnode->in_other_partition
> +	      || resolution_local_p (vnode->resolution))
> +	    resolved_locally = true;
> +	  if (resolution_to_local_definition_p (vnode->resolution))
> +	    resolved_to_local_def = true;
> +	}
>      }
>    else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp))
>      {

Not the same test for a function_decl?  That's certainly a mistake.

Indeed, I believe we can now unify these two cases by using the base
symtab_node instead of varpool_node and cgraph_node.

I'm a bit confused about why we have both resolved_to_local_def vs
resolved_locally.  At least within this function, which only cares about module
locality rather than object file locality.  Honza?

> @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>    else if (! TREE_PUBLIC (exp))
>      local_p = true;
>    /* A variable is local if the user has said explicitly that it will
> -     be.  */
> +     be and it is resolved or defined locally, not compiling for PIC,
> +     not weak or weak_dominate is false.  */
>    else if ((DECL_VISIBILITY_SPECIFIED (exp)
>  	    || resolved_to_local_def)
> +	   && (!weak_dominate
> +	       || resolved_locally
> +	       || !flag_pic
> +	       || !DECL_EXTERNAL (exp)
> +	       || !DECL_WEAK (exp))
>  	   && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
>      local_p = true;

I think this test is conflating several issues, and as such it's very confusing.

As an existing issue, I'm not sure why "specified" visibility is any different
from unspecified visibility.  As far as I'm aware, the "specified" bit simply
means that the decl doesn't inherit inherit visibility from the class, or from
the command-line.  But once we're this far, the visibility actually applied to
the symbol should be all that matters.

Unfortunately, this test is 11 years old, from r85167.  It came in as part of
the implementation of the visibility attribute for the class.

I believe the next test should be

  else if (DECL_WEAK (exp) && !resolved_locally)
    local_p = false;

Given the change above, resolved_locally will now be true for (weak && defined
&& weak_dominate), and therefore we need not reiterate that test.

I believe the next test should be dictated by normal non-lto usage like

  extern void bar(void) __attribute__((visibility("hidden")));
  void foo(void) { ... bar(); ...)

where the user is expecting that the function call make use of a simpler
instruction sequence, probably avoiding an PLT.  Therefore

  else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT)
    local_p = true;

Since we have already excluded undef-weak, we know that any symbol here is
either defined or strong, and non-default visibility must resolve it locally.

>    /* Variables defined outside this object might not be local.  */
> @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib)
>    else if (shlib)
>      local_p = false;
>    /* Uninitialized COMMON variable may be unified with symbols
> -     resolved from other modules.  */
> +     resolved from other modules unless weak_dominate is true.  */
>    else if (DECL_COMMON (exp)
> +	   && !weak_dominate
>  	   && !resolved_locally

Like the weak test, surely weak_dominate has already been accounted for.

> @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
>  {
>    /* We output the name if and only if TREE_SYMBOL_REFERENCED is
>       set in order to avoid putting out names that are never really
> -     used. */
> +     used.   Always output visibility specified in the source.  */
>    if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
> -      && targetm.binds_local_p (decl))
> +      && (DECL_VISIBILITY_SPECIFIED (decl)
> +	  || targetm.binds_local_p (decl)))
>      maybe_assemble_visibility (decl);

Explain?

I'm testing the following in the meantime, in order to validate my hypotheses
above.


r~

Attachment: z
Description: Text document