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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:240
   uint16_t getOptionalHeaderSize() const;
-  uint16_t getFlags() const { return FileHdrPtr->Flags; };
+  uint16_t getFlags() const;
+
----------------
I prefer to keep as uint16_t getFlags() const { return FileHdrPtr->Flags; };


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:125
 void XCOFFObjectFile::moveSymbolNext(DataRefImpl &Symb) const {
+  assert(!is64Bit() && "Symbol table support not implemented for 64-bit.");
   const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
----------------
there are assert(!is64Bit() && "Symbol table support not implemented for 64-bit."); in the toSymbolEntry() , I do not think we need  assert(!is64Bit() && "Symbol table support not implemented for 64-bit."); here. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:134
 Expected<StringRef> XCOFFObjectFile::getSymbolName(DataRefImpl Symb) const {
+  assert(!is64Bit() && "Symbol table support not implemented for 64-bit.");
   const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
----------------
ditto


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:186
 XCOFFObjectFile::getSymbolSection(DataRefImpl Symb) const {
+  assert(!is64Bit() && "Symbol table support not implemented for 64-bit.");
   const XCOFFSymbolEntry *SymEntPtr = toSymbolEntry(Symb);
----------------
ditto


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+
+uint32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const {
+  return is64Bit() ? toSection64(Sec)->Flags : toSection32(Sec)->Flags;
----------------
it looks the flag as 
  support::big32_t Flags
in the XCOFFObjectFile.h
  
uint32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const
should be int32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:501
   // Get pointer to the symbol table.
-  CurPtr = FileHdrPtr->SymbolTableOffset;
+  CurPtr = fileHeader32()->SymbolTableOffset;
   uint64_t SymbolTableSize = (uint64_t)(sizeof(XCOFFSymbolEntry)) *
----------------
if is64Bit(). it should return here.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:142
+    W.printNumber("NumberOfLineNumbers", Sec.NumberOfLineNumbers);
+    W.printHex("Flags", Sec.Flags);
+  }
----------------
do you want to try to use W.printEnum to output Sec.Flags, it will be more readable


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:145
+
+  if (opts::SectionRelocations)
+    llvm_unreachable("Dumping section relocations is unimplemented");
----------------
do we need to print relocation here ?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:148
+
+  if (opts::SectionSymbols)
+    llvm_unreachable("Dumping symbols is unimplemented");
----------------
ditto


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:151
+
+  if (opts::SectionData)
+    llvm_unreachable("Dumping section data is unimplemented");
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63843