[PATCH] D90614: [lld-macho] Handle paired relocs

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Nit: Commit title seems to indicate that we are adding support for paired relocs in this diff, but it seems like this diff is just paving the way for adding that support. Might be worth rephrasing... could also tag with `[nfc]` to make it explicit.

Also, would be good to add some comments explaining what a paired relocation is, especially since it appears to be a Mach-O specific thing. (I know Apple's `reloc.h` explains it, but ideally someone reading LLD's code would be able to understand what's going on without cross-referencing)



================
Comment at: lld/MachO/Arch/X86_64.cpp:73
 
-uint64_t X86_64::getImplicitAddend(MemoryBufferRef mb, const section_64 &sec,
-                                   const relocation_info &rel) const {
+bool X86_64::isPairedReloc(const relocation_info rel) const {
+  return (rel.r_type == X86_64_RELOC_SUBTRACTOR);
----------------
pass by const ref?


================
Comment at: lld/MachO/Arch/X86_64.cpp:74
+bool X86_64::isPairedReloc(const relocation_info rel) const {
+  return (rel.r_type == X86_64_RELOC_SUBTRACTOR);
+}
----------------
ditto about parens


================
Comment at: lld/MachO/Arch/X86_64.cpp:78-79
+uint64_t X86_64::getAddend(MemoryBufferRef mb, const section_64 &sec,
+                           const relocation_info rel,
+                           const relocation_info pairedRel) const {
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
----------------
pass by const ref


================
Comment at: lld/MachO/InputFiles.cpp:178
+
+  for (size_t i = 0; i < relInfos.size(); i++) {
+    const relocation_info pairedInfo = relInfos[i];
----------------
other reviewers have mentioned that LLVM style is to evaluate loop constants in the assignment part of the loop header (i.e. `size_t i= 0, e = < relInfos.size(); i < e; ...`). I don't really care much either way, just FYI


================
Comment at: lld/MachO/InputFiles.cpp:181
+    const relocation_info relInfo =
+        (target->isPairedReloc(pairedInfo) ? relInfos[++i] : pairedInfo);
+    assert(i < relInfos.size());
----------------
gkm wrote:
> gkm wrote:
> > compnerd wrote:
> > > You could also do `std::next(&RI)` if you want to continue using the range based for loop.
> > That's an improvement! 😺
> Only in concept ... it doesn't work in practice. `std::next()` accepts and returns an iterator. Range-based `for` keeps the iterator behind the scenes. The best I can do to make this more C++ish is to use an iterator object with `std::next()` vs. integer array index with ordinary arithmetic. Methinks there is no great advantage.
nit: I'd prefer to skip the extra parens around assignment and return expressions


================
Comment at: lld/MachO/InputFiles.cpp:196
     } else {
-      if (relInfo.r_symbolnum == 0 || relInfo.r_symbolnum > subsections.size())
-        fatal("invalid section index in relocation for offset " +
-              std::to_string(r.offset) + " in section " + sec.sectname +
-              " of " + getName());
-
+      assert(relInfo.r_symbolnum && relInfo.r_symbolnum <= subsections.size());
       SubsectionMap &referentSubsecMap = subsections[relInfo.r_symbolnum - 1];
----------------
gkm wrote:
> compnerd wrote:
> > This really isn't equivalent.  The `assert` may be compiled out, while the previous behaviour would have been preserved and reported an error to the user.
> I changed it to an assertion on the grounds that it is an internal "can't happen" error, rather than something a user might induce with malformed input.
A fuzzer could generate a file to trigger this error though, right?

(As an aside, we do need more compact ways of emitting error strings...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90614