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

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/Arch/X86_64.cpp:112
+  default:
+    error("TODO: Unhandled dylib relocation type " + std::to_string(type));
+    return 0;
----------------
smeenai wrote:
> 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).
I'm not sure if there are others, but I guess I can make this an assert for now and relax it if/when we discover more


================
Comment at: lld/MachO/InputFiles.h:68
+  // but it cannot be made private as we call it via make().
+  DylibFile();
+  static DylibFile *createLibSystemMock();
----------------
smeenai wrote:
> Why can't we just use the other constructor with your dummy MemoryBufferRef?
The DylibFile constructor attempts to read from the MemoryBufferRef and throws errors when it doesn't find what it expects


================
Comment at: lld/MachO/SyntheticSections.cpp:265
+    error("TODO: Support larger dylib symbol ordinals");
+    return opstreamOffset;
+  }
----------------
smeenai wrote:
> What's the purpose of this return, given we're gonna error anyway?
`error` doesn't cause the program to exit, for that we need `fatal`. But I guess this should really be a `fatal`.


================
Comment at: lld/MachO/SyntheticSections.h:102
 
+// The following sections implement lazy symbol binding -- very similar to the
+// PLT mechanism in ELF.
----------------
smeenai wrote:
> The comment is great!
Thanks! It took me quite a while to figure out how everything fit together, so it was fun to explain it :)


================
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]]
----------------
smeenai wrote:
> How come we aren't checking the value here?
I guess you mean on line 48? That's the offset into the bind opcode stream, and there's no nice arithmetic to describe it. Hard-coding the value will be brittle -- it will vary based on which entries appear earlier in the stream, and how large they are (which includes the length of their symbol names). There's the counterargument that these things aren't likely to change that often in our tests of course...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78270