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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:164
+static constexpr uint8_t XR_FIXUP_INDICATOR_MASK = 0x40;
+// The remaining bits specify the bit length of the relocatable reference
+// minus one.
----------------
hubert.reinterpretcast wrote:
> Same here.
changed .


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:168
+
+bool isRelocationSigned(XCOFFRelocation32 &Reloc);
+
----------------
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.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:264
                              SmallVectorImpl<char> &Result) const override;
-
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
----------------
hubert.reinterpretcast wrote:
> Leave the blank line that was there before this patch.
changed


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:319
   void checkSymbolEntryPointer(uintptr_t SymbolEntPtr) const;
+  // Relocation-related interfaces.
+  uint32_t getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec,
----------------
hubert.reinterpretcast wrote:
> Insert a blank line above the comment.
added


================
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:
> 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.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:561
+uint32_t XCOFFObjectFile::getLogicalNumberOfRelocationEntries(
+    const XCOFFSectionHeader32 &Sec, uint16_t SectionIndex) const {
+  if (Sec.NumberOfRelocations < USHRT_MAX)
----------------
hubert.reinterpretcast wrote:
> No need to pass in the index. The index can be retrieved from within this function.
good idea.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:562
+    const XCOFFSectionHeader32 &Sec, uint16_t SectionIndex) const {
+  if (Sec.NumberOfRelocations < USHRT_MAX)
+    return Sec.NumberOfRelocations;
----------------
hubert.reinterpretcast wrote:
> `RELOC_OVERFLOW `. `USHRT_MAX` does not need to be 65535.
changed.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:564
+    return Sec.NumberOfRelocations;
+  else {
+    for (const auto &Sec : sections32()) {
----------------
hubert.reinterpretcast wrote:
> No need for `else` here. Just place the `else` logic at the same level as the `if`.
thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008