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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:507
+  uint64_t CurOffset = 0;
+  auto Base = Obj->base();
+  auto Data = Obj->Data;
----------------
`const auto *Base`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:508
+  auto Base = Obj->base();
+  auto Data = Obj->Data;
+
----------------
`MemoryBufferRef Data`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:522
+
+  // Pase section header table if present.
+  if (Obj->getNumberOfSections()) {
----------------
s/Pase/Parse/;

Suggestion:
Parse the section header table if it is present.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:564
+                                  unsigned FileType) {
+  assert((FileType == Binary::ID_XCOFF32 || FileType == Binary::ID_XCOFF64) &&
+         "Non-XCOFF file type passed to createXCOFFObjectFile!");
----------------
The `XCOFFObjectFile` constructor will assert as appropriate without this `assert` here.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:154
+    W.printHex("LineNumberPointer", Sec.FileOffsetToLineNumberInfo);
+    W.printNumber("NumberOfRelocations", Sec.NumberOfRelocations);
+    W.printNumber("NumberOfLineNumbers", Sec.NumberOfLineNumbers);
----------------
`STYP_OVRFLO`/`_OVERFLOW_MARKER` handling TODO here.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:160
+    else
+      W.printEnum("Type", Sec.Flags, makeArrayRef(SectionTypeFlagsNames));
+  }
----------------
Please test `STYP_DWARF`. It looks like the handling of DWARF section subtypes is missing. At the least, `Sec.Flags & 0xffU` might help.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:164
+  if (opts::SectionRelocations)
+    llvm_unreachable("Dumping section relocations is unimplemented");
+
----------------
These should be `report_fatal_error`.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:44
+  if (Obj.is64Bit())
+    report_fatal_error("64-bit XCOFF files not suported yet.");
+  YAMLObj.Header.SymbolTableOffset = Obj.getSymbolTableOffset32();
----------------
s/suported/supported/;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63843