[PATCH] D116913: [lld-macho] Add --start-lib --end-lib

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 5 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/MachO/Driver.cpp:350-359
+    if ((magic == file_magic::macho_object || magic == file_magic::bitcode) &&
+        newFile->lazy && config->forceLoadObjC) {
+      for (Symbol *sym : newFile->symbols)
+        if (sym && sym->getName().startswith(objc::klass)) {
+          extract(*newFile, "-ObjC");
+          break;
+        }
----------------
oontvoo wrote:
> int3 wrote:
> > int3 wrote:
> > > nit: how about `isa<ObjFile>` and `isa<BitcodeFile>` since the line above is also using `isa<>`?
> > IIUC, we are evaluating lazy object files for ObjC members after doing regular symbol resolution. Archive loading (intentionally) does this in the opposite order: {D108781}. Would probably be good to follow suit
> is anything wrong with switching the previous predicate to checking the magic? (checking for the underlying type always seemed kind of iffy to me ... )
Thanks for the pointer. Unfortunately it is difficult to follow D108781 because without `parse` and `parseLazy`, we don't have `symbols` (to check ObjC symbols/sections).... For now I just add a test with TODO.

I think some heavy symbol table refactoring is needed to make `symbols` available before symbol resolution. https://github.com/llvm/llvm-project/issues/53104


================
Comment at: lld/MachO/InputFiles.cpp:1619
+  symbols.resize(obj->symbols().size());
+  for (auto it : llvm::enumerate(obj->symbols())) {
+    const lto::InputFile::Symbol &objSym = it.value();
----------------
oontvoo wrote:
> int3 wrote:
> > no need for `llvm::`
> Sorry, I'm missing something here but why not simply ` for (const lto::InputFile::Symbol &objSym : obj->symbols())` ?
> IOWs, why is `llvm::enumerate` needed?
`llvm::foo` for STL-like helpers (lower_bound, find_if, etc) has an advantage: prevent the confusing argument-dependent lookup. In other components of llvm-project `llvm::foo` for STL-like helpers is generally recommended.

So I am going to defend for the usage a bit...


================
Comment at: lld/MachO/InputFiles.cpp:1619
+  symbols.resize(obj->symbols().size());
+  for (auto it : llvm::enumerate(obj->symbols())) {
+    const lto::InputFile::Symbol &objSym = it.value();
----------------
MaskRay wrote:
> oontvoo wrote:
> > int3 wrote:
> > > no need for `llvm::`
> > Sorry, I'm missing something here but why not simply ` for (const lto::InputFile::Symbol &objSym : obj->symbols())` ?
> > IOWs, why is `llvm::enumerate` needed?
> `llvm::foo` for STL-like helpers (lower_bound, find_if, etc) has an advantage: prevent the confusing argument-dependent lookup. In other components of llvm-project `llvm::foo` for STL-like helpers is generally recommended.
> 
> So I am going to defend for the usage a bit...
`symbols[it.index()] ` is needed by `-ObjC` for ObjC symbol checking. It has another advantage: when a symbol table refactoring is done in the future, we can transfer the symbol table to the non-lazy state. See D116390 for ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116913