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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SymbolTable.cpp:120
       (isa<DylibSymbol>(s) && !isWeakDef && s->isWeakDef()))
-    replaceSymbol<DylibSymbol>(s, file, name, isWeakDef, isTlv);
+    replaceSymbol<DylibSymbol>(s, file, name, isWeakDef, isWeakRef, isTlv);
 
----------------
int3 wrote:
> thakis wrote:
> > Can it happen that a dylib symbol is added before the first undefined? If so, then the initial dylib has isWeakRef set to false, and it can then never become true. If I read Driver.cpp, this can happen if a dylib is passed before the .o referring to it is passed. (In my local patch I gave DylibSymbol an
> > 
> > ```
> > +  enum WeakState { Unknown, SawWeakRefOnly, SawStrongRef,  };
> > +  WeakState weakRefState;
> > ```
> > 
> > and a
> > 
> > ```
> > +  void sawUndefined(bool weakRef) {
> > +    switch (weakRefState) {
> > +    case Unknown:
> > +      weakRefState = weakRef ? SawWeakRefOnly : SawStrongRef;
> > +      break;
> > +    case SawWeakRefOnly:
> > +      if (!weakRef)
> > +        weakRefState = SawStrongRef;
> > +      break;
> > +    case SawStrongRef:
> > +      break;
> > +    }
> > +  }
> > ```
> > 
> > and made DylibSymbol start with `Unknown` and then in `addUndefined` did
> > 
> > ```
> > +  else if (auto *dylib = dyn_cast<DylibSymbol>(s))
> > +    dylib->sawUndefined(isWeakRef);
> > ```
> > 
> > I only sketched it locally and hadn't written tests yet so maybe this is overkill, but my read of Driver is that this is necessary. (It's noticeably more annoying to write than making one weak ref turn the whole symbol weak.)
> oh yes. I thought I was missing something this morning in my implementation but couldn't put my finger on it... thanks!
Okay, so I think the enum approach is the right one, though the reasons are a bit more complicated. I think handling the "dylib symbol before undefined symbol" case by itself could have been handled by initializing DylibSymbols with `weakRef = true`. However, there are two other cases for which having an "unreferenced" state is valuable:

1. Determining which symbols to emit in LC_SYMTAB. The current implementation emits DylibSymbols if they are in the GOT/PLT, but that misses out symbols which aren't in either (e.g. referenced by `.quad sym`). I'm addressing that in this diff since it is naturally covered within the `weak-references.s` test.
2. We should emit `LC_LOAD_WEAK_DYLIB` for libraries whose symbols are only referenced weakly. But dylibs whose symbols are not used at all should still be loaded strongly. I'm addressing that in {D93435}.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93369