[PATCH] D85255: [lld-macho] Generate ObjC symbols from .tbd files

[Jez Ng via Phabricator via llvm-commits]

int3 added a subscriber: ruiu.
int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:394
   dylibName = saver.save(interface->getInstallName());
+  auto addSymbol = [&](const Twine &name) -> void {
+    symbols.push_back(symtab->addDylib(saver.save(name), umbrella,
----------------
compnerd wrote:
> Does this really need the full context?
I don't think explicit captures add much to readability, especially for such a small lambda body...

(I don't think avoiding `auto` does either, that was @ruiu's preference)


================
Comment at: lld/MachO/InputFiles.cpp:421
+      break;
+    }
+  }
----------------
compnerd wrote:
> Same as last time - would be nice to actually compose with the user label prefix.
> 
> Remember that `x86_64-apple-coff` is a real target - and the user label prefix is empty there.  This breaks because you are assuming that the user label prefix is `_`.
> Remember that x86_64-apple-coff is a real target

oh what... is this actually used?


================
Comment at: lld/test/MachO/Inputs/MacOSX.sdk/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation.tbd:13
+    objc-ivars:    [ NSConstantArray._count ]
+    objc-eh-types: [ NSException ]
----------------
compnerd wrote:
> Id really rather see a second library added (`Foundation.tbd`) rather than put this into `CoreFoundation`.  This makes it confusing for others IMO.
these members are from CoreFoundation in the actual SDK, though...


================
Comment at: lld/test/MachO/stub-link.s:6
 # RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
-# RUN: lld -flavor darwinnew -o %t/test -Z -L%S/Inputs/MacOSX.sdk/usr/lib -lSystem %t/test.o
+# RUN: lld -flavor darwinnew -o %t/test -syslibroot %S/Inputs/MacOSX.sdk -lSystem -framework CoreFoundation %t/test.o
 #
----------------
compnerd wrote:
> I think that we should go through and actually improve all the tests to use `-syslibroot`.  This really looks far better (and matches the real world usage).
agreed, will do it in a separate commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85255