[PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-18 10:26 +0100, Jan Hubicka wrote:
> Hi,
> sorry I forgot to include cgraph and varpool changes in the patch.
> 
> Index: varpool.c
> ===================================================================
> --- varpool.c	(revision 279467)
> +++ varpool.c	(working copy)
> @@ -539,8 +539,7 @@ varpool_node::assemble_aliases (void)
>      {
>        varpool_node *alias = dyn_cast <varpool_node *> (ref->referring);
>        if (alias->symver)
> -	do_assemble_symver (alias->decl,
> -			    DECL_ASSEMBLER_NAME (decl));
> +	do_assemble_symver (alias->decl, decl);
>        else if (!alias->transparent_alias)
>  	do_assemble_alias (alias->decl,
>  			   DECL_ASSEMBLER_NAME (decl));
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c	(revision 279467)
> +++ cgraphunit.c	(working copy)
> @@ -2222,8 +2222,7 @@ cgraph_node::assemble_thunks_and_aliases
>  	     of buffering it in same alias pairs.  */
>  	  TREE_ASM_WRITTEN (decl) = 1;
>  	  if (alias->symver)
> -	    do_assemble_symver (alias->decl,
> -				DECL_ASSEMBLER_NAME (decl));
> +	    do_assemble_symver (alias->decl, decl);
>  	  else
>  	    do_assemble_alias (alias->decl,
>  			       DECL_ASSEMBLER_NAME (decl));
> Index: varasm.c
> ===================================================================
> --- varasm.c	(revision 279467)
> +++ varasm.c	(working copy)
> @@ -5970,9 +5970,47 @@ do_assemble_symver (tree decl, tree targ
>    ultimate_transparent_alias_target (&id);
>    ultimate_transparent_alias_target (&target);

ICE here.

lto1: internal compiler error: tree check: expected identifier_node, have
function_decl in ultimate_transparent_alias_target, at varasm.c:1308
0x6f9cfe tree_check_failed(tree_node const*, char const*, int, char const*, ...)
	../../gcc/gcc/tree.c:9685
0x714541 tree_check(tree_node*, char const*, int, char const*, tree_code)
	../../gcc/gcc/tree.h:3273
0x714541 ultimate_transparent_alias_target
	../../gcc/gcc/varasm.c:1308
0x714541 do_assemble_symver(tree_node*, tree_node*)
	../../gcc/gcc/varasm.c:5971

>  #ifdef ASM_OUTPUT_SYMVER_DIRECTIVE
> -  ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> -			       IDENTIFIER_POINTER (target),
> -			       IDENTIFIER_POINTER (id));
> +  if (TREE_PUBLIC (target) && DECL_VISIBILITY (target) == VISIBILITY_DEFAULT)
> +    ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> +			         IDENTIFIER_POINTER
> +				   (DECL_ASSEMBLER_NAME (target)),
> +			         IDENTIFIER_POINTER (id));
> +  else
> +    {
> +      int nameend;
> +      for (nameend = 0; IDENTIFIER_POINTER (id)[nameend] != '@'; nameend++)
> +	;
> +      if (IDENTIFIER_POINTER (id)[nameend + 1] != '@'
> +	  || IDENTIFIER_POINTER (id)[nameend + 2] == '@')
> +	{
> +	  sorry_at (DECL_SOURCE_LOCATION (target),
> +		    "can not produce %<symver%> of a symbol that is "
> +		    "not exported with default visibility");
> +	  return;

I think this does not make sense.  Some library authors may export "foo@VER_1"
but not "foo_v1" to ensure the programmers using the library upgrade their code
to use new "correct" ABI, instead of an old one.   This error makes it
impossible.

(Try to comment out "foo_v1" in version.map, in the testcase.)

> +	}
> +      tree tmpdecl = copy_node (decl);
> +      char buf[256];
> +      static int symver_labelno;
> +      targetm.asm_out.generate_internal_label (buf,
> +					       "LSYMVER", symver_labelno++);
> +      SET_DECL_ASSEMBLER_NAME (tmpdecl, get_identifier (buf));
> +      globalize_decl (tmpdecl);
> +#ifdef ASM_OUTPUT_DEF_FROM_DECLS
> +      ASM_OUTPUT_DEF_FROM_DECLS (asm_out_file, tmpdecl,
> +				 DECL_ASSEMBLER_NAME (target));
> +#else
> +      ASM_OUTPUT_DEF (asm_out_file,
> +		      IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (tmpdecl)),
> +		      IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (target)));
> +#endif
> +      memcpy (buf, IDENTIFIER_POINTER (id), nameend + 2);
> +      buf[nameend + 2] = '@';
> +      strcpy (buf + nameend + 3, IDENTIFIER_POINTER (id) + nameend + 2);

We can't replace a single "@" with "@@@".  So I think producing .LSYMVERx is not
an option for "old" versions like "foo@VER_1".

> +      ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> +				   IDENTIFIER_POINTER
> +					 (DECL_ASSEMBLER_NAME (tmpdecl)),
> +				   buf);
> +    }
>  #else
>    error ("symver is only supported on ELF platforms");
>  #endif
> Index: lto/lto-common.c
> ===================================================================
> --- lto/lto-common.c	(revision 279467)
> +++ lto/lto-common.c	(working copy)
> @@ -2818,6 +2818,10 @@ read_cgraph_and_symbols (unsigned nfiles
>  			   IDENTIFIER_POINTER
>  			     (DECL_ASSEMBLER_NAME (snode->decl)));
>  	  }
> +	/* Symbol versions are always used externally, but linker does not
> +	   report that correctly.  */
> +	else if (snode->symver && *res == LDPR_PREVAILING_DEF_IRONLY)
> +	  snode->resolution = LDPR_PREVAILING_DEF_IRONLY_EXP;

This is absolutely correct.

>  	else
>  	  snode->resolution = *res;
>        }

I still believe we should consider symver targets to be externally visible in
cgraph_externally_visible_p.  There is a comment saying "if linker counts on us,
we must preserve the function".  That's true in our case.

And, I think

.globl  .LSYMVER0
.set    .LSYMVER0, foo_v2
.symver .LSYMVER0, foo@@VERS_2

is exactly same as

.globl  foo_v2
.symver foo_v2, foo@@VERS_2

except there is an unnecessary ".LSYMVER0".  Adding ".globl foo_v2" or ".globl
foo_v1" won't cause them to be "global" in the final DSO because the linker will
hide them according to the version script.

So if it's safe we can force a ".globl foo_v2" before ".symver foo_v2,
foo@@VERS_2".  But I can't prove it's safe so I think it's better to consider
this case in cgraph_externally_visible_p.
-- 
Xi Ruoyao <xry111@mengyan1223.wang>
School of Aerospace Science and Technology, Xidian University