[ELF] Fix wrapping symbols produced during LTO codegen

[smeenai]

Summary

We were previously not correctly wrapping symbols that were only
produced during LTO codegen and unreferenced before then, or symbols
only referenced from such symbols. The root cause was that we weren't
marking the wrapped symbol as used if we only saw the use after LTO
codegen, leading to the failed wrapping.

Fix this by explicitly tracking whether a symbol will become referenced
after wrapping is done. We can use this property to tell LTO to preserve
such symbols, instead of overload isUsedInRegularObj for this purpose.
Since we're no longer setting isUsedInRegularObj for all symbols which
will be wrapped, its value at the time of performing the wrapping in the
symbol table will accurately reflect whether the symbol was actually
used in an object (including in an LTO-generated object), and we can
propagate that value to the wrapped symbol and thereby ensure we wrap
correctly.

This incorrect wrapping was the only scenario I was aware of where we
produced an invalid PLT relocation, which D123985 started diagnosing,
and with it fixed, we lose the test for that diagnosis. I think it's
worth keeping the diagnosis though, in case we run into other issues in
the future which would be caught by it.

Fixes PR50675.

[smeenai]

Add comment in SymbolTable::wrap

[smeenai]

Make git-clang-format happy

[smeenai]

Rebase on top of test removal from previous diff

[MaskRay]

Thanks for the patch. I think it is an important improvement.

Since we're no longer setting isUsedInRegularObj for all symbols which will be wrapped, its value at the time of performing the wrapping in the symbol table will accurately reflect whether the symbol was actually ...

I always need to wrap my head around when thinking about --wrap :-) And I did not find a good way to fix the inelegant condition

if (!real->isUsedInRegularObj && sym->isUndefined())
    sym->isUsedInRegularObj = false;

The new one is better! Let me play with this a bit...

I am also curious whether you want to do anything with D33621: the chunk of code is a bit of a hack

if (Res.LinkerRedefined)
  GV->setLinkage(GlobalValue::WeakAnyLinkage);

[MaskRay]

lld/ELF/Symbols.h

139

The name may be too long. We don't need to pursue perfect grammar, we can just call it referencedAfterWrap

[MaskRay]

lld/ELF/Symbols.h

133–134

Is this sentence stale now?

[MaskRay]

lld/ELF/SymbolTable.cpp

47

It will be good to have a comment for the if (sym->isUndefined()) condition.

[smeenai]

In D124056#3468783, @MaskRay wrote:

Thanks for the patch. I think it is an important improvement.

Thanks for the review!

I am also curious whether you want to do anything with D33621: the chunk of code is a bit of a hack

if (Res.LinkerRedefined)
  GV->setLinkage(GlobalValue::WeakAnyLinkage);

Yeah, I've been meaning to look into that as well ... there's at least one issue where the logic to restore the original binding of the symbol in the linker got dropped (https://github.com/llvm/llvm-project/issues/51346). I need to understand the LTO logic better first though.

lld/ELF/SymbolTable.cpp

47

Note that I'm tightening the condition further in D124065, which is a follow-up to this diff. Would you prefer adding a comment here and then changing it in D124065, or just adding the comment in D124065? (I think it'll be easier to explain the logic of the conditional with the change in D124065.)

lld/ELF/Symbols.h

133–134

Hmm. In a way it is because referencedAfterWrap will be used for the retaining now, but referenced on the pre-wrapped symbol is still used to set referencedAfterWrap on the wrapped symbol. I think it's fine to remove the sentence though, since the referencedAfterWrap comment also mentions that property being set based on if the original symbol is referenced.

139

Sure, I'll change it.

[MaskRay]

lld/ELF/SymbolTable.cpp

47

Ah adding the comment to D124065 will be good.

Are you splitting the two patches for easy bisection? I.e. in case the second patch breaks something?

[smeenai]

lld/ELF/SymbolTable.cpp

47

Yup, I wanted to keep the changes isolated so that each patch was an individual behavior change (i.e. this one only fixes the wrapping behavior and doesn't change any existing tests, and the next one changes symbol retention and needs an existing test to be modified).

[smeenai]

Address review comments

[MaskRay]

lld/test/ELF/lto/wrap-unreferenced-before-codegen.test

19

Prefer to add a space after callq

[smeenai]

Add space after callq

lld/ELF/Driver.cpp

Symbol *wrap =
         addUnusedUndefined(saver().save("__wrap_" + name), sym->binding);
     v.push_back({sym, real, wrap});
 
     // We want to tell LTO not to inline symbols to be overwritten
     // because LTO doesn't know the final symbol contents after renaming.
     real->scriptDefined = true;
     sym->scriptDefined = true;
 
-    // Tell LTO not to eliminate these symbols.
-    sym->isUsedInRegularObj = true;
-    // If sym is referenced in any object file, bitcode file or shared object,
-    // retain wrap which is the redirection target of sym. If the object file
-    // defining sym has sym references, we cannot easily distinguish the case
-    // from cases where sym is not referenced. Retain wrap because we choose to
-    // wrap sym references regardless of whether sym is defined
+    // If a symbol is referenced in any object file, bitcode file or shared
+    // object, mark its redirection target (foo for __real_foo and __wrap_foo
+    // for foo) as referenced after redirection, which will be used to tell LTO
+    // to not eliminate the redirection target. If the object file defining the
+    // symbol also references it, we cannot easily distinguish the case from
+    // cases where the symbol is not referenced. Retain the redirection target
+    // in this case because we choose to wrap symbol references regardless of
+    // whether the symbol is defined
     // (https://sourceware.org/bugzilla/show_bug.cgi?id=26358).
+    if (real->referenced || real->isDefined())
+      sym->referencedAfterWrap = true;
     if (sym->referenced || sym->isDefined())
-      wrap->isUsedInRegularObj = true;
+      wrap->referencedAfterWrap = true;
   }
   return v;
 }
 
 // Do renaming for --wrap and foo@v1 by updating pointers to symbols.
 //
 // When this function is executed, only InputFiles and symbol table
 // contain pointers to symbol objects. We visit them to replace pointers,

lld/ELF/LTO.cpp

for (size_t i = 0, e = syms.size(); i != e; ++i) {
     // be removed.
     r.Prevailing = !objSym.isUndefined() && sym->file == &f;
 
     // We ask LTO to preserve following global symbols:
     // 1) All symbols when doing relocatable link, so that them can be used
     //    for doing final link.
     // 2) Symbols that are used in regular objects.
     // 3) C named sections if we have corresponding __start_/__stop_ symbol.
-    // 4) Symbols that are defined in bitcode files and used for dynamic linking.
+    // 4) Symbols that are defined in bitcode files and used for dynamic
+    //    linking.
+    // 5) Symbols that will be referenced after linker wrapping is performed.
     r.VisibleToRegularObj = config->relocatable || sym->isUsedInRegularObj ||
+                            sym->referencedAfterWrap ||
                             (r.Prevailing && sym->includeInDynsym()) ||
                             usedStartStop.count(objSym.getSectionName());
     // Identify symbols exported dynamically, and that therefore could be
     // referenced by a shared library not visible to the linker.
     r.ExportDynamic =
         sym->computeBinding() != STB_LOCAL &&
         (config->exportDynamic || sym->exportDynamic || sym->inDynamicList);
     const auto *dr = dyn_cast<Defined>(sym);

lld/ELF/SymbolTable.cpp

void SymbolTable::wrap(Symbol *sym, Symbol *real, Symbol *wrap) {
   // Redirect __real_foo to the original foo and foo to the original __wrap_foo.
   int &idx1 = symMap[CachedHashStringRef(sym->getName())];
   int &idx2 = symMap[CachedHashStringRef(real->getName())];
   int &idx3 = symMap[CachedHashStringRef(wrap->getName())];
 
   idx2 = idx1;
   idx1 = idx3;
 
-  if (!real->isUsedInRegularObj && sym->isUndefined())
+  // Propagate symbol usage information to the redirected symbols.
+  if (sym->isUsedInRegularObj)
+    wrap->isUsedInRegularObj = true;
+  if (real->isUsedInRegularObj)
+    sym->isUsedInRegularObj = true;
+  else if (sym->isUndefined())
     sym->isUsedInRegularObj = false;
 
   // Now renaming is complete, and no one refers to real. We drop real from
   // .symtab and .dynsym. If real is undefined, it is important that we don't
   // leave it in .dynsym, because otherwise it might lead to an undefined symbol
   // error in a subsequent link. If real is defined, we could emit real as an
   // alias for sym, but that could degrade the user experience of some tools
   // that can print out only one symbol for each location: sym is a preferred

lld/ELF/Symbols.h

public:
 
   // True if the symbol is in the --dynamic-list file. A Defined symbol with
   // protected or default visibility with this property is required to be
   // exported into .dynsym.
   uint8_t inDynamicList : 1;
 
   // Used to track if there has been at least one undefined reference to the
   // symbol. For Undefined and SharedSymbol, the binding may change to STB_WEAK
   // if the first undefined reference from a non-shared object is weak.
-  //
-  // This is also used to retain __wrap_foo when foo is referenced.
   uint8_t referenced : 1;
 
+  // Used to track if this symbol will be referenced after wrapping is performed
+  // (i.e. this will be true for foo if __real_foo is referenced, and will be
+  // true for __wrap_foo if foo is referenced).
+  uint8_t referencedAfterWrap : 1;
+
   // True if this symbol is specified by --trace-symbol option.
   uint8_t traced : 1;
 
   // True if the name contains '@'.
   uint8_t hasVersionSuffix : 1;
 
   inline void replace(const Symbol &other);
 

 
 protected:
   Symbol(Kind k, InputFile *file, StringRef name, uint8_t binding,
          uint8_t stOther, uint8_t type)
       : file(file), nameData(name.data()), nameSize(name.size()), type(type),
         binding(binding), stOther(stOther), symbolKind(k),
         visibility(stOther & 3), isPreemptible(false),
         isUsedInRegularObj(false), used(false), exportDynamic(false),
-        inDynamicList(false), referenced(false), traced(false),
-        hasVersionSuffix(false), isInIplt(false), gotInIgot(false),
-        folded(false), needsTocRestore(false), scriptDefined(false),
-        needsCopy(false), needsGot(false), needsPlt(false), needsTlsDesc(false),
-        needsTlsGd(false), needsTlsGdToIe(false), needsGotDtprel(false),
-        needsTlsIe(false), hasDirectReloc(false) {}
+        inDynamicList(false), referenced(false), referencedAfterWrap(false),
+        traced(false), hasVersionSuffix(false), isInIplt(false),
+        gotInIgot(false), folded(false), needsTocRestore(false),
+        scriptDefined(false), needsCopy(false), needsGot(false),
+        needsPlt(false), needsTlsDesc(false), needsTlsGd(false),
+        needsTlsGdToIe(false), needsGotDtprel(false), needsTlsIe(false),
+        hasDirectReloc(false) {}
 
 public:
   // True if this symbol is in the Iplt sub-section of the Plt and the Igot
   // sub-section of the .got.plt or .got.
   uint8_t isInIplt : 1;
 
   // True if this symbol needs a GOT entry and its GOT entry is actually in
   // Igot. This will be true only for certain non-preemptible ifuncs.

lld/test/ELF/lto/wrap-unreferenced-before-codegen.test

+# REQUIRES: x86
+## Verify that we can correctly wrap symbols produced only during LTO codegen
+## and unreferenced before then.
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -triple x86_64-elf --filetype=obj -o %t/unwind.o %t/unwind.s
+# RUN: ld.lld -shared -o %t/libunwind.so -soname libunwind.so %t/unwind.o
+# RUN: llvm-as -o %t/resume.bc %t/resume.ll
+# RUN: ld.lld -shared -o %t/libresume.so -soname libresume.so %t/resume.bc \
+# RUN:   %t/libunwind.so --wrap _Unwind_Resume
+# RUN: llvm-objdump --dynamic-reloc --disassemble %t/libresume.so | \
+# RUN:   FileCheck --check-prefix=UNWIND-DISASM %s
+# RUN: llvm-readelf --dyn-syms %t/libresume.so | \
+# RUN:   FileCheck --check-prefix=UNWIND-DYNSYM %s
+
+# UNWIND-DISASM:       [[#%x,RELOC:]] R_X86_64_JUMP_SLOT __wrap__Unwind_Resume
+# UNWIND-DISASM-LABEL: <_Z1fv>:
+# UNWIND-DISASM:       callq {{.*}}<__wrap__Unwind_Resume@plt>
+# UNWIND-DISASM-LABEL: <__wrap__Unwind_Resume@plt>:
+# UNWIND-DISASM-NEXT:  jmpq *[[#]](%rip) # [[#%#x,RELOC]]
+
+# UNWIND-DYNSYM:      Symbol table '.dynsym' contains 5 entries:
+# UNWIND-DYNSYM:      NOTYPE  LOCAL  DEFAULT   UND
+# UNWIND-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT   UND throw
+# UNWIND-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT   UND _Unwind_Resume
+# UNWIND-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT   UND __wrap__Unwind_Resume
+# UNWIND-DYNSYM-NEXT: FUNC    GLOBAL DEFAULT     9 _Z1fv
+
+# RUN: llvm-mc -triple x86_64-elf -filetype=obj -o %t/malloc.o %t/malloc.s
+# RUN: ld.lld -shared -o %t/libmalloc.so -soname libmalloc.so %t/malloc.o
+# RUN: llvm-mc -triple x86_64-elf -filetype=obj -o %t/emutls.o %t/emutls.s
+# RUN: llvm-as -o %t/usetls.bc %t/usetls.ll
+# RUN: ld.lld -shared -o %t/libusetls.so %t/usetls.bc %t/libmalloc.so \
+# RUN:   --start-lib %t/emutls.o -mllvm -emulated-tls --wrap malloc
+# RUN: llvm-objdump --dynamic-reloc --disassemble %t/libusetls.so | \
+# RUN:   FileCheck --check-prefix=USETLS-DISASM %s
+# RUN: llvm-readelf --dyn-syms %t/libusetls.so | \
+# RUN:   FileCheck --check-prefix=USETLS-DYNSYM %s
+
+# USETLS-DISASM:       [[#%x,RELOC:]] R_X86_64_JUMP_SLOT __wrap_malloc
+# USETLS-DISASM-LABEL: <__emutls_get_address>:
+# USETLS-DISASM-NEXT:  jmp{{.*}}<__wrap_malloc@plt>
+# USETLS-DISASM-LABEL: <__wrap_malloc@plt>:
+# USETLS-DISASM-NEXT:  jmpq *[[#]](%rip) # [[#%#x,RELOC]]
+
+# USETLS-DYNSYM:      Symbol table '.dynsym' contains 6 entries:
+# USETLS-DYNSYM:      NOTYPE  LOCAL  DEFAULT   UND
+# USETLS-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT   UND malloc
+# USETLS-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT   UND __wrap_malloc
+# USETLS-DYNSYM-NEXT: FUNC    GLOBAL DEFAULT     6 f
+# USETLS-DYNSYM-NEXT: NOTYPE  GLOBAL DEFAULT     6 __emutls_get_address
+
+#--- unwind.s
+.globl _Unwind_Resume
+.globl __wrap__Unwind_Resume
+_Unwind_Resume:
+__wrap__Unwind_Resume:
+  retq
+
+#--- resume.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+define dso_local void @_Z1fv() optnone noinline personality i8* bitcast (void ()* @throw to i8*) {
+  invoke void @throw()
+          to label %unreachable unwind label %lpad
+lpad:
+  %1 = landingpad { i8*, i32 }
+          cleanup
+  resume { i8*, i32 } %1
+unreachable:
+  unreachable
+}
+declare void @throw()
+
+#--- malloc.s
+.globl malloc
+malloc:
+  retq
+
+#--- emutls.s
+.globl __emutls_get_address
+__emutls_get_address:
+  jmp malloc@plt
+
+#--- usetls.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+@x = dso_local thread_local global i32 0, align 4
+define dso_local i32 @f() {
+  %loaded = load i32, ptr @x, align 4
+  ret i32 %loaded
+}