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

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:91
+  memcpy(buf, stubHelperHeader, sizeof(stubHelperHeader));
+  write32le(buf + 3, in.imageLoaderCache->addr - (in.stubHelper->addr + 7));
+  write32le(buf + 11, in.got->addr +
----------------
alexshap wrote:
> i'm wondering if we can replace these numbers (7, 11, 15, ... ) either with some expression (like sizeof(..) etc) or named constants or add comments 
lld-ELF uses a bunch of literal constants too, but I agree it's not ideal. I've added offsets to the comments and added a helper function, hope it's more readable now :)


================
Comment at: lld/MachO/Arch/X86_64.cpp:105
+
+uint64_t X86_64::getDylibSymbolVA(const DylibSymbol &sym, uint8_t type) const {
+  switch (type) {
----------------
smeenai wrote:
> Nit: can `type` be a `RelocationInfoType` instead of a `uint8_t`?
The input parameters would need to be `static_cast`. Alternatively we could change the definition of `relocation_info::type` to be a `RelocationInfoType`, and declare the enum as `enum RelocationInfoType : uint8_t`, but that currently runs into compile-time errors in the for the lld-macho backend due to an ambiguous type conversion (since what was formerly a strict enum is now maybe also a uint8_t). Seems like too much work to sort it out right now


================
Comment at: lld/MachO/SyntheticSections.cpp:265
+    error("TODO: Support larger dylib symbol ordinals");
+    return opstreamOffset;
+  }
----------------
smeenai wrote:
> int3 wrote:
> > 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`.
> I know that `error` doesn't cause the program to exit right away, but it will eventually cause the program to exit with an error. Given that, I was wondering if not having the `return` here and instead falling through to the `os <<` below would cause any harm.
Chatted offline, and we agreed that `fatal` is fine for things that we haven't implemented yet. It's not a user error, so `error()` -- which allows for more than one error diagnostic to be emitted -- isn't really useful as the user can't do much to address the problem. And the code is not unreachable with valid inputs, so `llvm_unreachable` isn't quite right either.


================
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:
> int3 wrote:
> > 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...
> Yeah, we should strike a balance between testing everything and having brittle tests, but in this scenario, assuming we don't modify the test itself, I'd say it's worth checking the actual offset. I'd even consider checking the bind opcode bytes we're emitting (assuming those aren't likely to change from future linker changes, only from future changes to this test itself), to ensure we don't accidentally regress those in the future.
Fair enough. Re the bind opcodes, they're being indirectly tested via the "Bind table" and "Lazy bind table" checks above, so I think we've got that covered


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78270