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

[Jez Ng via Phabricator via llvm-commits]

int3 marked 6 inline comments as done.
int3 added inline comments.


================
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];
----------------
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


================
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);
     }
----------------
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.


================
Comment at: lld/test/MachO/relocations.s:48
+## References to this generate a section relocation
+L_.str:
+  .asciz "Private symbol\n"
----------------
smeenai wrote:
> How come the comment in the next test about needing to create a new section to force a section relocation doesn't apply? Is the cstring section a special case? (Wouldn't surprise me since IIRC the linker atomizes the cstring section automatically based on zero termination instead of atomizing based on symbols.)
oh, good observation... yeah, it seems like `__cstring` is special. I tried renaming it to `__not_cstring` while keeping `.asciz` values and this turned into a symbol relocation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79211