[lld/mac] Mark private externs with GOT relocs as LOCAL in indirect symbtab

[thakis]

Summary

prepareSymbolRelocation() in Writer.cpp adds both symbols that need binding and
symbols relocated with a pointer relocation to the got.

Pointer relocations are emitted for non-movq GOTPCREL(%rip) loads. (movqs
become GOT_LOADs so that the linker knows they can be relaxed to leaqs, while
others, such as addq, become just GOT -- a pointer relocation -- since they
can't be relaxed in that way).

For example, this C file produces a private_extern GOT relocation when
compiled with -O2 with clang:

extern const char kString[];
const char* g(int a) { return kString + a; }

Linkers need to put pointer-relocated symbols into the GOT, but ld64 marks them
as LOCAL in the indirect symbol table. This matters, since strip -x looks at
the indirect symbol table when deciding what to strip.

The indirect symtab emitting code was assuming that only symbols that need
binding are in the GOT, but pointer relocations where there too. Hence, the
code needs to explicitly check if a symbol is a private extern.

Fixes https://crbug.com/1242638, which has some more information in comments 14
and 15. With this patch, the output of nm -U on Chromium Framework after
stripping now contains just two symbols when using lld, just like with ld64.

[thakis]

@oontvoo This apparently breaks compact-unwind-both-local-and-dylib-personality.s which is new since I originally wrote this patch. Does the failure there make sense for you?

[thakis]

Looking at https://reviews.llvm.org/D107533, apparently we just ignore the symbol in UnwindInfoSection.cpp and pick a different one, but still write the old one in the symbol table. That looks pretty sketch :/

[thakis]

don't break existing test

[oontvoo]

In D111852#3065697, @thakis wrote:

Looking at https://reviews.llvm.org/D107533, apparently we just ignore the symbol in UnwindInfoSection.cpp and pick a different one, but still write the old one in the symbol table. That looks pretty sketch :/

It is absolutely sketchy as I was trying to imitate LD64's behaviour, which we'd agreed was inconsistent. The only reason I put it in was because it unbroke a lot of Chromium/Cronet based tests and it seemed fairly harmless to others at the time.

TBH, I've been pondering removing it - esp. jyknight@ has suggested a different solution (ie., removing chrome personality function - see the email he sent a month ago ...)

If it's blocking you, we could remove it now?

[oontvoo]

In D111852#3065704, @thakis wrote:

don't break existing test

P.S: Oh, good then! 👍

[thakis]

Thanks :)

Want to review the entire change while you're here? :)

[oontvoo]

lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s

42–45

LD64 seems to only produced the three personality that are "picked" here. (ie., GXX_PERSONALITY_HI isn't present).
I guess that's fine?

[thakis]

Thanks!

lld/MachO/SyntheticSections.cpp

void IndirectSymtabSection::finalizeContents() {
   in.tlvPointers->reserved1 = off;
   off += in.tlvPointers->getEntries().size();
   in.stubs->reserved1 = off;
   off += in.stubs->getEntries().size();
   in.lazyPointers->reserved1 = off;
 }
 
 static uint32_t indirectValue(const Symbol *sym) {
-  return sym->symtabIndex != UINT32_MAX ? sym->symtabIndex
-                                        : INDIRECT_SYMBOL_LOCAL;
+  if (sym->symtabIndex == UINT32_MAX)
+    return INDIRECT_SYMBOL_LOCAL;
+  if (auto *defined = dyn_cast<Defined>(sym))
+    if (defined->privateExtern)
+      return INDIRECT_SYMBOL_LOCAL;
+  return sym->symtabIndex;
 }
 
 void IndirectSymtabSection::writeTo(uint8_t *buf) const {
   uint32_t off = 0;
   for (const Symbol *sym : in.got->getEntries()) {
     write32le(buf + off * sizeof(uint32_t), indirectValue(sym));
     ++off;
   }

lld/test/MachO/compact-unwind-both-local-and-dylib-personality.s

 
 # RUN: llvm-objdump --macho --unwind-info --syms --indirect-symbols --bind %t/a.out | FileCheck %s --check-prefixes=A,CHECK -D#%x,OFF=0x100000000
 # RUN: llvm-objdump --macho --unwind-info --syms --indirect-symbols --bind %t/b.out | FileCheck %s --check-prefixes=BC,CHECK -D#%x,OFF=0x100000000
 # RUN: llvm-objdump --macho --unwind-info --syms --indirect-symbols --bind %t/c.out | FileCheck %s --check-prefixes=BC,CHECK -D#%x,OFF=0
 
 # A:      Indirect symbols for (__DATA_CONST,__got)
 # A-NEXT: address                    index name
 # A-DAG:  0x[[#%x,GXX_PERSONALITY_LO:]] [[#]] ___gxx_personality_v0
-# A-DAG:  0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0        
+# A-DAG:  0x[[#%x,GXX_PERSONALITY_HI:]] [[#]] ___gxx_personality_v0
 # A-DAG:  0x[[#%x,PERSONALITY_1:]]  LOCAL
 # A-DAG:  0x[[#%x,PERSONALITY_2:]]  LOCAL
 
 # BC:      Indirect symbols for (__DATA_CONST,__got)
 # BC-NEXT: address                    index name
+# BC-DAG:  0x[[#%x,GXX_PERSONALITY_HI:]] LOCAL
 # BC-DAG:  0x[[#%x,GXX_PERSONALITY_LO:]] LOCAL
 # BC-DAG:  0x[[#%x,PERSONALITY_1:]]  LOCAL
 # BC-DAG:  0x[[#%x,PERSONALITY_2:]]  LOCAL
 
 # CHECK:        Personality functions: (count = 3)
 # CHECK-DAG:     personality[{{[0-9]+}}]: 0x{{0*}}[[#GXX_PERSONALITY_LO-OFF]]
 # CHECK-DAG:     personality[{{[0-9]+}}]: 0x{{0*}}[[#PERSONALITY_1-OFF]]
 # CHECK-DAG:     personality[{{[0-9]+}}]: 0x{{0*}}[[#PERSONALITY_2-OFF]]
 
 # A: Bind table
 # A-NEXT: segment  section          address      type       addend dylib            symbol

lld/test/MachO/indirect-symtab.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/libfoo.s -o %t/libfoo.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/bar.s -o %t/bar.o
 # RUN: %lld -dylib %t/libfoo.o -o %t/libfoo.dylib -lSystem
-# RUN: %lld %t/test.o %t/libfoo.dylib -o %t/test -lSystem
+# RUN: %lld %t/test.o %t/bar.o %t/libfoo.dylib -o %t/test -lSystem
 # RUN: llvm-objdump --macho -d --no-show-raw-insn --indirect-symbols %t/test | FileCheck %s
 # RUN: llvm-otool -l %t/test | FileCheck --check-prefix=DYSYMTAB %s
 
 # CHECK:      (__TEXT,__text) section
 # CHECK-NEXT: _main:
 # CHECK-NEXT: movq  {{.*}}(%rip), %rax ## literal pool symbol address: _foo
 # CHECK-NEXT: movq  {{.*}}(%rip), %rax ## literal pool symbol address: _bar
+# CHECK-NEXT: leaq  _baz(%rip), %rax
+# CHECK-NEXT: addq  {{.*}}(%rip), %rax
 # CHECK-NEXT: movq  {{.*}}(%rip), %rax ## literal pool symbol address: _foo_tlv
 # CHECK-NEXT: movq  {{.*}}(%rip), %rax ## literal pool symbol address: _bar_tlv
 # CHECK-NEXT: callq {{.*}} ## symbol stub for: _foo_fn
 # CHECK-NEXT: callq {{.*}} ## symbol stub for: _bar_fn
 # CHECK-NEXT: retq
 
 # CHECK:      Indirect symbols for (__TEXT,__stubs) 2 entries
 # CHECK-NEXT: address            index name
 # CHECK-NEXT: _bar_fn
 # CHECK-NEXT: _foo_fn
-# CHECK-NEXT: Indirect symbols for (__DATA_CONST,__got) 3 entries
+# CHECK-NEXT: Indirect symbols for (__DATA_CONST,__got) 4 entries
 # CHECK-NEXT: address            index name
+# CHECK-NEXT: LOCAL
 # CHECK-NEXT: _bar
 # CHECK-NEXT: _foo
 # CHECK-NEXT: _stub_binder
 # CHECK-NEXT: Indirect symbols for (__DATA,__la_symbol_ptr) 2 entries
 # CHECK-NEXT: address            index name
 # CHECK-NEXT: _bar_fn
 # CHECK-NEXT: _foo_fn
 # CHECK-NEXT: Indirect symbols for (__DATA,__thread_ptrs) 2 entries
 # CHECK-NEXT: address            index name
 # CHECK-NEXT: _bar_tlv
 # CHECK-NEXT: _foo_tlv
 
-# DYSYMTAB: nindirectsyms 9
+# DYSYMTAB: nindirectsyms 10
 
 #--- libfoo.s
 
 .globl _foo, _foo_fn, _bar, _bar_fn
 _foo:
 _foo_fn:
 _bar:
 _bar_fn:
 
 .section  __DATA,__thread_vars,thread_local_variables
 .globl _foo_tlv, _bar_tlv
 _foo_tlv:
 _bar_tlv:
 
 #--- test.s
 
 .globl _main
 _main:
   movq _foo@GOTPCREL(%rip), %rax
   movq _bar@GOTPCREL(%rip), %rax
+  movq _baz@GOTPCREL(%rip), %rax
+  addq _quux@GOTPCREL(%rip), %rax
   mov _foo_tlv@TLVP(%rip), %rax
   mov _bar_tlv@TLVP(%rip), %rax
   callq _foo_fn
   callq _bar_fn
   ret
+
+#--- bar.s
+.data
+.globl _baz,_quux
+.private_extern _baz,_quux
+_baz:
+.asciz "baz"
+_quux:
+.asciz "quux"