[PATCH] D89049: [AIX][XCOFF] print out the traceback info

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:108
+
+  unsigned TabStop = NoShowRawInsn ? 16 : 40;
+  unsigned Column = OS.tell() - Start;
----------------
This should be 24 for non-X86 platform. Right now, our traceback table printing starts later in the column than other instructions, and this is why. 
The other thing is, since this function's content is taken entirely from `PrettyPrinter`'s `printInst`, have you thought about making this function callable for both the traceback table usage and `printInst`'s usage? For example, rename this function and make it a member function of `PrettyPrinter`, then pass the `PrettyPrinter` into `dumpTracebackTable`. Or just rename it and move it to common code of llvm-objdump, and call this function from `printInst`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:114
+StringRef getExtendedTBTableFlagString(ExtendedTBTableFlag Flag) {
+  switch (Flag) {
+  default:
----------------
The switch logic here doesn't work. This flag could represent multiply settings at the same time. 
For example, it could have TB_LONGTBTABLE2 and TB_SSP_CANARY at the same time.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:129
+}
+const char *SourceLanguageIdentifier[] = {
+    "C",     "FORTRAN", "Pascal",    "ADA",      "PL/I",
----------------
I would imagine this is something we are going to need in the encoding part as well. Should we put this as an enum in XCOFF.h.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:140-146
+#define SPLIT                                                                  \
+  OS << "\n";                                                                  \
+  OS.indent(TabStop)
+
+#define PRINTOUTBYTES(N)                                                       \
+  formatTracebackTableOutput(Bytes.slice(Index, N), Address + Index, OS);      \
+  Index += N;
----------------
If you don't need the convenient of a macro, could you write them in lambda's instead?
For example, the SPLIT and PRINTOUTBYTES.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:151
+  uint64_t Index = 0;
+  unsigned TabStop = (NoShowRawInsn ? 16 : 40) - 1;
+
----------------
Call getInstStartColumn(STI) to make the TabStop consistent between TracebackTable printing and other instruction printing. 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:201
+  // Print out the first byte of 8 bytes of mandatory fields.
+  PRINTOUTBYTES(1)
+  OS << "# Version = " << (int)TbTable.getVersion() << "\n";
----------------
nit: could we add `;` to the end of the line even if it's a function-like macro that already had `;` at the end? 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:202
+  PRINTOUTBYTES(1)
+  OS << "# Version = " << (int)TbTable.getVersion() << "\n";
+
----------------
What's the reason for the cast here?
If you really need the cast, please use static_cast instead of a c-style cast.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:214
+  PRINTOUTBYTES(1)
+  PRINTBOOL("#", TbTable, isGlobalLinkage);
+  PRINTBOOL(",", TbTable, isOutOfLineEpilogOrPrologue);
----------------
Do you need to pass `TbTable` into PRINTBOOL and PRINTGET? It seems you always pass TBTable right now, why not just hardcode it then?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:269
+    OS << "# " << #Field << " = " << TbTable.get##Field().getValue();          \
+  }
+
----------------
minor nit: I would prefer this function-like macro to resides with other function-like macro at the beginning of this function. 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:318
+    OS << "# AllocaRegister = "
+       << (unsigned int)TbTable.getAllocaRegister().getValue();
+  }
----------------
Do you need the cast? If you really do, please use static_cast instead of c style cast.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:346
+        (ExtendedTBTableFlag)TbTable.getExtensionTable().getValue();
+    OS << "# ExtensionTable = " << getExtendedTBTableFlagString(Flag).data();
+  }
----------------
Do you need .data() ?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:356-357
+  OS << "\n";
+  if (End - Address + Index >= 8)
+    OS << "\t\t...\n";
+  else {
----------------
`if (End - Address + Index >= 8)`
Do you actually meant to have `if (End - Address - Index >= 8)` ?

Also, it might not be safe to skip the bytes printing when the remaining bytes are larger than 8. As the following bytes might not be all zeros. In that case, we should print out the bytes. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049