[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.test:6
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-objdump -D --traceback-table --symbol-description %t.o | \
+# RUN:   FileCheck --match-full-lines --strict-whitespace %s
----------------
Is it important that this test uses `--disassemble-all`? `-d` is the short form of `--disassemble`.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:8
+# RUN:   FileCheck --match-full-lines --strict-whitespace %s
+# RUN: llvm-objdump --traceback-table --symbol-description %t.o | \
+# RUN:   FileCheck --match-full-lines --strict-whitespace %s
----------------
Probably worth a comment like "## Show that --traceback-table implies --disassemble."


================
Comment at: llvm/tools/llvm-objdump/ObjdumpOpts.td:70-73
+def traceback_table : Flag<["--"], "traceback-table">,
+  HelpText<"Decode traceback table in disassembly. This "
+           "option is for XCOFF files only and implies --disassemble"
+           " for XCOFF object file.">;
----------------
Couple of minor nits. Firstly, the help text shouldn't end with ".". Secondly, I think the "implies" bit should be in a different sentence to the "for XCOFF" bit, i.e. "Decode traceback table in disassembly. Implies --disassemble. This option is for XCOFF files only".


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:143
+  printRawData(Bytes.slice(Index, 4), Address, OS, STI);
+  OS << "\t# Traceback table begin\n";
+  Index += 4;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > "begin" is a verb meaning "to start something". "start" is what you are looking for.
> I think it maybe better to consistent with https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1991
> 
> if you strong suggestion to change to "start" , I can add a NFC for both after the patch commit.
Unfortunately, you didn't use a permalink, so since you included that link, the file has changed and I don't know what you're referring to. It might be you're referring to this line: https://github.com/llvm/llvm-project/blob/48027f03f2d5fef884ebe118c42f800b90fae2f9/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1987, but if so, that example is of use of it as a verb. Either way, grammatically, I am correct (citation: https://dictionary.cambridge.org/dictionary/english/begin, which lists only "verb" forms of "begin"), and this piece of code should be modified before committing anything (don't land something broken in the first place, even if it's not going to have a functional effect).


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:283
+
+    assert(FunctionNameLen > 0 &&
+           "The length of the function name must be greater than zero.");
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Should this assertion be an actual error? In other words, if the function name length stored was actually 0 (in a malformed object), would this code get here?
> I think I can keep  the assert here, If we want to check the "FunctionNameLen==0   and  isFuncNamePresent is true" malform. we can implement it in function XCOFFTracebackTable::XCOFFTracebackTable() as a new patch.(Current patch is big patch now).
> 
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Object/XCOFFObjectFile.cpp#L1443 
> 
> 
>   if (Cur && isFuncNamePresent()) {
>     uint16_t FunctionNameLen = DE.getU16(Cur);
>     if (Cur)
>       FunctionName = DE.getBytes(Cur, FunctionNameLen);
>   }
> 
We shouldn't land known broken code, and an assertion that can be hit by user code is broken. Whether that means adding the check in a prerequisite patch or this one depends on the solution you choose to go with.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049