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

[Vincent Lee via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/SyntheticSections.cpp:370
+    if ((p->opcode == BIND_OPCODE_DO_BIND_ADD_ADDR_ULEB) &&
+        (p->data < (BIND_IMMEDIATE_MASK * offsetWidth)) &&
+        ((p->data % offsetWidth) == 0)) {
----------------
int3 wrote:
> how about `p->data / offsetWidth <= BIND_IMMEDIATE_MASK`, to mirror the assignment below?
I changed it to `p->data / target->wordSize < BIND_IMMEDIATE_MASK`. I removed the equals comparison because when dyld uncompacts, it seems to add an extra `sizeof(intptr_t)`. 

See https://opensource.apple.com/source/dyld/dyld-852/src/ImageLoaderMachOCompressed.cpp.auto.html and search for `address += immediate*sizeof(intptr_t) + sizeof(intptr_t);`


================
Comment at: lld/test/MachO/bind-opcodes.s:1
 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
----------------
int3 wrote:
> thevinster wrote:
> > 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. 
> I think we can make it less messy :)
> 
> 1: yeah this makes sense
> 2: dumping the bind table is basically a sanity check: it decodes the bind opcodes into an easy-to-read form so we can verify that we encoded the right things. I think it's worth adding for 32-bit as well, but we can be clever and reuse the same code (see below)
> 3: I think we can reuse the same code. I haven't tested this but I think something like it should work:
> ```
> .ifdef PTR64
> .macro ptr val
>   .quad val
> .endm
> .endif
> 
> .ifdef PTR32
> .macro ptr val
>   .int val
> .endm
> .endif
> 
> ptr _foo
> ptr _bar
> ...
> ```
> PTR64 and PTR32 will have to be defined as part of llvm-mc's invocation. See https://github.com/llvm/llvm-project/blob/main/llvm/test/DebugInfo/X86/dwarfdump-header-64.s for an example.
> 4: FileCheck takes a `--check-prefix` argument, so we can put the checks in the same file.
All of them make sense and have been fixed. The file looks a lot cleaner now :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106128