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

[Vincent Lee via Phabricator via llvm-commits]

thevinster marked 3 inline comments as done.
thevinster added a comment.

In D106128#2888416 <https://reviews.llvm.org/D106128#2888416>, @int3 wrote:

> one nit about the commit title / description: there are lots of opcodes with immediates, I think it would be clearer to mention `DO_BIND_ADD_ADDR_IMM_SCALED` specifically in the title. Also "Implement pass 3 of bind opcodes from ld64" should be followed by a short description of what pass 3 does -- we shouldn't expect future readers to have to look it up :)

Done.



================
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:
> thevinster wrote:
> > 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);`
> I think the previous implementation was the right one. The DO_BIND opcode itself adds a `sizeof(intptr_t)`, which is probably why dyld is doing that.
> 
> From http://www.m4b.io/reverse/engineering/mach/binaries/2015/03/29/mach-binaries.html 's description of DO_BIND:
> 
> > Push the current record onto the "import stack", and then increment the current record's address offset by the size of the platform pointer (32 or 64 bit)
> 
> It would be good to have a test case that covers this edge case :)
Re-capping offline convo. In ld64's implementation, it uses "<" instead of "<=". It makes more sense to keep the same behavior to prevent any unknown deviations from ld64. It may be a typo on ld64, but this will prevent unknown mysterious bugs down the road.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106128