[PATCH] D86573: [lld-macho] Implement weak binding for branch relocations

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lld/MachO/Arch/X86_64.cpp:32
 
-  void writeStub(uint8_t *buf, const DylibSymbol &) const override;
+  void writeStub(uint8_t *buf, const macho::Symbol &) const override;
   void writeStubHelperHeader(uint8_t *buf) const override;
----------------
Out of curiosity, what makes us need the explicit `macho::` qualifier here?


================
Comment at: lld/MachO/Symbols.h:67
   uint32_t gotIndex = UINT32_MAX;
+  uint32_t stubsIndex = UINT32_MAX;
 
----------------
Nit: add a newline before this, to separate it from the block of `gotIndex` and its comment.


================
Comment at: lld/test/MachO/weak-binding.s:106
 
+_weak_external_fn:
+  ret
----------------
If I'm understanding the code correctly, a weak non-external function would behave the same as a weak external function (i.e. it would get a weak binding entry), which is different from non-external data. Is that intended, and either way, could you add a test?


================
Comment at: lld/test/MachO/weak-definition-order.s:28
 # RUN: lld -flavor darwinnew -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem -o %t/dylib21 -Z -L%t -lweak2 -lweak1 %t/test.o
-# RUN: llvm-objdump --macho --lazy-bind %t/dylib21 | FileCheck %s --check-prefix=DYLIB2
-## TODO: these should really be in the weak binding section, not the lazy binding section
-# DYLIB1: __DATA   __la_symbol_ptr    0x{{[0-9a-f]*}} libweak1         _foo
-# DYLIB2: __DATA   __la_symbol_ptr    0x{{[0-9a-f]*}} libweak2         _foo
+# RUN: llvm-objdump --macho --bind %t/dylib21 | FileCheck %s --check-prefix=DYLIB2
+# DYLIB1: __DATA   __la_symbol_ptr    0x{{[0-9a-f]*}} pointer 0 libweak1         _foo
----------------
Do we also wanna check the weak bind entries?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86573/new/

https://reviews.llvm.org/D86573