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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 9 inline comments as done.
sfertile 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;
+
----------------
DiggerLin wrote:
> I prefer to keep as uint16_t getFlags() const { return FileHdrPtr->Flags; };
We can't because of the underlying type depends on if its a 64-bit object or a 32-bit object, do you mean you would prefer if 

```
return is64Bit() ? fileHeader64()->Flags : fileHeader32()->Flags;
```

was inline here?


================
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);
----------------
DiggerLin wrote:
> 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. 
In the spirit of trying to keep this patch to a minimum I only implemented section headers and file-header support. I put defensive assert to indicate that I hadn't evaluated any of the symbol related interfaces for 64-bit. While this would technically work for 64-bit, that is only because the 64-bit symbol table entrey happens to also be 18 bytes of data. Casting a 64-bit symbol table entry to an `XCOFFSymbolEntry` is still wrong as they have different layouts.


================
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);
----------------
DiggerLin wrote:
> ditto
Again, the 64-bit and 32-bit symbol table entries have different layouts, this function AFAICT would **not** work for a 64-bit Symbol table entry. (Its String table offset  member is at offset 8 instead of offset 4).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+
+uint32_t XCOFFObjectFile::getSectionFlags(DataRefImpl Sec) const {
+  return is64Bit() ? toSection64(Sec)->Flags : toSection32(Sec)->Flags;
----------------
DiggerLin wrote:
> 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
Good catch, updated.


================
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)) *
----------------
DiggerLin wrote:
> if is64Bit(). it should return here.
is64Bit() has already returned on line 495.


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


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


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:148
+
+  if (opts::SectionSymbols)
+    llvm_unreachable("Dumping symbols is unimplemented");
----------------
DiggerLin wrote:
> ditto
Yep, once dumping symbols from objdump is supported.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63843