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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 2 inline comments as done.
int3 added inline comments.


================
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;
----------------
smeenai wrote:
> Out of curiosity, what makes us need the explicit `macho::` qualifier here?
it appears to be conflicting with `llvm::object::Archive::Symbol`. I didn't look into why it started conflicting in this diff and not before...


================
Comment at: lld/test/MachO/weak-binding.s:106
 
+_weak_external_fn:
+  ret
----------------
smeenai wrote:
> 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?
nope... weak non-externals cannot be coalesced at runtime. I'm not sure the weak flag does anything meaningful to internal symbols actually...

will add the 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
----------------
smeenai wrote:
> Do we also wanna check the weak bind entries?
the weak bind info doesn't include the dylib name, so it won't tell us which symbol got priority here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86573