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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:3
+
+# RUN: llvm-objdump -D --traceback-table  --symbol-description %p/Inputs/xcoff-traceback-table.o | \
+# RUN:   FileCheck %s
----------------
I noticed that, on AIX, we can do:
`objdump --private=traceback  xcoff-traceback-table.o` to print out traceback table alone.
But with llvm-objdump, we are not possible to do that since we emit traceback table info alone with each byte.
I am wondering are we intentional to not support using `--private=traceback` alone with llvm-objdump?


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:7
+## xcoff-traceback-table.o compiled with IBM XL C/C++ for AIX, V16.1.0
+## compiler command: xlc -o xcoff-traceback-table.o -c foo.c
+## foo.c:
----------------
Should we use xlclang here?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:106
+    while (Pos < StrMsg.size() && isdigit(StrMsg[Pos]))
+      Pos++;
+    std::string NumString = StrMsg.substr(PrePos, Pos - PrePos);
----------------
minor: Please use `++Pos` to avoid extra copy of variable value.


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


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:203
+  TracebackTable::LanguageID LangId =
+      static_cast<TracebackTable::LanguageID>(TbTable.getLanguageID());
+  OS << "\t# Language = " << getNameForTracebackTableLanguageId(LangId) << "\n";
----------------
I am wondering what's the point of getting a `uint8_t` and cast it to `TracebackTable::LanguageID`? Can we adjust the interface to the following?
```
TracebackTable::LanguageID getLanguageID() const;
```


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:205
+  OS << "\t# Language = " << getNameForTracebackTableLanguageId(LangId) << "\n";
+  // Print out the third byte of 8 bytes of mandatory fields.
+  printOutBytes(1);
----------------
minor: also add a blank line above this line


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:276
+    assert(FunctionNameLen > 0 &&
+           "The length of function name must large than zero.");
+
----------------
minor:



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:291
+
+      if (!HasPrinted) {
+        OS << "\t# FunctionName = " << TbTable.getFunctionName().getValue();
----------------
Is that intentional that we omit printing out function name after the first no more than 4 bytes?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:322
+    printOutBytes(4);
+    OS << "\t# VectorParmsInfoString = " << VecExt.getVectorParmsInfo();
+  }
----------------
This line is causing build failure when I build. Should it be `getVectorParmsInfoString`?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:329
+    ExtendedTBTableFlag Flag =
+        (ExtendedTBTableFlag)TbTable.getExtensionTable().getValue();
+    OS << "\t# ExtensionTable = " << getExtendedTBTableFlagString(Flag);
----------------
The same question as Jason already mentioned? Please use c++ style cast `static_cast` if you have to. And also is that possible to adjust the return value of `getExtensionTable()` to `ExtendedTBTableFlag` enum type?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:338
+
+  // Print out all padding.
+  OS << "\n";
----------------
minor: s/padding/paddings


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:351
+
+  uint16_t AlignmentLen = 4 - Index % 4;
+  printRawData(Bytes.slice(Index, AlignmentLen), Address + Index, OS, STI);
----------------
Can we add testcase to get line 351 - line 359 get tested?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:155
+using namespace llvm;
+unsigned getInstStartColumn(const MCSubtargetInfo &STI);
+void printRawData(llvm::ArrayRef<uint8_t> Bytes, uint64_t Address,
----------------
May I ask why we don't put these two functions in objdump namespace as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049