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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

This LGTM with the comments. @ruiu, @MaskRay, any further comments? I think it's fine leaving the tests as YAML for now, since subsequent diffs in the stack will enabling LLD to produce dylibs itself and we'll replace the tests at that point.



================
Comment at: lld/MachO/InputFiles.cpp:225
+  // Initialize symbols.
+  if (const load_command *cmd = findCommand(hdr, LC_SYMTAB)) {
+    auto *c = (const symtab_command *)cmd;
----------------
Is it correct to just consider LC_SYMTAB for this, or should we also be consulting LC_DYSYMTAB and/or the export trie? If so, we don't have to address that in this diff, but we should add a TODO.

We should also add a TODO about handling a missing LC_SYMTAB.


================
Comment at: lld/MachO/InputFiles.h:37
 
+  StringRef path;
   MemoryBufferRef mb;
----------------
Where is this used?


================
Comment at: lld/MachO/SyntheticSections.h:23
+// dylib symbols.
+class GotSection : public InputSection {
+public:
----------------
It's kinda interesting to me that GotSection would inherit from InputSection, since it's not an input section, of course.

LLD ELF explains this design decision like so:

```
// Synthetic sections are designed as input sections as opposed to
// output sections because we want to allow them to be manipulated
// using linker scripts just like other input sections from regular
// files.
```

For Mach-O, we don't have linker scripts, so perhaps that reasoning doesn't hold.

COFF uses Chunks instead of InputSections, since there's more synthesized data (https://lld.llvm.org/NewLLD.html#important-data-structures). I think Mach-O has a good amount of linker-synthesized data and input section processing as well, so a COFF-like design might make more sense. It's hard to say until we have more of the implementation though.

For now, I think leaving this inheritance as-is is fine, but we should add a TODO to reconsider it once we've implemented more of the linker. @ruiu, @MaskRay, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76252