[PATCH] D60784: [Object][XCOFF] Add intial support for parsing/dumping section header table.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:125
 
+  Expected<uint64_t> getStartAddress() const override;
 }; // XCOFFObjectFile
----------------
In keeping with the ordering of overriden functions in this file, this declaration should occur between `getFeatures` and `isRelocatableObject` as that is the ordering present in the definition of `class ObjectFile`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:40
+
+
 // Sets Obj unless any bytes in [addr, addr + size) fall outsize of m.
----------------
There is an extra blank line here in comparison to the number of blank lines used between the next few functions below.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:54
 
+template <typename T> const T *viewAs(uintptr_t in) {
+  return reinterpret_cast<const T *>(in);
----------------
This should be `static` or otherwise made to have internal linkage to avoid ODR violations.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:139
+            ? SH->Name
+            : StringRef(SH->Name, XCOFF::SectionNameSize);
   return std::error_code();
----------------
Do we want to use the version that avoids embedded NUL characters in this patch?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:181
+  unsigned Flags = toSection(Sec)->Flags;
+  return (Flags & XCOFF::STYP_DATA) || (Flags & XCOFF::STYP_TDATA);
 }
----------------
It seems `(Flags & (XCOFF::STYP_DATA | XCOFF::STYP_TDATA))` is a more common pattern in the codebase.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:186
+  unsigned Flags = toSection(Sec)->Flags;
+  return (Flags & XCOFF::STYP_BSS) || (Flags & XCOFF::STYP_TBSS);
 }
----------------
Same comment as above.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:310
+  if (getNumberOfSections() != 0) {
+    if (!checkSize(Data, EC, CurPtr,
+                   getNumberOfSections() * getSectionHeaderSize()))
----------------
`getObject` (called below) takes a `Size` parameter. It can be used to check for the right number of sections without this call to `checkSize`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60784