[PATCH] D78270: [lld-macho] Support calls to functions in dylibs
Shoaib Meenai via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 29 19:29:32 PDT 2020
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
More information about the llvm-commits
mailing list