Bug 26978 - Inconsistency for defined foo@v1 and foo@@v1
Summary: Inconsistency for defined foo@v1 and foo@@v1
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: 2.36
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-30 00:15 UTC by Fangrui Song
Modified: 2021-08-28 00:50 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-12-02 00:00:00


Attachments
patch under test (2.16 KB, patch)
2020-12-02 02:56 UTC, Alan Modra
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2020-11-30 00:15:11 UTC
cat > ./def1.s <<eof
.globl foo
.symver foo, foo@@@v1
foo:
eof
cat > ./def1w.s <<eof
.weak foo
.symver foo, foo@@@v1
foo:
eof
cat > ./hid1.s <<eof
.globl foo_v1
.symver foo_v1, foo@v1
foo_v1:
  ret
eof
cat > ./hid1w.s <<eof
.weak foo_v1
.symver foo_v1, foo@v1
foo_v1:
  ret
eof
cat > ./ver <<eof
v1 {};
eof
gcc -c *.s


% ld.bfd -shared --version-script=ver def1.o hid1.o
ld.bfd: hid1.o: in function `foo_v1':
(.text+0x0): multiple definition of `foo@@v1'; def1.o:(.text+0x0): first defined here
% ld.bfd -shared --version-script=ver def1w.o hid1.o
ld.bfd: hid1.o: in function `foo_v1':
(.text+0x0): multiple definition of `foo@v1'
ld.bfd: hid1.o:(*IND*+0x0): multiple definition of `foo'
% ld.bfd -shared --version-script=ver def1.o hid1w.o
% ld.bfd -shared --version-script=ver def1w.o hid1w.o


If weak foo@@v1 + global foo@v1 errors, global foo@@v1 + weak foo@v1 should error as well.

An alternative design is that weak foo@@v1 + global foo@v1 should not error. Of the 4 combinations, only global foo@@v1 + global foo@v1 errors.
Comment 1 H.J. Lu 2020-12-01 14:45:08 UTC
(In reply to Fangrui Song from comment #0)

> 
> % ld.bfd -shared --version-script=ver def1.o hid1.o
> ld.bfd: hid1.o: in function `foo_v1':
> (.text+0x0): multiple definition of `foo@@v1'; def1.o:(.text+0x0): first
> defined here
> % ld.bfd -shared --version-script=ver def1w.o hid1.o
> ld.bfd: hid1.o: in function `foo_v1':
> (.text+0x0): multiple definition of `foo@v1'
> ld.bfd: hid1.o:(*IND*+0x0): multiple definition of `foo'

Correct behavior.  Otherwise, we will get

Symbol table '.dynsym' contains 4 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000001000     0 NOTYPE  WEAK   DEFAULT    7 foo@@v1
     2: 0000000000001002     0 NOTYPE  GLOBAL DEFAULT    7 foo_v1
     3: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS v1

and the weak foo@@v1 overrides the non-weak foo@v1.
Comment 2 Fangrui Song 2020-12-01 17:18:34 UTC
> Correct behavior.  Otherwise, we will get

This is a surprising behavior. Normally, STB_GLOBAL wins over STB_WEAK. Did you argue that in the case of @ and @@ definitions, the rule "@@ wins over @" taking precedence?

I would think the two bits are independent. global foo@v1 + weak foo@@v1 => global foo@@v1.
Comment 3 H.J. Lu 2020-12-01 17:27:56 UTC
(In reply to Fangrui Song from comment #2)
> > Correct behavior.  Otherwise, we will get
> 
> This is a surprising behavior. Normally, STB_GLOBAL wins over STB_WEAK. Did
> you argue that in the case of @ and @@ definitions, the rule "@@ wins over
> @" taking precedence?
> 
> I would think the two bits are independent. global foo@v1 + weak foo@@v1 =>
> global foo@@v1.

If we allow this to link, weak foo@@v1 will be in .dynsym which overrides
the non-weak foo@v1.
Comment 4 Alan Modra 2020-12-02 00:18:27 UTC
I do think we have a problem here.

ld.bfd -shared --version-script=ver hid1.o def1w.o
and
ld.bfd -shared --version-script=ver def1w.o hid1.o

ought to give the same results.
Comment 5 Alan Modra 2020-12-02 02:56:57 UTC
Created attachment 13019 [details]
patch under test
Comment 6 H.J. Lu 2020-12-02 14:00:56 UTC
(In reply to Alan Modra from comment #4)
> I do think we have a problem here.
> 
> ld.bfd -shared --version-script=ver hid1.o def1w.o
> and
> ld.bfd -shared --version-script=ver def1w.o hid1.o
> 
> ought to give the same results.

Both should fail to link.
Comment 7 H.J. Lu 2020-12-02 14:52:22 UTC
A patch is posted at

https://sourceware.org/pipermail/binutils/2020-December/114357.html
Comment 8 cvs-commit@gcc.gnu.org 2020-12-04 00:49:44 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=726d7d1ecfd1fc0966983e1d5e59b527b90cf7c5

commit 726d7d1ecfd1fc0966983e1d5e59b527b90cf7c5
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Dec 2 13:03:23 2020 +1030

    PR26978, Inconsistency for strong foo@v1 and weak foo@@v1
    
    Prior to this patch
      ld -shared --version-script=pr26979.ver pr26978a.o pr26978b.o
    results in
      ld: pr26978b.o: in function `foo_v1':
      (.text+0x0): multiple definition of `foo@v1'
      ld: pr26978b.o:(*IND*+0x0): multiple definition of `foo'
    while
      ld -shared --version-script=pr26979.ver pr26978b.o pr26978a.o
    results in no error, but some odd dynamic symbols.
      ... 0 NOTYPE  GLOBAL DEFAULT    7 foo@v1
      ... 0 NOTYPE  WEAK   DEFAULT    7 foo@@v1
    
    When linking an undecorated reference to foo against such a shared
    library, ld complains about multiple definitions of foo@v1 while gold
    creates a dynamic reference to foo@v1.  That results in foo@v1 being
    used at runtime.
    
    While we could error in both cases, it is reasonable to say foo@v1 and
    foo@@v1 are in fact the same symbol.  (Same name, same version.  The
    only real difference is that foo@@v1 satisfies a reference to plain
    foo, while foo@v1 does not.)  Just as merging a weak undecorated sym
    with a strong sym results in the strong sym prevailing, so should the
    strong foo@v1 prevail.  And since there is a definition that satisfies
    plain foo, the foo@@v1 variety of dynamic symbol should be emitted at
    the foo@v1 value.  That makes the testcase that currently links
    continue to produce a shared library, and that shared library can now
    be used by both ld and gold with the same runtime behaviour as when
    using gold with the odd dynamic symbol library.
    
    bfd/
            PR 26978
            * elflink.c (_bfd_elf_add_default_symbol): Handle the case where
            a new weak sym@@ver should be overridden by an existing sym@ver.
            (elf_link_add_object_symbols): Don't _bfd_elf_add_default_symbol
            for a new weak sym@ver when sym@@ver already exists.
            * linker.c (link_action): Choose MIND for previous indirect,
            current def, rather than MDEF.
            (_bfd_generic_link_add_one_symbol <MIND>): Handle redefinition of
            weak indirect symbol.
    ld/
            * testsuite/ld-elf/pr26978a.d,
            * testsuite/ld-elf/pr26978a.s,
            * testsuite/ld-elf/pr26978b.d,
            * testsuite/ld-elf/pr26978b.s: New tests.
Comment 9 Alan Modra 2020-12-14 03:00:07 UTC
Fixed
Comment 10 cvs-commit@gcc.gnu.org 2021-08-28 00:50:44 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b05929a20efa7caa40415d2f069e44f7e76615e4

commit b05929a20efa7caa40415d2f069e44f7e76615e4
Author: Alan Modra <amodra@gmail.com>
Date:   Thu Aug 26 12:19:35 2021 +0930

    PR28264, ld.bfd crash on linking efivar with LTO
    
            PR 28264
            PR 26978
            * linker.c (_bfd_generic_link_add_one_symbol <MIND>): Check
            that string is non-NULL.