[PATCH] D114397: [lld-macho] Mark dylib symbols coming from -weak_framework as weak-def.

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> What I said was mainly on the handling of the symbols from weak libraries/frameworks.

Right, so there are two parts to your commit message...

> Symbols imported from weak framework should be marked as weak-def. Otherwise, we may get runtime crash due to symbols not found.

This part is what your diff fixes

> Additionally, frameworks/libraries can be specified multiple times (eg. -framework Foo -weak_frameworks Foo). When that happens,
> the "forceWeakImport" should be "contagious"

This is already existing behavior

> IMO, since the NewCoreFoundation is weak, the symbols should come from the non-weak one, ie CoreFoundation.

"weak" is an (unfortunately) overloaded term here. From reading ld64's manpage, it seems that "weak library" is just short for "weakly referenced library". There's nothing there that says that they contain weak definitions as well.

And in the future please highlight any intentional deviations from ld64's behavior in the commit message :)

> done (real)

Explanatory comment is done, but we still need a test involving a re-exported symbol



================
Comment at: lld/MachO/Symbols.h:239
+
+  // Libraries and framework can be loaded multiple times with different
+  // forceWeakImport (eg., first loaded as strong then as weak)
----------------



================
Comment at: lld/MachO/Symbols.h:242-243
+  // When that happens we must ensure the weak-property sticks.
+  // Do not cache the result of this expression because `file` could change
+  // as we loaded more libraries/frameworks.
+  bool isWeakDef() const override {
----------------
not really sure this comment is important -- we don't actually call `isWeakRef` until after all the file parsing is done anyway. Also the computation involved is pretty trivial, I don't think we'd ever consider it as a candidate for caching


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114397