[PATCH] D86572: [lld-macho] Implement weak bindings for GOT/TLV

[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/Symbols.h:78
+  Defined(StringRefZ name, InputSection *isec, uint32_t value, bool isWeakDef,
+          bool isLocal = true)
+      : Symbol(DefinedKind, name), isec(isec), value(value), weakDef(isWeakDef),
----------------
How much more work would it be to have this not have a default value, and instead set it explicitly for each constructor call? I'd like to explicit with this if it's not too onerous.


================
Comment at: lld/MachO/Symbols.h:86
 
+  bool isLocal() const { return local; }
+
----------------
The terminology used by Mach-O for this appears to be external/non-external, vs. global/local (as ELF does it). I'd prefer using that terminology in the linker as well.


================
Comment at: lld/MachO/SyntheticSections.cpp:224
+
+bool WeakBindingSection::isNeeded() const { return !bindings.empty(); }
+
----------------
Nit: put this in the header?


================
Comment at: lld/MachO/SyntheticSections.h:216
+
+// Add bindings for symbols that neeed weak or non-lazy bindings.
+void addNonLazyBindingEntries(const Symbol *, SectionPointerUnion,
----------------
Typo: neeed -> need


================
Comment at: lld/test/MachO/weak-binding.s:82
+
+_weak_local:
+  .quad 0x1234
----------------
Mind adding a test for a weak local TLV symbol as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86572