as: Replace the removed symbol with the versioned symbol

Message ID 20210801151620.2819820-1-hjl.tools@gmail.com
State New
Headers show
Series
  • as: Replace the removed symbol with the versioned symbol
Related show

Commit Message

Alan Modra via Binutils Aug. 1, 2021, 3:16 p.m.
When a symbol removed by .symver is used in relocation and there is one
and only one versioned symbol, don't remove the symbol.  Instead, mark
it to be removed and replace the removed symbol used in relocation with
the versioned symbol before generating relocation.

	PR gas/28157
	* symbols.c (symbol_flags): Add removed.
	(symbol_entry_find): Updated.
	(symbol_mark_removed): New function.
	(symbol_removed_p): Likewise.
	* symbols.h (symbol_mark_removed): New prototype.
	(symbol_removed_p): Likewise.
	* write.c (write_relocs): Call obj_fixup_removed_symbol on
	removed fixp->fx_addsy and fixp->fx_subsy if defined.
	(set_symtab): Don't add a symbol if symbol_removed_p returns true.
	* config/obj-elf.c (elf_frob_symbol):  Don't remove the symbol
	if it is used on relocation.  Instead, mark it as to be removed
	and issue an error if the symbol has more than one versioned name.
	(elf_fixup_removed_symbol): New function.
	* config/obj-elf.h (elf_fixup_removed_symbol): New prototype.
	(obj_fixup_removed_symbol): New.
	* testsuite/gas/symver/symver11.d: Updated expected error
	message.
	* testsuite/gas/symver/symver16.d: New file.
	* testsuite/gas/symver/symver16.s: Likewise.
---
 gas/config/obj-elf.c                | 28 ++++++++++++++++++++++++++--
 gas/config/obj-elf.h                |  5 +++++
 gas/symbols.c                       | 26 +++++++++++++++++++++++++-
 gas/symbols.h                       |  2 ++
 gas/testsuite/gas/symver/symver11.d |  2 +-
 gas/testsuite/gas/symver/symver16.d | 13 +++++++++++++
 gas/testsuite/gas/symver/symver16.s | 16 ++++++++++++++++
 gas/write.c                         | 21 +++++++++++++++------
 8 files changed, 103 insertions(+), 10 deletions(-)
 create mode 100644 gas/testsuite/gas/symver/symver16.d
 create mode 100644 gas/testsuite/gas/symver/symver16.s

-- 
2.31.1

Comments

Fangrui Song Aug. 10, 2021, 9:56 p.m. | #1
On 2021-08-01, H.J. Lu via Binutils wrote:
>When a symbol removed by .symver is used in relocation and there is one

>and only one versioned symbol, don't remove the symbol.  Instead, mark

>it to be removed and replace the removed symbol used in relocation with

>the versioned symbol before generating relocation.


Thanks for the patch.
The behavior looks good.

.symver foo, foo@v1, remove
.globl foo
foo:
   call foo   # R_X86_64_PLT32 foo@v1



I think this can be extended to non-remove one-@ as well.
The unadorned symbol and relocations referencing it just add complexity
to linker internals.

.symver foo, foo@v1
Alan Modra via Binutils Aug. 11, 2021, 1:03 p.m. | #2
On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:
>

> On 2021-08-01, H.J. Lu via Binutils wrote:

> >When a symbol removed by .symver is used in relocation and there is one

> >and only one versioned symbol, don't remove the symbol.  Instead, mark

> >it to be removed and replace the removed symbol used in relocation with

> >the versioned symbol before generating relocation.

>

> Thanks for the patch.

> The behavior looks good.

>

> .symver foo, foo@v1, remove

> .globl foo

> foo:

>    call foo   # R_X86_64_PLT32 foo@v1

>

>

>

> I think this can be extended to non-remove one-@ as well.

> The unadorned symbol and relocations referencing it just add complexity

> to linker internals.

>

> .symver foo, foo@v1


This usage is perfectly fine and quite normal.

-- 
H.J.
Fangrui Song Aug. 12, 2021, 1:11 a.m. | #3
On 2021-08-11, H.J. Lu wrote:
>On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:

>>

>> On 2021-08-01, H.J. Lu via Binutils wrote:

>> >When a symbol removed by .symver is used in relocation and there is one

>> >and only one versioned symbol, don't remove the symbol.  Instead, mark

>> >it to be removed and replace the removed symbol used in relocation with

>> >the versioned symbol before generating relocation.

>>

>> Thanks for the patch.

>> The behavior looks good.

>>

>> .symver foo, foo@v1, remove

>> .globl foo

>> foo:

>>    call foo   # R_X86_64_PLT32 foo@v1

>>

>>

>>

>> I think this can be extended to non-remove one-@ as well.

>> The unadorned symbol and relocations referencing it just add complexity

>> to linker internals.

>>

>> .symver foo, foo@v1

>

>This usage is perfectly fine and quite normal.


It isn't.

cat > a.s <<eof
.symver foo, foo@v1
.globl foo
foo:
eof
cat > a.ver <<eof
v1 {};
v2 { foo*; };
eof
cc -c a.s

% ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so
0000000000001000 R foo@v1
0000000000000000 A v1
0000000000000000 A v2


If
.symver foo_v1, foo@v1

% ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so
0000000000001000 R foo@v1
0000000000001000 R foo_v1@@v2
0000000000000000 A v1
0000000000000000 A v2


Merging (unadorned) foo and (non-default versioned) foo@v1 is a hack, in all of GNU ld, gold, and ld.lld.
Alan Modra via Binutils Aug. 12, 2021, 12:32 p.m. | #4
On Wed, Aug 11, 2021 at 6:11 PM Fangrui Song <i@maskray.me> wrote:
>

>

> On 2021-08-11, H.J. Lu wrote:

> >On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:

> >>

> >> On 2021-08-01, H.J. Lu via Binutils wrote:

> >> >When a symbol removed by .symver is used in relocation and there is one

> >> >and only one versioned symbol, don't remove the symbol.  Instead, mark

> >> >it to be removed and replace the removed symbol used in relocation with

> >> >the versioned symbol before generating relocation.

> >>

> >> Thanks for the patch.

> >> The behavior looks good.

> >>

> >> .symver foo, foo@v1, remove

> >> .globl foo

> >> foo:

> >>    call foo   # R_X86_64_PLT32 foo@v1

> >>

> >>

> >>

> >> I think this can be extended to non-remove one-@ as well.

> >> The unadorned symbol and relocations referencing it just add complexity

> >> to linker internals.

> >>

> >> .symver foo, foo@v1

> >

> >This usage is perfectly fine and quite normal.

>

> It isn't.

>

> cat > a.s <<eof

> .symver foo, foo@v1

> .globl foo

> foo:

> eof


We must keep foo since there may be relocations
against foo in this file as well as in other files.

> cat > a.ver <<eof

> v1 {};

> v2 { foo*; };

> eof

> cc -c a.s

>

> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

> 0000000000001000 R foo@v1

> 0000000000000000 A v1

> 0000000000000000 A v2


This looks normal.

>

> If

> .symver foo_v1, foo@v1

>

> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

> 0000000000001000 R foo@v1

> 0000000000001000 R foo_v1@@v2

> 0000000000000000 A v1

> 0000000000000000 A v2


This looks normal.

>

> Merging (unadorned) foo and (non-default versioned) foo@v1 is a hack, in all of GNU ld, gold, and ld.lld.




-- 
H.J.
Fangrui Song Aug. 12, 2021, 10:52 p.m. | #5
On 2021-08-12, H.J. Lu wrote:
>On Wed, Aug 11, 2021 at 6:11 PM Fangrui Song <i@maskray.me> wrote:

>>

>>

>> On 2021-08-11, H.J. Lu wrote:

>> >On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:

>> >>

>> >> On 2021-08-01, H.J. Lu via Binutils wrote:

>> >> >When a symbol removed by .symver is used in relocation and there is one

>> >> >and only one versioned symbol, don't remove the symbol.  Instead, mark

>> >> >it to be removed and replace the removed symbol used in relocation with

>> >> >the versioned symbol before generating relocation.

>> >>

>> >> Thanks for the patch.

>> >> The behavior looks good.

>> >>

>> >> .symver foo, foo@v1, remove

>> >> .globl foo

>> >> foo:

>> >>    call foo   # R_X86_64_PLT32 foo@v1

>> >>

>> >>

>> >>

>> >> I think this can be extended to non-remove one-@ as well.

>> >> The unadorned symbol and relocations referencing it just add complexity

>> >> to linker internals.

>> >>

>> >> .symver foo, foo@v1

>> >

>> >This usage is perfectly fine and quite normal.

>>

>> It isn't.

>>

>> cat > a.s <<eof

>> .symver foo, foo@v1

>> .globl foo

>> foo:

>> eof

>

>We must keep foo since there may be relocations

>against foo in this file as well as in other files.


In practice, no project uses this.
(OK, I am less confident on glibc but I am hoping it mostly does the right thing.)

Unversioned 'foo' references from other files should bind the default versioned definition,
not foo@v1.

If 'foo' does not have a default version definition, the references
should be versioned (`.symver foo, foo@@@v1`).

>> cat > a.ver <<eof

>> v1 {};

>> v2 { foo*; };

>> eof

>> cc -c a.s

>>

>> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

>> 0000000000001000 R foo@v1

>> 0000000000000000 A v1

>> 0000000000000000 A v2

>

>This looks normal.


I'd argue that the good design should leave a dynamic `foo@@v2`,
matching the case below.
Linkers would have been doing this if gas had done the right thing in
the first place.

>>

>> If

>> .symver foo_v1, foo@v1

>>

>> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

>> 0000000000001000 R foo@v1

>> 0000000000001000 R foo_v1@@v2

>> 0000000000000000 A v1

>> 0000000000000000 A v2

>

>This looks normal.


This is indeed normal.

>>

>> Merging (unadorned) foo and (non-default versioned) foo@v1 is a hack, in all of GNU ld, gold, and ld.lld.

>

>

>

>-- 

>H.J.
Alan Modra via Binutils Aug. 13, 2021, 9:01 p.m. | #6
On Thu, Aug 12, 2021 at 3:52 PM Fangrui Song <i@maskray.me> wrote:
>

>

> On 2021-08-12, H.J. Lu wrote:

> >On Wed, Aug 11, 2021 at 6:11 PM Fangrui Song <i@maskray.me> wrote:

> >>

> >>

> >> On 2021-08-11, H.J. Lu wrote:

> >> >On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:

> >> >>

> >> >> On 2021-08-01, H.J. Lu via Binutils wrote:

> >> >> >When a symbol removed by .symver is used in relocation and there is one

> >> >> >and only one versioned symbol, don't remove the symbol.  Instead, mark

> >> >> >it to be removed and replace the removed symbol used in relocation with

> >> >> >the versioned symbol before generating relocation.

> >> >>

> >> >> Thanks for the patch.

> >> >> The behavior looks good.

> >> >>

> >> >> .symver foo, foo@v1, remove

> >> >> .globl foo

> >> >> foo:

> >> >>    call foo   # R_X86_64_PLT32 foo@v1

> >> >>

> >> >>

> >> >>

> >> >> I think this can be extended to non-remove one-@ as well.

> >> >> The unadorned symbol and relocations referencing it just add complexity

> >> >> to linker internals.

> >> >>

> >> >> .symver foo, foo@v1

> >> >

> >> >This usage is perfectly fine and quite normal.

> >>

> >> It isn't.

> >>

> >> cat > a.s <<eof

> >> .symver foo, foo@v1

> >> .globl foo

> >> foo:

> >> eof

> >

> >We must keep foo since there may be relocations

> >against foo in this file as well as in other files.

>

> In practice, no project uses this.

> (OK, I am less confident on glibc but I am hoping it mostly does the right thing.)


This is the standard usage in glibc.

> Unversioned 'foo' references from other files should bind the default versioned definition,

> not foo@v1.


Unversioned 'foo' references from other files will be resolved to the
unversioned
foo definition which will be turned into a local symbol.

>

> If 'foo' does not have a default version definition, the references

> should be versioned (`.symver foo, foo@@@v1`).

>

> >> cat > a.ver <<eof

> >> v1 {};

> >> v2 { foo*; };

> >> eof

> >> cc -c a.s

> >>

> >> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

> >> 0000000000001000 R foo@v1

> >> 0000000000000000 A v1

> >> 0000000000000000 A v2

> >

> >This looks normal.

>

> I'd argue that the good design should leave a dynamic `foo@@v2`,

> matching the case below.

> Linkers would have been doing this if gas had done the right thing in

> the first place.

>

> >>

> >> If

> >> .symver foo_v1, foo@v1

> >>

> >> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

> >> 0000000000001000 R foo@v1

> >> 0000000000001000 R foo_v1@@v2

> >> 0000000000000000 A v1

> >> 0000000000000000 A v2

> >

> >This looks normal.

>

> This is indeed normal.

>

> >>

> >> Merging (unadorned) foo and (non-default versioned) foo@v1 is a hack, in all of GNU ld, gold, and ld.lld.

> >

> >

> >

> >--

> >H.J.




-- 
H.J.
Fangrui Song Aug. 13, 2021, 10:29 p.m. | #7
On 2021-08-13, H.J. Lu wrote:
>On Thu, Aug 12, 2021 at 3:52 PM Fangrui Song <i@maskray.me> wrote:

>>

>>

>> On 2021-08-12, H.J. Lu wrote:

>> >On Wed, Aug 11, 2021 at 6:11 PM Fangrui Song <i@maskray.me> wrote:

>> >>

>> >>

>> >> On 2021-08-11, H.J. Lu wrote:

>> >> >On Tue, Aug 10, 2021 at 2:56 PM Fangrui Song <i@maskray.me> wrote:

>> >> >>

>> >> >> On 2021-08-01, H.J. Lu via Binutils wrote:

>> >> >> >When a symbol removed by .symver is used in relocation and there is one

>> >> >> >and only one versioned symbol, don't remove the symbol.  Instead, mark

>> >> >> >it to be removed and replace the removed symbol used in relocation with

>> >> >> >the versioned symbol before generating relocation.

>> >> >>

>> >> >> Thanks for the patch.

>> >> >> The behavior looks good.

>> >> >>

>> >> >> .symver foo, foo@v1, remove

>> >> >> .globl foo

>> >> >> foo:

>> >> >>    call foo   # R_X86_64_PLT32 foo@v1

>> >> >>

>> >> >>

>> >> >>

>> >> >> I think this can be extended to non-remove one-@ as well.

>> >> >> The unadorned symbol and relocations referencing it just add complexity

>> >> >> to linker internals.

>> >> >>

>> >> >> .symver foo, foo@v1

>> >> >

>> >> >This usage is perfectly fine and quite normal.

>> >>

>> >> It isn't.

>> >>

>> >> cat > a.s <<eof

>> >> .symver foo, foo@v1

>> >> .globl foo

>> >> foo:

>> >> eof

>> >

>> >We must keep foo since there may be relocations

>> >against foo in this file as well as in other files.

>>

>> In practice, no project uses this.

>> (OK, I am less confident on glibc but I am hoping it mostly does the right thing.)

>

>This is the standard usage in glibc.

>

>> Unversioned 'foo' references from other files should bind the default versioned definition,

>> not foo@v1.

>

>Unversioned 'foo' references from other files will be resolved to the

>unversioned

>foo definition which will be turned into a local symbol.


If you think about it, keeping the 'foo' definition is highly
error-prone.

First, it is unusual for a non-default version definition
to be referenced from outside the TU.

Second, if glibc defines foo@GLIBC_2.3 in one TU and has a version node
GLIBC_2.4 { foo; };, there will be a foo@@GLIBC_2.4 beside
foo@GLIBC_2.3, and the unadorned reference will be silently bound to
foo@@GLIBC_2.4. There is no error reporting.

The sensible behavior is

.symver foo, foo@@@v2 only defines foo@@v2
.symver foo, foo@v1, remove only defines foo_v1.

An unadorned reference cannot be bound to a default-version definition.
Non-default version references must use .symver .

Linkers have well established rules that a foo@@v2 definition can
satisfy foo.


I want to check what on earth could break in glibc
if I add `, remove` but I cannot test currently because
relocations don't work.
With this patch we will be able to test, and hopefully the result isn't
that bad.

>>

>> If 'foo' does not have a default version definition, the references

>> should be versioned (`.symver foo, foo@@@v1`).

>>

>> >> cat > a.ver <<eof

>> >> v1 {};

>> >> v2 { foo*; };

>> >> eof

>> >> cc -c a.s

>> >>

>> >> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

>> >> 0000000000001000 R foo@v1

>> >> 0000000000000000 A v1

>> >> 0000000000000000 A v2

>> >

>> >This looks normal.

>>

>> I'd argue that the good design should leave a dynamic `foo@@v2`,

>> matching the case below.

>> Linkers would have been doing this if gas had done the right thing in

>> the first place.

>>

>> >>

>> >> If

>> >> .symver foo_v1, foo@v1

>> >>

>> >> % ld.bfd --version-script=a.ver -shared a.o -o a.so && nm -D a.so

>> >> 0000000000001000 R foo@v1

>> >> 0000000000001000 R foo_v1@@v2

>> >> 0000000000000000 A v1

>> >> 0000000000000000 A v2

>> >

>> >This looks normal.

>>

>> This is indeed normal.

>>

>> >>

>> >> Merging (unadorned) foo and (non-default versioned) foo@v1 is a hack, in all of GNU ld, gold, and ld.lld.

>> >

>> >

>> >

>> >--

>> >H.J.

>

>

>

>-- 

>H.J.
Alan Modra via Binutils Aug. 13, 2021, 10:44 p.m. | #8
On Sun, Aug 1, 2021 at 8:16 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> When a symbol removed by .symver is used in relocation and there is one

> and only one versioned symbol, don't remove the symbol.  Instead, mark

> it to be removed and replace the removed symbol used in relocation with

> the versioned symbol before generating relocation.


Hi Nick, Alan,

Can you review this patch?

Thanks.

>         PR gas/28157

>         * symbols.c (symbol_flags): Add removed.

>         (symbol_entry_find): Updated.

>         (symbol_mark_removed): New function.

>         (symbol_removed_p): Likewise.

>         * symbols.h (symbol_mark_removed): New prototype.

>         (symbol_removed_p): Likewise.

>         * write.c (write_relocs): Call obj_fixup_removed_symbol on

>         removed fixp->fx_addsy and fixp->fx_subsy if defined.

>         (set_symtab): Don't add a symbol if symbol_removed_p returns true.

>         * config/obj-elf.c (elf_frob_symbol):  Don't remove the symbol

>         if it is used on relocation.  Instead, mark it as to be removed

>         and issue an error if the symbol has more than one versioned name.

>         (elf_fixup_removed_symbol): New function.

>         * config/obj-elf.h (elf_fixup_removed_symbol): New prototype.

>         (obj_fixup_removed_symbol): New.

>         * testsuite/gas/symver/symver11.d: Updated expected error

>         message.

>         * testsuite/gas/symver/symver16.d: New file.

>         * testsuite/gas/symver/symver16.s: Likewise.

> ---

>  gas/config/obj-elf.c                | 28 ++++++++++++++++++++++++++--

>  gas/config/obj-elf.h                |  5 +++++

>  gas/symbols.c                       | 26 +++++++++++++++++++++++++-

>  gas/symbols.h                       |  2 ++

>  gas/testsuite/gas/symver/symver11.d |  2 +-

>  gas/testsuite/gas/symver/symver16.d | 13 +++++++++++++

>  gas/testsuite/gas/symver/symver16.s | 16 ++++++++++++++++

>  gas/write.c                         | 21 +++++++++++++++------

>  8 files changed, 103 insertions(+), 10 deletions(-)

>  create mode 100644 gas/testsuite/gas/symver/symver16.d

>  create mode 100644 gas/testsuite/gas/symver/symver16.s

>

> diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c

> index 42a3851e772..a6087a21eb3 100644

> --- a/gas/config/obj-elf.c

> +++ b/gas/config/obj-elf.c

> @@ -2705,7 +2705,7 @@ elf_frob_symbol (symbolS *symp, int *puntp)

>                 S_SET_EXTERNAL (symp2);

>             }

>

> -         switch (symbol_get_obj (symp)->visibility)

> +         switch (sy_obj->visibility)

>             {

>             case visibility_unchanged:

>               break;

> @@ -2716,7 +2716,18 @@ elf_frob_symbol (symbolS *symp, int *puntp)

>               elfsym->internal_elf_sym.st_other |= STV_HIDDEN;

>               break;

>             case visibility_remove:

> -             symbol_remove (symp, &symbol_rootP, &symbol_lastP);

> +             /* Don't remove the symbol if it is used in relocation.

> +                Instead, mark it as to be removed and issue an error

> +                if the symbol has more than one versioned name.  */

> +             if (symbol_used_in_reloc_p (symp))

> +               {

> +                 if (sy_obj->versioned_name->next != NULL)

> +                   as_bad (_("symbol '%s' with multiple versions cannot be used in relocation"),

> +                           S_GET_NAME (symp));

> +                 symbol_mark_removed (symp);

> +               }

> +             else

> +               symbol_remove (symp, &symbol_rootP, &symbol_lastP);

>               break;

>             case visibility_local:

>               S_CLEAR_EXTERNAL (symp);

> @@ -2734,6 +2745,19 @@ elf_frob_symbol (symbolS *symp, int *puntp)

>      }

>  }

>

> +/* Fix up SYMPP which has been marked to be removed by .symver.  */

> +

> +void

> +elf_fixup_removed_symbol (symbolS **sympp)

> +{

> +  symbolS *symp = *sympp;

> +  struct elf_obj_sy *sy_obj = symbol_get_obj (symp);

> +

> +  /* Replace the removed symbol with the versioned symbol.  */

> +  symp = symbol_find (sy_obj->versioned_name->name);

> +  *sympp = symp;

> +}

> +

>  struct group_list

>  {

>    asection **head;             /* Section lists.  */

> diff --git a/gas/config/obj-elf.h b/gas/config/obj-elf.h

> index d1fd3152998..763c58dfcad 100644

> --- a/gas/config/obj-elf.h

> +++ b/gas/config/obj-elf.h

> @@ -273,6 +273,11 @@ extern void elf_frob_symbol (symbolS *, int *);

>  #define obj_frob_symbol(symp, punt) elf_frob_symbol (symp, &punt)

>  #endif

>

> +extern void elf_fixup_removed_symbol (symbolS **);

> +#ifndef obj_fixup_removed_symbol

> +#define obj_fixup_removed_symbol(sympp) elf_fixup_removed_symbol (sympp)

> +#endif

> +

>  extern void elf_pop_insert (void);

>  #ifndef obj_pop_insert

>  #define obj_pop_insert()       elf_pop_insert()

> diff --git a/gas/symbols.c b/gas/symbols.c

> index 302eb4bd6f7..3cb9425c4ce 100644

> --- a/gas/symbols.c

> +++ b/gas/symbols.c

> @@ -78,6 +78,10 @@ struct symbol_flags

>       before.  It is cleared as soon as any direct reference to the

>       symbol is present.  */

>    unsigned int weakrefd : 1;

> +

> +  /* Whether the symbol has been marked to be removed by a .symver

> +     directive.  */

> +  unsigned int removed : 1;

>  };

>

>  /* A pointer in the symbol may point to either a complete symbol

> @@ -194,7 +198,7 @@ static void *

>  symbol_entry_find (htab_t table, const char *name)

>  {

>    hashval_t hash = htab_hash_string (name);

> -  symbol_entry_t needle = { { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },

> +  symbol_entry_t needle = { { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },

>                               hash, name, 0, 0, 0 } };

>    return htab_find_with_hash (table, &needle, hash);

>  }

> @@ -2807,6 +2811,26 @@ symbol_written_p (symbolS *s)

>    return s->flags.written;

>  }

>

> +/* Mark a symbol as to be removed.  */

> +

> +void

> +symbol_mark_removed (symbolS *s)

> +{

> +  if (s->flags.local_symbol)

> +    return;

> +  s->flags.removed = 1;

> +}

> +

> +/* Return whether a symbol has been marked to be removed.  */

> +

> +int

> +symbol_removed_p (symbolS *s)

> +{

> +  if (s->flags.local_symbol)

> +    return 0;

> +  return s->flags.removed;

> +}

> +

>  /* Mark a symbol has having been resolved.  */

>

>  void

> diff --git a/gas/symbols.h b/gas/symbols.h

> index 91f69882a7e..317252c7fa1 100644

> --- a/gas/symbols.h

> +++ b/gas/symbols.h

> @@ -193,6 +193,8 @@ extern int symbol_mri_common_p (symbolS *);

>  extern void symbol_mark_written (symbolS *);

>  extern void symbol_clear_written (symbolS *);

>  extern int symbol_written_p (symbolS *);

> +extern void symbol_mark_removed (symbolS *);

> +extern int symbol_removed_p (symbolS *);

>  extern void symbol_mark_resolved (symbolS *);

>  extern int symbol_resolved_p (symbolS *);

>  extern int symbol_section_p (symbolS *);

> diff --git a/gas/testsuite/gas/symver/symver11.d b/gas/testsuite/gas/symver/symver11.d

> index caa76e167da..10f8ef810b2 100644

> --- a/gas/testsuite/gas/symver/symver11.d

> +++ b/gas/testsuite/gas/symver/symver11.d

> @@ -1,2 +1,2 @@

>  #name: symver symver11

> -#error: .*symbol cannot be used on reloc

> +#error: .*: symbol 'foo' with multiple versions cannot be used in relocation

> diff --git a/gas/testsuite/gas/symver/symver16.d b/gas/testsuite/gas/symver/symver16.d

> new file mode 100644

> index 00000000000..cdf0ddde57d

> --- /dev/null

> +++ b/gas/testsuite/gas/symver/symver16.d

> @@ -0,0 +1,13 @@

> +#name: symver symver16

> +#readelf: -srW

> +

> +#...

> +Relocation section .*

> +#...

> +[0-9a-f]+[ \t]+[0-9a-f]+[ \t]+R_.*[ \t]+[0-9a-f]+[ \t]+foo@@VERS_1.*

> +#...

> +[0-9a-f]+[ \t]+[0-9a-f]+[ \t]+R_.*[ \t]+[0-9a-f]+[ \t]+bar@VERS_1.*

> +#...

> + +[0-9]+: 0+ +1 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +foo@@VERS_1

> + +[0-9]+: 0+1 +1 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +bar@VERS_1

> +#pass

> diff --git a/gas/testsuite/gas/symver/symver16.s b/gas/testsuite/gas/symver/symver16.s

> new file mode 100644

> index 00000000000..df330fd4f25

> --- /dev/null

> +++ b/gas/testsuite/gas/symver/symver16.s

> @@ -0,0 +1,16 @@

> +       .data

> +       .type foo,%object

> +foo:

> +       .byte 0

> +       .size foo,.-foo

> +       .globl foo

> +       .symver foo,foo@@VERS_1,remove

> +       .globl bar

> +       .symver bar,bar@VERS_1,remove

> +       .type bar,%object

> +bar:

> +       .byte 0

> +       .size bar,.-bar

> +       .balign 8

> +       .dc.a foo

> +       .dc.a bar

> diff --git a/gas/write.c b/gas/write.c

> index 253dfc476f8..e2c7bf29249 100644

> --- a/gas/write.c

> +++ b/gas/write.c

> @@ -1289,6 +1289,13 @@ write_relocs (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,

>         as_bad_where (fixp->fx_file, fixp->fx_line,

>                       _("internal error: fixup not contained within frag"));

>

> +#ifdef obj_fixup_removed_symbol

> +      if (fixp->fx_addsy && symbol_removed_p (fixp->fx_addsy))

> +       obj_fixup_removed_symbol (&fixp->fx_addsy);

> +      if (fixp->fx_subsy && symbol_removed_p (fixp->fx_subsy))

> +       obj_fixup_removed_symbol (&fixp->fx_subsy);

> +#endif

> +

>  #ifndef RELOC_EXPANSION_POSSIBLE

>        *reloc = tc_gen_reloc (sec, fixp);

>  #else

> @@ -1755,9 +1762,10 @@ set_symtab (void)

>       two.  Generate unused section symbols only if needed.  */

>    nsyms = 0;

>    for (symp = symbol_rootP; symp; symp = symbol_next (symp))

> -    if (bfd_keep_unused_section_symbols (stdoutput)

> -       || !symbol_section_p (symp)

> -       || symbol_used_in_reloc_p (symp))

> +    if (!symbol_removed_p (symp)

> +       && (bfd_keep_unused_section_symbols (stdoutput)

> +           || !symbol_section_p (symp)

> +           || symbol_used_in_reloc_p (symp)))

>        nsyms++;

>

>    if (nsyms)

> @@ -1768,9 +1776,10 @@ set_symtab (void)

>        asympp = (asymbol **) bfd_alloc (stdoutput, amt);

>        symp = symbol_rootP;

>        for (i = 0; i < nsyms; symp = symbol_next (symp))

> -       if (bfd_keep_unused_section_symbols (stdoutput)

> -           || !symbol_section_p (symp)

> -           || symbol_used_in_reloc_p (symp))

> +       if (!symbol_removed_p (symp)

> +           && (bfd_keep_unused_section_symbols (stdoutput)

> +               || !symbol_section_p (symp)

> +               || symbol_used_in_reloc_p (symp)))

>           {

>             asympp[i] = symbol_get_bfdsym (symp);

>             if (asympp[i]->flags != BSF_SECTION_SYM

> --

> 2.31.1

>



-- 
H.J.
Alan Modra via Binutils Aug. 16, 2021, 9:01 a.m. | #9
On Fri, Aug 13, 2021 at 03:44:12PM -0700, H.J. Lu wrote:
> On Sun, Aug 1, 2021 at 8:16 AM H.J. Lu <hjl.tools@gmail.com> wrote:

> >         PR gas/28157

> >         * symbols.c (symbol_flags): Add removed.

> >         (symbol_entry_find): Updated.

> >         (symbol_mark_removed): New function.

> >         (symbol_removed_p): Likewise.

> >         * symbols.h (symbol_mark_removed): New prototype.

> >         (symbol_removed_p): Likewise.

> >         * write.c (write_relocs): Call obj_fixup_removed_symbol on

> >         removed fixp->fx_addsy and fixp->fx_subsy if defined.

> >         (set_symtab): Don't add a symbol if symbol_removed_p returns true.

> >         * config/obj-elf.c (elf_frob_symbol):  Don't remove the symbol

> >         if it is used on relocation.  Instead, mark it as to be removed

> >         and issue an error if the symbol has more than one versioned name.

> >         (elf_fixup_removed_symbol): New function.

> >         * config/obj-elf.h (elf_fixup_removed_symbol): New prototype.

> >         (obj_fixup_removed_symbol): New.

> >         * testsuite/gas/symver/symver11.d: Updated expected error

> >         message.

> >         * testsuite/gas/symver/symver16.d: New file.

> >         * testsuite/gas/symver/symver16.s: Likewise.


OK.

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/gas/config/obj-elf.c b/gas/config/obj-elf.c
index 42a3851e772..a6087a21eb3 100644
--- a/gas/config/obj-elf.c
+++ b/gas/config/obj-elf.c
@@ -2705,7 +2705,7 @@  elf_frob_symbol (symbolS *symp, int *puntp)
 		S_SET_EXTERNAL (symp2);
 	    }
 
-	  switch (symbol_get_obj (symp)->visibility)
+	  switch (sy_obj->visibility)
 	    {
 	    case visibility_unchanged:
 	      break;
@@ -2716,7 +2716,18 @@  elf_frob_symbol (symbolS *symp, int *puntp)
 	      elfsym->internal_elf_sym.st_other |= STV_HIDDEN;
 	      break;
 	    case visibility_remove:
-	      symbol_remove (symp, &symbol_rootP, &symbol_lastP);
+	      /* Don't remove the symbol if it is used in relocation.
+		 Instead, mark it as to be removed and issue an error
+		 if the symbol has more than one versioned name.  */
+	      if (symbol_used_in_reloc_p (symp))
+		{
+		  if (sy_obj->versioned_name->next != NULL)
+		    as_bad (_("symbol '%s' with multiple versions cannot be used in relocation"),
+			    S_GET_NAME (symp));
+		  symbol_mark_removed (symp);
+		}
+	      else
+		symbol_remove (symp, &symbol_rootP, &symbol_lastP);
 	      break;
 	    case visibility_local:
 	      S_CLEAR_EXTERNAL (symp);
@@ -2734,6 +2745,19 @@  elf_frob_symbol (symbolS *symp, int *puntp)
     }
 }
 
+/* Fix up SYMPP which has been marked to be removed by .symver.  */
+
+void
+elf_fixup_removed_symbol (symbolS **sympp)
+{
+  symbolS *symp = *sympp;
+  struct elf_obj_sy *sy_obj = symbol_get_obj (symp);
+
+  /* Replace the removed symbol with the versioned symbol.  */
+  symp = symbol_find (sy_obj->versioned_name->name);
+  *sympp = symp;
+}
+
 struct group_list
 {
   asection **head;		/* Section lists.  */
diff --git a/gas/config/obj-elf.h b/gas/config/obj-elf.h
index d1fd3152998..763c58dfcad 100644
--- a/gas/config/obj-elf.h
+++ b/gas/config/obj-elf.h
@@ -273,6 +273,11 @@  extern void elf_frob_symbol (symbolS *, int *);
 #define obj_frob_symbol(symp, punt) elf_frob_symbol (symp, &punt)
 #endif
 
+extern void elf_fixup_removed_symbol (symbolS **);
+#ifndef obj_fixup_removed_symbol
+#define obj_fixup_removed_symbol(sympp) elf_fixup_removed_symbol (sympp)
+#endif
+
 extern void elf_pop_insert (void);
 #ifndef obj_pop_insert
 #define obj_pop_insert()	elf_pop_insert()
diff --git a/gas/symbols.c b/gas/symbols.c
index 302eb4bd6f7..3cb9425c4ce 100644
--- a/gas/symbols.c
+++ b/gas/symbols.c
@@ -78,6 +78,10 @@  struct symbol_flags
      before.  It is cleared as soon as any direct reference to the
      symbol is present.  */
   unsigned int weakrefd : 1;
+
+  /* Whether the symbol has been marked to be removed by a .symver
+     directive.  */
+  unsigned int removed : 1;
 };
 
 /* A pointer in the symbol may point to either a complete symbol
@@ -194,7 +198,7 @@  static void *
 symbol_entry_find (htab_t table, const char *name)
 {
   hashval_t hash = htab_hash_string (name);
-  symbol_entry_t needle = { { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+  symbol_entry_t needle = { { { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
 			      hash, name, 0, 0, 0 } };
   return htab_find_with_hash (table, &needle, hash);
 }
@@ -2807,6 +2811,26 @@  symbol_written_p (symbolS *s)
   return s->flags.written;
 }
 
+/* Mark a symbol as to be removed.  */
+
+void
+symbol_mark_removed (symbolS *s)
+{
+  if (s->flags.local_symbol)
+    return;
+  s->flags.removed = 1;
+}
+
+/* Return whether a symbol has been marked to be removed.  */
+
+int
+symbol_removed_p (symbolS *s)
+{
+  if (s->flags.local_symbol)
+    return 0;
+  return s->flags.removed;
+}
+
 /* Mark a symbol has having been resolved.  */
 
 void
diff --git a/gas/symbols.h b/gas/symbols.h
index 91f69882a7e..317252c7fa1 100644
--- a/gas/symbols.h
+++ b/gas/symbols.h
@@ -193,6 +193,8 @@  extern int symbol_mri_common_p (symbolS *);
 extern void symbol_mark_written (symbolS *);
 extern void symbol_clear_written (symbolS *);
 extern int symbol_written_p (symbolS *);
+extern void symbol_mark_removed (symbolS *);
+extern int symbol_removed_p (symbolS *);
 extern void symbol_mark_resolved (symbolS *);
 extern int symbol_resolved_p (symbolS *);
 extern int symbol_section_p (symbolS *);
diff --git a/gas/testsuite/gas/symver/symver11.d b/gas/testsuite/gas/symver/symver11.d
index caa76e167da..10f8ef810b2 100644
--- a/gas/testsuite/gas/symver/symver11.d
+++ b/gas/testsuite/gas/symver/symver11.d
@@ -1,2 +1,2 @@ 
 #name: symver symver11
-#error: .*symbol cannot be used on reloc
+#error: .*: symbol 'foo' with multiple versions cannot be used in relocation
diff --git a/gas/testsuite/gas/symver/symver16.d b/gas/testsuite/gas/symver/symver16.d
new file mode 100644
index 00000000000..cdf0ddde57d
--- /dev/null
+++ b/gas/testsuite/gas/symver/symver16.d
@@ -0,0 +1,13 @@ 
+#name: symver symver16
+#readelf: -srW
+
+#...
+Relocation section .*
+#...
+[0-9a-f]+[ \t]+[0-9a-f]+[ \t]+R_.*[ \t]+[0-9a-f]+[ \t]+foo@@VERS_1.*
+#...
+[0-9a-f]+[ \t]+[0-9a-f]+[ \t]+R_.*[ \t]+[0-9a-f]+[ \t]+bar@VERS_1.*
+#...
+ +[0-9]+: 0+ +1 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +foo@@VERS_1
+ +[0-9]+: 0+1 +1 +OBJECT +GLOBAL +DEFAULT +[0-9]+ +bar@VERS_1
+#pass
diff --git a/gas/testsuite/gas/symver/symver16.s b/gas/testsuite/gas/symver/symver16.s
new file mode 100644
index 00000000000..df330fd4f25
--- /dev/null
+++ b/gas/testsuite/gas/symver/symver16.s
@@ -0,0 +1,16 @@ 
+	.data
+	.type foo,%object
+foo:
+	.byte 0
+	.size foo,.-foo
+	.globl foo
+	.symver foo,foo@@VERS_1,remove
+	.globl bar
+	.symver bar,bar@VERS_1,remove
+	.type bar,%object
+bar:
+	.byte 0
+	.size bar,.-bar
+	.balign 8
+	.dc.a foo
+	.dc.a bar
diff --git a/gas/write.c b/gas/write.c
index 253dfc476f8..e2c7bf29249 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1289,6 +1289,13 @@  write_relocs (bfd *abfd ATTRIBUTE_UNUSED, asection *sec,
 	as_bad_where (fixp->fx_file, fixp->fx_line,
 		      _("internal error: fixup not contained within frag"));
 
+#ifdef obj_fixup_removed_symbol
+      if (fixp->fx_addsy && symbol_removed_p (fixp->fx_addsy))
+	obj_fixup_removed_symbol (&fixp->fx_addsy);
+      if (fixp->fx_subsy && symbol_removed_p (fixp->fx_subsy))
+	obj_fixup_removed_symbol (&fixp->fx_subsy);
+#endif
+
 #ifndef RELOC_EXPANSION_POSSIBLE
       *reloc = tc_gen_reloc (sec, fixp);
 #else
@@ -1755,9 +1762,10 @@  set_symtab (void)
      two.  Generate unused section symbols only if needed.  */
   nsyms = 0;
   for (symp = symbol_rootP; symp; symp = symbol_next (symp))
-    if (bfd_keep_unused_section_symbols (stdoutput)
-	|| !symbol_section_p (symp)
-	|| symbol_used_in_reloc_p (symp))
+    if (!symbol_removed_p (symp)
+	&& (bfd_keep_unused_section_symbols (stdoutput)
+	    || !symbol_section_p (symp)
+	    || symbol_used_in_reloc_p (symp)))
       nsyms++;
 
   if (nsyms)
@@ -1768,9 +1776,10 @@  set_symtab (void)
       asympp = (asymbol **) bfd_alloc (stdoutput, amt);
       symp = symbol_rootP;
       for (i = 0; i < nsyms; symp = symbol_next (symp))
-	if (bfd_keep_unused_section_symbols (stdoutput)
-	    || !symbol_section_p (symp)
-	    || symbol_used_in_reloc_p (symp))
+	if (!symbol_removed_p (symp)
+	    && (bfd_keep_unused_section_symbols (stdoutput)
+		|| !symbol_section_p (symp)
+		|| symbol_used_in_reloc_p (symp)))
 	  {
 	    asympp[i] = symbol_get_bfdsym (symp);
 	    if (asympp[i]->flags != BSF_SECTION_SYM