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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks fine now, apart from a few small things, but someone familiar with XCOFF needs to review this too.



================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-disassemble.test:6
 
+## Show that disassembly is enabled by default for --traceback-table.
+# RUN: llvm-objdump %t.o --traceback-table  | FileCheck %s --implicit-check-not=Disassembly
----------------
Sorry, I should have mentioned this before: I reckon having a bit of extra info in the comment would be useful - see the inline edit.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:150
+  Expected<XCOFFTracebackTable> TTOrErr =
+      XCOFFTracebackTable::create(Bytes.data() + Index, Size, Is64Bit);
+
----------------
I'm going to assume that `XCOFFTracebackTable::create` actually needs to modify `Size` for some other use-case, rather than just taking it by value.

Rather than resetting the Size value a few lines down, I think it might be better to create a copy of the `Size` variable and pass that in, e.g:
```
// XCOFFTracebackTable::create modifies the size parameter, so ensure Size isn't changed.
uint64_t SizeCopy = End - Address;
Expected<XCOFFTracebackTable> TTOrErr =
    XCOFFTracebackTable::create(Bytes.data() + Index, SizeCopy, Is64Bit);
```


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:154
+    reportWarning(Twine("failure parsing traceback table with address: 0x") +
+                      utohexstr(Address) + " \n>>> " +
+                      toString(TTOrErr.takeError()) +
----------------
Don't print trailing whitespace.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:172
+
+    formatted_raw_ostream FOS(errs());
+    while (Index < LastNonZero) {
----------------
Sorry, I missed this earlier: I don't like how you print the warning using reportWarning, and then go on to add more content to stderr outside the reportWarning function. This bakes in an assumption that reportWarning actually prints the warning to stderr. This is fine for now, but imagine if somebody wanted to add a --suppress-warnings option to llvm-objdump, or another option that changed where warnings are printed to. In those cases, the main warning would be hidden, but not the body of the message.

Better would be to use a single local stream that includes the initial part of the message and to which the rest of the warning content is printed. You then finish off this block with a call to reportWarning, passing in the whole string you built up, with the complete message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049