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

Saleem Abdulrasool via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 2 08:40:55 PST 2020


compnerd added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:29
+  bool isPairedReloc(const relocation_info) const override;
+  uint64_t getAddend(MemoryBufferRef, const section_64 &, const relocation_info,
+                     const relocation_info) const override;
----------------
I wonder if it makes sense to use a `std::pair<const relocation_info, const relocation_info> &`.  It should be tested, but I could see the compiler being smart enough to unpack the pair if possible, which just makes it much more clear that that the two parameters are the paired relocation.  It took me a moment to realize that.

What happens in the case of an unpaired relocation?


================
Comment at: lld/MachO/InputFiles.cpp:181
+    const relocation_info relInfo =
+        (target->isPairedReloc(pairedInfo) ? relInfos[++i] : pairedInfo);
+    assert(i < relInfos.size());
----------------
You could also do `std::next(&RI)` if you want to continue using the range based for loop.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90614



More information about the llvm-commits mailing list