[PATCH] D67008: [llvm-readobj][XCOFF]implement parsing relocation information for 32-bit xcoff objectfile
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 11 10:33:20 PDT 2019
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
More information about the llvm-commits
mailing list