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

[Greg McGary via Phabricator via llvm-commits]

gkm marked 5 inline comments as done.
gkm added inline comments.


================
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);
----------------
int3 wrote:
> pass by const ref?
`relocation_info` is a 64-bit struct that can pass-by-value in a register. Making it a reference introduces unnecessary indirection.


================
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());
----------------
int3 wrote:
> pass by const ref
`relocation_info` is a 64-bit struct that can pass-by-value in a register. Making it a reference introduces unnecessary indirection.


================
Comment at: lld/MachO/InputFiles.cpp:227-236
+    // Note: X86 does not use *_RELOC_ADDEND because it can embed an
+    // addend into the instruction stream. On X86, a relocatable address
+    // field always occupies an entire contiguous sequence of byte(s),
+    // so there is no need to merge opcode bits with address
+    // bits. Therefore, it's easy and convenient to store addends in the
+    // instruction-stream bytes that would otherwise contain zeroes. By
+    // contrast, RISC ISAs such as ARM64 mix opcode bits with with
----------------
int3 wrote:
> int3 wrote:
> > love the explanation!
> > inconvenient and more costly at runtime.
> 
> I might be misunderstanding, but do you mean link time instead of runtime?
Better.


================
Comment at: lld/MachO/InputFiles.cpp:178
+
+  for (size_t i = 0; i < relInfos.size(); i++) {
+    const relocation_info pairedInfo = relInfos[i];
----------------
int3 wrote:
> 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
I did a quick audit for this in lld. The recommended LLVM style is used approx 1/3 of the time.


================
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];
----------------
int3 wrote:
> 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...)
No, the assertion is checking for malformed symbol-table entries. Local symbol numbering is not accessible from assembler source.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90614