[PATCH] D81585: [AIX][XCOFF] Decode trace back table information for xcoff object file for llvm-objdump -d

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please run clang-format on all your new code. You're adding an awful lot in one go, and I can't believe that it's all got test cases. I think you'd be better to split it down into smaller pieces, perhaps only implementing a small part of the dumper functionality in each.

Is there an existing tool that the output format is supposed to match? I'm not particularly keen on the output as-is, so would like to suggest changes if you're not trying to match an existing style.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:288
+struct Traceback_Table {
+  // Byte 1
+  uint8_t Version; ///< Traceback format version
----------------
I'm not sure these "Byte 1" etc comments are useful. It's clear from the types that the first byte is `Version`, the second `LanguageId` etc.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:295
+  // Byte 3
+  unsigned GlobaLinkage : 1; ///< Set if this routine is GLobal Linkage.
+                             ///< specified, and the FP_PRESET bit is on.
----------------
is -> has


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:303
+  unsigned FP_Present : 1;   ///< Set to 1 if the function uses floating-point
+                             ///< processor instructions
+  unsigned Log_Abort : 1;    ///< Set if the log/abort compiler option was
----------------
Missing full stop.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:304
+                             ///< processor instructions
+  unsigned Log_Abort : 1;    ///< Set if the log/abort compiler option was
+
----------------
This comment looks incomplete - was what?
Also, missing full stop.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:316
+  // Byte 4
+  unsigned Int_Handler : 1;  ///< Set if routine is interrupt handler
+  unsigned Name_Present : 1; ///< Set if name is present in proc table.
----------------
Missing full stop.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:334
+  unsigned Stores_BC : 1; ///< Set if procedure stores the backchain.
+  unsigned Fixup : 1;     ///< Set if code is generated fixup code.
+  unsigned FP_Saved : 6;  ///< umber of FPRs saved, max of 32.
----------------
Is this saying the fixup code is generated, or that the code is making fixup code? If the latter, then use "generating" not "generated".


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:346
+  unsigned Spare4 : 1;
+  unsigned GP_SAVED : 6; ///< Number of GPRs saved, max of 32
+  static constexpr uint8_t Has_Vec_Info_Mask = 0x80;
----------------
Nit: missing full stop.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-tracebacktable.test:91
+CHECK-NEXT:             ...
+
----------------
Nit: don't need the extra blank line at table end.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:95
+bool objdump::isXCOFFTracebackTableBegin(ArrayRef<uint8_t> Bytes) {
+  // Tableback table begin with sizeof(long) bytes zero.
+  if (Bytes.size() == 4)
----------------
Xiangling_L wrote:
> nit: s/Tableback table/Traceback table?
begin -> begins


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:153
+  // Backtrace table version.
+  auto CurBytes = Bytes.slice(Index, 1);
+  formatTracebackTableOutput(CurBytes, Address + Index, OS);
----------------
Don't use `auto` here. The type isn't obvious from the immediate context.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:155
+  formatTracebackTableOutput(CurBytes, Address + Index, OS);
+  OS << "# Version = " << (int)CurBytes.front() << "\n";
+  Index++;
----------------
I'd prefer casting to `uint32_t`, since we're dealing with an unsigned type already.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:156
+  OS << "# Version = " << (int)CurBytes.front() << "\n";
+  Index++;
+
----------------
Prefer `++Index` here and below


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:169
+
+  // Decode the 3th byte of the backtrace table.
+  CurBytes = Bytes.slice(Index, 1);
----------------
3th -> 3rd


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:240
+
+  // Decode parameter type if there is.
+  if (Tb.NumberOfFixedPara || Tb.NumberOfFPPara) {
----------------
is -> is one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585