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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

As a gentle reminder, please could you make sure you do a personal review of your own code before uploading a patch for review (including updates to an existing patch). If you are working in a company, it is generally a good idea to get a second person to take a look at your code within your company before pushing to the open source community for review. This will help reduce the number of silly mistakes, improving the time it takes to get a patch through review, due to fewer iterations of the patch being needed, and increasing the desire from LLVM community reviewers , such as myself, to review your code (which again will improve the time it takes to get a patch reviewed).



================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll:49
 
 ; COMMON:       .vbyte  4, 0x00000000                   # Traceback table begin
 ; COMMON-NEXT:  .byte   0x00                            # Version = 0
----------------
Please make a separate small commit to change the comment here that is emitted.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble.test:4
 # RUN: llvm-objdump %t.o -d | FileCheck %s --implicit-check-not=Disassembly
+# RUN: llvm-objdump %t.o --traceback-table  | FileCheck %s --implicit-check-not=Disassembly
 # RUN: llvm-objdump %t.o --disassemble-all | FileCheck %s --check-prefixes=CHECK,ALL
----------------
I think this case deserves a comment. To avoid confusing the issue, I'd move the RUN line to the end of the test (after the checks), and add a comment saying that this shows that disassembly is enabled by default for --traceback-table, or something to that effect.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:34
+
+# WARN: warning: '[[FILE]]': the length of the function name must be greater than zero if the isFuncNamePresent bit is set in the traceback table 
----------------
I think this line has trailing whitespace you can remove?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3239
       !DynamicSymbolTable && !UnwindInfo && !FaultMapSection && !Offloading &&
+      !TracebackTable &&
       !(MachOOpt &&
----------------
You don't need this anymore, because the `Disassemble` value is checked. See point 2 in my comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049