[PATCH] D86181: [lld-macho] Implement -ObjC

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.

LGTM



================
Comment at: lld/MachO/Driver.cpp:254
+        if (sym.getName().startswith(objc::klass))
+          symtab->addUndefined(sym.getName());
+
----------------
I don't think this'll cause any issues with actual usage, but I'll just note that this isn't strictly equivalent to forcing a load of the archive member with that symbol. For example, if you already had a symbol with the same name (e.g. from an object file that's already been processed), this implementation won't load the archive member containing that symbol, whereas I believe ld64 would (and give you a multiply defined symbol error).


================
Comment at: lld/MachO/Driver.cpp:264
+
     inputFiles.push_back(make<ArchiveFile>(std::move(file)));
     break;
----------------
So we'd be adding ObjC-containing members twice, once as an ObjFile above and once as part of this archive? I don't think it'll cause any issues in practice, but it feels a little ugly conceptually. I think this is fine for now, but long-term it'd be nice to have something like LazyObjFile (like COFF and ELF do) to add each member only once (as either an ObjFile or a LazyObjFile). That would also address the TODO above and my previous comment.


================
Comment at: lld/test/MachO/objc.s:1
+# REQUIRES: x86
+# RUN: split-file %s %t
----------------
Can you add an input archive with multiple members and show that only the member containing the Obj-C symbols/sections gets pulled in? Right now I think all the tests would pass if you replaced `-ObjC` with `-all_load`; I want a test which shows the difference between the two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86181