[PATCH] D79211: [lld-macho] Support pc-relative section relocations

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lld/MachO/InputFiles.cpp:173
+        if (rel.r_symbolnum == 0 || rel.r_symbolnum > sections.size())
+          fatal("invalid section index in relocation");
+        r.target = sections[rel.r_symbolnum - 1];
----------------
int3 wrote:
> smeenai wrote:
> > Should add more details to the error message, e.g. the object file being processed and details about the relocation.
> hm, I think this is a really unlikely (and therefore fatal) error for sane inputs, but okay... I do agree that `error()` messages should give useful hints to the user though
Yup, but given that this is a user-facing error for a corrupted input file, it's helpful to give as many details as possible about the corruption.


================
Comment at: lld/MachO/InputSection.cpp:48
+      // TODO: Figure out what to do for non-pcrel section relocations.
+      addend -= isec->header->addr - (r.offset + 4);
     }
----------------
int3 wrote:
> smeenai wrote:
> > `r.offset` is relative to the start of the input section containing the relocation. Doesn't this need to be adjusted for the address of that input section as well? As in:
> > 
> > ```
> >       addend -= isec->header->addr - (header->addr + r.offset + 4);
> > ```
> > 
> > I was gonna comment that the `+ 4` seems specific to x86-64; on ARM64, for example, the PC refers to the current instruction and not the next one. However, I actually can't get LLVM to produce a non-extern relocation for ARM64. ld64 does handle non-extern ARM64_RELOC_UNSIGNED relocations, but for anything I feed to LLVM, it converts local symbols to linker-local symbols and just uses an extern relocation.
> > 
> > The other thing I noticed looking at ld64 is that we aren't using or validating the length field of the relocation anywhere, which is potentially problematic.
> Oops, I guess I hadn't realized this since the relocations have all been in the `__text` section...
> 
> Re ARM, I guess we'll eventually have something similar to the `getDylibSymbolVA` above. I'll add a TODO comment.
> 
> And yeah, I need to figure out the significance of the length field.
Awesome, thanks for adding a test as well!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79211