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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai 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];
----------------
Should add more details to the error message, e.g. the object file being processed and details about the relocation.


================
Comment at: lld/MachO/InputSection.cpp:46
+      // in terms of the addresses in the input file. Here we adjust it so that
+      // it describes the offset from the start of the relocated section.
+      // TODO: Figure out what to do for non-pcrel section relocations.
----------------
Perhaps "the relocated target section", to make it completely clear which section is being referred to?


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


================
Comment at: lld/test/MachO/relocations.s:48
+## References to this generate a section relocation
+L_.str:
+  .asciz "Private symbol\n"
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79211