Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On Wed, Aug 26, 2020 at 5:24 AM Martin Liška wrote: > > On 8/26/20 2:22 PM, H.J. Lu wrote: > > What is the question? > > What are the scenarios where the new syntax: > >.symver foo, foo@VERS_1, local# Change foo to a local symbol. foo is local to the file. Use it there is no global reference to foo. It is usually used to define foo@VERS_1. >.symver foo, foo@VERS_2, hidden # Change foo to a hidden symbol. > foo is local to shared object or executable. It provides internal reference to foo. The difference is internal reference to foo. -- H.J.
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On 8/26/20 2:22 PM, H.J. Lu wrote: What is the question? What are the scenarios where the new syntax: .symver foo, foo@VERS_1, local# Change foo to a local symbol. .symver foo, foo@VERS_2, hidden # Change foo to a hidden symbol. is useful? Thanks, Martin
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On Wed, Aug 26, 2020 at 4:34 AM Martin Liška wrote:
>
> On 8/26/20 1:27 PM, Jan Hubicka wrote:
> >> On 8/26/20 11:22 AM, Jan Hubicka wrote:
> On 8/25/20 8:46 PM, Jan Hubicka wrote:
> > What will happen here with protected visibility?
>
> I forgot about it. Should it be mapped also to "local"?
>
> + const char *visibility = NULL;
> + if (!TREE_PUBLIC (origin_decl))
> +visibility = "remove";
> + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL
> + || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED)
> +visibility = "local";
> + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN)
> +visibility = "hidden";
> >>>
> >>> I have no idea (depends what gas will do), you need to check if the
> >>> resulting symbol will indeed have right visibility in all of the cases.
> >>> If some of them are not possible, I suppose we could just reject the
> >>> comination.
> >>
> >> Ok, so I experimented a bit with the .symver attribute I don't see how
> >> is the new syntax handy (I mean .symver foo, foo@VERS_2, hidden and
> >> .symver foo, foo@VERS_1, local)?
> >>
> >> For instance:
> >>
> >> $ cat vi2.c
> >> int
> >> __attribute__((visibility ("hidden")))
> >> hidden_object;
> >>
> >> extern int
> >> __attribute__ ((__symver__ ("foo@@VERS_2")))
> >> __attribute__ ((alias ("hidden_object")))
> >> symver_foo_v1;
> >>
> >> $ gcc vi2.c -S
> >> $ cat vi2.s | grep '\.symver'
> >> .symver symver_foo_v1, foo@@VERS_2
> >>
> >> $ readelf -s vi2.o -W
> >> ...
> >> 8: 4 OBJECT GLOBAL HIDDEN 3 hidden_object
> >> 9: 4 OBJECT GLOBAL DEFAULT3 symver_foo_v1
> >> 10: 4 OBJECT GLOBAL DEFAULT3 foo@@VERS_2
> >>
> >> Which seems fine to me. Similarly one can directly modify visibility of
> >> symver_foo_v1 with:
> >> .hidden symver_foo_v1
> >>
> >> Or do I miss something?
> >
> > At low-level (and how GCC symtab should understand it), symver is just
> > a symbol with funny name. Alias is just another symbol pointing to same
> > place and in general we want to handle versions as aliases with funny
> > names.
> >
> > But it is not how gas interpret its.
> > Writting .symver foo, foo@VERS_2 in my understnading does two things
> > 1) it makes gas to turn all apperances of references to "foo" to
> > "foo@VERS_2"
> > this is necessary since gas syntax does not allow names with @ in it
> > so otherwise we have no way to reffer to it
> > 2) it either exports the foo symbol (and you can add visibility).
> > or makes foo@VERS_2 disappear depending how you declare visibilities
> > of foo.
> >
> > With this we lose way to refernece to actul symbol foo (and not
> > foo@VERS_2) which may be needed in LTO if one symbol declares foo and
> > other declares foo with symtab attribute.
> >
> > So I think we want symver attributes of symbol "foo" to produce a local
> > alias "foo.localalias" which will be renamed for GAS to "foo@VERS_2".
> > Symtab can understand this via syntactic aliases. Then we have way to
> > refer to symbol "foo" if we want "foo" and "foo.localalias" if we want
> > "foo@VERS_2". I had patch for that but I stopped on the situation that
> > there was no way to prevent gas from doing 2).
>
> Now I see how is this more complicated :) Can you please send the patch
> you have for this?
>
> >
> > So I see reason for .symbol X,Y, local
> > I do not see much morivation for others, that is why I stopped on
> > updating the patch for new gas syntax - wanted to take some time to
> > understand it (and the time did not materialized).
> >
> > So it seems to me that taking that old patch (or patch of yours) and add
> > the local alias machinery will do the trick, but I may be missing
> > something.
>
> Maybe H.J. can chime in what was motivation for implementation of the syntax?
What is the question?
--
H.J.
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On 8/26/20 1:27 PM, Jan Hubicka wrote:
On 8/26/20 11:22 AM, Jan Hubicka wrote:
On 8/25/20 8:46 PM, Jan Hubicka wrote:
What will happen here with protected visibility?
I forgot about it. Should it be mapped also to "local"?
+ const char *visibility = NULL;
+ if (!TREE_PUBLIC (origin_decl))
+visibility = "remove";
+ else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL
+ || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED)
+visibility = "local";
+ else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN)
+visibility = "hidden";
I have no idea (depends what gas will do), you need to check if the
resulting symbol will indeed have right visibility in all of the cases.
If some of them are not possible, I suppose we could just reject the
comination.
Ok, so I experimented a bit with the .symver attribute I don't see how
is the new syntax handy (I mean .symver foo, foo@VERS_2, hidden and
.symver foo, foo@VERS_1, local)?
For instance:
$ cat vi2.c
int
__attribute__((visibility ("hidden")))
hidden_object;
extern int
__attribute__ ((__symver__ ("foo@@VERS_2")))
__attribute__ ((alias ("hidden_object")))
symver_foo_v1;
$ gcc vi2.c -S
$ cat vi2.s | grep '\.symver'
.symver symver_foo_v1, foo@@VERS_2
$ readelf -s vi2.o -W
...
8: 4 OBJECT GLOBAL HIDDEN 3 hidden_object
9: 4 OBJECT GLOBAL DEFAULT3 symver_foo_v1
10: 4 OBJECT GLOBAL DEFAULT3 foo@@VERS_2
Which seems fine to me. Similarly one can directly modify visibility of
symver_foo_v1 with:
.hidden symver_foo_v1
Or do I miss something?
At low-level (and how GCC symtab should understand it), symver is just
a symbol with funny name. Alias is just another symbol pointing to same
place and in general we want to handle versions as aliases with funny
names.
But it is not how gas interpret its.
Writting .symver foo, foo@VERS_2 in my understnading does two things
1) it makes gas to turn all apperances of references to "foo" to "foo@VERS_2"
this is necessary since gas syntax does not allow names with @ in it
so otherwise we have no way to reffer to it
2) it either exports the foo symbol (and you can add visibility).
or makes foo@VERS_2 disappear depending how you declare visibilities
of foo.
With this we lose way to refernece to actul symbol foo (and not
foo@VERS_2) which may be needed in LTO if one symbol declares foo and
other declares foo with symtab attribute.
So I think we want symver attributes of symbol "foo" to produce a local
alias "foo.localalias" which will be renamed for GAS to "foo@VERS_2".
Symtab can understand this via syntactic aliases. Then we have way to
refer to symbol "foo" if we want "foo" and "foo.localalias" if we want
"foo@VERS_2". I had patch for that but I stopped on the situation that
there was no way to prevent gas from doing 2).
Now I see how is this more complicated :) Can you please send the patch
you have for this?
So I see reason for .symbol X,Y, local
I do not see much morivation for others, that is why I stopped on
updating the patch for new gas syntax - wanted to take some time to
understand it (and the time did not materialized).
So it seems to me that taking that old patch (or patch of yours) and add
the local alias machinery will do the trick, but I may be missing
something.
Maybe H.J. can chime in what was motivation for implementation of the syntax?
MArtin
Honza
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
> On 8/26/20 11:22 AM, Jan Hubicka wrote:
> > > On 8/25/20 8:46 PM, Jan Hubicka wrote:
> > > > What will happen here with protected visibility?
> > >
> > > I forgot about it. Should it be mapped also to "local"?
> > >
> > > + const char *visibility = NULL;
> > > + if (!TREE_PUBLIC (origin_decl))
> > > +visibility = "remove";
> > > + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL
> > > + || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED)
> > > +visibility = "local";
> > > + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN)
> > > +visibility = "hidden";
> >
> > I have no idea (depends what gas will do), you need to check if the
> > resulting symbol will indeed have right visibility in all of the cases.
> > If some of them are not possible, I suppose we could just reject the
> > comination.
>
> Ok, so I experimented a bit with the .symver attribute I don't see how
> is the new syntax handy (I mean .symver foo, foo@VERS_2, hidden and
> .symver foo, foo@VERS_1, local)?
>
> For instance:
>
> $ cat vi2.c
> int
> __attribute__((visibility ("hidden")))
> hidden_object;
>
> extern int
> __attribute__ ((__symver__ ("foo@@VERS_2")))
> __attribute__ ((alias ("hidden_object")))
> symver_foo_v1;
>
> $ gcc vi2.c -S
> $ cat vi2.s | grep '\.symver'
> .symver symver_foo_v1, foo@@VERS_2
>
> $ readelf -s vi2.o -W
> ...
> 8: 4 OBJECT GLOBAL HIDDEN 3 hidden_object
> 9: 4 OBJECT GLOBAL DEFAULT3 symver_foo_v1
> 10: 4 OBJECT GLOBAL DEFAULT3 foo@@VERS_2
>
> Which seems fine to me. Similarly one can directly modify visibility of
> symver_foo_v1 with:
> .hidden symver_foo_v1
>
> Or do I miss something?
At low-level (and how GCC symtab should understand it), symver is just
a symbol with funny name. Alias is just another symbol pointing to same
place and in general we want to handle versions as aliases with funny
names.
But it is not how gas interpret its.
Writting .symver foo, foo@VERS_2 in my understnading does two things
1) it makes gas to turn all apperances of references to "foo" to "foo@VERS_2"
this is necessary since gas syntax does not allow names with @ in it
so otherwise we have no way to reffer to it
2) it either exports the foo symbol (and you can add visibility).
or makes foo@VERS_2 disappear depending how you declare visibilities
of foo.
With this we lose way to refernece to actul symbol foo (and not
foo@VERS_2) which may be needed in LTO if one symbol declares foo and
other declares foo with symtab attribute.
So I think we want symver attributes of symbol "foo" to produce a local
alias "foo.localalias" which will be renamed for GAS to "foo@VERS_2".
Symtab can understand this via syntactic aliases. Then we have way to
refer to symbol "foo" if we want "foo" and "foo.localalias" if we want
"foo@VERS_2". I had patch for that but I stopped on the situation that
there was no way to prevent gas from doing 2).
So I see reason for .symbol X,Y, local
I do not see much morivation for others, that is why I stopped on
updating the patch for new gas syntax - wanted to take some time to
understand it (and the time did not materialized).
So it seems to me that taking that old patch (or patch of yours) and add
the local alias machinery will do the trick, but I may be missing
something.
Honza
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On 8/26/20 11:22 AM, Jan Hubicka wrote:
On 8/25/20 8:46 PM, Jan Hubicka wrote:
What will happen here with protected visibility?
I forgot about it. Should it be mapped also to "local"?
+ const char *visibility = NULL;
+ if (!TREE_PUBLIC (origin_decl))
+visibility = "remove";
+ else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL
+ || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED)
+visibility = "local";
+ else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN)
+visibility = "hidden";
I have no idea (depends what gas will do), you need to check if the
resulting symbol will indeed have right visibility in all of the cases.
If some of them are not possible, I suppose we could just reject the
comination.
Ok, so I experimented a bit with the .symver attribute I don't see how
is the new syntax handy (I mean .symver foo, foo@VERS_2, hidden and
.symver foo, foo@VERS_1, local)?
For instance:
$ cat vi2.c
int
__attribute__((visibility ("hidden")))
hidden_object;
extern int
__attribute__ ((__symver__ ("foo@@VERS_2")))
__attribute__ ((alias ("hidden_object")))
symver_foo_v1;
$ gcc vi2.c -S
$ cat vi2.s | grep '\.symver'
.symver symver_foo_v1, foo@@VERS_2
$ readelf -s vi2.o -W
...
8: 4 OBJECT GLOBAL HIDDEN 3 hidden_object
9: 4 OBJECT GLOBAL DEFAULT3 symver_foo_v1
10: 4 OBJECT GLOBAL DEFAULT3 foo@@VERS_2
Which seems fine to me. Similarly one can directly modify visibility of
symver_foo_v1 with:
.hidden symver_foo_v1
Or do I miss something?
Martin
Honza
Thanks,
Martin
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
> On 8/25/20 8:46 PM, Jan Hubicka wrote: > > What will happen here with protected visibility? > > I forgot about it. Should it be mapped also to "local"? > > + const char *visibility = NULL; > + if (!TREE_PUBLIC (origin_decl)) > +visibility = "remove"; > + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL > + || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED) > +visibility = "local"; > + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN) > +visibility = "hidden"; I have no idea (depends what gas will do), you need to check if the resulting symbol will indeed have right visibility in all of the cases. If some of them are not possible, I suppose we could just reject the comination. Honza > > Thanks, > Martin
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
On 8/25/20 8:46 PM, Jan Hubicka wrote: What will happen here with protected visibility? I forgot about it. Should it be mapped also to "local"? + const char *visibility = NULL; + if (!TREE_PUBLIC (origin_decl)) +visibility = "remove"; + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL + || DECL_VISIBILITY (origin_decl) == VISIBILITY_PROTECTED) +visibility = "local"; + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN) +visibility = "hidden"; Thanks, Martin
Re: [PATCH 2/2] IPA symver: support visibility and static symbols.
>
> gcc/ChangeLog:
>
> * cgraphunit.c (process_symver_attribute): Remove checks that
> are not needed now.
> (cgraph_node::assemble_thunks_and_aliases): Change second
> argument to decl.
> * config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): Add new
> VISIBILITY parameter.
> * doc/extend.texi: Document that .symver supports visibility.
> * symtab.c (symtab_node::verify_base): Remove checks that
> are not needed now.
> * varasm.c (do_assemble_symver): Detect visibility .symver
> directive argument.
> * varpool.c (varpool_node::assemble_aliases): Change second
> argument to decl.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/ipa/symver2.c: New test.
> * gcc.dg/ipa/symver3.c: New test.
Thanks for looking into this!
> void
> -do_assemble_symver (tree decl, tree target)
> +do_assemble_symver (tree decl, tree origin_decl)
> {
> + tree target = DECL_ASSEMBLER_NAME (origin_decl);
>tree id = DECL_ASSEMBLER_NAME (decl);
>ultimate_transparent_alias_target ();
>ultimate_transparent_alias_target ();
> #ifdef ASM_OUTPUT_SYMVER_DIRECTIVE
> + const char *visibility = NULL;
> + if (!TREE_PUBLIC (origin_decl))
> +visibility = "remove";
> + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_INTERNAL)
> +visibility = "local";
> + else if (DECL_VISIBILITY (origin_decl) == VISIBILITY_HIDDEN)
> +visibility = "hidden";
What will happen here with protected visibility?
Otherwise patch makes sense to me.
Honza
>ASM_OUTPUT_SYMVER_DIRECTIVE (asm_out_file,
> IDENTIFIER_POINTER (target),
> -IDENTIFIER_POINTER (id));
> +IDENTIFIER_POINTER (id), visibility);
> #else
>error ("symver is only supported on ELF platforms");
> #endif
> diff --git a/gcc/varpool.c b/gcc/varpool.c
> index 458cdf1bf37..95d7844a398 100644
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -540,8 +540,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));
[PATCH 2/2] IPA symver: support visibility and static symbols.
gcc/ChangeLog:
* cgraphunit.c (process_symver_attribute): Remove checks that
are not needed now.
(cgraph_node::assemble_thunks_and_aliases): Change second
argument to decl.
* config/elfos.h (ASM_OUTPUT_SYMVER_DIRECTIVE): Add new
VISIBILITY parameter.
* doc/extend.texi: Document that .symver supports visibility.
* symtab.c (symtab_node::verify_base): Remove checks that
are not needed now.
* varasm.c (do_assemble_symver): Detect visibility .symver
directive argument.
* varpool.c (varpool_node::assemble_aliases): Change second
argument to decl.
gcc/testsuite/ChangeLog:
* gcc.dg/ipa/symver2.c: New test.
* gcc.dg/ipa/symver3.c: New test.
---
gcc/cgraphunit.c | 15 +--
gcc/config/elfos.h | 20 +++-
gcc/doc/extend.texi| 3 +--
gcc/symtab.c | 16
gcc/testsuite/gcc.dg/ipa/symver2.c | 9 +
gcc/testsuite/gcc.dg/ipa/symver3.c | 13 +
gcc/varasm.c | 12 ++--
gcc/varpool.c | 3 +--
8 files changed, 46 insertions(+), 45 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/ipa/symver2.c
create mode 100644 gcc/testsuite/gcc.dg/ipa/symver3.c
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index fa3aec79a48..c85d3482c8b 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -765,18 +765,6 @@ process_symver_attribute (symtab_node *n)
"% cannot be versioned");
return;
}
- if (!TREE_PUBLIC (n->decl))
- {
- error_at (DECL_SOURCE_LOCATION (n->decl),
- "versioned symbol must be public");
- return;
- }
- if (DECL_VISIBILITY (n->decl) != VISIBILITY_DEFAULT)
- {
- error_at (DECL_SOURCE_LOCATION (n->decl),
- "versioned symbol must have default visibility");
- return;
- }
/* Create new symbol table entry representing the version. */
tree new_decl = copy_node (n->decl);
@@ -2239,8 +2227,7 @@ cgraph_node::assemble_thunks_and_aliases (void)
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));
diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h
index 74a3eafda6b..6c9b73320b9 100644
--- a/gcc/config/elfos.h
+++ b/gcc/config/elfos.h
@@ -248,15 +248,17 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
} \
while (0)
-#define ASM_OUTPUT_SYMVER_DIRECTIVE(FILE, NAME, NAME2) \
- do\
-{\
- fputs ("\t.symver\t", (FILE));\
- assemble_name ((FILE), (NAME));\
- fputs (", ", (FILE)); \
- assemble_name ((FILE), (NAME2));\
- fputc ('\n', (FILE)); \
-}\
+#define ASM_OUTPUT_SYMVER_DIRECTIVE(FILE, NAME, NAME2, VISIBILITY) \
+ do\
+{\
+ fputs ("\t.symver\t", (FILE));\
+ assemble_name ((FILE), (NAME));\
+ fputs (", ", (FILE)); \
+ assemble_name ((FILE), (NAME2));\
+ if (visibility != NULL) \
+ fprintf ((FILE), ", %s", (VISIBILITY)); \
+ fputc ('\n', (FILE)); \
+}\
while (0)
/* The following macro defines the format used to output the second
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 97ae1ee0843..22e62f36714 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3729,8 +3729,7 @@ 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
-default visibility.
+shared library.
@smallexample
__attribute__ ((__symver__ ("foo@@VERS_1"))) int
diff --git a/gcc/symtab.c b/gcc/symtab.c
index d7dfbb676df..51628fe625f 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -1178,22 +1178,6 @@ symtab_node::verify_base (void)
error ("node is symver but not alias");
error_found = true;
}
- /* Limitation of gas requires us to output targets of symver aliases as
- global symbols. This is binutils PR 25295. */
- if (symver
- && (!TREE_PUBLIC (get_alias_target ()->decl)
- || DECL_VISIBILITY (get_alias_target ()->decl) != VISIBILITY_DEFAULT))
-{
- error ("symver target is not exported with default visibility");
- error_found = true;
-}
- if (symver
- && (!TREE_PUBLIC (decl)
- || DECL_VISIBILITY (decl) != VISIBILITY_DEFAULT))
-{
- error ("symver is not exported with default visibility");
-
