[PATCH] D78270: [lld-macho] Support calls to functions in dylibs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:73
+  memcpy(buf, stub, 2); // just copy the two nonzero bytes
+  uint64_t rip = in.stubs->addr + (sym.stubsIndex + 1) * sizeof(stub);
+  write32le(buf + 2, in.lazyPointers->addr + sym.stubsIndex * WordSize - rip);
----------------
I'd add a comment that the `+ 1` is because RIP on x86-64 reflects the next instruction instead of the current instruction.

Or perhaps that can be a more general comment above this section, since it's relevant for the other stubs as well.


================
Comment at: lld/MachO/Arch/X86_64.cpp:105
+
+uint64_t X86_64::getDylibSymbolVA(const DylibSymbol &sym, uint8_t type) const {
+  switch (type) {
----------------
Nit: can `type` be a `RelocationInfoType` instead of a `uint8_t`?


================
Comment at: lld/MachO/Arch/X86_64.cpp:112
+  default:
+    error("TODO: Unhandled dylib relocation type " + std::to_string(type));
+    return 0;
----------------
Are there any other sorts of relocations we would conceivably need to handle for this in the future? If not, this should be an `llvm_unreachable`

>From playing around with this a bit, clang generates an `X86_64_RELOC_GOT` for dylib function calls when you compile with `-fno-plt`, so perhaps that's at least one other case we need to care about (eventually).


================
Comment at: lld/MachO/Driver.cpp:161
+  // dyld requires us to load libSystem. Since we may run tests on non-OSX
+  // systems which do not have libSystem, we mock it out here.
+  if (StringRef(getenv("LLD_IN_TEST")) == "1" &&
----------------
Perhaps add a TODO to replace this with a stub tbd file when we have TAPI support?


================
Comment at: lld/MachO/InputFiles.h:68
+  // but it cannot be made private as we call it via make().
+  DylibFile();
+  static DylibFile *createLibSystemMock();
----------------
Why can't we just use the other constructor with your dummy MemoryBufferRef?


================
Comment at: lld/MachO/SyntheticSections.cpp:265
+    error("TODO: Support larger dylib symbol ordinals");
+    return opstreamOffset;
+  }
----------------
What's the purpose of this return, given we're gonna error anyway?


================
Comment at: lld/MachO/SyntheticSections.h:102
 
+// The following sections implement lazy symbol binding -- very similar to the
+// PLT mechanism in ELF.
----------------
The comment is great!


================
Comment at: lld/MachO/SyntheticSections.h:151
+
+// This section contains space for just a single word, and will be used by dyld
+// to cache an address to the image loader it uses. Note that unlike the other
----------------
Huh, interesting.

CC @Ktwu 


================
Comment at: lld/test/MachO/dylink-lazy.s:48
+# CHECK-NEXT:    jmp 0x[[#STUB_HELPER_ENTRY]]
+# CHECK-NEXT:    pushq $
+# CHECK-NEXT:    jmp 0x[[#STUB_HELPER_ENTRY]]
----------------
How come we aren't checking the value here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78270