[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:77
+  support::ubig32_t BssDataSize;
+  support::ubig32_t EntryPointVirtualAddr;
+  support::ubig32_t TextStartAddr;
----------------
EntryPointVirtualAddr -> EntryPointAddr.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:90
+  support::ubig16_t ModuleType;
+  char CpuFlag;
+  char CpuType;
----------------
Why do we use `char` and not `uint8_t`?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:107
+                      ///< default value is 0 (system-selected page size).
+  char Flag;
+  support::ubig16_t SecNumOfTData;
----------------
Flag -> FlagAndTDataAlignment


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:142
+  support::ubig16_t SecNumOfTBSS;
+  support::ubig16_t X64bitsFlag;
+};
----------------
X64bitsFlag -> XCOFF64Flags


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:229
   const XCOFFSectionHeader64 *sectionHeaderTable64() const;
-
   size_t getFileHeaderSize() const;
----------------
Looks like the blank line here is for separating the functions. Why do we want to remove it?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:139
+const XCOFFAuxiliaryHeader64 *XCOFFObjectFile::AuxiliaryHeader64() const {
+  assert(is64Bit() && "64-bit interface called on a 64-bit object file.");
+  return static_cast<const XCOFFAuxiliaryHeader64 *>(AuxiliaryHeader);
----------------
64-bit interface called on a 32-bit object file.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:105
+    const XCOFFAuxiliaryHeader64 *AuxHeader64Prt = Obj.AuxiliaryHeader64();
+    printAuxiliaryHeaders(AuxHeader64Prt);
+  } else {
----------------
I don't think you need to define an extra variable here.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:494
+
+void XCOFFDumper::printAuxiliaryHeaders(const XCOFFAuxiliaryHeader32 *AuxHeader) {
+  if (AuxHeader == nullptr) {
----------------
Please consider combine 32 bit and 64 bit version of this function using template, as most of the fields have the same name. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:501
+  DictScope DS(W, "AuxiliaryHeader");
+  PrintAuxMember(Hex, "Magic", AuxHeader->AuxMagic, AuxSize);
+  PrintAuxMember(Hex, "Version", AuxHeader->Version, AuxSize);
----------------
Why do you need to pass in `AuxSize` to the macro function when all inputs are the same?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:594
+                 AuxSize);
+  PrintAuxMember(Number, "64 bits XCOFF Flag", AuxHeader->X64bitsFlag, AuxSize);
+}
----------------
We should print this as hex. 


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:178
+  // --auxiliary-headers
+  cl::opt<bool>
+      XCOFFAuxiliaryHeaders("auxiliary-headers",
----------------
I'm assuming we need to add it somewhere in the llvm docs about this new option. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549