[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/Object/XCOFFObjectFile.h:168
+
+bool isRelocationSigned(XCOFFRelocation32 &Reloc);
+
----------------
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`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:555
                                         TablePtr + getNumberOfSections());
 }
+// In an XCOFF32 file, if more than 65,534 relocation entries are required,
----------------
Please add a blank line after the function body here.


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


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


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


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


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:571
+  }
+  report_fatal_error("Can not find sepcific relocation overflow section.");
+}
----------------
This is not an internal-error situation. A different error handling mechanism is needed.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:577
+                             uint16_t SectionIndex) const {
+  if (is64Bit())
+    report_fatal_error("64-bit relocation support not implemented yet.");
----------------
Why a `is64Bit` check in a file that needs an `XCOFFSectionHeader32` in order to call it?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:584
+  auto RelocationOrErr = getObject<XCOFFRelocation32>(
+      Data, (void *)RelocAddr,
+      Sec.NumberOfRelocations * sizeof(XCOFFRelocation32));
----------------
Write the cast out as `reinterpret_cast`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:585
+      Data, (void *)RelocAddr,
+      Sec.NumberOfRelocations * sizeof(XCOFFRelocation32));
+  if (Error E = RelocationOrErr.takeError())
----------------
This also needs to use the logical number of relocation entries.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67008