[PATCH] D95121: [lld-macho][NFC] refactor relocation handling

[Greg McGary via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/InputFiles.cpp:238-260
+    uint64_t pairedAddend = 0;
+    relocation_info relInfo = relInfos[i];
+    if (target->hasRelocAttrBit(relInfo.r_type, RelocAttrBits::ADDEND)) {
+      pairedAddend = llvm::SignExtend64<24>(relInfo.r_symbolnum);
+      relInfo = relInfos[++i];
+    }
     assert(i < relInfos.size());
----------------
int3 wrote:
> ideally we would only add functionality in the diff where it gets tested, but I guess diff splitting is kinda hairy at times... can we at least note in the commit message that this gets tested in D88629?
`X86_64_RELOC_SUBTRACTOR` exists, so it can and should be tested in this diff. I added a test.


================
Comment at: lld/MachO/Target.h:38-39
+enum class RelocAttrBits {
+  _1 = 1 << 0,         // invalid 1 byte length
+  _2 = 1 << 1,         // invalid 2 byte length
+  _4 = 1 << 2,         // occupies 4 bytes
----------------
int3 wrote:
> why do we need to define these if they are invalid?
It's a minor expediency for checking validity of reloc length: `1 << r.length` vs. `1 << (r.length - 2)`


================
Comment at: lld/MachO/Target.h:50-51
+  GOT = 1 << 11,       // Has a Global Offset Table slot
+  TLV = 1 << 12,       // Has a Thread-Local Variable slot
+  TLS = 1 << 13,       // Can reside in Thread-Local Storage
+  MORPH = 1 << 14,     // morph instruction: load into add
----------------
int3 wrote:
> I think ld64 (and other bits of macOS source) use "TLV" in place of "TLS". Pointers to TLVs are abbreviated as `TLVP`s.
That's a real distinction, and apparently needs more commentary to avoid confusion:
TLV = //Thread-Local Variable// and pertains to the datum and its symbol.
TLS = //Thread-Local Storage// and pertains to the section that contains the TLVs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95121