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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

I've marked comments (all minor) that have not yet been addressed.



================
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.
----------------
hubert.reinterpretcast wrote:
> The typo, "refrenced", is still here.
Still not seeing the change: https://reviews.llvm.org/D67008?id=222860#inline-616695


================
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
----------------
hubert.reinterpretcast wrote:
> Still missing the hyphen for "non-modifiable".
Still not seeing the change: https://reviews.llvm.org/D67008?id=222860#inline-616694


================
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.
----------------
hubert.reinterpretcast wrote:
> Either remove the "the" for this line or add "relocation" after "R_BA".
Still not seeing the change: https://reviews.llvm.org/D67008?id=222860#inline-616692


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:165
+};
 class XCOFFObjectFile : public ObjectFile {
 private:
----------------
Blank line between the class definitions please.


================
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
----------------
hubert.reinterpretcast wrote:
> 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.
Still not seeing the change: https://reviews.llvm.org/D67008?id=222237#inline-613037


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:593
+                                      Sec.FileOffsetToRelocationInfo);
+  auto RelocEntNumOrErr = getLogicalNumberOfRelocationEntries(Sec);
+  if (Error E = RelocEntNumOrErr.takeError())
----------------
hubert.reinterpretcast wrote:
> Suggestion: `NumRelocEntriesOrErr`
Still not seeing the change: https://reviews.llvm.org/D67008?id=222860#inline-616702


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:597
+
+  uint32_t RelocEntNum = RelocEntNumOrErr.get();
+
----------------
hubert.reinterpretcast wrote:
> Suggestion: `NumRelocEntries`
Still not seeing the change: https://reviews.llvm.org/D67008?id=222860#inline-616704


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008