[PATCH] D93369: [lld-macho] Add support for weak references

[Nico Weber via Phabricator via llvm-commits]

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

Nice! I fumbled around a bit with this yesterday and wrote almost exactly the same change, except I didn't have the critical bind opcode bits (just the symbol table) since I don't know that part well yet. And in my local diff I made it so that a single non-weak ref makes the whole symbol non-weak while here a single weak ref makes the whole symbol weak (see first comment below). But in any case, this is great progress and definitely going in the right direction :)



================
Comment at: lld/MachO/SymbolTable.cpp:75
+  else if (auto *dynsym = dyn_cast<DylibSymbol>(s))
+    dynsym->weakRef |= isWeakRef;
+  else if (auto *undefined = dyn_cast<Undefined>(s))
----------------
Is that how it works? One weak ref makes all refs weak? I would've expected that one non-weak ref makes the whole symbol non-weak, but I haven't checked. Semantically it kind of makes sense: If your program has a single non-weak call to a symbol, there can't be a need for the symbol to be weak. Maybe that's what explains the ld64 behavior for functions?


================
Comment at: lld/MachO/SymbolTable.cpp:112
+      if (isWeakDef)
+        if (!defined->isWeakDef())
+          defined->overridesWeakDef = true;
----------------
Maybe combine these two nested ifs using the spiffy `&&` operator? :)


================
Comment at: lld/test/MachO/symtab.s:61
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: dyld_stub_binder (15)
+# CHECK-NEXT:     Extern
----------------
(a bit weird that llvm-readobj output doesn't include weakness)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93369