[PATCH] D76252: [lld-macho] Add basic support for linking against dylibs

[Jez Ng via Phabricator via llvm-commits]

int3 marked 2 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:236
+      StringRef name = strtab + sym.n_strx;
+      symbols.push_back(symtab->addDylib(name, this));
+    }
----------------
MaskRay wrote:
> MaskRay wrote:
> > smeenai wrote:
> > > We shouldn't be doing this for symbols that are undefined in the dylib's symbol table, right? (And if so, we should add a test for that, though I don't think that'll be possible till after your follow-ups.)
> > > 
> > > Looks like LLD ELF adds both defined and undefined symbols from dylibs into the symbol table, presumably to be able to check if all undefined symbols for dylibs are satisfied when linking an executable. Whether or not we do that depends on ld64's behavior there.
> > > Looks like LLD ELF adds both defined and undefined symbols from dylibs into the symbol table, presumably to be able to check if all undefined symbols for dylibs are satisfied when linking an executable.
> > 
> > Yes. See D57385 and D57569.
> It is also used to set the `exportDynamic` property of a symbol. When linking an executable, the symbols used by DSOs will be added to `.dynsym`
I think re-exported symbols also appear as undefined in the symbol table... I need to figure out how to distinguish them. I'll add a TODO.


================
Comment at: lld/MachO/InputFiles.h:37
 
+  StringRef path;
   MemoryBufferRef mb;
----------------
int3 wrote:
> smeenai wrote:
> > Where is this used?
> `error("dylib " + path + " missing LC_ID_DYLIB load command");` in InputFiles.cpp
Actually, I see that we already have `getName()`. It just doesn't always work because we neglected to copy the strings from our input arguments before discarding them. I'll fix it and use that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76252