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

[Peter Smith via Phabricator via llvm-commits]

peter.smith updated this revision to Diff 146299.
peter.smith added a comment.
Herald added a reviewer: javed.absar.

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?


https://reviews.llvm.org/D43126

Files:
  ELF/Symbols.cpp
  test/ELF/Inputs/versiondef.s
  test/ELF/version-exclude-libs.s


Index: test/ELF/version-exclude-libs.s
===================================================================
--- /dev/null
+++ test/ELF/version-exclude-libs.s
@@ -0,0 +1,30 @@
+// 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 at VER has undefined version VER
+
+ .text
+ .globl _start
+ .globl func
+_start:
+ ret
+
+ .data
+ .quad func
Index: test/ELF/Inputs/versiondef.s
===================================================================
--- /dev/null
+++ test/ELF/Inputs/versiondef.s
@@ -0,0 +1,9 @@
+.text
+.globl func_impl
+func_impl:
+  ret
+.globl func_impl2
+func_impl2:
+  ret
+.symver func_impl, func@@VER2
+.symver func_impl2, func at VER
Index: ELF/Symbols.cpp
===================================================================
--- ELF/Symbols.cpp
+++ ELF/Symbols.cpp
@@ -196,8 +196,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)
     error(toString(File) + ": symbol " + S + " has undefined version " +
           Verstr);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43126.146299.patch
Type: text/x-patch
Size: 2195 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180511/8f07d803/attachment.bin>