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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:192
+               ///< displacement that is the difference between the address of
+               ///< the refrenced symbol and the address of the refrenced branch
+               ///< instruction. References a non modifiable instruction.
----------------
The typo, "refrenced", is still here.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:193
+               ///< the refrenced symbol and the address of the refrenced branch
+               ///< instruction. References a non modifiable instruction.
+  R_RBA = 0x18, ///< Branch absolute relocation. Similar to the R_BA but
----------------
Still missing the hyphen for "non-modifiable".


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:194
+               ///< instruction. References a non modifiable instruction.
+  R_RBA = 0x18, ///< Branch absolute relocation. Similar to the R_BA but
+                ///< references a modifiable instruction.
----------------
Either remove the "the" for this line or add "relocation" after "R_BA".


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:168
+
+bool isRelocationSigned(XCOFFRelocation32 &Reloc);
+
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Do these need to be declared in the header? Are they called only in one `.cpp` file? If so, they can be made `static` in the `.cpp` file. Otherwise, it seems odd that these aren't `const` member functions of `XCOFFRelocation32`.
> the llvm-readobj is using those function and obj2yaml will use them too.
It is still odd to me that these aren't `const` non-static member functions of `XCOFFRelocation32`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:556
 }
+// In an XCOFF32 file, if more than 65,534 relocation entries are required,
+// the field value will be 65535, and an STYP_OVRFLO section header will
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > We can reduce the amount of background for the comment to what is necessary to understand the code here:
> > In an XCOFF32 file, when the field value is 65535, then an STYP_OVRFLO section header contains the actual count of relocation entries in the s_paddr field. STYP_OVRFLO headers contain the section index of their corresponding sections as their raw "NumberOfRelocations" field value.
> added.
I am not seeing the change.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:593
+                                      Sec.FileOffsetToRelocationInfo);
+  auto RelocEntNumOrErr = getLogicalNumberOfRelocationEntries(Sec);
+  if (Error E = RelocEntNumOrErr.takeError())
----------------
Suggestion: `NumRelocEntriesOrErr`


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:597
+
+  uint32_t RelocEntNum = RelocEntNumOrErr.get();
+
----------------
Suggestion: `NumRelocEntries`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008