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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

I like this enum-bit-based design :)



================
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());
----------------
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?


================
Comment at: lld/MachO/InputFiles.cpp:241
+    if (target->hasRelocAttrBit(relInfo.r_type, RelocAttrBits::ADDEND)) {
+      pairedAddend = llvm::SignExtend64<24>(relInfo.r_symbolnum);
+      relInfo = relInfos[++i];
----------------
no need for `llvm::`


================
Comment at: lld/MachO/InputFiles.cpp:253
+
+    Reloc p{0xff, 0, 0, 0, 0, 0};
+    if (target->hasRelocAttrBit(relInfo.r_type, RelocAttrBits::SUBTRAHEND)) {
----------------
we should put these as default field initializers in the struct definition. And perhaps we can also add a RelocationInfoType::INVALID enum value instead of using `0xff`


================
Comment at: lld/MachO/InputFiles.cpp:258
+      p.pcrel = false;
+      p.length = p.offset = p.addend = 0;
+      relInfo = relInfos[++i];
----------------
seems unnecessary


================
Comment at: lld/MachO/OutputSegment.cpp:49-50
   osec->parent = this;
+  if (osec->name == section_names::ehFrame)
+    return;
   sections.push_back(osec);
----------------
this change seems out of place in this diff


================
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
----------------
why do we need to define these if they are invalid?


================
Comment at: lld/MachO/Target.h:91
                                    uint8_t type) const = 0;
+  virtual void morphLoadIntoAdd(uint8_t *loc, uint8_t type) const = 0;
+
----------------
naming nit 1: I think the usual terminology is "relax", not "morph". LLD-ELF uses this at least.
nit 2: I guess "load" and "add" are the instructions on ARM, but the equivalent ones on x86 are "mov" and "lea", which makes this naming a bit confusing, especially since "lea" = *load* effective address. How about `relaxGotLoad()`?


================
Comment at: lld/MachO/Target.h:95
+    return relocAttrsByType[type].hasRelocAttrBit(bit);
+  }
+  bool validateRelocationInfo(llvm::MemoryBufferRef,
----------------
nit: add newline after multi-line function definition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95121