[LLD][ELF] Do not error for missing version when symbol has local version.

[peter.smith]

Summary

If a symbol with an undefined version in a DSO is not going to be exported into the dynamic symbol table then do not give an error message for the missing version. This can happen with the --exclude-libs option which implicitly gives all symbols in a static library the local version. This matches the behavior of ld.gold and is exploited by the Bionic dynamic linker on Arm.

This was found when trying to link the bionic dynamic linker on Arm and got link errors with LLD. I've reported to Bionic under https://issuetracker.google.com/issues/73020933
as ld.bfd was also giving the same error message, but ld.gold was succeeding. I think on balance ld.gold is more sensible and we should match it.

Fixes PR36295

[peter.smith]

Peter Smith via Phabricator <[e-mail]> writes:
Index: ELF/Symbols.cpp
===================================================================

• • ELF/Symbols.cpp +++ ELF/Symbols.cpp @@ -206,8 +206,10 @@ It is an error if the specified version is not defined. Usually version script is not provided when linking executable, // but we may still want to override a versioned symbol from DSO,
• // so we do not report error in this case.
• if (Config->Shared) + so we do not report error in this case. We also do not error + if the symbol has a local version as it won't be in the dynamic + // symbol table. + if (Config->Shared && VersionId != VER_NDX_LOCAL)

Is the Config->Shared part still required?

At the moment it is; I get the error messages I remove it. If we could somehow arrange the default VersionId for symbols in executables to have VersionId == VER_NDX_LOCAL then the Config->Shared could be dropped. I can investigate that if you'd prefer it?

Should an error be reported when linking an executable with
--export-dynamic?

A good question. In theory it should as the symbol is exported, in practice I fear that the likelihood of anyone using export-dynamic with a @version definition is lower than the likelihood of using export-dynamic in a large project with a lurking @version definition that they did not know about (like the Bionic case). It seems like there isn't much consistency with how this is handled, and gold doesn't seem to handle the case sensibly. Table renders best in phab:

Linker	With version script	Without version script
bfd	symbol given version from script	symbol not given version
gold	symbol given global version	version defined, but symbol not given version
lld	symbol given version from script	symbol not given version

In summary, I don't have a strong opinion either way but I'm leaning towards not giving an error due to the risk of breaking existing programs.

[peter.smith]

Rebased against master. I'd like to see if I can progress this a bit further as there is now interest in Android to switch to LLD and this issue is preventing some of Bionic from being linked with LLD.

As it stands this change replicates the behaviour of gold. It must be said that the specification symbol versioning information is not clear on what the behaviour should be in this case. The symbol version is indeed missing from the executable, but exclude-libs has removed all symbols that use the version from the dynamic symbol table so the symbol version doesn't need to exist.

Rafael had a comment:

Should an error be reported when linking an executable with --export-dynamic?

My response was that I thought the risk of breaking existing programs was too high as symbol version scripts were not intended to be used with executables.

Does anyone have any input on how to move this forward?

[grimar]

LGTM. Let's try this at least.

[peter.smith]

Thanks, I'll wait till next Monday before committing.

ELF/Symbols.cpp

for (VersionDefinition &Ver : Config->VersionDefinitions) {
     else
       VersionId = Ver.Id | VERSYM_HIDDEN;
     return;
   }
 
   // It is an error if the specified version is not defined.
   // Usually version script is not provided when linking executable,
   // but we may still want to override a versioned symbol from DSO,
-  // so we do not report error in this case.
-  if (Config->Shared)
+  // so we do not report error in this case. We also do not error
+  // if the symbol has a local version as it won't be in the dynamic
+  // symbol table.
+  if (Config->Shared && VersionId != VER_NDX_LOCAL)
     error(toString(File) + ": symbol " + S + " has undefined version " +
           Verstr);
 }
 
 InputFile *Lazy::fetch() {
   if (auto *S = dyn_cast<LazyArchive>(this))
     return S->fetch();
   return cast<LazyObject>(this)->fetch();

test/ELF/Inputs/versiondef.s

+.text
+.globl func_impl
+func_impl:
+  ret
+.globl func_impl2
+func_impl2:
+  ret
+.symver func_impl, func@@VER2
+.symver func_impl2, func@VER

test/ELF/version-exclude-libs.s

+// REQUIRES: x86
+// RUN: llvm-mc %p/Inputs/versiondef.s -o %t.o -filetype=obj -triple=x86_64-pc-linux
+// RUN: llvm-ar -r %t.a %t.o
+// RUN: llvm-mc %s -o %t2.o -filetype=obj -triple=x86_64-pc-linux
+// RUN: ld.lld %t2.o %t.a --shared --exclude-libs ALL -o %t.so
+// RUN: llvm-readobj -symbols %t.so | FileCheck %s
+// RUN: llvm-readobj -dyn-symbols %t.so | FileCheck -check-prefix CHECK-DYN %s
+// RUN: not ld.lld %t2.o %t.a --shared -o %t.so 2>&1 | FileCheck -check-prefix=CHECK-ERR %s
+
+// Test that we do not give an error message for undefined versions when the
+// symbol is not exported to the dynamic symbol table.
+
+// CHECK:          Name: func
+// CHECK-NEXT:     Value:
+// CHECK-NEXT:     Size:
+// CHECK-NEXT:     Binding: Local (0x0)
+
+// CHECK-DYN-NOT: func
+
+// CHECK-ERR: symbol func@@VER2 has undefined version VER2
+// CHECK-ERR-NEXT: symbol func@VER has undefined version VER
+
+ .text
+ .globl _start
+ .globl func
+_start:
+ ret
+
+ .data
+ .quad func