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

[Jez Ng via Phabricator via llvm-commits]

int3 marked an inline comment as done.
int3 added inline comments.


================
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))
----------------
thakis wrote:
> 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?
> I would've expected that one non-weak ref makes the whole symbol non-weak

I expected this too, but ld64 doesn't do this for non-function symbols. I can't see a good reason for the inconsistency between functions and non-functions, so I guess we have to make a choice. Making weakness take priority has the advantage of being more permissive -- if ld64 succeeds in linking a given binary, so should we. But there's a good argument for not implementing dubious behavior until we have a concrete use case.

tl;dr I'll switch to having non-weak take priority


================
Comment at: lld/test/MachO/symtab.s:61
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name: dyld_stub_binder (15)
+# CHECK-NEXT:     Extern
----------------
thakis wrote:
> (a bit weird that llvm-readobj output doesn't include weakness)
it does -- these particular symbols are just not weak references. I modified this test case because we're now marking all dylib symbols as `extern`, which I believe is the right behavior. And that was done because `nm` doesn't interpret the weakref flag correctly if `extern` isn't set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93369