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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:1
+## Test that "llvm-objdump --traceback-table" warn at FunctionNameLen=0 
+
----------------
jhenderson wrote:
> 
thanks.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3231-3232
 
   if (DisassembleAll || PrintSource || PrintLines ||
       !DisassembleSymbols.empty())
     Disassemble = true;
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > I'd be okay with moving the "if traceback table, enable --disassemble" logic to here, like the other cases, and dropping the "if XCOFF" check. It will simplify the code slightly, as you can then drop the `!TracebackTable` check from the below if clause. It also means that disassembly is printed for both XCOFF and non-XCOFF objects in the same invocation of llvm-objdump, which is good for consistency.
> > > 
> > > (If you do this, it might be worth adding a trivial test case showing that disassembly is enabled for non-XCOFF objects with --traceback-table).
> > Sorry that I can not understand the comment, I do not change code above your comment.
> In this version of the patch, at line 2834, you add a check for `(O->isXCOFF() && TracebackTable)` to cause disassembly to be printed. This is different to how other command-line options, such as `-S` (`PrintSource`) cause `printDisassembly` to be called, if `--disassemble` is specified. I think you should change how `--traceback-table` causes disassembly to be printed, to be the same as these other options.
> 
> By changing this, there are a few effects:
> 1) You can remove the change you made at line 2834.
> 2) You can remove the `!TracebackTable` check at line 3239, since it will be redundant.
> 3) If you use `--traceback-table` when you provide a non-XCOFF file, disassembly will be printed, even if the user hasn't specified `--disassemble`. There should be a new test case to show this behaviour.
> 
> Does that make things clearer?
thanks a lot for explain.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049