Re: [PATCH] docs: update symver attribute description

[Martin Liška]

On 4/12/21 3:50 PM, H.J. Lu wrote:
> GCC failed to bootstrap:
> 
> /export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3870: misplaced {
> /export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3872: misplaced }
> /export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3874: unknown
> command `VERS'
> make[2]: *** [Makefile:3396: doc/gcc.info] Error 1

Whoops. Sorry for that, should be fixed now.

Martin

Re: [PATCH] docs: update symver attribute description

[H.J. Lu via Gcc-patches]

On Mon, Apr 12, 2021 at 5:54 AM Martin Liška  wrote:
>
> On 4/12/21 2:18 PM, Jakub Jelinek wrote:
> > On Mon, Apr 12, 2021 at 02:15:16PM +0200, Martin Liška wrote:
> >> The old syntax with the alias is quite ugly..
> >
> > Not that much.  And users have no other option (besides inline asm
> > but that doesn't work with LTO well).
> >
> >>> so that people who don't have gcc configured against
> >>> binutils 2.35 or newer know what to do instead.
> >>
> >> ... and symver support for older binutils releases is fragile due to:
> >>
> >> Changes in 2.35:
> >>
> >> * Extend .symver directive to update visibility of the original symbol
> >>   and assign one original symbol to different versioned symbols.
> >
> > That is a change that allows the new syntax.
> > The old syntax (on the as/ld side) have worked for the last 20+ years
> > just fine.
> >
> >   Jakub
> >
>
> All right, so something like this?
>
> Thanks,
> Martin

GCC failed to bootstrap:

/export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3870: misplaced {
/export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3872: misplaced }
/export/gnu/import/git/sources/gcc/gcc/doc/extend.texi:3874: unknown
command `VERS'
make[2]: *** [Makefile:3396: doc/gcc.info] Error 1


-- 
H.J.

Re: [PATCH] docs: update symver attribute description

[Jakub Jelinek via Gcc-patches]

On Mon, Apr 12, 2021 at 03:24:00PM +0200, Martin Liška wrote:
> On 4/12/21 2:45 PM, Jakub Jelinek wrote:
> > On Mon, Apr 12, 2021 at 02:32:35PM +0200, Martin Liška wrote:
> >> +If you have an older release of binutils release, then symbol alias needs 
> >> to
> > 
> > s/binutils release/binutils/
> 
> Fixed.
> 
> > 
> >> +be used:
> >> +
> >> +@smallexample
> >> +__attribute__ ((__symver__ ("foo@@VERS_2")))
> >> +__attribute__ ((alias ("foo_v1")))
> >> +int symver_foo_v1 (void);
> >> +@end smallexample
> > 
> > The example should show two versions of foo rather than just one, otherwise
> > it will confuse users.  For symbol versions which just a single symbol they
> > don't need any aliases...
> 
> Very good point! Is it fine now?

Ok, thanks.

> >From 6dda0ec10a1b0c60e6e9afe7fc45370d0132b5e3 Mon Sep 17 00:00:00 2001
> From: Martin Liska 
> Date: Mon, 12 Apr 2021 13:42:33 +0200
> Subject: [PATCH] docs: update symver attribute description
> 
> gcc/ChangeLog:
> 
>   * doc/extend.texi: Be more precise in documentation
>   of symver attribute.
> ---
>  gcc/doc/extend.texi | 28 +++-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index e28e1860990..6542ada6583 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3848,23 +3848,33 @@ foo_v1 (void)
>  Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
>  output. 
>  
> -One can also define multiple version for a given symbol.
> +One can also define multiple version for a given symbol
> +(starting from binutils 2.35).
>  
>  @smallexample
>  __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
>  int symver_foo_v1 (void)
>  @{
>  @}
> -
> -__attribute__ ((__symver__ ("bar@@VERS_2")))
> -__attribute__ ((__symver__ ("bar@@VERS_3")))
> -int symver_bar_v1 (void)
> -@{
> -@}
>  @end smallexample
>  
> -This example creates an alias of @code{foo_v1} with symbol name
> -@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
> +This example creates a symbol name @code{symver_foo_v1}
> +which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
> +
> +If you have an older release of binutils, then symbol alias needs to
> +be used:
> +
> +@smallexample
> +__attribute__ ((__symver__ ("foo@@VERS_2")))
> +int foo_v1 (void)
> +{
> +  return 0;
> +}
> +
> +__attribute__ ((__symver__ ("foo@VERS_3")))
> +__attribute__ ((alias ("foo_v1")))
> +int symver_foo_v1 (void);
> +@end smallexample
>  
>  Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
>  addition to creating a symbol version (as if
> -- 
> 2.31.1
> 


Jakub

Re: [PATCH] docs: update symver attribute description

[Martin Liška]

On 4/12/21 2:45 PM, Jakub Jelinek wrote:
> On Mon, Apr 12, 2021 at 02:32:35PM +0200, Martin Liška wrote:
>> +If you have an older release of binutils release, then symbol alias needs to
> 
> s/binutils release/binutils/

Fixed.

> 
>> +be used:
>> +
>> +@smallexample
>> +__attribute__ ((__symver__ ("foo@@VERS_2")))
>> +__attribute__ ((alias ("foo_v1")))
>> +int symver_foo_v1 (void);
>> +@end smallexample
> 
> The example should show two versions of foo rather than just one, otherwise
> it will confuse users.  For symbol versions which just a single symbol they
> don't need any aliases...

Very good point! Is it fine now?

Thanks,
Martin

> 
>   Jakub
> 

>From 6dda0ec10a1b0c60e6e9afe7fc45370d0132b5e3 Mon Sep 17 00:00:00 2001
From: Martin Liska 
Date: Mon, 12 Apr 2021 13:42:33 +0200
Subject: [PATCH] docs: update symver attribute description

gcc/ChangeLog:

	* doc/extend.texi: Be more precise in documentation
	of symver attribute.
---
 gcc/doc/extend.texi | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e28e1860990..6542ada6583 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3848,23 +3848,33 @@ foo_v1 (void)
 Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
 output. 
 
-One can also define multiple version for a given symbol.
+One can also define multiple version for a given symbol
+(starting from binutils 2.35).
 
 @smallexample
 __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
 int symver_foo_v1 (void)
 @{
 @}
-
-__attribute__ ((__symver__ ("bar@@VERS_2")))
-__attribute__ ((__symver__ ("bar@@VERS_3")))
-int symver_bar_v1 (void)
-@{
-@}
 @end smallexample
 
-This example creates an alias of @code{foo_v1} with symbol name
-@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
+This example creates a symbol name @code{symver_foo_v1}
+which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
+
+If you have an older release of binutils, then symbol alias needs to
+be used:
+
+@smallexample
+__attribute__ ((__symver__ ("foo@@VERS_2")))
+int foo_v1 (void)
+{
+  return 0;
+}
+
+__attribute__ ((__symver__ ("foo@VERS_3")))
+__attribute__ ((alias ("foo_v1")))
+int symver_foo_v1 (void);
+@end smallexample
 
 Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
 addition to creating a symbol version (as if
-- 
2.31.1

Re: [PATCH] docs: update symver attribute description

[Jakub Jelinek via Gcc-patches]

On Mon, Apr 12, 2021 at 02:32:35PM +0200, Martin Liška wrote:
> +If you have an older release of binutils release, then symbol alias needs to

s/binutils release/binutils/

> +be used:
> +
> +@smallexample
> +__attribute__ ((__symver__ ("foo@@VERS_2")))
> +__attribute__ ((alias ("foo_v1")))
> +int symver_foo_v1 (void);
> +@end smallexample

The example should show two versions of foo rather than just one, otherwise
it will confuse users.  For symbol versions which just a single symbol they
don't need any aliases...

Jakub

Re: [PATCH] docs: update symver attribute description

[Martin Liška]

On 4/12/21 2:18 PM, Jakub Jelinek wrote:
> On Mon, Apr 12, 2021 at 02:15:16PM +0200, Martin Liška wrote:
>> The old syntax with the alias is quite ugly..
> 
> Not that much.  And users have no other option (besides inline asm
> but that doesn't work with LTO well).
> 
>>> so that people who don't have gcc configured against
>>> binutils 2.35 or newer know what to do instead.
>>
>> ... and symver support for older binutils releases is fragile due to:
>>
>> Changes in 2.35:
>>
>> * Extend .symver directive to update visibility of the original symbol
>>   and assign one original symbol to different versioned symbols.
> 
> That is a change that allows the new syntax.
> The old syntax (on the as/ld side) have worked for the last 20+ years
> just fine.
> 
>   Jakub
> 

All right, so something like this?

Thanks,
Martin
>From 750b715225d480fcb74e765623d54acc42ac25e3 Mon Sep 17 00:00:00 2001
From: Martin Liska 
Date: Mon, 12 Apr 2021 13:42:33 +0200
Subject: [PATCH] docs: update symver attribute description

gcc/ChangeLog:

	* doc/extend.texi: Be more precise in documentation
	of symver attribute.
---
 gcc/doc/extend.texi | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e28e1860990..75e4a43a8c5 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3848,23 +3848,27 @@ foo_v1 (void)
 Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
 output. 
 
-One can also define multiple version for a given symbol.
+One can also define multiple version for a given symbol
+(starting from binutils 2.35).
 
 @smallexample
 __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
 int symver_foo_v1 (void)
 @{
 @}
-
-__attribute__ ((__symver__ ("bar@@VERS_2")))
-__attribute__ ((__symver__ ("bar@@VERS_3")))
-int symver_bar_v1 (void)
-@{
-@}
 @end smallexample
 
-This example creates an alias of @code{foo_v1} with symbol name
-@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
+This example creates a symbol name @code{symver_foo_v1}
+which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
+
+If you have an older release of binutils release, then symbol alias needs to
+be used:
+
+@smallexample
+__attribute__ ((__symver__ ("foo@@VERS_2")))
+__attribute__ ((alias ("foo_v1")))
+int symver_foo_v1 (void);
+@end smallexample
 
 Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
 addition to creating a symbol version (as if
-- 
2.31.1

Re: [PATCH] docs: update symver attribute description

[Jakub Jelinek via Gcc-patches]

On Mon, Apr 12, 2021 at 02:15:16PM +0200, Martin Liška wrote:
> The old syntax with the alias is quite ugly..

Not that much.  And users have no other option (besides inline asm
but that doesn't work with LTO well).

> > so that people who don't have gcc configured against
> > binutils 2.35 or newer know what to do instead.
> 
> ... and symver support for older binutils releases is fragile due to:
> 
> Changes in 2.35:
> 
> * Extend .symver directive to update visibility of the original symbol
>   and assign one original symbol to different versioned symbols.

That is a change that allows the new syntax.
The old syntax (on the as/ld side) have worked for the last 20+ years
just fine.

Jakub

Re: [PATCH] docs: update symver attribute description

[Martin Liška]

On 4/12/21 1:50 PM, Jakub Jelinek wrote:
> On Mon, Apr 12, 2021 at 01:44:54PM +0200, Martin Liška wrote:
>> This improves documentation as noticed by Jakub.
>>
>> Ready for master?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>>  * doc/extend.texi: Be more precise in documentation
>>  of symver attribute.
> 
> Ok, but I'd prefer to see the old example with the old description in the
> documentation too

The old syntax with the alias is quite ugly..

> so that people who don't have gcc configured against
> binutils 2.35 or newer know what to do instead.

... and symver support for older binutils releases is fragile due to:

Changes in 2.35:

* Extend .symver directive to update visibility of the original symbol
  and assign one original symbol to different versioned symbols.

Martin

> 
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -3848,7 +3848,8 @@ foo_v1 (void)
>>  Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
>>  output. 
>>  
>> -One can also define multiple version for a given symbol.
>> +One can also define multiple version for a given symbol
>> +(starting from binutils 2.35).
>>  
>>  @smallexample
>>  __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
>> @@ -3863,8 +3864,8 @@ int symver_bar_v1 (void)
>>  @}
>>  @end smallexample
>>  
>> -This example creates an alias of @code{foo_v1} with symbol name
>> -@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
>> +This example creates a symbol name @code{symver_foo_v1}
>> +which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
>>  
>>  Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
>>  addition to creating a symbol version (as if
>> -- 
>> 2.31.1
> 
>   Jakub
> 

Re: [PATCH] docs: update symver attribute description

[Jakub Jelinek via Gcc-patches]

On Mon, Apr 12, 2021 at 01:44:54PM +0200, Martin Liška wrote:
> This improves documentation as noticed by Jakub.
> 
> Ready for master?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
>   * doc/extend.texi: Be more precise in documentation
>   of symver attribute.

Ok, but I'd prefer to see the old example with the old description in the
documentation too, so that people who don't have gcc configured against
binutils 2.35 or newer know what to do instead.

> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3848,7 +3848,8 @@ foo_v1 (void)
>  Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
>  output. 
>  
> -One can also define multiple version for a given symbol.
> +One can also define multiple version for a given symbol
> +(starting from binutils 2.35).
>  
>  @smallexample
>  __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
> @@ -3863,8 +3864,8 @@ int symver_bar_v1 (void)
>  @}
>  @end smallexample
>  
> -This example creates an alias of @code{foo_v1} with symbol name
> -@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
> +This example creates a symbol name @code{symver_foo_v1}
> +which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
>  
>  Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
>  addition to creating a symbol version (as if
> -- 
> 2.31.1

Jakub

[PATCH] docs: update symver attribute description

[Martin Liška]

This improves documentation as noticed by Jakub.

Ready for master?
Thanks,
Martin

gcc/ChangeLog:

* doc/extend.texi: Be more precise in documentation
of symver attribute.
---
 gcc/doc/extend.texi | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e28e1860990..61d9a684b24 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3848,7 +3848,8 @@ foo_v1 (void)
 Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
 output. 
 
-One can also define multiple version for a given symbol.
+One can also define multiple version for a given symbol
+(starting from binutils 2.35).
 
 @smallexample
 __attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
@@ -3863,8 +3864,8 @@ int symver_bar_v1 (void)
 @}
 @end smallexample
 
-This example creates an alias of @code{foo_v1} with symbol name
-@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
+This example creates a symbol name @code{symver_foo_v1}
+which will be version @code{VERS_2} and @code{VERS_3} of @code{foo}.
 
 Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
 addition to creating a symbol version (as if
-- 
2.31.1

doc: Fix up symver attribute documentation

[Jakub Jelinek via Gcc-patches]

Hi!

When looking at the symver documentation, I've noticed a couple of
syntax errors in it.

Committed to trunk as obvious.

But looking more at it, the
r11-2859-g363080bb8bd2cca81dd9e2e774910a8c8226f430
change that introduced this has another problem - the documentation
still talks about foo_v1 but the alias attribute has been removed
from the example.

Maybe it would be better to restore to the old way for symver_foo_v1
with its GCC 10 attributes, describe what that does, then have the
bar example and add baz example with multiple symver attributes
in the same attribute and describe what each of them does and mention
that only binutils 2.35 and later actually handle some of the forms.

2021-04-01  Jakub Jelinek  

* doc/extend.texi (symver attribute): Fix up syntax errors
in the examples.

--- gcc/doc/extend.texi.jj  2021-03-03 09:43:57.898723872 +0100
+++ gcc/doc/extend.texi 2021-04-01 10:56:25.604914998 +0200
@@ -3851,13 +3851,13 @@ output.
 One can also define multiple version for a given symbol.
 
 @smallexample
-__attribute__ ((__symver__ ("foo@@VERS_2"), ("foo@@VERS_3")))
+__attribute__ ((__symver__ ("foo@@VERS_2"), __symver__ ("foo@@VERS_3")))
 int symver_foo_v1 (void)
 @{
 @}
 
-__attribute__ ((__symver__ ("bar@@VERS_2"
-__attribute__ ((__symver__ ("bar@@VERS_3"
+__attribute__ ((__symver__ ("bar@@VERS_2")))
+__attribute__ ((__symver__ ("bar@@VERS_3")))
 int symver_bar_v1 (void)
 @{
 @}

Jakub

Re: [PATCH] doc: Add @cindex to symver attribute

[Sandra Loosemore]

On 8/4/20 2:46 AM, Jakub Jelinek wrote:

Hi!

When looking at the symver attr documentation in html, I found there is no
name to refer to for it.
The following patch fixes that, bootstrapped on x86_64-linux, ok for trunk
and 10.3?

2020-08-04  Jakub Jelinek  

* doc/extend.texi (symver): Add @cindex for symver function attribute.


Thanks!  This looks fine.

-Sandra

[PATCH] doc: Add @cindex to symver attribute

[Jakub Jelinek via Gcc-patches]

Hi!

When looking at the symver attr documentation in html, I found there is no
name to refer to for it.
The following patch fixes that, bootstrapped on x86_64-linux, ok for trunk
and 10.3?

2020-08-04  Jakub Jelinek  

* doc/extend.texi (symver): Add @cindex for symver function attribute.

--- gcc/doc/extend.texi.jj  2020-07-28 15:39:09.849758414 +0200
+++ gcc/doc/extend.texi 2020-08-03 17:02:06.221350978 +0200
@@ -3723,6 +3723,7 @@ Function Attributes}, @ref{PowerPC Funct
 for details.
 
 @item symver ("@var{name2}@@@var{nodename}")
+@cindex @code{symver} function attribute
 On ELF targets this attribute creates a symbol version.  The @var{name2} part
 of the parameter is the actual name of the symbol by which it will be
 externally referenced.  The @code{nodename} portion should be the name of a

Jakub

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> On Wed, 2019-12-18 at 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  (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));
> [ ... ]
> Do you plan on committing these bits?

I think we got into agreement that gas extension is needed, since there
is name@@@nodename that does what we need for name@@nodename
but there is nothing that would do it for name@nodename.  I filled in
GAS PR for that.

Honza
> 
> jeff
> > 
> 

Re: [PATCH] Fix symver attribute with LTO

[Jeff Law]

On Wed, 2019-12-18 at 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  (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));
[ ... ]
Do you plan on committing these bits?

jeff
> 

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> On 2019-12-19 19:12 +0800, Xi Ruoyao wrote:
> > On 2019-12-19 11:06 +0100, Jan Hubicka wrote:
> > > -  /* See if we have linker information about symbol not being used or
> > > - if we need to make guess based on the declaration.
> > > +  /* Limitation of gas requires us to output targets of symver aliases as
> > > + global symbols.  This is binutils PR 25295.  */
> > > +  ipa_ref *ref;
> > > +  FOR_EACH_ALIAS (this, ref)
> > > +if (ref->referring->symver)
> > > +  return true;
> > >  
> > > - Even if the linker clams the symbol is unused, never bring internal
> > > - symbols that are declared by user as used or externally visible.
> > > - This is needed for i.e. references from asm statements.   */
> > > -  if (used_from_object_file_p ())
> > > -return true;
> > 
> > Are these two lines removed intentionally?
> 
> Oh I see, it was a duplicated branch.
> 
> Sorry for noise.
I should have mentioned that in the changelog. Indeed that function was
checked twice.

Looking into it more, we still have issue since
call foo_v1
tranlsates as
5:   e8 00 00 00 00  callq  a 
while gcc believes that really foo_v1 is called.  
So after symver i guess foo_v1 symbol becomes completely unreachable
from the current unit which is not what gcc expect.

Since we do not allow any references to the symbol and we hacked gcc to
believe that symvered symbol is externaly visible, I suppose things are
more or less safe, but it is not working at it should.

It would probably be best if gas supported something like
.symver_set
with similar semantics of .set but creating a symbol version alias
which woud not add the translation of all references from symbol to its
version.

Honza

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-19 19:12 +0800, Xi Ruoyao wrote:
> On 2019-12-19 11:06 +0100, Jan Hubicka wrote:
> > -  /* See if we have linker information about symbol not being used or
> > - if we need to make guess based on the declaration.
> > +  /* Limitation of gas requires us to output targets of symver aliases as
> > + global symbols.  This is binutils PR 25295.  */
> > +  ipa_ref *ref;
> > +  FOR_EACH_ALIAS (this, ref)
> > +if (ref->referring->symver)
> > +  return true;
> >  
> > - Even if the linker clams the symbol is unused, never bring internal
> > - symbols that are declared by user as used or externally visible.
> > - This is needed for i.e. references from asm statements.   */
> > -  if (used_from_object_file_p ())
> > -return true;
> 
> Are these two lines removed intentionally?

Oh I see, it was a duplicated branch.

Sorry for noise.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-19 11:06 +0100, Jan Hubicka wrote:
> This is variant of your patch I comitted. It also adds verification so
> we get ICE rather then wrong code.  In addition I moved the checks away
> rom used_from_object_file.  This function is about non-LTO objects
> linked into the DSO and thus does not really fit for the check.
> Lastly we can not rely on symver attribute to still be present here.
> 
> Regtested x86_64-linux and comitted.
> Honza
>   * cgraph.c (cgraph_node_cannot_be_local_p_1): Prevent targets of
>   symver attributes to be localized.
>   * ipa-visibility.c (cgraph_externally_visible_p,
>   varpool_node::externally_visible_p): Likewise.
>   * symtab.c (symtab_node::verify_base): Check visibility of symbol
>   versions.
> 
>   * lto-common.c (read_cgraph_and_symbols): Work around binutils
>   PR25424

/* snip */

> Index: ipa-visibility.c
> ===
> --- ipa-visibility.c  (revision 279523)
> +++ ipa-visibility.c  (working copy)
> @@ -220,6 +220,14 @@ cgraph_externally_visible_p (struct cgra
>&& lookup_attribute ("dllexport",
>  DECL_ATTRIBUTES (node->decl)))
>  return true;
> +
> +  /* Limitation of gas requires us to output targets of symver aliases as
> + global symbols.  This is binutils PR 25295.  */
> +  ipa_ref *ref;
> +  FOR_EACH_ALIAS (node, ref)
> +if (ref->referring->symver)
> +  return true;
> +
>if (node->resolution == LDPR_PREVAILING_DEF_IRONLY)
>  return false;
>/* When doing LTO or whole program, we can bring COMDAT functoins static.
> @@ -284,14 +292,13 @@ varpool_node::externally_visible_p (void
>  DECL_ATTRIBUTES (decl)))
>  return true;
>  
> -  /* See if we have linker information about symbol not being used or
> - if we need to make guess based on the declaration.
> +  /* Limitation of gas requires us to output targets of symver aliases as
> + global symbols.  This is binutils PR 25295.  */
> +  ipa_ref *ref;
> +  FOR_EACH_ALIAS (this, ref)
> +if (ref->referring->symver)
> +  return true;
>  
> - Even if the linker clams the symbol is unused, never bring internal
> - symbols that are declared by user as used or externally visible.
> - This is needed for i.e. references from asm statements.   */
> -  if (used_from_object_file_p ())
> -return true;

Are these two lines removed intentionally?

>if (resolution == LDPR_PREVAILING_DEF_IRONLY)
>  return false;
>  
> Index: lto/lto-common.c
> ===
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> On 2019-12-18 14:19 +0100, Jan Hubicka wrote:
> > The problem here is that we lie to the compiler (by pretending that
> > foo_v2 is exported from DSO while it is not) and force user to do the
> > same.
> > 
> > We support two ways to hide symbol - either at compile time via
> > attribute((visibility("hidden"))) or at link-time via map file.  The
> > first produces better code because compiler can do more optimizations
> > knowing that the symbol can not be interposed.
> 
> I just get your point: if the library calls foo_v2 it won't be interposed.  If
> it supposes a call to be interposed it should call foo() [foo@@VER_2] instead 
> of
> foo_v2().
> 
> But it seems there is no way we can do this [even with traditional
> __asm__("symver foo, foo@@VER_2")].  For this purpose we should either:
> 
> 1. Change GAS (introducing some new syntax like '' or '.symver_export')
> 
> or
> 
> 2. Add some mangled symbol name in GCC (like ".LSYMVERx" or
> "foo_v2.symver_export").

I agree.  The problem with mangled symbol names is that we will require
users to hide them from map files, so I think it is not best answer
either.  I have filled binutils
https://sourceware.org/bugzilla/show_bug.cgi?id=25295

This is variant of your patch I comitted. It also adds verification so
we get ICE rather then wrong code.  In addition I moved the checks away
rom used_from_object_file.  This function is about non-LTO objects
linked into the DSO and thus does not really fit for the check.
Lastly we can not rely on symver attribute to still be present here.

Regtested x86_64-linux and comitted.
Honza
* cgraph.c (cgraph_node_cannot_be_local_p_1): Prevent targets of
symver attributes to be localized.
* ipa-visibility.c (cgraph_externally_visible_p,
varpool_node::externally_visible_p): Likewise.
* symtab.c (symtab_node::verify_base): Check visibility of symbol
versions.

* lto-common.c (read_cgraph_and_symbols): Work around binutils
PR25424

Index: cgraph.c
===
--- cgraph.c(revision 279523)
+++ cgraph.c(working copy)
@@ -2226,6 +2226,9 @@ cgraph_node_cannot_be_local_p_1 (cgraph_
 {
   return !(!node->force_output
   && !node->ifunc_resolver
+  /* Limitation of gas requires us to output targets of symver aliases
+ as global symbols.  This is binutils PR 25295.  */
+  && !node->symver
   && ((DECL_COMDAT (node->decl)
&& !node->forced_by_abi
&& !node->used_from_object_file_p ()
Index: ipa-visibility.c
===
--- ipa-visibility.c(revision 279523)
+++ ipa-visibility.c(working copy)
@@ -220,6 +220,14 @@ cgraph_externally_visible_p (struct cgra
   && lookup_attribute ("dllexport",
   DECL_ATTRIBUTES (node->decl)))
 return true;
+
+  /* Limitation of gas requires us to output targets of symver aliases as
+ global symbols.  This is binutils PR 25295.  */
+  ipa_ref *ref;
+  FOR_EACH_ALIAS (node, ref)
+if (ref->referring->symver)
+  return true;
+
   if (node->resolution == LDPR_PREVAILING_DEF_IRONLY)
 return false;
   /* When doing LTO or whole program, we can bring COMDAT functoins static.
@@ -284,14 +292,13 @@ varpool_node::externally_visible_p (void
   DECL_ATTRIBUTES (decl)))
 return true;
 
-  /* See if we have linker information about symbol not being used or
- if we need to make guess based on the declaration.
+  /* Limitation of gas requires us to output targets of symver aliases as
+ global symbols.  This is binutils PR 25295.  */
+  ipa_ref *ref;
+  FOR_EACH_ALIAS (this, ref)
+if (ref->referring->symver)
+  return true;
 
- Even if the linker clams the symbol is unused, never bring internal
- symbols that are declared by user as used or externally visible.
- This is needed for i.e. references from asm statements.   */
-  if (used_from_object_file_p ())
-return true;
   if (resolution == LDPR_PREVAILING_DEF_IRONLY)
 return false;
 
Index: lto/lto-common.c
===
--- lto/lto-common.c(revision 279523)
+++ lto/lto-common.c(working copy)
@@ -2818,6 +2818,11 @@ 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.
+  This is binutils PR25924.  */
+   else if (snode->symver && *res == LDPR_PREV

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-18 14:19 +0100, Jan Hubicka wrote:
> The problem here is that we lie to the compiler (by pretending that
> foo_v2 is exported from DSO while it is not) and force user to do the
> same.
> 
> We support two ways to hide symbol - either at compile time via
> attribute((visibility("hidden"))) or at link-time via map file.  The
> first produces better code because compiler can do more optimizations
> knowing that the symbol can not be interposed.

I just get your point: if the library calls foo_v2 it won't be interposed.  If
it supposes a call to be interposed it should call foo() [foo@@VER_2] instead of
foo_v2().

But it seems there is no way we can do this [even with traditional
__asm__("symver foo, foo@@VER_2")].  For this purpose we should either:

1. Change GAS (introducing some new syntax like '' or '.symver_export')

or

2. Add some mangled symbol name in GCC (like ".LSYMVERx" or
"foo_v2.symver_export").
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-18 14:19 +0100, Jan Hubicka wrote:
> > 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
> 
> Interesting that it works for me, but indeed we can remove that call
> since there is no way to do weakref of symbol version.
> > >  #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 % 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.)
> 
> The problem here is that we lie to the compiler (by pretending that
> foo_v2 is exported from DSO while it is not) and force user to do the
> same.
> 
> We support two ways to hide symbol - either at compile time via
> attribute((visibility("hidden"))) or at link-time via map file.  The
> first produces better code because compiler can do more optimizations
> knowing that the symbol can not be interposed.
> 
> Generally we want users to use visiblity attribute or -fvisibility since
> it leads to better code. However now we tell users to use
> attribute((symver("..."))) to produce symbol version, but at the same
> time not use attribute((visibility("hidden"))).

Could a symver symbol be interposed?  I'll do some test to see.

> > > +  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".
> 
> I wonder why gas implements the .symver this way at first place. Does
> the linker really need the global symbol foo_v1 to produce the
> version (in addition to foo@VER_1 that is in symbol table as well)?

I don't think the global symbol foo_v1 is necessary.  But I can't find a way
telling gas to make foo@VER_1 global and foo_v1 local.

> > > + /* 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.
> 
> Good, I will go ahead with filling in binutils PR on the wrong LDPR
> value and apply the hack.
> > >   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
> I produce
>   .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.
> 
> The difference is that in first case compiler can fully control foo_v2
> symbol (with LTO it will turn it into static symbol, it will inline
> calls to it and do other things), while in the second case we need to
> treat foo_v2 specially.
> > So if it's safe we can force a ".globl foo_v2" before ".symver foo_v2,
>

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> 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

Interesting that it works for me, but indeed we can remove that call
since there is no way to do weakref of symbol version.
> 
> >  #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 % 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.)

The problem here is that we lie to the compiler (by pretending that
foo_v2 is exported from DSO while it is not) and force user to do the
same.

We support two ways to hide symbol - either at compile time via
attribute((visibility("hidden"))) or at link-time via map file.  The
first produces better code because compiler can do more optimizations
knowing that the symbol can not be interposed.

Generally we want users to use visiblity attribute or -fvisibility since
it leads to better code. However now we tell users to use
attribute((symver("..."))) to produce symbol version, but at the same
time not use attribute((visibility("hidden"))).

> > +  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".

I wonder why gas implements the .symver this way at first place. Does
the linker really need the global symbol foo_v1 to produce the
version (in addition to foo@VER_1 that is in symbol table as well)?

> > +   /* 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.

Good, I will go ahead with filling in binutils PR on the wrong LDPR
value and apply the hack.
> 
> > 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
I produce
  .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.

The difference is that in first case compiler can fully control foo_v2
symbol (with LTO it will turn it into static symbol, it will inline
calls to it and do other things), while in the second case we need to
treat foo_v2 specially.
> 
> 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.

It sort of makes things work, but for example it will prevent gcc from
inlining calls to foo_v2.  I think we will still need to do something
about -fvisibility=hidden.

It is sad that we do not have way to produce symbol version without a
corresponding symbol of global visiblity.  If we had we could 

Re: [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  (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)
> @@ -,8 +,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 % 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,1

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

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  (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)
@@ -,8 +,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);
 #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 % of a symbol that is "
+   "not exported with default visibility");
+ return;
+   }
+  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);
+  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;
else
  snode->resolution = *res;
   }

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-17 18:47 +0100, Jan Hubicka wrote:
> > Would it be equivalent to:
> > 1) output foo_v2 local
> > 2) producing static alias with local name (.L1)
> > 3) do .symver .L1,foo@@@VERS_2
> > That is somewhat more systematic and would not lead to false
> > visibilities.
> 
> I spent some time playing with this.  An in order to 
> 1) be able to handle foo_v2 according to the resolution info
>(so it behaves like a regular symbol and can be called dirrectly,
> localized and optimized)
> 2) get intended objdump -T relocations
> 3) do not polute global symbol tables
> 
> I ended up with the following codegen:
> 
>   .type   foo_v2, @function
> foo_v2:
> .LFB1:
>   .cfi_startproc
>   movl$2, %eax
>   ret
>   .cfi_endproc
> .LFE1:
>   .size   foo_v2, .-foo_v2
>   .globl  .LSYMVER0
>   .set.LSYMVER0,foo_v2
>   .symver .LSYMVER0, foo@@@VERS_2
> 
> This uses @@@ symver version of gas which seems to have odd semantics of
> requiring to be passed global symbol name which it then tkes away and
> produces foo@@VERS_2.
> 
> So the nm outoutp of the ltrans unit is:
>  T foo_v1
> 0010 t foo_v2
>  T foo@VERS_1
> 0010 T foo@@VERS_2
> 
> So the difference to your patch is that foo_v2 is static which enables
> normal optimizations.
> 
> Since additional symbol alias is produced this would also make it
> possible to attach multiple symver attributes with @@ string.
> 
> Does somehting like this make sense to you? Modulo the obvious buffer
> overflow issue?
> Honza

Unfortunately, I got an ICE with my testcase with the patch applied to trunk.

lto1: internal compiler error: tree check: expected tree that contains ‘decl
minimal’ structure, have ‘identifier_node’ in do_assemble_symver, at
varasm.c:5986
0x6fa648 tree_contains_struct_check_failed(tree_node const*,
tree_node_structure_enum, char const*, int, char const*)
../../gcc/gcc/tree.c:9859
0x71466e contains_struct_check(tree_node*, tree_node_structure_enum, char
const*, int, char const*)
../../gcc/gcc/tree.h:3387
0x71466e do_assemble_symver(tree_node*, tree_node*)
../../gcc/gcc/varasm.c:5986
0x89e409 cgraph_node::assemble_thunks_and_aliases()
../../gcc/gcc/cgraphunit.c:2225
0x89e698 cgraph_node::expand()
../../gcc/gcc/cgraphunit.c:2351
0x89f62f expand_all_functions
../../gcc/gcc/cgraphunit.c:2456
0x89f62f symbol_table::compile()
../../gcc/gcc/cgraphunit.c:2806
0x7fb589 lto_main()
../../gcc/gcc/lto/lto.c:658
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
lto-wrapper: fatal error: /home/xry111/gcc-test/bin/gcc returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
make: *** [Makefile:4: obj/test.so] Error 1

The change to lto/lto-common.c makes sense.  I tried it instead of my change to
cgraph.h and everything is OK.  I'll investigate the change to varasm.c a
little.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> Would it be equivalent to:
> 1) output foo_v2 local
> 2) producing static alias with local name (.L1)
> 3) do .symver .L1,foo@@@VERS_2
> That is somewhat more systematic and would not lead to false
> visibilities.

I spent some time playing with this.  An in order to 
1) be able to handle foo_v2 according to the resolution info
   (so it behaves like a regular symbol and can be called dirrectly,
localized and optimized)
2) get intended objdump -T relocations
3) do not polute global symbol tables

I ended up with the following codegen:

.type   foo_v2, @function
foo_v2:
.LFB1:
.cfi_startproc
movl$2, %eax
ret
.cfi_endproc
.LFE1:
.size   foo_v2, .-foo_v2
.globl  .LSYMVER0
.set.LSYMVER0,foo_v2
.symver .LSYMVER0, foo@@@VERS_2

This uses @@@ symver version of gas which seems to have odd semantics of
requiring to be passed global symbol name which it then tkes away and
produces foo@@VERS_2.

So the nm outoutp of the ltrans unit is:
 T foo_v1
0010 t foo_v2
 T foo@VERS_1
0010 T foo@@VERS_2

So the difference to your patch is that foo_v2 is static which enables
normal optimizations.

Since additional symbol alias is produced this would also make it
possible to attach multiple symver attributes with @@ string.

Does somehting like this make sense to you? Modulo the obvious buffer
overflow issue?
Honza

Index: lto/lto-common.c
===
--- lto/lto-common.c(revision 279178)
+++ 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;
else
  snode->resolution = *res;
   }
Index: varasm.c
===
--- varasm.c(revision 279178)
+++ 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);
 #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 % of a symbol that is "
+   "not exported with default visibility");
+ return;
+   }
+  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);
+  ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
+  IDENTIFIER_POINTER
+(DECL_ASSEMBLER_NAME (tmpdecl)),
+  buf);
+}
 #else
   error ("symver is only supported on ELF platforms");
 #endif

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> Hi Jan,
> 
> I'm using GNU ld 2.33.1.
> 
> I'll attach a testcase simplified from fuse-3.9 code.  "local: *;" in the
> versioning script triggers the issue.  Without it there would be no problem.

Thanks.
You are right that I did not play with local:. Now I wonder what is the
intended behaviour here.

In resolution file I see:
1
foo.o 4
205 dc8dc21a4ac8d072 PREVAILING_DEF_IRONLY_EXP foo_v1
207 dc8dc21a4ac8d072 PREVAILING_DEF_IRONLY foo@VERS_1
216 dc8dc21a4ac8d072 PREVAILING_DEF_IRONLY foo_v2
218 dc8dc21a4ac8d072 PREVAILING_DEF_IRONLY foo@@VERS_2

If I link the DSO w/o -flto I get with objdump -T:
1100 gDF .text  0006 (VERS_1) foo
1100 gDF .text  0006  VERS_2  foo_v1
 gDO *ABS*    VERS_1  VERS_1
1110 gDF .text  0006  VERS_2  foo
 gDO *ABS*    VERS_2  VERS_2

So I think linker is right here that foo_v1 is exported.  I would have
expected PREVAILING_DEF_IRONLY_EXP for foo@VERS_1 and foo@@VERS_2 since
that symbols do get exported even though they land in special way in the
DSO symbol table.  So I think meaingful behaviour would be
 1) make linker plugin interface to not annotate symbol version symbols
with any resolution at all, since they are "special"
 2) make them PREVAILING_DEF_IRONLY_EXP/PREVAILING_DEF since they always
get exported to non-LTO land.
We could workaround that on GCC side but if you agree with this
understanding I would fill in binutils PR. Also since we use resolution
info at many places, I would simply add logic working around this at a
time we read resolution rahter than on one of places we use it.

Comparing to objedump -T

1100 gDF .text  0006  VERS_2  foo_v1
 gDO *ABS*    VERS_1  VERS_1
 gDO *ABS*    VERS_2  VERS_2

Why we also miss
1100 gDF .text  0006 (VERS_1) foo
and what does it mean?

I am not too happy about forcing GCC to keep foo_v2 exported when it is
not.  For example, calls to foo_v2 will then not be optimized as they
could if GCC knew it is not used by non-LTO world (except for fact that
symver is bound to it).

Would it be equivalent to:
1) output foo_v2 local
2) producing static alias with local name (.L1)
3) do .symver .L1,foo@@@VERS_2
That is somewhat more systematic and would not lead to false
visibilities.

Honza

> 
> > > 2. The actual function body implementing the symver-ed function is also
> > > marked
> > >as PREVAILING_DEF_IRONLY and then removed or marked as local.  So no
> > > ".globl"
> > >directive is outputed for it.
> > 
> > Here is the symver-ed function exported from the DSO (or is it set
> > to have hidden attribute)?
> > Again this was working for me, so it would be good to understand this
> > issue.
> 
> It's also triggered by "local: *;".
> 
> Untar the attachment and use "make" to build it, then "make show-dynamic-syms"
> to dump the dynamic symbol table.  I believe (with 99% chance) you'll see only
> foo (VERS_1) and foo_v1 (because foo_v1 is marked as global in the version
> script).  And foo (VERS_2) would be missing.  With this patch foo (VERS_2) 
> would
> show up.
> 
> We can't mark "foo_v2" to be "global" because it should not be a part of DSO
> ABI.
> 
> The other 1% chance would be a regression in Binutils.
> -- 
> Xi Ruoyao 
> School of Aerospace Science and Technology, Xidian University

Re: [PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

On 2019-12-17 09:32 +0100, Jan Hubicka wrote:
> > Hi,
> > with Jan's patch commited in r278878 we can use symver attribute for
> > functions
> > and variables.  The symver attribute is designed for replacing toplevel asm
> > statements containing ".symver" which may be removed by LTO.  Unfortunately,
> > a quick test shown GCC still generates buggy so file with LTO and new symver
> > attribute.
> 
> Thanks for looking into this.  It was on my TODo list to actually
> convert some packages, so it is great you did that.
> > Two issues:
> > 
> > 1. The symver node in symtab is marked as PREVAILING_DEF_IRONLY (no EXP) and
> >then removed by LTO.
> 
> This is however wrong - linker should not mark it as
> PREVAILING_DEF_IRONLY if it is used externally.  What linker do you use?
> On my testcases this was working with
>  GNU ld (GNU Binutils) 2.31.51.20181222
> I could easily imagine that some linkers get it wrong which should be
> reported to bintuils bugzilla but it is also easy to work around as done
> in your patch.

Hi Jan,

I'm using GNU ld 2.33.1.

I'll attach a testcase simplified from fuse-3.9 code.  "local: *;" in the
versioning script triggers the issue.  Without it there would be no problem.

> > 2. The actual function body implementing the symver-ed function is also
> > marked
> >as PREVAILING_DEF_IRONLY and then removed or marked as local.  So no
> > ".globl"
> >directive is outputed for it.
> 
> Here is the symver-ed function exported from the DSO (or is it set
> to have hidden attribute)?
> Again this was working for me, so it would be good to understand this
> issue.

It's also triggered by "local: *;".

Untar the attachment and use "make" to build it, then "make show-dynamic-syms"
to dump the dynamic symbol table.  I believe (with 99% chance) you'll see only
foo (VERS_1) and foo_v1 (because foo_v1 is marked as global in the version
script).  And foo (VERS_2) would be missing.  With this patch foo (VERS_2) would
show up.

We can't mark "foo_v2" to be "global" because it should not be a part of DSO
ABI.

The other 1% chance would be a regression in Binutils.
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University


pr48200.tar.gz
Description: application/compressed-tar

Re: [PATCH] Fix symver attribute with LTO

[Jan Hubicka]

> Hi,
> with Jan's patch commited in r278878 we can use symver attribute for functions
> and variables.  The symver attribute is designed for replacing toplevel asm
> statements containing ".symver" which may be removed by LTO.  Unfortunately,
> a quick test shown GCC still generates buggy so file with LTO and new symver
> attribute.

Thanks for looking into this.  It was on my TODo list to actually
convert some packages, so it is great you did that.
> 
> Two issues:
> 
> 1. The symver node in symtab is marked as PREVAILING_DEF_IRONLY (no EXP) and
>then removed by LTO.

This is however wrong - linker should not mark it as
PREVAILING_DEF_IRONLY if it is used externally.  What linker do you use?
On my testcases this was working with
 GNU ld (GNU Binutils) 2.31.51.20181222
I could easily imagine that some linkers get it wrong which should be
reported to bintuils bugzilla but it is also easy to work around as done
in your patch.

> 2. The actual function body implementing the symver-ed function is also marked
>as PREVAILING_DEF_IRONLY and then removed or marked as local.  So no 
> ".globl"
>directive is outputed for it.

Here is the symver-ed function exported from the DSO (or is it set
to have hidden attribute)?
Again this was working for me, so it would be good to understand this
issue.

I was thinking to extend the patch to also use name@@@nodename syntax in
case the target node is static.  This also needs bit extra work since
during LTO partitioning we can not bring such symbol hidden and need to
introduce additional attribute.  I can look into that tomorrow.

Honza
> 
> Both issue cause symbols with symver missing in DSO (with LTO enabled).
> 
> I modified fuse-3.9.0 code to use new symver attribute and tried to build it
> with GCC trunk and LTO.  The result is a buggy DSO.  With this patch applied,
> fuse-3.9.0 can be built with LTO enabled and no problem.
> 
> I'll test symver patch and this patch with more packages.
> 
> Bootstrapped/regtested x86_64-linux.  I'm not a maintainer.
> 
> gcc/ChangeLog:
> 
> 2019-12-17  Xi Ruoyao  
> 
>   * cgraph.h (symtab_node::used_from_object_file_p): Symver nodes are
>   part of DSO ABI so always used by non-LTO object files.
>   * ipa-visibility.c (cgraph_externally_visible_p): Functions with symver
>   attributes should always be visible.
> 
> Index: gcc/cgraph.h
> ===
> --- gcc/cgraph.h  (revision 279452)
> +++ gcc/cgraph.h  (working copy)
> @@ -2682,7 +2682,7 @@ symtab_node::used_from_object_file_p (vo
>  {
>if (!TREE_PUBLIC (decl) || DECL_EXTERNAL (decl))
>  return false;
> -  if (resolution_used_from_other_file_p (resolution))
> +  if (symver || resolution_used_from_other_file_p (resolution))
>  return true;
>return false;
>  }
> Index: gcc/ipa-visibility.c
> ===
> --- gcc/ipa-visibility.c  (revision 279452)
> +++ gcc/ipa-visibility.c  (working copy)
> @@ -216,6 +216,8 @@ cgraph_externally_visible_p (struct cgra
>  return true;
>if (lookup_attribute ("noipa", DECL_ATTRIBUTES (node->decl)))
>  return true;
> +  if (lookup_attribute ("symver", DECL_ATTRIBUTES (node->decl)))
> +return true;
>if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
>&& lookup_attribute ("dllexport",
>  DECL_ATTRIBUTES (node->decl)))
> -- 
> Xi Ruoyao 
> School of Aerospace Science and Technology, Xidian University
> 

[PATCH] Fix symver attribute with LTO

[Xi Ruoyao]

Hi,
with Jan's patch commited in r278878 we can use symver attribute for functions
and variables.  The symver attribute is designed for replacing toplevel asm
statements containing ".symver" which may be removed by LTO.  Unfortunately,
a quick test shown GCC still generates buggy so file with LTO and new symver
attribute.

Two issues:

1. The symver node in symtab is marked as PREVAILING_DEF_IRONLY (no EXP) and
   then removed by LTO.
2. The actual function body implementing the symver-ed function is also marked
   as PREVAILING_DEF_IRONLY and then removed or marked as local.  So no ".globl"
   directive is outputed for it.

Both issue cause symbols with symver missing in DSO (with LTO enabled).

I modified fuse-3.9.0 code to use new symver attribute and tried to build it
with GCC trunk and LTO.  The result is a buggy DSO.  With this patch applied,
fuse-3.9.0 can be built with LTO enabled and no problem.

I'll test symver patch and this patch with more packages.

Bootstrapped/regtested x86_64-linux.  I'm not a maintainer.

gcc/ChangeLog:

2019-12-17  Xi Ruoyao  

* cgraph.h (symtab_node::used_from_object_file_p): Symver nodes are
part of DSO ABI so always used by non-LTO object files.
* ipa-visibility.c (cgraph_externally_visible_p): Functions with symver
attributes should always be visible.

Index: gcc/cgraph.h
===
--- gcc/cgraph.h(revision 279452)
+++ gcc/cgraph.h(working copy)
@@ -2682,7 +2682,7 @@ symtab_node::used_from_object_file_p (vo
 {
   if (!TREE_PUBLIC (decl) || DECL_EXTERNAL (decl))
 return false;
-  if (resolution_used_from_other_file_p (resolution))
+  if (symver || resolution_used_from_other_file_p (resolution))
 return true;
   return false;
 }
Index: gcc/ipa-visibility.c
===
--- gcc/ipa-visibility.c(revision 279452)
+++ gcc/ipa-visibility.c(working copy)
@@ -216,6 +216,8 @@ cgraph_externally_visible_p (struct cgra
 return true;
   if (lookup_attribute ("noipa", DECL_ATTRIBUTES (node->decl)))
 return true;
+  if (lookup_attribute ("symver", DECL_ATTRIBUTES (node->decl)))
+return true;
   if (TARGET_DLLIMPORT_DECL_ATTRIBUTES
   && lookup_attribute ("dllexport",
   DECL_ATTRIBUTES (node->decl)))
-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: Symver attribute

[Jan Hubicka]

> On Mon, Dec 02, 2019 at 01:53:09PM +0100, Jan Hubicka wrote:
> > > > It would be great to convert libstdc++ to the new infrastructure so it
> > > > becomes LTO safe and gives some real world testing to this
> > > > infrastructure.  I tried that but found it is bit non-trivial since
> > > > currently way we need to attach the attribute to definition itself,
> > > > while current .symver output is done in separate C++ files.
> 
> What is the reason for this limitation?  I think that is pretty severe.
> Wouldn't it be enough to accept symver attribute on any declaration, but at
> the end verify that declarations that have that attribute have the
> definition in the current TU?
> 
> I mean, it is fairly common to:
> void foo ()
> {
>...
> }
> 
> asm (".symver ...");
> where the foo definition comes from one source and symver from another one,
> which includes the first source.
> 
> So, the corresponding new rewrite for that might be:
> void foo ()
> {
>   ...
> }
> 
> extern __typeof (foo) foo __attribute__((symver ("bar@@BAZ")));

Aha, this works :)  I basically meant that I need the declaration which
I found somewhat nontrivial to dig out of the libstdc++ headers.  I did
not think of typeof. I suppose I can add this as an example to the
dcumentation (as an example convertion .symver assembly into attribute).

Honza
> 
>   Jakub
> 

Re: Symver attribute

[Jakub Jelinek]

On Mon, Dec 02, 2019 at 01:53:09PM +0100, Jan Hubicka wrote:
> > > It would be great to convert libstdc++ to the new infrastructure so it
> > > becomes LTO safe and gives some real world testing to this
> > > infrastructure.  I tried that but found it is bit non-trivial since
> > > currently way we need to attach the attribute to definition itself,
> > > while current .symver output is done in separate C++ files.

What is the reason for this limitation?  I think that is pretty severe.
Wouldn't it be enough to accept symver attribute on any declaration, but at
the end verify that declarations that have that attribute have the
definition in the current TU?

I mean, it is fairly common to:
void foo ()
{
   ...
}

asm (".symver ...");
where the foo definition comes from one source and symver from another one,
which includes the first source.

So, the corresponding new rewrite for that might be:
void foo ()
{
  ...
}

extern __typeof (foo) foo __attribute__((symver ("bar@@BAZ")));

Jakub

Re: Symver attribute

[Jan Hubicka]

> > It would be great to convert libstdc++ to the new infrastructure so it
> > becomes LTO safe and gives some real world testing to this
> > infrastructure.  I tried that but found it is bit non-trivial since
> > currently way we need to attach the attribute to definition itself,
> > while current .symver output is done in separate C++ files.
> 
> I will add it to my TODO list.

Thank you! It would be great to have some evidence that things works as
expected and the attribute is practically useful for real world
projects.

I will update the documentation by your suggestions.

Honza

Re: Symver attribute

[Jonathan Wakely]

On 30/11/19 22:13 +0100, Jan Hubicka wrote:

Hi,
this is patch incorporating (I hope) all the suggestions and corrections
which I applied.  I will work incremnetaly on supporting the name@@@node
semantics which is bit different from @ and @@ one by actually removing
the original symbol.  Here I need to change assembler name of the symbol
itself but then I still need an alternative assembler name for the local
use. This is bit like weakref rewriting but not quite and it seems it is
better to do that incrementally.

It would be great to convert libstdc++ to the new infrastructure so it
becomes LTO safe and gives some real world testing to this
infrastructure.  I tried that but found it is bit non-trivial since
currently way we need to attach the attribute to definition itself,
while current .symver output is done in separate C++ files.


I will add it to my TODO list.



Index: doc/extend.texi
===
--- doc/extend.texi (revision 278877)
+++ doc/extend.texi (working copy)
@@ -3711,6 +3711,41 @@ Function Attributes}, @ref{PowerPC Funct
@ref{Nios II Function Attributes}, and @ref{S/390 Function Attributes}
for details.

+@item symver ("@var{name2}@@@var{nodename}")
+On ELF targets this attribute creates a symbol version.  The @var{name2} part
+of the parameter is the actual name of the symbol by which it will be
+externally referenced.  The @code{nodename} portion should be the name of a
+node specified in the version script supplied to the linker when building a
+shared library.  Versioned symbol must be defined and must be exported with


I think this should be either "A versioned symbol" or "Versioned
symbols".


+default visibility.
+
+@smallexample
+__attribute__ ((__symver__ ("foo@@VERS_1"))) int
+foo_v1 (void)
+@{
+@}
+@end smallexample
+
+Will produce a @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
+output.
+
+It's an error to define multiple version of a given symbol.  In such case


"In such cases"


+an alias can be used.
+
+@smallexample
+__attribute__ ((__symver__ ("foo@@VERS_2")))
+__attribute__ ((alias ("foo_v1")))
+int symver_foo_v1 (void);
+@end smallexample
+
+This example creates an alias of @code{foo_v1} with symbol name
+@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
+
+Finally if the parameter is @code{"@var{name2}@var{nodename}"} then in
+addition to creating a symbol version (as if
+@code{"@var{name2}@@@var{nodename}"} was used) the version will be also used


s/was used/had been used/

s/will be also used/will also be used/

Re: Symver attribute

[Jan Hubicka]

Hi,
this is patch incorporating (I hope) all the suggestions and corrections
which I applied.  I will work incremnetaly on supporting the name@@@node
semantics which is bit different from @ and @@ one by actually removing
the original symbol.  Here I need to change assembler name of the symbol
itself but then I still need an alternative assembler name for the local
use. This is bit like weakref rewriting but not quite and it seems it is
better to do that incrementally.

It would be great to convert libstdc++ to the new infrastructure so it
becomes LTO safe and gives some real world testing to this
infrastructure.  I tried that but found it is bit non-trivial since
currently way we need to attach the attribute to definition itself,
while current .symver output is done in separate C++ files.

Bootstrapped/regtested x86_64-linux, comitted.  I will work on the @@@
support and symbol_ref attribute for non-definitions.

Honza

2019-11-30  Jan Hubicka  

* cgraph.h (symtab_node): Add symver flag.
* cgraphunit.c (process_symver_attribute): New.
(process_common_attributes): Use process_symver_attribute.
* lto-cgraph.c (lto_output_node): Stream symver.
(lto_output_varpool_node): Stream symver.
(input_overwrite_node): Stream symver.
(input_varpool_node): Stream symver.
* output.h (do_assemble_symver): Decalre.
* symtab.c (symtab_node::dump_base): Dump symver.
(symtab_node::verify_base): Verify symver.
(symtab_node::resolve_alias): Handle symver.
* varasm.c (do_assemble_symver): New function.
* varpool.c (varpool_node::assemble_aliases): Use it.
* doc/extend.texi: (symver attribute): Document.
* config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): New.

c-family/ChangeLog:

2019-11-30  Jan Hubicka  

* c-attribs.c (handle_symver_attribute): New function
(c_common_attributes): Add symver.


Index: c-family/c-attribs.c
===
--- c-family/c-attribs.c(revision 278877)
+++ c-family/c-attribs.c(working copy)
@@ -66,6 +66,7 @@ static tree handle_stack_protect_attribu
 static tree handle_noinline_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noclone_attribute (tree *, tree, tree, int, bool *);
 static tree handle_nocf_check_attribute (tree *, tree, tree, int, bool *);
+static tree handle_symver_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noicf_attribute (tree *, tree, tree, int, bool *);
 static tree handle_noipa_attribute (tree *, tree, tree, int, bool *);
 static tree handle_leaf_attribute (tree *, tree, tree, int, bool *);
@@ -475,6 +476,8 @@ const struct attribute_spec c_common_att
  NULL },
   { "nocf_check",0, 0, false, true, true, true,
  handle_nocf_check_attribute, NULL },
+  { "symver",1, -1, true, false, false, false,
+ handle_symver_attribute, NULL},
   { "copy",   1, 1, false, false, false, false,
  handle_copy_attribute, NULL },
   { "noinit",0, 0, true,  false, false, false,
@@ -2335,6 +2338,62 @@ handle_noplt_attribute (tree *node, tree
   return NULL_TREE;
 }
 
+/* Handle a "symver" attribute.  */
+
+static tree
+handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree symver;
+  const char *symver_str;
+
+  if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
+{
+  warning (OPT_Wattributes,
+  "% attribute only applies to functions and variables");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  if (!decl_in_symtab_p (*node))
+{
+  warning (OPT_Wattributes,
+  "% attribute is only applicable to symbols");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  for (; args; args = TREE_CHAIN (args))
+{
+  symver = TREE_VALUE (args);
+  if (TREE_CODE (symver) != STRING_CST)
+   {
+ error ("% attribute argument not a string constant");
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+
+  symver_str = TREE_STRING_POINTER (symver);
+
+  int ats = 0;
+  for (int n = 0; (int)n < TREE_STRING_LENGTH (symver); n++)
+   if (symver_str[n] == '@')
+ ats++;
+
+  if (ats != 1 && ats != 2)
+   {
+ error ("symver attribute argument must have format 
%");
+ error ("% attribute argument %qs must contain one or two "
+"%<@%>", symver_str);
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+}
+
+  return NULL_TREE;
+}
+
+
 /* Handle an "alias" or &qu

Re: Symver attribute

[Jan Hubicka]

> On Sat, 30 Nov 2019, Jan Hubicka wrote:
> 
> > > I'd rather GCC created those aliases automatically (with names that can't 
> > > be used as C symbols, e.g. containing '.', if possible, or failing that 
> > > implementation-namespace names that are unlikely to conflict with C 
> > > symbols), so that the API doesn't replicate a peculiarity of the 
> > > assembler 
> > > implementation.
> > 
> > OK, this is quite easy to implement incrementally.  So the idea would be
> > to accept
> > __attribute__ ((__symver__ ("foo@VERS_1"))) int
> > __attribute__ ((__symver__ ("foo@VERS_2"))) int
> > foo_v1 (void)
> > {
> > }
> > and make GCC to produce two public symbols (foo_v1 and
> > foo_v1.somemangling) and attaching first symver alias
> > to foo_v1 and other to foo_v1.somemangling?
> 
> Yes, that sort of thing.  For glibc we'd want to be able to add extra 
> symbol versions on separate declarations of foo_v1 after it's been defined 
> rather than requiring all the attributes to be on the definition.

OK, if I get it right, the gcc-generated mangled symbol will be exported
and end up in the DSO symbol table that is not very cool.  Can we use
local aliases here (where such thing would be OK) with foo@@@vers_1
.symver directive?  This seem to produce symbol version but does not
export anything?

Honza

Re: Symver attribute

[Joseph Myers]

On Sat, 30 Nov 2019, Jan Hubicka wrote:

> > I'd rather GCC created those aliases automatically (with names that can't 
> > be used as C symbols, e.g. containing '.', if possible, or failing that 
> > implementation-namespace names that are unlikely to conflict with C 
> > symbols), so that the API doesn't replicate a peculiarity of the assembler 
> > implementation.
> 
> OK, this is quite easy to implement incrementally.  So the idea would be
> to accept
> __attribute__ ((__symver__ ("foo@VERS_1"))) int
> __attribute__ ((__symver__ ("foo@VERS_2"))) int
> foo_v1 (void)
> {
> }
> and make GCC to produce two public symbols (foo_v1 and
> foo_v1.somemangling) and attaching first symver alias
> to foo_v1 and other to foo_v1.somemangling?

Yes, that sort of thing.  For glibc we'd want to be able to add extra 
symbol versions on separate declarations of foo_v1 after it's been defined 
rather than requiring all the attributes to be on the definition.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Symver attribute

[Jan Hubicka]

Hi,
> > +static tree
> > +handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
> > +int ARG_UNUSED (flags), bool *no_add_attrs)
> > +{
> > +  tree symver;
> > +  const char *symver_str;
> > +
> > +  if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
> 
> Should the attribute apply to member functions as well?  If not,
> FWIW, I think VAR_OR_FUNCTION_DECL_P() tests for just functions
> and variables.

I do not see particular reason why one would not want to create versions
of member functions.  So I guess i will keep it here.
> 
> > +{
> > +  warning (OPT_Wattributes,
> > +  "% attribute is only applicable on functions and "
> > +  "variables");
> 
> "applicable to functions" as below (though I think the far more
> common phrase among GCC messages is "applies to functions").

Thanks, I updated the wording.
> I'm not sure I understand what the above restriction means.  That
> "it's an error to define multiple versions of the same symbol?"

yes, though the thread with Jason suggestst that we may want to lift the
restriction and maek GCC to implicitly produce the aliases.

I have updated the patch.

Honza

Re: Symver attribute

[Jan Hubicka]

> On Fri, 15 Nov 2019, Jan Hubicka wrote:
> 
> > I was originaly supporting multiple symvers like:
> > __attribute__ ((__symver__ ("foo@VERS_1"))) int
> > __attribute__ ((__symver__ ("foo@VERS_2"))) int
> > foo_v1 (void)
> > {
> > }
> > 
> > but then noticed it is rejected by gas.
> 
> That's .
> 
> > int symver_foo_v1 (void)
> > __attribute__ ((__symver__ ("foo@VERS_2")))
> > __attribute__ ((alias ("foo_v1")))
> > 
> > This describes what you want to do: create an alias symbol and then
> > attach version to it.  This is broken in current patch (though i
> 
> I'd rather GCC created those aliases automatically (with names that can't 
> be used as C symbols, e.g. containing '.', if possible, or failing that 
> implementation-namespace names that are unlikely to conflict with C 
> symbols), so that the API doesn't replicate a peculiarity of the assembler 
> implementation.

OK, this is quite easy to implement incrementally.  So the idea would be
to accept
__attribute__ ((__symver__ ("foo@VERS_1"))) int
__attribute__ ((__symver__ ("foo@VERS_2"))) int
foo_v1 (void)
{
}
and make GCC to produce two public symbols (foo_v1 and
foo_v1.somemangling) and attaching first symver alias
to foo_v1 and other to foo_v1.somemangling?

Honza
> 
> -- 
> Joseph S. Myers
> jos...@codesourcery.com

Re: Symver attribute

[Jan Hubicka]

> On 11/15/19 10:05 AM, Jan Hubicka wrote:
> > I am by no means expert in the area so feedback is welcome.  I would
> > like to get the patch to trunk early next week if it works well.
> 
> Thank you Honza for the patch. The idea looks nice to me and I have just
> few comments:
> 
> 1) we should also support foo@@@VER_1 format:
> https://sourceware.org/binutils/docs/as/Symver.html#Symver

We currently support symvers only on definitions, since symvers to
external symbols has different semantics (so it should go in
incrementally).  @@@ makes sense only if you do not attach attribute
dirrectly to a definition as we require right now.

   The third usage of the '.symver' directive is:
.symver NAME, NAME2@@@NODENAME
   When NAME is not defined within the file being assembled, it is
   treated as NAME2@NODENAME.  When NAME is defined within the file
   being
   assembled, the symbol name, NAME, will be changed to NAME2@@NODENAME.

I really think versioned symbol references should have different
attribute (such as symver_ref since it does different thing). 
> 
> 2) I quoted some error format messages

Thanks,
I updated the patch.

Honza
> 
> Martin

> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 8dcd38631d2..1723111bec0 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -2338,12 +2338,11 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED 
> (name), tree args,
>  {
>tree symver;
>const char *symver_str;
> -  unsigned n;
>  
>if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
>  {
>warning (OPT_Wattributes,
> -"symver attribute is only applicable on functions and 
> variables");
> +"% attribute is only applicable on functions and 
> variables");
>*no_add_attrs = true;
>return NULL_TREE;
>  }
> @@ -2351,7 +2350,7 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED 
> (name), tree args,
>if (!decl_in_symtab_p (*node))
>  {
>warning (OPT_Wattributes,
> -"symver attribute is only applicable to symbols");
> +"% attribute is only applicable to symbols");
>*no_add_attrs = true;
>return NULL_TREE;
>  }
> @@ -2361,7 +2360,7 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED 
> (name), tree args,
>symver = TREE_VALUE (args);
>if (TREE_CODE (symver) != STRING_CST)
>   {
> -   error ("symver attribute argument not a string constant");
> +   error ("% attribute argument not a string constant");
> *no_add_attrs = true;
> return NULL_TREE;
>   }
> @@ -2369,13 +2368,14 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED 
> (name), tree args,
>symver_str = TREE_STRING_POINTER (symver);
>  
>int ats = 0;
> -  for (n = 0; n < TREE_STRING_LENGTH (symver); n++)
> +  for (int n = 0; n < TREE_STRING_LENGTH (symver); n++)
>   if (symver_str[n] == '@')
> ats++;
>  
> -  if (ats != 1 && ats != 2)
> +  if (ats < 1 || ats > 3)
>   {
> -   error ("symver attribute argument must have format 
> %");
> +   error ("% attribute argument %qs must contain one, "
> +  "two or three %<@%>", symver_str);
> *no_add_attrs = true;
> return NULL_TREE;
>   }
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index b45a864f3ed..c7caff5023a 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -776,7 +776,7 @@ process_symver_attribute (symtab_node *n)
>return;
>  }
>  
> -  /* Create new symbol table entry represneting the version.  */
> +  /* Create new symbol table entry representing the version.  */
>tree new_decl = copy_node (n->decl);
>  
>DECL_INITIAL (new_decl) = NULL_TREE;

Re: Symver attribute

[Jakub Jelinek]

On Tue, Nov 19, 2019 at 10:19:05AM -0600, Segher Boessenkool wrote:
> On Tue, Nov 19, 2019 at 03:53:43PM +0100, Jan Hubicka wrote:
> > > Sounds perfectly fine to me, but how many tests will need changing?  Is
> > > it only those that use symbol versioning directly?
> > 
> > There are no tests presently, I plan to write some so those will get
> > dg-require-symver.
> > 
> > .symver is output only if you use explicit symver function/variable
> > attribute. So the "only" downdisde of this is that instead of getting
> > friendly error message from GCC that your target does not support
> > symvers (because we can not easily check for it) you will get less
> > friendly error message from assembler.  I hope that is acceptale since
> > we have pre-existing situations like that already.
> 
> Ah, I thought this would happen for various things from libc as well, so
> there would be a lot of random fallout.  I probably misunderstood :-)

glibc so far uses inline asm with .symver directives.  That could change one
day of course conditionally to use the GCC symver attribute.

Jakub

Re: Symver attribute

[Segher Boessenkool]

On Tue, Nov 19, 2019 at 03:53:43PM +0100, Jan Hubicka wrote:
> > Sounds perfectly fine to me, but how many tests will need changing?  Is
> > it only those that use symbol versioning directly?
> 
> There are no tests presently, I plan to write some so those will get
> dg-require-symver.
> 
> .symver is output only if you use explicit symver function/variable
> attribute. So the "only" downdisde of this is that instead of getting
> friendly error message from GCC that your target does not support
> symvers (because we can not easily check for it) you will get less
> friendly error message from assembler.  I hope that is acceptale since
> we have pre-existing situations like that already.

Ah, I thought this would happen for various things from libc as well, so
there would be a lot of random fallout.  I probably misunderstood :-)


Segher

Re: Symver attribute

[Jan Hubicka]

> On Tue, Nov 19, 2019 at 07:29:37AM +0100, Jan Hubicka wrote:
> > Current patch makes GCC to accept symver attribute on all ELF targets
> > and simply output .symver directive into the assembly file hoping for
> > the best.  I hope that is acceptable since user will be informed by
> > assembler that symver is not supported.
> > 
> > For testsuite we however needs tests to not fail when .symver is not
> > accepted by assembler that can probably by tested by the dg-require
> > infrastructure...
> > 
> > It is not perfect solution but also not much worse than what we do
> > elsewhere...
> 
> Sounds perfectly fine to me, but how many tests will need changing?  Is
> it only those that use symbol versioning directly?

There are no tests presently, I plan to write some so those will get
dg-require-symver.

.symver is output only if you use explicit symver function/variable
attribute. So the "only" downdisde of this is that instead of getting
friendly error message from GCC that your target does not support
symvers (because we can not easily check for it) you will get less
friendly error message from assembler.  I hope that is acceptale since
we have pre-existing situations like that already.

Honza


> 
> 
> Segher

Re: Symver attribute

[Segher Boessenkool]

On Tue, Nov 19, 2019 at 07:29:37AM +0100, Jan Hubicka wrote:
> Current patch makes GCC to accept symver attribute on all ELF targets
> and simply output .symver directive into the assembly file hoping for
> the best.  I hope that is acceptable since user will be informed by
> assembler that symver is not supported.
> 
> For testsuite we however needs tests to not fail when .symver is not
> accepted by assembler that can probably by tested by the dg-require
> infrastructure...
> 
> It is not perfect solution but also not much worse than what we do
> elsewhere...

Sounds perfectly fine to me, but how many tests will need changing?  Is
it only those that use symbol versioning directly?


Segher

Re: Symver attribute

[Jan Hubicka]

> On Mon, 18 Nov 2019, Rainer Orth wrote:
> 
> > So you certainly need such an effective-target test and, at least as
> > importantly, a configure test at build time that you can assemble, link,
> > and run a test correctly before enabling it in gcc.  Just
> > unconditionally dropping it into elfos.h is wrong.
> 
> Tests in gcc/configure for linking or running something are a bad idea 
> because they don't work for cross compilation (at all, for run tests; when 
> bootstrapping and target libc hasn't been built, for link tests).
> 
> I'd assume any bare-metal ELF target used with GNU binutils *could* 
> support symbol versioning if it happens to be used with an RTOS that has a 
> dynamic linker with that support, even if in fact it's much more likely to 
> be used with static linking only.  So I don't think the information is 
> available when GCC is built, beyond (a) it's definitely not supported for 
> non-ELF targets (as determined through by a list of targets such as in 
> config/elf.m4:ACX_ELF_TARGET_IFELSE) and (b) there might be a list of ELF 
> OS targets known to lack support.

Current patch makes GCC to accept symver attribute on all ELF targets
and simply output .symver directive into the assembly file hoping for
the best.  I hope that is acceptable since user will be informed by
assembler that symver is not supported.

For testsuite we however needs tests to not fail when .symver is not
accepted by assembler that can probably by tested by the dg-require
infrastructure...

It is not perfect solution but also not much worse than what we do
elsewhere...

Honza

Re: Symver attribute

[Joseph Myers]

On Mon, 18 Nov 2019, Rainer Orth wrote:

> So you certainly need such an effective-target test and, at least as
> importantly, a configure test at build time that you can assemble, link,
> and run a test correctly before enabling it in gcc.  Just
> unconditionally dropping it into elfos.h is wrong.

Tests in gcc/configure for linking or running something are a bad idea 
because they don't work for cross compilation (at all, for run tests; when 
bootstrapping and target libc hasn't been built, for link tests).

I'd assume any bare-metal ELF target used with GNU binutils *could* 
support symbol versioning if it happens to be used with an RTOS that has a 
dynamic linker with that support, even if in fact it's much more likely to 
be used with static linking only.  So I don't think the information is 
available when GCC is built, beyond (a) it's definitely not supported for 
non-ELF targets (as determined through by a list of targets such as in 
config/elf.m4:ACX_ELF_TARGET_IFELSE) and (b) there might be a list of ELF 
OS targets known to lack support.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Symver attribute

[Florian Weimer]

* Rainer Orth:

> Florian Weimer  writes:
>
>> * Rainer Orth:
>>
>>> Hi Jan,
>>>
 Also I wonder for testsuite bits, I think I need to implement
 dl-require-symver and then use it to gate the individual tests? Or do we
 have some common way to check for ELF?
>>>
>>> there's a misunderstanding throughout here: symbol versioning is *not* a
>>> (generic) ELF feature, i.e. it is not part of the ELF gABI.  Instead it
>>> started off as a Solaris extension initially, later to be adopted and
>>> extended again by gas/gld/glibc.
>>
>> The GNU implementation is incompatible at a conceptual level, it's not
>> just an extension.
>
> Huh?  The gld manual claims otherwise:
>
> https://sourceware.org/binutils/docs/ld/VERSION.html#VERSION
>
> and the ELF representation is certainly the same.  Check
>
> https://docs.oracle.com/cd/E37838_01/html/E36783/chapter5-84101.html#scrolltoc
>
>>> There are certainly ELF platforms that don't support it at all
>>> (cf. HP-UX in gccinstall, maybe some of the BSDs, historically even
>>> more like IRIX), so you cannot assume that any ELF platform will
>>> support it.  On top of that, the GNU extension of having the same
>>> symbol in multiple versions was never taken up by Solaris (and never
>>> will be AFAICT), so even when using gas and gld, uses will break when
>>> they reach ld.so.1.
>>
>> I think symbols don't carry versions in Solaris, which is the conceptual
>> difference. 8-)
>
> Huh again: you can certainly bind a symbol to a particular version (just
> one though) using linker scripts which use exactly the same format as
> gld (minus support for C++ mangling and globbing).

Ahh, sorry, I looked at this from the dynamic linker perspective.  I
still think the Solaris link editor uses those tables to map symbols to
versions.  But the correspondence is gone when loading.

The question is to what extent does this matter for .symver support in
GCC?  Does Solaris support a way to encode versioned symbol definitions
in ET_REL files?

Thanks,
Florian

Re: Symver attribute

[Rainer Orth]

Florian Weimer  writes:

> * Rainer Orth:
>
>> Hi Jan,
>>
>>> Also I wonder for testsuite bits, I think I need to implement
>>> dl-require-symver and then use it to gate the individual tests? Or do we
>>> have some common way to check for ELF?
>>
>> there's a misunderstanding throughout here: symbol versioning is *not* a
>> (generic) ELF feature, i.e. it is not part of the ELF gABI.  Instead it
>> started off as a Solaris extension initially, later to be adopted and
>> extended again by gas/gld/glibc.
>
> The GNU implementation is incompatible at a conceptual level, it's not
> just an extension.

Huh?  The gld manual claims otherwise:

https://sourceware.org/binutils/docs/ld/VERSION.html#VERSION

and the ELF representation is certainly the same.  Check

https://docs.oracle.com/cd/E37838_01/html/E36783/chapter5-84101.html#scrolltoc

>> There are certainly ELF platforms that don't support it at all
>> (cf. HP-UX in gccinstall, maybe some of the BSDs, historically even
>> more like IRIX), so you cannot assume that any ELF platform will
>> support it.  On top of that, the GNU extension of having the same
>> symbol in multiple versions was never taken up by Solaris (and never
>> will be AFAICT), so even when using gas and gld, uses will break when
>> they reach ld.so.1.
>
> I think symbols don't carry versions in Solaris, which is the conceptual
> difference. 8-)

Huh again: you can certainly bind a symbol to a particular version (just
one though) using linker scripts which use exactly the same format as
gld (minus support for C++ mangling and globbing).

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Symver attribute

[Rainer Orth]

Hi Jan,

> thanks for feedback. Here is updated patch that incorporates Martin's
> changes, formatting corrections and makes symvers of aliases work.

just a few nits.

> Index: doc/extend.texi
> ===
> --- doc/extend.texi   (revision 278293)
> +++ doc/extend.texi   (working copy)
> @@ -3640,6 +3640,34 @@ Function Attributes}, @ref{PowerPC Funct
>  @ref{Nios II Function Attributes}, and @ref{S/390 Function Attributes}
>  for details.
>  
> +@item symver ("@var{name2}@@@var{nodename}")
> +On ELF targets this attribute creates symbol version.  The @var{name2} part 
> of
> +the parameter is the actual name of the symbol by which it will be externally
> +referenced.  The @code{nodename} portion of the alias should be the name of a

Better be consistent and use @var{} in both cases.

> +node specified in the version script supplied to the linker when building a
> +shared library.  Versioned symbol must be defined and must be exported with
> +default visibility.
> +
> +@smallexample
> +__attribute__ ((__symver__ ("foo@@VERS_1"))) int
> +foo_v1 (void)
> +@{
> +@}
> +@end smallexample
> +
> +Will produce @code{.symver foo_v1, foo@@VERS_1} directive in the assembler
> +output.  It is not allowed to make multiple versions out of one symbol, 
> however
> +versioned symbol may also be an alias.
> +
> +@smallexample
> +__attribute__ ((__symver__ ("foo@VERS_2")))

this broke bootstrap:

/vol/gcc/src/hg/trunk/local/gcc/doc/extend.texi:3663: unknown command `VERS'
make[3]: *** [Makefile:3273: doc/gcc.info] Error 1

You need to be careful to properly quote @'s in texinfo.

> +__attribute__ ((alias ("foo_v1")))
> +int symver_foo_v1 (void);
> +@end smallexample
> +
> +This example creates alias an of @code{foo_v1} with symbol name
> +@code{symver_foo_v1} which will be version @code{VERS_2} of @code{foo}.
> +
>  @item target_clones (@var{options})
>  @cindex @code{target_clones} function attribute
>  The @code{target_clones} attribute is used to specify that a function

Martin already commented on the doc snippet.  I guess you'd better have
it double-checked by a native speaker.

> Index: symtab.c
> ===
> --- symtab.c  (revision 278293)
> +++ symtab.c  (working copy)
> @@ -848,6 +848,8 @@ symtab_node::dump_base (FILE *f)
>  fprintf (f, " transparent_alias");
>if (weakref)
>  fprintf (f, " weakref");
> +  if (symver)
> +fprintf (f, " symver");
>if (cpp_implicit_alias)
>  fprintf (f, " cpp_implicit_alias");
>if (alias_target)
> @@ -1145,6 +1147,11 @@ symtab_node::verify_base (void)
>error ("node is transparent_alias but not an alias");
>error_found = true;
>  }
> +  if (symver && !alias)
> +{
> +  error ("node is symver but not alias");
> +  error_found = true;
> +}
>if (same_comdat_group)
>  {
>symtab_node *n = same_comdat_group;
> @@ -1780,7 +1787,9 @@ symtab_node::resolve_alias (symtab_node
> if (target->get_comdat_group ())
>   alias_alias->add_to_same_comdat_group (target);
>   }
> -  if (!alias_alias->transparent_alias || transparent)
> +  if ((!alias_alias->transparent_alias
> +&& !alias_alias->symver)

No need for the linebreak.

> +   || transparent)
>   {
> alias_alias->remove_all_references ();
> alias_alias->create_reference (target, IPA_REF_ALIAS, NULL);
> Index: varasm.c
> ===
> --- varasm.c  (revision 278293)
> +++ varasm.c  (working copy)
> @@ -1921,6 +1921,7 @@ assemble_end_function (tree decl, const
>  switch_to_section (function_section (decl));
>ASM_DECLARE_FUNCTION_SIZE (asm_out_file, fnname, decl);
>  #endif
> +
>if (! CONSTANT_POOL_BEFORE_FUNCTION)
>  {
>output_constant_pool (fnname, decl);
> @@ -5960,6 +5961,23 @@ do_assemble_alias (tree decl, tree targe
>  #endif
>  }
>  
> +/* Output .symver directive.  */
> +
> +void
> +do_assemble_symver (tree decl, tree target)
> +{
> +  tree id = DECL_ASSEMBLER_NAME (decl);
> +  ultimate_transparent_alias_target (&id);
> +  ultimate_transparent_alias_target (&target);
> +#ifdef ASM_OUTPUT_SYMVER_DIRECTIVE
> +  ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> +IDENTIFIER_POINTER (target),
> +IDENTIFIER_POINTER (id));
> +#else
> +  error ("symver is only supported on ELF platforms");

As I said, this is misleading: better say something like "not supported
in this configuration".

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Symver attribute

[Florian Weimer]

* Rainer Orth:

> Hi Jan,
>
>> Also I wonder for testsuite bits, I think I need to implement
>> dl-require-symver and then use it to gate the individual tests? Or do we
>> have some common way to check for ELF?
>
> there's a misunderstanding throughout here: symbol versioning is *not* a
> (generic) ELF feature, i.e. it is not part of the ELF gABI.  Instead it
> started off as a Solaris extension initially, later to be adopted and
> extended again by gas/gld/glibc.

The GNU implementation is incompatible at a conceptual level, it's not
just an extension.

> There are certainly ELF platforms that don't support it at all
> (cf. HP-UX in gccinstall, maybe some of the BSDs, historically even
> more like IRIX), so you cannot assume that any ELF platform will
> support it.  On top of that, the GNU extension of having the same
> symbol in multiple versions was never taken up by Solaris (and never
> will be AFAICT), so even when using gas and gld, uses will break when
> they reach ld.so.1.

I think symbols don't carry versions in Solaris, which is the conceptual
difference. 8-)

The musl dynamic loader does not support symbol versioning (of either
kind), but binutils targeting for musl still has support for it because
there isn't really a separate non-GNU target for musl, which means that
some run-time tests involving symbol versions will fail.  (musl doesn't
want to identify as musl in its headers, so it's not really possible to
write a straightforward compile-time check for this, but maybe the GCC
build process already has this information from somewhere else.)

Thanks,
Florian

Re: Symver attribute

[Rainer Orth]

Hi Jan,

> Also I wonder for testsuite bits, I think I need to implement
> dl-require-symver and then use it to gate the individual tests? Or do we
> have some common way to check for ELF?

there's a misunderstanding throughout here: symbol versioning is *not* a
(generic) ELF feature, i.e. it is not part of the ELF gABI.  Instead it
started off as a Solaris extension initially, later to be adopted and
extended again by gas/gld/glibc.  There are certainly ELF platforms that
don't support it at all (cf. HP-UX in gccinstall, maybe some of the
BSDs, historically even more like IRIX), so you cannot assume that any
ELF platform will support it.  On top of that, the GNU extension of
having the same symbol in multiple versions was never taken up by
Solaris (and never will be AFAICT), so even when using gas and gld, uses
will break when they reach ld.so.1.

So you certainly need such an effective-target test and, at least as
importantly, a configure test at build time that you can assemble, link,
and run a test correctly before enabling it in gcc.  Just
unconditionally dropping it into elfos.h is wrong.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Symver attribute

[Martin Sebor]

On 11/15/19 9:04 AM, Jan Hubicka wrote:

Hi,
thanks for feedback. Here is updated patch that incorporates Martin's
changes, formatting corrections and makes symvers of aliases work.


Just a couple of questions and a few minor nits, mostly having to
do with my favorite subject of quoting things in diagnostics ;-)



Honza

* c-family/c-attribs.c (handle_symver_attribute): New.
(c_common_attribytes): Add symver.
* cgraph.h (symtab_node): Add symver flag.
* cgraphunit.c (process_symver_attribute): New.
(process_common_attributes): Use it.
(cgraph_node::assemble_thunks_and_aliases): Assemble symvers.
* config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): New.
* lto-cgraph.c (lto_output_node, lto_output_varpool_node,
input_overwrite_node, input_varpool_node): Stream symver attributes.
* output.h (do_assemble_symver): Declare.
* symtab.c (symtab_node::dump_base): Dump symver flag.
(symtab_node::verify_base): Sanity check symver.
(symtab_node::resolve_alias): Add support for symvers.
* varasm.c (do_assemble_symver): New.
* varpool.c (varpool_node::assemble_aliases): Output symver.

* doc/extend.texi (symver): Document new attribute.
Index: c-family/c-attribs.c
===
--- c-family/c-attribs.c(revision 278293)
+++ c-family/c-attribs.c(working copy)
@@ -146,6 +146,7 @@ static tree handle_omp_declare_target_at
  static tree handle_designated_init_attribute (tree *, tree, tree, int, bool 
*);
  static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
   int, bool *);
+static tree handle_symver_attribute (tree *, tree, tree, int, bool *);
  static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
  
  /* Helper to define attribute exclusions.  */

@@ -473,6 +474,8 @@ const struct attribute_spec c_common_att
  NULL },
{ "nocf_check", 0, 0, false, true, true, true,
  handle_nocf_check_attribute, NULL },
+  { "symver",  1, -1, true, false, false, false,
+ handle_symver_attribute, NULL},
{ "copy",   1, 1, false, false, false, false,
  handle_copy_attribute, NULL },
{ "noinit", 0, 0, true,  false, false, false,
@@ -2329,6 +2332,59 @@ handle_noplt_attribute (tree *node, tree
return NULL_TREE;
  }
  
+static tree

+handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree symver;
+  const char *symver_str;
+
+  if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)


Should the attribute apply to member functions as well?  If not,
FWIW, I think VAR_OR_FUNCTION_DECL_P() tests for just functions
and variables.


+{
+  warning (OPT_Wattributes,
+  "% attribute is only applicable on functions and "
+  "variables");


"applicable to functions" as below (though I think the far more
common phrase among GCC messages is "applies to functions").


+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  if (!decl_in_symtab_p (*node))
+{
+  warning (OPT_Wattributes,
+  "% attribute is only applicable to symbols");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  for (; args; args = TREE_CHAIN (args))
+{
+  symver = TREE_VALUE (args);
+  if (TREE_CODE (symver) != STRING_CST)
+   {
+ error ("% attribute argument not a string constant");
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+
+  symver_str = TREE_STRING_POINTER (symver);
+
+  int ats = 0;
+  for (int n = 0; n < TREE_STRING_LENGTH (symver); n++)
+       if (symver_str[n] == '@')
+ ats++;
+
+  if (ats != 1 && ats != 2)
+   {
+ error ("symver attribute argument must have format "


The "symver" part should be quoted same as above.


+"%");
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+}
+
+  return NULL_TREE;
+}
+
  /* Handle an "alias" or "ifunc" attribute; arguments as in
 struct attribute_spec.handler, except that IS_ALIAS tells us
 whether this is an alias as opposed to ifunc attribute.  */
Index: cgraph.h
===
--- cgraph.h(revision 278293)
+++ cgraph.h(working copy)
@@ -497,6 +497,8 @@ public:
   and their visibility needs to be copied from their "masters" at
   the end of parsing.  */
unsigned cpp_implicit_alia

Re: Symver attribute

[Joseph Myers]

On Fri, 15 Nov 2019, Jan Hubicka wrote:

> I was originaly supporting multiple symvers like:
> __attribute__ ((__symver__ ("foo@VERS_1"))) int
> __attribute__ ((__symver__ ("foo@VERS_2"))) int
> foo_v1 (void)
> {
> }
> 
> but then noticed it is rejected by gas.

That's .

> int symver_foo_v1 (void)
> __attribute__ ((__symver__ ("foo@VERS_2")))
> __attribute__ ((alias ("foo_v1")))
> 
> This describes what you want to do: create an alias symbol and then
> attach version to it.  This is broken in current patch (though i

I'd rather GCC created those aliases automatically (with names that can't 
be used as C symbols, e.g. containing '.', if possible, or failing that 
implementation-namespace names that are unlikely to conflict with C 
symbols), so that the API doesn't replicate a peculiarity of the assembler 
implementation.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Symver attribute

[Jan Hubicka]

Hi,
thanks for feedback. Here is updated patch that incorporates Martin's
changes, formatting corrections and makes symvers of aliases work.

Honza

* c-family/c-attribs.c (handle_symver_attribute): New.
(c_common_attribytes): Add symver.
* cgraph.h (symtab_node): Add symver flag.
* cgraphunit.c (process_symver_attribute): New.
(process_common_attributes): Use it.
(cgraph_node::assemble_thunks_and_aliases): Assemble symvers.
* config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): New.
* lto-cgraph.c (lto_output_node, lto_output_varpool_node,
input_overwrite_node, input_varpool_node): Stream symver attributes.
* output.h (do_assemble_symver): Declare.
* symtab.c (symtab_node::dump_base): Dump symver flag.
(symtab_node::verify_base): Sanity check symver.
(symtab_node::resolve_alias): Add support for symvers.
* varasm.c (do_assemble_symver): New.
* varpool.c (varpool_node::assemble_aliases): Output symver.

* doc/extend.texi (symver): Document new attribute.
Index: c-family/c-attribs.c
===
--- c-family/c-attribs.c(revision 278293)
+++ c-family/c-attribs.c(working copy)
@@ -146,6 +146,7 @@ static tree handle_omp_declare_target_at
 static tree handle_designated_init_attribute (tree *, tree, tree, int, bool *);
 static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
   int, bool *);
+static tree handle_symver_attribute (tree *, tree, tree, int, bool *);
 static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
 
 /* Helper to define attribute exclusions.  */
@@ -473,6 +474,8 @@ const struct attribute_spec c_common_att
  NULL },
   { "nocf_check",0, 0, false, true, true, true,
  handle_nocf_check_attribute, NULL },
+  { "symver",1, -1, true, false, false, false,
+ handle_symver_attribute, NULL},
   { "copy",   1, 1, false, false, false, false,
  handle_copy_attribute, NULL },
   { "noinit",0, 0, true,  false, false, false,
@@ -2329,6 +2332,59 @@ handle_noplt_attribute (tree *node, tree
   return NULL_TREE;
 }
 
+static tree
+handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree symver;
+  const char *symver_str;
+
+  if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
+{
+  warning (OPT_Wattributes,
+  "% attribute is only applicable on functions and "
+  "variables");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  if (!decl_in_symtab_p (*node))
+{
+  warning (OPT_Wattributes,
+  "% attribute is only applicable to symbols");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  for (; args; args = TREE_CHAIN (args))
+{
+  symver = TREE_VALUE (args);
+  if (TREE_CODE (symver) != STRING_CST)
+   {
+ error ("% attribute argument not a string constant");
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+
+  symver_str = TREE_STRING_POINTER (symver);
+
+  int ats = 0;
+  for (int n = 0; n < TREE_STRING_LENGTH (symver); n++)
+   if (symver_str[n] == '@')
+ ats++;
+
+  if (ats != 1 && ats != 2)
+   {
+ error ("symver attribute argument must have format "
+"%");
+ *no_add_attrs = true;
+ return NULL_TREE;
+   }
+}
+
+  return NULL_TREE;
+}
+
 /* Handle an "alias" or "ifunc" attribute; arguments as in
struct attribute_spec.handler, except that IS_ALIAS tells us
whether this is an alias as opposed to ifunc attribute.  */
Index: cgraph.h
===
--- cgraph.h(revision 278293)
+++ cgraph.h(working copy)
@@ -497,6 +497,8 @@ public:
  and their visibility needs to be copied from their "masters" at
  the end of parsing.  */
   unsigned cpp_implicit_alias : 1;
+  /* The alias is a symbol version.  */
+  unsigned symver : 1;
   /* Set once the definition was analyzed.  The list of references and
  other properties are built during analysis.  */
   unsigned analyzed : 1;
Index: cgraphunit.c
===
--- cgraphunit.c(revision 278293)
+++ cgraphunit.c(working copy)
@@ -711,6 +711,89 @@ symbol_table::process_same_body_aliases
   cpp_implicit_aliases_done = true;
 }
 
+/* Process a symver attribute.  */
+
+static void
+process_symver_

Re: Symver attribute

[Martin Liška]

On 11/15/19 10:05 AM, Jan Hubicka wrote:

I am by no means expert in the area so feedback is welcome.  I would
like to get the patch to trunk early next week if it works well.


Thank you Honza for the patch. The idea looks nice to me and I have just
few comments:

1) we should also support foo@@@VER_1 format:
https://sourceware.org/binutils/docs/as/Symver.html#Symver

2) I quoted some error format messages

Martin
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 8dcd38631d2..1723111bec0 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2338,12 +2338,11 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
 {
   tree symver;
   const char *symver_str;
-  unsigned n;
 
   if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
 {
   warning (OPT_Wattributes,
-	   "symver attribute is only applicable on functions and variables");
+	   "% attribute is only applicable on functions and variables");
   *no_add_attrs = true;
   return NULL_TREE;
 }
@@ -2351,7 +2350,7 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
   if (!decl_in_symtab_p (*node))
 {
   warning (OPT_Wattributes,
-	   "symver attribute is only applicable to symbols");
+	   "% attribute is only applicable to symbols");
   *no_add_attrs = true;
   return NULL_TREE;
 }
@@ -2361,7 +2360,7 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
   symver = TREE_VALUE (args);
   if (TREE_CODE (symver) != STRING_CST)
 	{
-	  error ("symver attribute argument not a string constant");
+	  error ("% attribute argument not a string constant");
 	  *no_add_attrs = true;
 	  return NULL_TREE;
 	}
@@ -2369,13 +2368,14 @@ handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
   symver_str = TREE_STRING_POINTER (symver);
 
   int ats = 0;
-  for (n = 0; n < TREE_STRING_LENGTH (symver); n++)
+  for (int n = 0; n < TREE_STRING_LENGTH (symver); n++)
 	if (symver_str[n] == '@')
 	  ats++;
 
-  if (ats != 1 && ats != 2)
+  if (ats < 1 || ats > 3)
 	{
-	  error ("symver attribute argument must have format %");
+	  error ("% attribute argument %qs must contain one, "
+		 "two or three %<@%>", symver_str);
 	  *no_add_attrs = true;
 	  return NULL_TREE;
 	}
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index b45a864f3ed..c7caff5023a 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -776,7 +776,7 @@ process_symver_attribute (symtab_node *n)
   return;
 }
 
-  /* Create new symbol table entry represneting the version.  */
+  /* Create new symbol table entry representing the version.  */
   tree new_decl = copy_node (n->decl);
 
   DECL_INITIAL (new_decl) = NULL_TREE;

Re: Symver attribute

[Martin Liška]

On 11/15/19 10:05 AM, Jan Hubicka wrote:

Index: params.opt
===
--- params.opt  (revision 278220)
+++ params.opt  (working copy)
@@ -483,7 +483,7 @@ Common Joined UInteger Var(param_max_inl
  The maximum number of instructions in a single function eligible for inlining 
with -O3 and -Ofast.
  
  -param=max-inline-insns-single-O2=

-Common Joined UInteger Var(param_max_inline_insns_single_o2) Init(30) Param
+Common Joined UInteger Var(param_max_inline_insns_single_o2) Init(70) Param
  The maximum number of instructions in a single function eligible for inlining.
  
  -param=max-inline-insns-size=


The following hunk is definitely not intended :)

Martin

Re: Symver attribute

[Florian Weimer]

* Jan Hubicka:

>> It's sometimes useful to define multiple versions for a single symbol.
>> For maximum binutils compatibility, you would have to use intermediate
>> aliases.  __attribute__ ((__symver__ ("foo@VERS_1", "foo@@VERS_2")))
>> could turn into this:
>> 
>>   .set foo_v1.symver.1, foo_v1
>>   .symver foo_v1.symver.1, foo_v1@VERS_1
>>   .set foo_v1.symver.2, foo_v1
>>   .symver foo_v1.symver.2, foo_v1@@VERS_2
>
> I was originaly supporting multiple symvers like:
> __attribute__ ((__symver__ ("foo@VERS_1"))) int
> __attribute__ ((__symver__ ("foo@VERS_2"))) int
> foo_v1 (void)
> {
> }
>
> but then noticed it is rejected by gas.

Yeah, I think that failure is somewhat dependent on the target and the
GAS version.

> I intended to support:
>
> __attribute__ ((__symver__ ("foo@VERS_1"))) int
> foo_v1 (void)
> {
> }
>
> int symver_foo_v1 (void)
> __attribute__ ((__symver__ ("foo@VERS_2")))
> __attribute__ ((alias ("foo_v1")))

That's a bit of a mouthful.  We probably could hide it behind a macro
using __COUNTER.

> It would be prettier to support former but we need an exported symbol
> name for the second alis, right?

I assumed that .symver would make the versioned alias global, but
sadly it does not.  You are right, you currently need to start with a
global symbol.  Maybe there is a way around that.  I'm going to ask on
the binutils list.

Re: Symver attribute

[Jan Hubicka]

> * Jan Hubicka:
> 
> > Internally the patch tries to mimic what happens in ELF.  In particular
> >
> > __attribute__ ((__symver__ ("foo@VERS_1"))) int
> > foo_v1 (void)
> > {
> > }
> >
> > creates a special form of alias with symver flag. Its assembler name is
> > foo@VERS_1 since it is what happens in ELF too (normally @ is not
> > allowed in symbol name).  Rest of GCC and LTO path handles is as normal
> > alias (since it seems what binutils does as well) and instead of
> > outputting it via .set we use .symver directive at the end.
> 
> Do you support foo@@VERS_1 for setting the default version as well?

Yes, i only reject the string if number of @'s is less then 1 or more
than 2 :)
(I wonder if there is better checking on that string)
> 
> It's sometimes useful to define multiple versions for a single symbol.
> For maximum binutils compatibility, you would have to use intermediate
> aliases.  __attribute__ ((__symver__ ("foo@VERS_1", "foo@@VERS_2")))
> could turn into this:
> 
>   .set foo_v1.symver.1, foo_v1
>   .symver foo_v1.symver.1, foo_v1@VERS_1
>   .set foo_v1.symver.2, foo_v1
>   .symver foo_v1.symver.2, foo_v1@@VERS_2

I was originaly supporting multiple symvers like:
__attribute__ ((__symver__ ("foo@VERS_1"))) int
__attribute__ ((__symver__ ("foo@VERS_2"))) int
foo_v1 (void)
{
}

but then noticed it is rejected by gas.

I intended to support:

__attribute__ ((__symver__ ("foo@VERS_1"))) int
foo_v1 (void)
{
}

int symver_foo_v1 (void)
__attribute__ ((__symver__ ("foo@VERS_2")))
__attribute__ ((alias ("foo_v1")))

This describes what you want to do: create an alias symbol and then
attach version to it.  This is broken in current patch (though i
indended it to work).  I will fix that:
.symver foo_v1, foo@VERS_2
.globl  symver_foo_v1
.setsymver_foo_v1,foo_v1
.symver foo_v1, foo@VERS_1
GCC currently redirect aliases to the their ultimate targets. I need to
implement an exception here. (We already do that for weakrefs and
syntactic aliases)

It would be prettier to support former but we need an exported symbol
name for the second alis, right?

Honza
> 
> Sometimes it's also necessary to reference a symbol version.  I
> suspect we'd need a separate attribute for that, or enhance the
> __asm__ alias code to recognize @ and @@ symbols (also creating
> internal aliases).
> 
> We would use both extensions (multiple symbol versions and symbol
> references) internally for glibc eventually, once compiler versions
> before GCC 10 are no longer supported.

Re: Symver attribute

[Florian Weimer]

* Jan Hubicka:

> Internally the patch tries to mimic what happens in ELF.  In particular
>
> __attribute__ ((__symver__ ("foo@VERS_1"))) int
> foo_v1 (void)
> {
> }
>
> creates a special form of alias with symver flag. Its assembler name is
> foo@VERS_1 since it is what happens in ELF too (normally @ is not
> allowed in symbol name).  Rest of GCC and LTO path handles is as normal
> alias (since it seems what binutils does as well) and instead of
> outputting it via .set we use .symver directive at the end.

Do you support foo@@VERS_1 for setting the default version as well?

It's sometimes useful to define multiple versions for a single symbol.
For maximum binutils compatibility, you would have to use intermediate
aliases.  __attribute__ ((__symver__ ("foo@VERS_1", "foo@@VERS_2")))
could turn into this:

  .set foo_v1.symver.1, foo_v1
  .symver foo_v1.symver.1, foo_v1@VERS_1
  .set foo_v1.symver.2, foo_v1
  .symver foo_v1.symver.2, foo_v1@@VERS_2

Sometimes it's also necessary to reference a symbol version.  I
suspect we'd need a separate attribute for that, or enhance the
__asm__ alias code to recognize @ and @@ symbols (also creating
internal aliases).

We would use both extensions (multiple symbol versions and symbol
references) internally for glibc eventually, once compiler versions
before GCC 10 are no longer supported.

Symver attribute

[Jan Hubicka]

Hi,
this patch implements symver attribute which can be use din place of
.symver directive and is LTO friendly.  I would welcome feedback for
this, in particular some evidence that codebases can be easily converted
to use it.

There is some discussion in the PR and its duplicate filled by Carlos.
He suggests bit more elaborate syntax then just passing name@nodename
as a string.  I can implement it easily if that is preferred based on
the current infrastructure (basically handle_symver_attribute will need
to parse the parameters and produce the actual assembler name).

I found the PR only after drafing the patch and my goal was to keep
things similar to .symver GAS directive.  I am not sure which variant is
preferred. I sort of like to keep symver attribute name same as the gas
directive and i think users are used to the node@nodename strings since
they appear everywhere else.

Internally the patch tries to mimic what happens in ELF.  In particular

__attribute__ ((__symver__ ("foo@VERS_1"))) int
foo_v1 (void)
{
}

creates a special form of alias with symver flag. Its assembler name is
foo@VERS_1 since it is what happens in ELF too (normally @ is not
allowed in symbol name).  Rest of GCC and LTO path handles is as normal
alias (since it seems what binutils does as well) and instead of
outputting it via .set we use .symver directive at the end.

I added some sanity checks: for duplicated directives, for versions of
weakrefs and commons (which are not accepted by gas) and for versions of
symbols not exported to final symbol table (which are accepted by gas
but seems to have no effect).

I am by no means expert in the area so feedback is welcome.  I would
like to get the patch to trunk early next week if it works well.

Also I wonder for testsuite bits, I think I need to implement
dl-require-symver and then use it to gate the individual tests? Or do we
have some common way to check for ELF?

Honza

PR lto/48200
* c-family/c-attribs.c (handle_symver_attribute): New.
(c_common_attribytes): Add symver.
* cgraph.h (symtab_node): Add symver flag.
* cgraphunit.c (process_symver_attribute): New.
(process_common_attributes): Use it.
(cgraph_node::assemble_thunks_and_aliases): Assemble symvers.
* config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): New.
* lto-cgraph.c (lto_output_node, lto_output_varpool_node,
input_overwrite_node, input_varpool_node): Stream symver attributes.
* output.h (do_assemble_symver): Declare.
* symtab.c (symtab_node::dump_base): Dump symver flag.
(symtab_node::verify_base): Sanity check symver.
* varasm.c (do_assemble_symver): New.
* varpool.c (varpool_node::assemble_aliases): Output symver.

* doc/extend.texi (symver): Document new attribute.

Index: c-family/c-attribs.c
===
--- c-family/c-attribs.c(revision 278220)
+++ c-family/c-attribs.c(working copy)
@@ -149,6 +149,7 @@ static tree handle_designated_init_attri
 static tree handle_fallthrough_attribute (tree *, tree, tree, int, bool *);
 static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
   int, bool *);
+static tree handle_symver_attribute (tree *, tree, tree, int, bool *);
 static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
 
 /* Helper to define attribute exclusions.  */
@@ -476,6 +477,8 @@ const struct attribute_spec c_common_att
  NULL },
   { "nocf_check",0, 0, false, true, true, true,
  handle_nocf_check_attribute, NULL },
+  { "symver",1, -1, true, false, false, false,
+ handle_symver_attribute, NULL},
   { "copy",   1, 1, false, false, false, false,
  handle_copy_attribute, NULL },
   { "noinit",0, 0, true,  false, false, false,
@@ -2332,6 +2335,58 @@ handle_noplt_attribute (tree *node, tree
   return NULL_TREE;
 }
 
+static tree
+handle_symver_attribute (tree *node, tree ARG_UNUSED (name), tree args,
+int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree symver;
+  const char *symver_str;
+  unsigned n;
+
+  if (TREE_CODE (*node) != FUNCTION_DECL && TREE_CODE (*node) != VAR_DECL)
+    {
+  warning (OPT_Wattributes,
+  "symver attribute is only applicable on functions and 
variables");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  if (!decl_in_symtab_p (*node))
+    {
+  warning (OPT_Wattributes,
+  "symver attribute is only applicable to symbols");
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+  for (; args; args = TREE_CHAIN (args))
+{
+  symver = TREE_VALUE (args);
+  if (