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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 12 inline comments as done.
DiggerLin 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
----------------
Xiangling_L wrote:
> 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?
llvm-objdump do not support  --private= now.


================
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:
----------------
Xiangling_L wrote:
> Should we use xlclang here?
yes, of course, we can use xlclang here, but  I think using xlc is ok too.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:203
+  TracebackTable::LanguageID LangId =
+      static_cast<TracebackTable::LanguageID>(TbTable.getLanguageID());
+  OS << "\t# Language = " << getNameForTracebackTableLanguageId(LangId) << "\n";
----------------
Xiangling_L wrote:
> 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;
> ```
of course , we can do as your suggestion. but I prefer use raw data type uint8_t as return type of getLanguageID() , and if we change the interface , there still need static_cast<TracebackTable::LanguageID> in the getLanguageID() , if you strong suggest it, I can change the interface. 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:291
+
+      if (!HasPrinted) {
+        OS << "\t# FunctionName = " << TbTable.getFunctionName().getValue();
----------------
Xiangling_L wrote:
> Is that intentional that we omit printing out function name after the first no more than 4 bytes?
We do not omit printing out the function name after the first no more 4 bytes.
what we do here is: 
For hex, we print every four bytes in a line for function name . we print out whole function name in string  in the first line of hex format as comment.
for example:

CHECK-NEXT:      da: 66 6f 6f 6f        # FunctionName = fooooo
CHECK-NEXT:      de: 6f 6f
CHECK-NEXT:      e0: 1f                    # AllocaRegister = 31


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:322
+    printOutBytes(4);
+    OS << "\t# VectorParmsInfoString = " << VecExt.getVectorParmsInfo();
+  }
----------------
Xiangling_L wrote:
> This line is causing build failure when I build. Should it be `getVectorParmsInfoString`?
the patch has parent patch. https://reviews.llvm.org/D93659 , I think  need to apply the parent patch before compile this patch.


================
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,
----------------
Xiangling_L wrote:
> May I ask why we don't put these two functions in objdump namespace as well?

the call stack as 
PrettyPrinter::printInst() ->printRawData() ->getInstStartColumn()

the class PrettyPrinter is defined in anonymous  namespace  and  these two new function only used by the class PrettyPrinter. So I define two function as the same namespace as  PrettyPrinter .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049