Refine our --wrap implementation

[• rafael]

Summary

Before this patch we copy foo into real_foo and wrap_foo into foo. The net result is that __wrap_foo shows up twice in the symbol table.

With this patch we save a copy of real_foo and put it in the symbol table slow that was used by the original wrap_foo. We also omit it from the final output if it was undefined.

The net result is that

• Anything using foo now uses __wrap_foo
• Anything using __real_foo now uses foo.
• Anything using __wrap_foo still does.

And

• The slot for foo now has __wrap_foo
• The slot for __real_foo now has foo
• The slot for wrap_foo now has real_foo

Which I think is the desired behavior.

[jhenderson]

Could we get the full diff context, please?

[• rafael]

full context

[jhenderson]

I'm not sure I understand the motivation behind this change (or @ruiu's original change to add the symbol). Why are we creating __real_foo at all?

test/ELF/wrap.s

20

This doesn't look right to me at all. __real_foo, __wrap_foo and foo can't all have different values. Surely there should be only two different values between these three, because there are only two functions in the code?

[ruiu]

test/ELF/wrap.s

20

__wrap_foo and foo should have the same value, but __real_foo shouldn't, because it should have a value of the original function instead of your wrapper function. So I think this is correct.

[jhenderson]

test/ELF/wrap.s

20

I disagree with this. The current meaning of --wrap in other linkers is to redirect references to symbols, not to change the symbols themselves, so if anything, __real_foo and foo should have the same value and __wrap_foo a second, different value. (As an aside, the current test has three different values, not two, so is definitely wrong).

I don't understand why we need __real_foo at all - it's not actually a function in the user's source for example.

There is a serious danger that this could confuse users when they try to debug their code. By having multiple symbols for the same address, there is a potentially negative impact on the call stack displayed by the debugger - it has the right to pick whichever symbol it feels like (it may not even be deterministic about this). For example, this could mean that when the code crashes in the __wrap_foo() function, it could say that the crash occurs in foo(). The user then stares at the foo() function in their source code and cannot see anything wrong with it, because the problem isn't there.

[jhenderson]

test/ELF/wrap.s

20

Rafael posted the following on the mailing list, so I've included it here for completeness:

This actually implements the same behavior as bfd. The symbol references
become:

201000:       ba 10 10 01 00          mov    $0x11010,%edx
 201005:       ba 10 10 01 00          mov    $0x11010,%edx
 20100a:       ba 00 10 01 00          mov    $0x11000,%edx

And the symbols in the symbol table are unchanged:

0000000000011000 A foo
0000000000011020 A __real_foo
0000000000011010 A __wrap_foo

Cheers,
Rafael

Oh, I see what's going on now! I'd completely missed the second input to the test that defined all three symbols, so I was incorrectly assuming that only __wrap_foo and foo were defined. My apologies for the confusion.

FWIW, if __real_foo isn't defined in the test input, then it's not included in the bfd output, which was the case I was thinking of. It feels like this test case is missing.

I still disagree with @ruiu about __wrap_foo and foo having the same value in the symbol table (I don't think they should, and neither does bfd), although of course references to them in the code should be patched to the same value (i.e. to __wrap_foo) when using --wrap.

I now have no objection to this test change.

[• rafael]

Drop __real_foo from the symbol table if it was originally undefined.

[smeenai]

Context is missing again.

[• rafael]

now with context

[ruiu]

ELF/SymbolTable.cpp

169

Foo?

170

Real is already a Symbol*.

190–192

Is this correct? I thought foo becomes real_foo and wrap_foo becomes foo.

[• rafael]

Fix silly variable name.

[ruiu]

ELF/SymbolTable.cpp

190–192

I think I figured out why you want to rotate three symbols instead of just overwriting two symbols foo and wrap_foo. So, you want to keep these three symbols somehow in the symbol table so that they'll be emitted to the output symbol table, and you are reusing the symbol table slot for wrap_foo to keep real_foo, right?

What confused me was, after rotating three symbols, find("wrap_foo") returns real_foo. That is counter-intuitive because I didn't expect "--wrap foo" changes not only foo and real_foo but wrap_foo. Can you fix this?

[• rafael]

Use a new slot for __real_foo.

[ruiu]

LGTM

[• espindola]

315097

ELF/SymbolTable.h

struct SymbolRenaming {
     Symbol *Dst;
     Symbol *Src;
     uint8_t Binding;
   };
 
   // For -defsym or -wrap.
   std::vector<SymbolRenaming> Defsyms;
 
+  // For -wrap.
+  std::vector<std::pair<Symbol *, Symbol *>> WrapSymbols;
+
   // For LTO.
   std::unique_ptr<BitcodeCompiler> LTO;
 };
 
 extern SymbolTable *Symtab;
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/SymbolTable.cpp

template <class ELFT> void SymbolTable::addSymbolWrap(StringRef Name) {
   if (!B)
     return;
   Symbol *Sym = B->symbol();
   Symbol *Real = addUndefined<ELFT>(Saver.save("__real_" + Name));
   Symbol *Wrap = addUndefined<ELFT>(Saver.save("__wrap_" + Name));
 
   defsym(Real, Sym);
   defsym(Sym, Wrap);
+
+  WrapSymbols.push_back({Wrap, Real});
 }
 
 // Creates alias for symbol. Used to implement --defsym=ALIAS=SYM.
 template <class ELFT>
 void SymbolTable::addSymbolAlias(StringRef Alias, StringRef Name) {
   SymbolBody *B = find(Name);
   if (!B) {
     error("-defsym: undefined symbol: " + Name);
     return;
   }
 
   defsym(addUndefined<ELFT>(Alias), B->symbol());
 }
 
 // Apply symbol renames created by -wrap and -defsym. The renames are created
 // before LTO in addSymbolWrap() and addSymbolAlias() to have a chance to inform
 // LTO (if LTO is running) not to include these symbols in IPO. Now that the
 // symbols are finalized, we can perform the replacement.
 void SymbolTable::applySymbolRenames() {
+  // This function rotates 3 symbols:
+  //
+  // __real_foo becomes foo
+  // foo        becomes __wrap_foo
+  // __wrap_foo becomes __real_foo
+  //
+  // The last part is special in that we don't want to change what references to
+  // __wrap_foo point to, we just want have __real_foo in the symbol table.
+
+  // First make a copy of __real_foo
+  std::vector<Symbol> Origs;
+  for (const auto &P : WrapSymbols)
+    Origs.push_back(*P.second);
+
+  // Replace __real_foo with foo and foo with __wrap_foo
   for (SymbolRenaming &S : Defsyms) {
     S.Dst->body()->copyFrom(S.Src->body());
     S.Dst->File = S.Src->File;
     S.Dst->Binding = S.Binding;
   }
+
+  // Hide one of the copies of __wrap_foo, create a new symbol and copy
+  // __real_foo into it.
+  for (unsigned I = 0, N = WrapSymbols.size(); I < N; ++I) {
+    // We now have two copies of __wrap_foo. Drop one.
+    Symbol *Wrap = WrapSymbols[I].first;
+    Wrap->IsUsedInRegularObj = false;
+
+    Symbol *Real = &Origs[I];
+    // If __real_foo was undefined, we don't want it in the symbol table.
+    if (Real->body()->isUndefined())
+      continue;
+
+    auto *NewSym = make<Symbol>();
+    memcpy(NewSym, Real, sizeof(Symbol));
+    SymVector.push_back(NewSym);
+  }
 }
 
 static uint8_t getMinVisibility(uint8_t VA, uint8_t VB) {
   if (VA == STV_DEFAULT)
     return VB;
   if (VB == STV_DEFAULT)
     return VA;
   return std::min(VA, VB);

test/ELF/Inputs/wrap-no-real.s

+.globl foo, __wrap_foo
+foo = 0x11000
+__wrap_foo = 0x11010

test/ELF/lto/wrap-2.ll

 
 ; THIN:      foo:
 ; THIN-NEXT: pushq  %rax
 ; THIN-NEXT: callq{{.*}}<__wrap_bar>
 ; THIN-NEXT: popq  %rax
 ; THIN-NEXT: jmp{{.*}}<bar>
 
 ; Check that bar and __wrap_bar retain their original binding.
-; BIND:      Name: bar
+; BIND:      Name: __wrap_bar
 ; BIND-NEXT: Value:
 ; BIND-NEXT: Size:
 ; BIND-NEXT: Binding: Local
-; BIND:      Name: __wrap_bar
+; BIND:      Name: bar
 ; BIND-NEXT: Value:
 ; BIND-NEXT: Size:
 ; BIND-NEXT: Binding: Local
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @bar()
 declare void @__real_bar()
 
 define void @foo() {
   call void @bar()
   call void @__real_bar()
   ret void
 }

test/ELF/wrap-no-real.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/wrap-no-real.s -o %t2.o
+
+// RUN: ld.lld -o %t %t1.o %t2.o -wrap foo
+// RUN: llvm-objdump -d -print-imm-hex %t | FileCheck %s
+
+// CHECK: _start:
+// CHECK-NEXT: movl $0x11010, %edx
+// CHECK-NEXT: movl $0x11010, %edx
+// CHECK-NEXT: movl $0x11000, %edx
+
+// RUN: llvm-readobj -t -s %t | FileCheck -check-prefix=SYM %s
+// SYM-NOT:  Name: __real_foo
+// SYM:      Name: foo
+// SYM-NEXT: Value: 0x11000
+// SYM-NOT:  Name: __real_foo
+// SYM:      Name: __wrap_foo
+// SYM-NEXT: Value: 0x11010
+// SYM-NOT:  Name: __real_foo
+
+.global _start
+_start:
+  movl $foo, %edx
+  movl $__wrap_foo, %edx
+  movl $__real_foo, %edx

test/ELF/wrap.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/wrap.s -o %t2
 
 // RUN: ld.lld -o %t3 %t %t2 -wrap foo -wrap nosuchsym
 // RUN: llvm-objdump -d -print-imm-hex %t3 | FileCheck %s
 // RUN: ld.lld -o %t3 %t %t2 --wrap foo -wrap=nosuchsym
 // RUN: llvm-objdump -d -print-imm-hex %t3 | FileCheck %s
 
 // CHECK: _start:
 // CHECK-NEXT: movl $0x11010, %edx
 // CHECK-NEXT: movl $0x11010, %edx
 // CHECK-NEXT: movl $0x11000, %edx
 
-// This shows an oddity of our implementation. The symbol foo gets
-// mapped to __wrap_foo, but stays in the symbol table. This results
-// in it showing up twice in the output.
-
 // RUN: llvm-readobj -t -s %t3 | FileCheck -check-prefix=SYM %s
 // SYM:      Name: foo
 // SYM-NEXT: Value: 0x11000
 // SYM:      Name: __wrap_foo
 // SYM-NEXT: Value: 0x11010
-// SYM:      Name: __wrap_foo
-// SYM-NEXT: Value: 0x11010
+// SYM:      Name: __real_foo
+// SYM-NEXT: Value: 0x11020
 
 .global _start
 _start:
   movl $foo, %edx
   movl $__wrap_foo, %edx
   movl $__real_foo, %edx