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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added a comment.

In D89049#4276745 <https://reviews.llvm.org/D89049#4276745>, @jhenderson wrote:

> 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).

I understand your frustration. In fact, every time when I update, I conduct a self-review, but I still have blind spots. I will ask my internal team member to do some review and reduce your work. Sorry for the inconvenient.



================
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
----------------
jhenderson wrote:
> Please make a separate small commit to change the comment here that is emitted.
yes, I will do it in a separate patch.


================
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
----------------
jhenderson wrote:
> 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.
I put it after the "# RUN: llvm-objdump %t.o -d | FileCheck %s --implicit-check-not=Disassembly" because option  "--traceback-table"  implies enable -d or "--disassemble" not implies enable "--disassemble-all". 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3239
       !DynamicSymbolTable && !UnwindInfo && !FaultMapSection && !Offloading &&
+      !TracebackTable &&
       !(MachOOpt &&
----------------
jhenderson wrote:
> You don't need this anymore, because the `Disassemble` value is checked. See point 2 in my comment above.
thanks. actually, I take a reivew myself when I tried to update the patch, sorry for I do not find the error. thanks for your time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049