[ELF] Warn for duplicate symbols in version scripts instead of erroring out

[davide]

See my comment on http://reviews.llvm.org/D21555 for the rationale behind this change.

[ruiu]

LGTM. If it's still too verbose, we should just remove this code.

[grimar]

In D21781#468584, @ruiu wrote:

LGTM. If it's still too verbose, we should just remove this code.

I am not sure we should. That is definitely a error in script to have that. Users should fix their scripts instead.

[ruiu]

It may be a misuse of a feature, but is it really an error?

[grimar]

In D21781#468631, @ruiu wrote:

It may be a misuse of a feature, but is it really an error?

Well, listing the same symbol under one version does not seem to be a error for me,
but listing the same symbol under different versions at least worth a warning as
it is possible that linker override one and that is confusing.

[• rafael]

I really think this should be an error. If we need a hack for now it
should be warning, but we should have an open bug on making this an
error again.

Both gold and bfd put the symbol just in the first version. We put it
just in the last version. Both are really confusing.

I will open a bug on making this a error again.

Cheers,
Rafael

[emaste]

In D21781#468584, @ruiu wrote:

LGTM. If it's still too verbose, we should just remove this code.

I definitely don't want to see the warning removed altogether (and have a slight preference for seeing this become an error again eventually).

FreeBSD's libcxxrt version map has had a copy-and-paste error for 2.5 years (since it was introduced) resulting in missing exported symbols: it had duplicated _ZTIPDn entries, instead of _ZTIPDn and _ZTSPDn. The mistake went unnoticed until I tried building with lld and encountered the duplicate symbol error from lld. So I think this warning / error is not spurious and will catch real mistakes that need to be fixed in version scripts.

https://reviews.freebsd.org/D7011

[joerg]

I agree with Rafael that it should be an error. There is one situation that we want to handle smarter, but that's it. If the version is the result from an explicit match and a wildcard, the explicit match should win and the wildcard be ignored. But that's the only exception I can sanely see.

[grimar]

In D21781#470015, @joerg wrote:

I agree with Rafael that it should be an error. There is one situation that we want to handle smarter, but that's it. If the version is the result from an explicit match and a wildcard, the explicit match should win and the wildcard be ignored. But that's the only exception I can sanely see.

And if there is more than 1 match using wildcard, the last matching version tag in the file should be used.
(http://www.airs.com/blog/archives/300).

lld/trunk/ELF/SymbolTable.cpp

template <class ELFT> void SymbolTable<ELFT>::scanVersionScript() {
   // 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 (B->symbol()->VersionId != VER_NDX_GLOBAL &&
             B->symbol()->VersionId != VER_NDX_LOCAL)
-          error("duplicate symbol " + Name + " in version script");
+          warning("duplicate symbol " + Name + " in version script");
         B->symbol()->VersionId = I;
       }
     ++I;
   }
 }
 
 // Print the module names which define the notified
 // symbols provided through -y or --trace-symbol option.

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

 # 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 %t6.script -shared %t.o %t2.so -o %t6.so 2>&1 | \
+# RUN:   FileCheck -check-prefix=WARN2 %s
+# WARN2: 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