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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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 
+
----------------



================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:4
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-objdump -d --traceback-table --symbol-description %t.o 2>&1| \
+# RUN:   FileCheck --implicit-check-not="warning:" --check-prefixes=WARN %s
----------------
Nit


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:14
+    SectionData:     "9421ffc0000000000000204080000201000000000000000400004164644e756d00000000"
+
+Symbols:
----------------
Nit: normally, there isn't blank lines between parts of YAML.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test:35
+
+# WARN:     {{.*}}: warning: '{{.*}}': The length of the function name must be greater than zero if the isFuncNamePresent is set in the trace backbak table.
----------------
Please a) review the error message content and fix it (I'm not going to point it out - it should be obvious to you with a quick look), b) review the LLVM coding standards for formatting of error and warning messages and fix accordingly. If you're still not sure what needs to be changed, please ask, but I don't want to need to explain everything in detail when things should be obvious.

There's no need for the extra space indentation at the start of the CHECK line, i.e. it should be `# WARN: {{.*}}` etc. Also, there's no real need for the first `{{.*}}: ` before `warning:`. Finally, it would be good to check the actual file name after `warning:`. You can add `-D FILE=%t.o` to the FileCheck invocation and then use `[[FILE]]` in place of the `{{.*}}` for the file path.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3231-3232
 
   if (DisassembleAll || PrintSource || PrintLines ||
       !DisassembleSymbols.empty())
     Disassemble = true;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049