[PATCH] D67008: implement parsing relocation information for 32-bit xcoff objectfile

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:158
+  R_TOC = 0x03,
+  R_TRL = 0x12,
+  R_TRLA = 0x13,
----------------
This is 0x4 in the [[ https://www.ibm.com/support/knowledgecenter/en/ssw_aix_71/filesreference/XCOFF.html#XCOFF__sua3i125jbau | XCOFF Object Format Docs]], do they need to be updated or is the patch wrong?


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:165
+  R_RL = 0x0c,
+  R_RLA = 0x0d,
+  R_REF = 0x0f,
----------------
Minor nit: IIUC `R_RL` and 'R_RLA` are handled the same as `R_POS`, maybe we should keep them together.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:170
+  R_RBA = 0x18,
+  R_RBR = 0x1a,
+
----------------
Missing `R_BR` here.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:176
+  R_TLS_LE = 0x23,
+  R_TLSM = 0x24,
+
----------------
Missing `R_TLSML`.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:173
+// the instruction the relocation refers to.
+template <typename T> bool isFixupIndicated(T Reloc) {
+  return Reloc.Info & XR_FIXUP_INDICATOR_MASK;
----------------
I imagine this (and `getRelocatedLength`) was templated to handle both `XCOFFRelocation32` and `XCOFFRelocation64` eventually? Lets just take an argument of ` XCOFFRelocation32` for now,  and template them once we land  64-bit object file relocation support. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:499
+
+  if (Index > getLogicalNumberOfSymbolTableEntries32())
+    return errorCodeToError(object_error::invalid_symbol_index);
----------------
`>=`


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:53
   static constexpr unsigned SectionFlagsReservedMask = 0x7;
+  template <typename T> void printRelocations(ArrayRef<T> Sections);
   const XCOFFObjectFile &Obj;
----------------
Minor nit: Take the concrete type, and we can change to a templated function when we land 64-bit relocation support.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008