[ELF] Combine foo@v1 and foo with the same versionId if both are defined

[MaskRay]

Summary

Due to an assembler design flaw (IMO), .symver foo,foo@v1 produces two symbols`foo` and foo@v1 if foo is defined.

• v1 {}; produces both foo and foo@v1 but GNU ld only produces foo@v1
• v1 { foo; }; produces both foo@@v1 and foo@v1 but GNU ld only produces foo@v1
• v2 { foo; }; produces both foo@@v2 and foo@v1, matching GNU ld. (Tested by symver.s)

This patch implements the GNU ld behavior by reusing the symbol redirection mechanism
in D92259. The new test symver-non-default.s checks the first two cases.

Without the patch, the second case will produce foo@v1 and foo@@v1 which
looks weird and makes foo unnecessarily default versioned.

Note: .symver foo,foo@v1,remove exists but the unfortunate foo will not go
away anytime soon.

[peter.smith]

LGTM. One quick suggestion to improve the comment and one typo in the comment.

lld/ELF/Driver.cpp

2073–2074

I think this comment is now incomplete. Perhaps worth changing to something like:

For version symbol foo@v1 check the existing symbol foo.
We have two special cases to handle:

• There is a definition of foo@v1 and foo@@v1
• There is a definition of foo@v1 and foo

2097

typo elimiates -> eliminates

[MaskRay]

Thanks for the review!

Address comments...

[peter.smith]

Thanks for the update. The elimiates typo on line 2097 looks like it is still there, but that is easily fixed up.

lld/ELF/Driver.cpp

for (const WrappedSymbol &w : wrapped) {
     map[w.real] = w.sym;
   }
   for (Symbol *sym : symtab->symbols()) {
     // Enumerate symbols with a non-default version (foo@v1).
     StringRef name = sym->getName();
     const char *suffix1 = sym->getVersionSuffix();
     if (suffix1[0] != '@' || suffix1[1] == '@')
       continue;
 
-    // Check whether the default version foo@@v1 exists. If it exists, the
-    // symbol can be found by the name "foo" in the symbol table.
-    Symbol *maybeDefault = symtab->find(name);
-    if (!maybeDefault)
-      continue;
-    const char *suffix2 = maybeDefault->getVersionSuffix();
-    if (suffix2[0] != '@' || suffix2[1] != '@' ||
-        strcmp(suffix1 + 1, suffix2 + 2) != 0)
+    // Check the existing symbol foo. We have two special cases to handle:
+    //
+    // * There is a definition of foo@v1 and foo@@v1.
+    // * There is a definition of foo@v1 and foo.
+    Defined *sym2 = dyn_cast_or_null<Defined>(symtab->find(name));
+    if (!sym2)
       continue;
-
+    const char *suffix2 = sym2->getVersionSuffix();
+    if (suffix2[0] == '@' && suffix2[1] == '@' &&
+        strcmp(suffix1 + 1, suffix2 + 2) == 0) {
       // foo@v1 and foo@@v1 should be merged, so redirect foo@v1 to foo@@v1.
-    map.try_emplace(sym, maybeDefault);
+      map.try_emplace(sym, sym2);
       // If both foo@v1 and foo@@v1 are defined and non-weak, report a duplicate
       // definition error.
-    maybeDefault->resolve(*sym);
+      sym2->resolve(*sym);
       // Eliminate foo@v1 from the symbol table.
       sym->symbolKind = Symbol::PlaceholderKind;
+    } else if (auto *sym1 = dyn_cast<Defined>(sym)) {
+      if (sym2->versionId > VER_NDX_GLOBAL
+              ? config->versionDefinitions[sym2->versionId].name == suffix1 + 1
+              : sym1->section == sym2->section && sym1->value == sym2->value) {
+        // Due to an assembler design flaw, if foo is defined, .symver foo,
+        // foo@v1 defines both foo and foo@v1. Unless foo is bound to a
+        // different version, GNU ld makes foo@v1 canonical and elimiates foo.
+        // Emulate its behavior, otherwise we would have foo or foo@@v1 beside
+        // foo@v1. foo@v1 and foo combining does not apply if they are not
+        // defined in the same place.
+        map.try_emplace(sym2, sym);
+        sym2->symbolKind = Symbol::PlaceholderKind;
+      }
+    }
   }
 
   if (map.empty())
     return;
 
   // Update pointers in input files.
   parallelForEach(objectFiles, [&](InputFile *file) {
     MutableArrayRef<Symbol *> syms = file->getMutableSymbols();

lld/test/ELF/symver-non-default.s

+# REQUIRES: x86
+## Test symbol resolution related to .symver produced non-default version symbols.
+
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/ref.s -o %t/ref.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/def1.s -o %t/def1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/def2.s -o %t/def2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %t/def3.s -o %t/def3.o
+
+## foo@v1 & foo defined at the same location are combined.
+# RUN: ld.lld -shared --version-script=%t/ver1 %t/def1.o %t/ref.o -o %t1
+# RUN: llvm-readelf --dyn-syms %t1 | FileCheck %s --check-prefix=CHECK1
+# RUN: ld.lld -shared --version-script=%t/ver2 %t/def1.o %t/ref.o -o %t1
+# RUN: llvm-readelf --dyn-syms %t1 | FileCheck %s --check-prefix=CHECK1
+
+# CHECK1:       1: {{.*}} NOTYPE GLOBAL DEFAULT [[#]] foo@v1
+# CHECK1-EMPTY:
+
+## def2.o doesn't define foo. foo@v1 & undefined foo are unrelated.
+# RUN: ld.lld -shared --version-script=%t/ver1 %t/def2.o %t/ref.o -o %t2
+# RUN: llvm-readelf -r --dyn-syms %t2 | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2:       R_X86_64_JUMP_SLOT {{.*}} foo + 0
+# CHECK2:       1: {{.*}} NOTYPE GLOBAL DEFAULT UND   foo{{$}}
+# CHECK2-NEXT:  2: {{.*}} NOTYPE GLOBAL DEFAULT [[#]] foo@v1
+# CHECK2-EMPTY:
+
+## def2.o doesn't define foo. foo@v1 & defined foo are unrelated.
+# RUN: ld.lld -shared --version-script=%t/ver1 %t/def2.o %t/def3.o %t/ref.o -o %t3
+# RUN: llvm-readelf -r --dyn-syms %t3 | FileCheck %s --check-prefix=CHECK3
+
+# CHECK3:       R_X86_64_JUMP_SLOT {{.*}} foo + 0
+# CHECK3:       1: {{.*}} NOTYPE GLOBAL DEFAULT [[#]] foo@v1
+# CHECK3-NEXT:  2: {{.*}} NOTYPE GLOBAL DEFAULT [[#]] foo{{$}}
+# CHECK3-EMPTY:
+
+## foo@v1 overrides the defined foo which is affected by a version script.
+# RUN: ld.lld -shared --version-script=%t/ver2 %t/def2.o %t/def3.o %t/ref.o -o %t4
+# RUN: llvm-readelf -r --dyn-syms %t4 | FileCheck %s --check-prefix=CHECK4
+
+# CHECK4:       R_X86_64_JUMP_SLOT {{.*}} foo@v1 + 0
+# CHECK4:       1: {{.*}} NOTYPE GLOBAL DEFAULT [[#]] foo@v1
+# CHECK4-EMPTY:
+
+#--- ver1
+v1 {};
+
+#--- ver2
+v1 { foo; };
+
+#--- ref.s
+call foo
+
+#--- def1.s
+.globl foo
+.symver foo, foo@v1
+foo:
+  ret
+
+#--- def2.s
+.globl foo_v1
+.symver foo_v1, foo@v1, remove
+foo_v1:
+  ret
+
+#--- def3.s
+.globl foo
+foo:
+  ret

lld/test/ELF/version-script-symver.s

 # REQUIRES: x86
 ## Test how .symver interacts with --version-script.
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo 'call foo3; call foo4' > %tref.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %tref.s -o %tref.o
 
 # RUN: echo 'v1 { local: foo1; }; v2 { local: foo2; };' > %t1.script
 # RUN: ld.lld --version-script %t1.script -shared %t.o -o %t1.so
 # RUN: llvm-readelf --dyn-syms %t1.so | FileCheck --check-prefix=EXACT %s
 # EXACT:      UND
-# EXACT-NEXT: [[#]] foo3{{$}}
 # EXACT-NEXT: [[#]] foo4@@v2
 # EXACT-NEXT: [[#]] _start{{$}}
 # EXACT-NEXT: [[#]] foo3@v1
 # EXACT-NOT:  {{.}}
 
 # RUN: echo 'v1 { local: foo*; }; v2 {};' > %t2.script
 # RUN: ld.lld --version-script %t2.script -shared %t.o -o %t2.so
 # RUN: llvm-readelf --dyn-syms %t2.so | FileCheck --check-prefix=WC %s

 # MIX1-NOT:  {{.}}
 
 # RUN: echo 'v1 { global: foo*; local: *; }; v2 { global: foo4; local: *; };' > %t4.script
 # RUN: ld.lld --version-script %t4.script -shared %t.o -o %t4.so
 # RUN: llvm-readelf --dyn-syms %t4.so | FileCheck --check-prefix=MIX2 %s
 # MIX2:      UND
 # MIX2-NEXT: [[#]] foo1@@v1
 # MIX2-NEXT: [[#]] foo2@@v1
-# MIX2-NEXT: [[#]] foo3@@v1
 # MIX2-NEXT: [[#]] foo4@@v2
 # MIX2-NEXT: [[#]] foo3@v1
 # MIX2-NOT:  {{.}}
 
+# RUN: ld.lld --version-script %t4.script -shared %t.o %tref.o -o %t5.so
+# RUN: llvm-readelf -r %t5.so | FileCheck --check-prefix=RELOC %s
+
+# RELOC: R_X86_64_JUMP_SLOT {{.*}} foo3@v1 + 0
+# RELOC: R_X86_64_JUMP_SLOT {{.*}} foo4@@v2 + 0
+
 .globl foo1; foo1: ret
 .globl foo2; foo2: ret
 .globl foo3; .symver foo3,foo3@v1; foo3: ret
 .globl foo4; .symver foo4,foo4@@v2; foo4: ret
 
 .globl _start; _start: ret

lld/test/ELF/version-symbol-undef.s

 // REQUIRES: x86
 // RUN: echo ".data; \
 // RUN:       .quad \"basename\"; \
 // RUN:       .quad \"basename@FBSD_1.0\"; \
 // RUN:       .quad \"basename@FBSD_1.1\" " > %t.s
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t.s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t2.o
-// RUN: echo "FBSD_1.0 { global: basename; local: *; }; FBSD_1.1 { };" > %t2.ver
+// RUN: echo "FBSD_1.0 { global: basename; local: *; }; FBSD_1.1 { basename; };" > %t2.ver
 // RUN: ld.lld --shared --version-script %t2.ver %t2.o -o %t2.so
 // RUN: echo "FBSD_1.0 { }; FBSD_1.1 { }; LIBPKG_1.3 { };" > %t.ver
 // RUN: ld.lld --shared %t.o --version-script %t.ver %t2.so -o %t.so
 // RUN: llvm-readobj --dyn-symbols -r %t.so | FileCheck %s
 
 // Test that each relocation points to the correct version.
 
 // CHECK:      Section ({{.*}}) .rela.dyn {