[PATCH] D80854: [lld-macho] Properly handle & validate relocation r_length

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

This is great!



================
Comment at: lld/MachO/Arch/X86_64.cpp:58
+
+  std::string msg = getErrorLocation(mb, sec, rel) + ": relocations of type " +
+                    std::to_string(rel.r_type) + " must have r_length of ";
----------------
It's an error path so efficiency doesn't matter, but in general, Twine is a better choice for concatenations like this.


================
Comment at: lld/MachO/Arch/X86_64.cpp:76
   case X86_64_RELOC_BRANCH:
+    // TODO: Make test case for X86_64_RELOC_BRANCH with r_length = 0
+    validateLength(mb, sec, rel, {0, 2});
----------------
It might be better to disallow the 0 case for now, and comment that we're diverging from ld64's behavior there. That way we'll get a loud error if we do end up seeing that case in the wild, and we can figure out what's going on with it then :)

Short jumps are a thing (where the offset is a single byte), but I can't imagine when we'd emit a relocation for one :/


================
Comment at: lld/MachO/Arch/X86_64.cpp:124
     // since the RIP has advanced by 4 at this point.
-    write32le(loc, val - 4);
+    val -= 4;
     break;
----------------
In particular, this is incorrect if we're allowing the r_length for an X86_64_RELOC_BRANCH to be 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80854