[ELF] Warn rather than error when duplicate version assignments occur

[MaskRay]

Summary

In lvm2, libdevmapper.so is linked with a version script with duplicate
version assignments:

DM_1_02_138 { global: ... dm_bitset_parse_list; ... };
DM_1_02_129 { global: ... dm_bitset_parse_list; ... };

ld.bfd silently accepts this while gold issues a warning. We currently
error, thus inhibit producing the executable. Change the error to
warning to allow this case, and improve the message.

There are some cases where ld.bfd error
anonymous version tag cannot be combined with other version tags
but we just warn. It is probably OK for now.

[ruiu]

ELF/SymbolTable.cpp

212–214 ↗

(On Diff #209140)

Is there any way to do early-continue to reduce indentation?

215 ↗

(On Diff #209140)

I would inline this and remove the variable.

[grimar]

test/ELF/version-script-reassign.s

23 ↗

(On Diff #209140)

Does it make sence to print the old version of foo in this message too?
like

symbol 'foo' of version X was reassigned to version 'Y'

[MaskRay]

Improve warning message.
Update tests.

Note this doesn't fix PR38549, but it is related. And I can fix it in another change

[MaskRay]

Delete %t2w.s which will be included in my next change

[grimar]

LGTM, but please wait until someone else take another look too.

ELF/SymbolTable.cpp

227 ↗

(On Diff #209148)

Seems you can omit the conversion to .str(), since warn takes const Twine&:

warn("attempt to reassign symbol '" + ver.name + "' of " +
      getName(sym->versionId) + " to " + getName(versionId));

[MaskRay]

Delete redundant .str()

[ruiu]

LGTM

ELF/SymbolTable.cpp

204 ↗

(On Diff #209151)

It looks like this function doesn't capture anything, so I believe you can remove &.

[MaskRay]

Remove & from the capture

lld/trunk/ELF/SymbolTable.cpp

void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
   std::vector<Symbol *> syms = findByVersion(ver);
   if (syms.empty()) {
     if (!config->undefinedVersion)
       error("version script assignment of '" + versionName + "' to symbol '" +
             ver.name + "' failed: symbol not defined");
     return;
   }
 
+  auto getName = [](uint16_t ver) -> std::string {
+    if (ver == VER_NDX_LOCAL)
+      return "VER_NDX_LOCAL";
+    if (ver == VER_NDX_GLOBAL)
+      return "VER_NDX_GLOBAL";
+    return ("version '" + config->versionDefinitions[ver - 2].name + "'").str();
+  };
+
   // Assign the version.
   for (Symbol *sym : syms) {
     // Skip symbols containing version info because symbol versions
     // specified by symbol names take precedence over version scripts.
     // See parseSymbolVersion().
     if (sym->getName().contains('@'))
       continue;
 
-    if (sym->versionId != config->defaultSymbolVersion &&
-        sym->versionId != versionId)
-      error("duplicate symbol '" + ver.name + "' in version script");
+    if (sym->versionId == config->defaultSymbolVersion)
       sym->versionId = versionId;
+    if (sym->versionId == versionId)
+      continue;
+
+    warn("attempt to reassign symbol '" + ver.name + "' of " +
+         getName(sym->versionId) + " to " + getName(versionId));
   }
 }
 
 void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId) {
   if (!ver.hasWildcard)
     return;
 
   // Exact matching takes precendence over fuzzy matching,

lld/trunk/test/ELF/version-script-reassign.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '{ local: foo; };' > %tl.ver
+# RUN: echo '{ global: foo; local: *; };' > %tg.ver
+# RUN: echo 'V1 { global: foo; };' > %t1.ver
+# RUN: echo 'V2 { global: foo; };' > %t2.ver
+
+## Note, ld.bfd errors on the two cases.
+# RUN: ld.lld -shared %t.o --version-script %tl.ver --version-script %t1.ver \
+# RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=LOCAL %s
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=LOCAL-SYM %s
+# RUN: ld.lld -shared %t.o --version-script %tg.ver --version-script %t1.ver \
+# RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=GLOBAL %s
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=GLOBAL-SYM %s
+
+## Note, ld.bfd silently accepts this case.
+# RUN: ld.lld -shared %t.o --version-script %t1.ver --version-script %t2.ver \
+# RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=V1-WARN %s
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=V1-SYM %s
+
+# LOCAL: warning: attempt to reassign symbol 'foo' of VER_NDX_LOCAL to version 'V1'
+# LOCAL-SYM-NOT: foo
+
+# GLOBAL: warning: attempt to reassign symbol 'foo' of VER_NDX_GLOBAL to version 'V1'
+# GLOBAL-SYM: foo{{$}}
+
+# V1-WARN: warning: attempt to reassign symbol 'foo' of version 'V1' to version 'V2'
+# V1-SYM: foo@@V1
+
+.globl foo
+foo:

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

 # ERR1: anonymous version definition is used in combination with other version definitions
 
 # RUN: echo "{ global: foo1; local: *; };" > %t5.script
 # RUN: echo "VERSION_2.0 { global: foo3; local: *; };" >> %t5.script
 # RUN: not ld.lld --version-script %t5.script -shared %t.o %t2.so -o %t5.so 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR2 %s
 # ERR2: EOF expected, but got VERSION_2.0
 
-# RUN: echo "VERSION_1.0 { global: foo1; local: *; };" > %t6.script
-# RUN: echo "VERSION_2.0 { global: foo1; local: *; };" >> %t6.script
-# RUN: not ld.lld --version-script %t6.script -shared %t.o %t2.so -o /dev/null 2>&1 | \
-# RUN:   FileCheck -check-prefix=ERR3 %s
-# ERR3: duplicate symbol 'foo1' in version script
-
 # RUN: echo "{ foo1; foo2; };" > %t.list
 # RUN: ld.lld --version-script %t.script --dynamic-list %t.list %t.o %t2.so -o %t2
 # RUN: llvm-readobj %t2 > /dev/null
 
 ## Check that we can handle multiple "--version-script" options.
 # RUN: echo "VERSION_1.0 { global : foo1; local : *; };" > %t7a.script
 # RUN: echo "VERSION_2.0 { global: foo3; local: *; };" > %t7b.script
 # RUN: ld.lld --version-script %t7a.script --version-script %t7b.script -shared %t.o %t2.so -o %t7.so