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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:158
+  R_TOC = 0x03,
+  R_TRL = 0x12,
+  R_TRLA = 0x13,
----------------
sfertile wrote:
> 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?
I think it is Doc wrong, The value in the patch come from aix OS header file . I  opened ticket to confirm it.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:165
+  R_RL = 0x0c,
+  R_RLA = 0x0d,
+  R_REF = 0x0f,
----------------
sfertile wrote:
> Minor nit: IIUC `R_RL` and 'R_RLA` are handled the same as `R_POS`, maybe we should keep them together.
the enum is keep in value order, I am prefer to keep them as now. 


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:170
+  R_RBA = 0x18,
+  R_RBR = 0x1a,
+
----------------
sfertile wrote:
> Missing `R_BR` here.
good catch, added it , thanks


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:176
+  R_TLS_LE = 0x23,
+  R_TLSM = 0x24,
+
----------------
sfertile wrote:
> Missing `R_TLSML`.
good catch, thanks


================
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;
----------------
sfertile wrote:
> 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. 
yes, all template here will be used to support XCOFFRelocation32 and XCOFFRelocation64. we support 32bits now, we will support 64bits later.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:499
+
+  if (Index > getLogicalNumberOfSymbolTableEntries32())
+    return errorCodeToError(object_error::invalid_symbol_index);
----------------
sfertile wrote:
> `>=`
yes, you are correct, symbol index are from zero.


================
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;
----------------
sfertile wrote:
> Minor nit: Take the concrete type, and we can change to a templated function when we land 64-bit relocation support.
we will have a 64bits support later, I am prefer to keep the templated function here. if you strong need to use a concrete type now, I will do it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008