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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't feel confident enough to be able to say whether various cases have been tested or not. I'd like to leave that to someone with more XCOFF knowledge.



================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:179-180
   // Two of the bits that haven't got used in the mask.
-  if (Flag & 0x06)
+  if (Flag & 0x06 || Flag == 0x0)
     Res += "Unknown ";
 
----------------
Is this change tightly linked to the rest of the patch, or could it be done first with an appropriate separate test case?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1463-1464
       VectorParmsNum = VecExt->getNumberOfVectorParms();
+      // Skip two bytes of padding after vector info.
+      DE.skip(Cur, 2);
     }
----------------
Do we have testing that these two bytes are now skipped (and which would have failed before because they weren't)?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1488
 
+    if (ExtensionTable.value() & ExtendedTBTableFlag::TB_EH_INFO) {
+      // eh_info displacement must be 4-byte aligned.
----------------
Noting that there needs to be test cases both with and without this flag.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll:109
+
+
+; OBJ-DIS:           9c: 00 00 00 00    # Traceback table begin
----------------
Nit: double blank line.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test:6
+# RUN: llvm-objdump -D --traceback-table --symbol-description %t.o 2>&1 | \
+# RUN:   FileCheck --implicit-check-not warning: --check-prefixes=WARN %s
+
----------------
It's not strictly required, but I think the suggested edit would improve readability.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:2
+## Test that "llvm-objdump --traceback-table" decodes the ControlledStorageInfo,
+## AllocaRegister, and extension table of the traceback table which cannot
+## currently be generated by llc.
----------------
Sorry, missed this last time.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:7
+# RUN: llvm-objdump -D --traceback-table --symbol-description %t.o | \
+# RUN:   FileCheck %s
+
----------------
As you've got lots of whitespace formatting as part of your changes, you should use `--strict-whitespace` and `--match-full-lines` in your FileCheck command.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:122
+#define PRINTGET(Prefix, Obj, Field)                                           \
+  OS << Prefix << " " << #Field << " = " << (unsigned)(Obj.get##Field())
+
----------------
Prefer `static_cast` to C-style casts.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:124-126
+#define SPLIT                                                                  \
+  OS << "\n";                                                                  \
+  OS.indent(TabStop)
----------------
This should be a function, not a macro.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:160
+
+    uint64_t LastNoZero = Index;
+
----------------
Also move it to the line immediately before the `for` loop a bit below where it is updated.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:162-163
+
+    // The value of Size has been changed in function
+    // XCOFFTracebackTable::create().
+    Size = End - Address;
----------------



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:166
+
+    for (uint64_t I = Index; I < Size; I = I + 4) {
+      if (support::endian::read32be(Bytes.slice(I, 4).data()) != 0)
----------------



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:182
+    // Print all remaining zero as ...
+    if (Size - LastNoZero >= 8)
+      errs() << "\t\t...\n";
----------------
Why 8?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:381
+  printRawData(Bytes.slice(Index, AlignmentLen), Address + Index, OS, STI);
+  OS << "\t# Paddings\n";
+  Index += AlignmentLen;
----------------
"Padding" -> "Padding" or "Padding bytes"


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:385
+    printRawData(Bytes.slice(Index, 4), Address + Index, OS, STI);
+    OS << "\t# Padding\n";
+    Index += 4;
----------------
I'm not sure you really need this comment, given you've already labelled the previous bit with "Padding".


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:12-14
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Object/XCOFFObjectFile.h"
+#include "llvm/Support/FormattedStream.h"
----------------
Can you use forward declarations instead of additional includes here?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:162
 } // end namespace llvm
-
 #endif
----------------
Why have you deleted this blank line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049