[PATCH] D67008: implement parsing relocation information for 32-bit xcoff objectfile
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 7 19:15:06 PDT 2019
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
More information about the llvm-commits
mailing list