[PATCH] D63843: [Object][XCOFF] Add support for 64-bit file header and section header dumping.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:49
 
+static uintptr_t getWithOffset(uintptr_t Base, ptrdiff_t Offset) {
+  return reinterpret_cast<uintptr_t>(reinterpret_cast<const char *>(Base) +
----------------
`getWithOffset` does not seem useful: it just does a plus operation.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:67
+  if (Addr < TableAddress ||
+      Addr >= getWithOffset(TableAddress,
+                            getSectionHeaderSize() * getNumberOfSections()))
----------------
You may use `Offset >= getSectionHeaderSize() * getNumberOfSections()` instead.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:83
+#endif
+  return viewAs<XCOFFSectionHeader32>(Ref.p);
+}
----------------
I think inline reinterpret_cast may be clearer, then the `viewAs` helper can be deleted.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:130
   Symb.p = reinterpret_cast<uintptr_t>(SymEntPtr);
+  return;
 }
----------------
`return;` is redundant.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:364
 
-std::error_code
-XCOFFObjectFile::getSectionByNum(int16_t Num,
-                                 const XCOFFSectionHeader *&Result) const {
-  if (Num > 0 && static_cast<uint16_t>(Num) <= getNumberOfSections()) {
-    Result = SectionHdrTablePtr + (Num - 1);
-    return std::error_code();
-  }
+size_t XCOFFObjectFile::getFileHeaderSize() const {
+  return is64Bit() ? sizeof(XCOFFFileHeader64) : sizeof(XCOFFFileHeader32);
----------------
Consider making `XCOFFObjectFile` a class template.

See ELFObjectFile<ELFT> as an example, then you may avoid various is64Bit() dispatching in this file.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:478
   uint64_t CurPtr = 0;
-
-  if ((EC = getObject(FileHdrPtr, Data, base() + CurPtr)))
+  if (EC = getObject(FileHeader, Data, base() + CurPtr, getFileHeaderSize()))
     return;
----------------
`warning: using the result of an assignment as a condition without parentheses [-Wparentheses]`

I remember I said in the initial check-in of this file: lib/Object code is moving away from `std::error_code`, `ErrorOr` and `errorCodeToError`. `Error` `Expected` etc should be used instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63843