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

[Greg McGary via Phabricator via llvm-commits]

gkm marked an inline comment as done.
gkm 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;
----------------
compnerd wrote:
> 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?
I looked at some cases with [[ godbolt.org | godbolt.org ]] for paired vs. unpaired caller & callee. When the args are scalars, scalar replacement of aggregates works properly for callees, and functions that accept `foo(long a, long b)` vs. `foo(std::pair<long, long> x)` produce identical argument handling code. Note that `x` here is a value not a reference, otherwise there is extra indirection. When I looked beyond scalars toward `struct MachO::relocation_info`, I found that neither GCC nor LLVM could apply SRoA to both layers of pair + struct. On the caller side, there is overhead associated with making pairs. Sooo ... pairs come at //some// runtime cost.

The paired reloc is only valid when it has `r_type` of `*_RELOC_ADDEND` or `*_RELOC_SUBTRACTOR`. Otherwise it is ignored.

I fiddled writing it using `std::pair<>` and was not impressed that it improve clarity, while imposing runtime overhead.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90614