[ELF] - Do not allow mix global symbols version.

[grimar]

Summary

For next version script:

VER1{
  global:
  a;
};

VER2{
  global:
  a;
};

gold would produce warning like:
"warning: using 'VER1' as version for 'a' which is also named in version 'VER1' in script."

Documentation also says we do not want this duplications (https://people.freebsd.org/~deischen/symver/library_versioning.txt):
"Note that you do not want to duplicate symbols in the map file. The .symver directives are all that is required to add compatibility
symbols into old versions."

So I suggest to error out such cases.

[davide]

I was a little bit confused when I originally tried versioning and noticed you can (both in gold and bfd) duplicate symbols in the map. The fact they just emit a warning (and not an error) makes me think there's some software in the wild which abuses of this feature.

That said, I entirely advocate being a little bit more strict, but, I'd rather finish the version script support (or at least make it good enough that it can understand FreeBSD base system, large C++ applications etc..) before introducing these restrictions.
If you want a large test case, you can try linking gentoo packages/FreeBSD packages. I tried the latter and realized many failures happened in version script support (e.g. comments not parsed correctly).

tl;dr: I'd rather put this on hold until we got the basics right, or at least until we realized being more strict doesn't cause additional headache down the road. YMMV.

[• rafael]

I disagree with Davide on this one.

The code is very simple and can be easily reverted if we find scripts depending on it. While that, having this restriction makes it easier to reason about the rest of the implementation without having to think what the correct behaviour is when a symbol is in two versions.

ELF/SymbolTable.cpp

535 ↗

(On Diff #61362)

Using > with an opaque constant is a bit odd. Please use !=.

[emaste]

I agree with @rafael. It's easy to revert later if necessary and I'd rather we default to erroring out on bogus input until/unless we identify some 3rd party software that depends on it for a non-accidental reason and it cannot be fixed.

[ruiu]

LGTM

[davide]

Fair enough, let's see how this works and revert if needed.

[davide]

Hmm, I think this breaks libreoffice, sorry.

duplicate symbol _ZThn32_N4cppu16OComponentHelper14queryInterfaceERKN3com3sun4star3uno4TypeE in version script
duplicate symbol _ZThn48_N4cppu16OComponentHelper14queryInterfaceERKN3com3sun4star3uno4TypeE in version script
duplicate symbol _ZThn56_N4cppu16OComponentHelper14queryInterfaceERKN3com3sun4star3uno4TypeE in version script

cppuhelper/source/gcc3.map lists multiple times the same symbol :(
https://gerrit.libreoffice.org/gitweb?p=core.git;a=blob;f=cppuhelper/source/gcc3.map;h=12c29834ab1687d5747808b8a308e550962e6721;hb=refs/heads/master

_ZThn*_N4cppu16OComponentHelper7acquireEv;

It's actually even worse than that, the symbol is *defined* multiple times under the same version. I think they should fix their version script (and in fact I plan to propose a change upstream), but in the time being, I'd rather switch this back to a warning. I'm sending a patch soon.

lld/trunk/ELF/SymbolTable.cpp

if (!Config->VersionScriptGlobals.empty()) {
     return;
   }
 
   // If we have symbols version declarations, we should
   // assign version references for each symbol.
   size_t I = 2;
   for (Version &V : Config->SymbolVersions) {
     for (StringRef Name : V.Globals)
-      if (SymbolBody *B = find(Name))
+      if (SymbolBody *B = find(Name)) {
+        if (B->symbol()->VersionId != VER_NDX_GLOBAL &&
+            B->symbol()->VersionId != VER_NDX_LOCAL)
+          error("duplicate symbol " + Name + " in version script");
         B->symbol()->VersionId = I;
+      }
     ++I;
   }
 }
 
 template class elf::SymbolTable<ELF32LE>;
 template class elf::SymbolTable<ELF32BE>;
 template class elf::SymbolTable<ELF64LE>;
 template class elf::SymbolTable<ELF64BE>;

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

 # RUN:          global: foo1; \
 # RUN:          local: *; };  \
 # RUN:       VERSION_2.0 {    \
 # RUN:          global: foo3; \
 # RUN:          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=ERR %s
 
+# RUN: echo "VERSION_1.0{     \
+# RUN:          global: foo1; \
+# RUN:          local: *; };  \
+# RUN:       VERSION_2.0 {    \
+# RUN:          global: foo1; \
+# RUN:          local: *; }; " > %t6.script
+# RUN: not ld.lld --version-script %t6.script -shared %t.o %t2.so -o %t6.so 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR2 %s
+# ERR2: duplicate symbol foo1 in version script
+
 # RUN: ld.lld --version-script %t.script --dynamic-list %t.list %t.o %t2.so -o %t2
 # RUN: llvm-readobj %t2 > /dev/null
 
 # DSO:      DynamicSymbols [
 # DSO-NEXT:   Symbol {
 # DSO-NEXT:     Name: @ (0)
 # DSO-NEXT:     Value: 0x0
 # DSO-NEXT:     Size: 0