[PATCH] D106128: [lld-macho] Use immediate encodings for bind opcodes

Vincent Lee via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 16 17:11:17 PDT 2021


thevinster added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:368
+    if ((p->opcode == BIND_OPCODE_DO_BIND_ADD_ADDR_ULEB)
+          && (p->data < (15*sizeof(uint64_t)))
+          && ((p->data % sizeof(uint64_t)) == 0)) {
----------------
thevinster wrote:
> int3 wrote:
> > thevinster wrote:
> > > int3 wrote:
> > > > thevinster wrote:
> > > > > I believe `ld64` switches off the type based on whether it operates on a 32-bit or a 64-bit. I didn't get a chance to verify it because LLD doesn't seem to support `i386` (https://github.com/llvm/llvm-project/blob/main/lld/MachO/Driver.cpp#L700-L711). 
> > > > can we have a comment explaining how `BIND_OPCODE_DO_BIND_ADD_ADDR_IMM_SCALED` works, and where the 15 and `sizeof(uint64_t)` is coming from?
> > > That was quick! Will add a comment about the `15`. As far as the `sizeof(uint64_t)`, I wrote a comment above describing that situation. 
> > I'm confused as to why LLD not supporting i386 matters for testing ld64's behavior. llvm-mc can emit i386 object files that we can pass to ld64...
> I get the following error when trying to pass an i386 object file to LLD using x86_64 arch. `ld64.lld: error: /Users/leevince/local/llvm-project/build/Debug/tools/lld/test/MachO/Output/bind-opcodes.s.tmp/foo.o has architecture i386 which is incompatible with target architecture x86_64`. I'm unsure how to go about this without having to support i386. 
Spoken offline - tested 32-bit arch using arm64_32, and used that as comparison with x86_64. 


================
Comment at: lld/MachO/SyntheticSections.cpp:368-369
+    if ((p->opcode == BIND_OPCODE_DO_BIND_ADD_ADDR_ULEB)
+          && (p->data < (15*sizeof(uint64_t)))
+          && ((p->data % sizeof(uint64_t)) == 0)) {
+      p->opcode = BIND_OPCODE_DO_BIND_ADD_ADDR_IMM_SCALED;
----------------
thevinster wrote:
> thevinster wrote:
> > int3 wrote:
> > > thevinster wrote:
> > > > int3 wrote:
> > > > > thevinster wrote:
> > > > > > I believe `ld64` switches off the type based on whether it operates on a 32-bit or a 64-bit. I didn't get a chance to verify it because LLD doesn't seem to support `i386` (https://github.com/llvm/llvm-project/blob/main/lld/MachO/Driver.cpp#L700-L711). 
> > > > > can we have a comment explaining how `BIND_OPCODE_DO_BIND_ADD_ADDR_IMM_SCALED` works, and where the 15 and `sizeof(uint64_t)` is coming from?
> > > > That was quick! Will add a comment about the `15`. As far as the `sizeof(uint64_t)`, I wrote a comment above describing that situation. 
> > > I'm confused as to why LLD not supporting i386 matters for testing ld64's behavior. llvm-mc can emit i386 object files that we can pass to ld64...
> > I get the following error when trying to pass an i386 object file to LLD using x86_64 arch. `ld64.lld: error: /Users/leevince/local/llvm-project/build/Debug/tools/lld/test/MachO/Output/bind-opcodes.s.tmp/foo.o has architecture i386 which is incompatible with target architecture x86_64`. I'm unsure how to go about this without having to support i386. 
> Spoken offline - tested 32-bit arch using arm64_32, and used that as comparison with x86_64. 
I switched the use of `15` to `BIND_IMMEDIATE_MASK` which I think should be enough to cover adding the extra comment, but I'm happy to add it if it's still confusing to readers. 


================
Comment at: lld/test/MachO/bind-opcodes.s:1
 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
----------------
This file is pretty messy and hard to read. So I'll try to condense what I did here.
1/ In order to run both 64-bit tests and 32-bit tests in one file, I had to separate the suffix the input and output with the arch.
2/ Nothing changed with `llvm-objdump`. It was merely a forklift from the bottom to the top. There isn't a corresponding one for the arm64_32. I'm not exactly familiar with what it does, but I'm happy to add it if it provides value.
3/ In order to satisfy the linker, I had to switch the use of `quad` to `int` otherwise I get relocation errors. The order of the bind opcodes between 32-bit and 64-bit are the same (with slightly offsets)
4/ `CHECK` are in its own separate files now in order to isolate between different arch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106128



More information about the llvm-commits mailing list