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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 14 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:140-146
+#define SPLIT                                                                  \
+  OS << "\n";                                                                  \
+  OS.indent(TabStop)
+
+#define PRINTOUTBYTES(N)                                                       \
+  formatTracebackTableOutput(Bytes.slice(Index, N), Address + Index, OS);      \
+  Index += N;
----------------
jasonliu wrote:
> If you don't need the convenient of a macro, could you write them in lambda's instead?
> For example, the SPLIT and PRINTOUTBYTES.
I change the PRINTOUTBYTES to lambda, but I can not see a benefit of change SPLIT from Macro to lambda


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:164-169
+    outs().flush();
+    WithColor::warning(errs(), "")
+        << " Parse traceback table failure(The value in error msg is "
+           "calculated based on traceback begin) \n"
+        << "\t" << toString(TTOrErr.takeError()) << " from traceback begin."
+        << "\n\tThe raw data of traceback table as : \n";
----------------
jhenderson wrote:
> Use `objdump::reportWarning` rather than printing to the warning stream directly, I recommend. It does the flushing and printing to the `errs()` stream itself.
> 
> If for some reason, you want to deliberately differ to what llvm-objdump does normally when printing warnings and not print the tool name, you could add an optional argument to `reportWarning` to suppress it.
> 
> The warning message you are printing itself needs a number of grammar fixes, but I'm not familiar enough with XCOFF terminology to have a reasonable recommendation. Perhaps the following (note included whitespace and capitalization fixes in the message):
> 
> `Twine("traceback table parsing failure\n>>> ") + toString(TTOrErr.takeError()) + " from the start of the traceback table.\n>>> Raw traceback table data is:"`
> 
> I'm hesitant about including the "from the start of the traceback table" bit, because it bakes in an assumption about what the underlying error message will look like, which may no longer apply in the future if the message changes. Personally, I'm not sure it's needed, and would be inclined to remove it.
> 
> Rather than using hard tabs in the output, I've followed an approach taken from LLD's error message styling too. Hard tabs are bad and should be avoided in my opinion, because the output will appear different depending on people's window settings.
> 
> I've omitted the "The value in..." bit, as I'm really not sure what value it's referring to here, and I suspect a user might get more confused by it than not, but if you want to include it, I'd add it as a note after the first line, saying "note: the value in ...".
> 
> Note also that `reportWarning` includes a trailing new line, so there is no need to explicitly add one, just as it also does `outs().flush()`.
There is filename in the reportWarning in the llvm-objdump, I do not want the file name show in the warning message .  I added a new function "AddOffsetToValueInString" to convert the error message with the address from begin of object file.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:202
+  PRINTOUTBYTES(1)
+  OS << "# Version = " << (int)TbTable.getVersion() << "\n";
+
----------------
jasonliu wrote:
> What's the reason for the cast here?
> If you really need the cast, please use static_cast instead of a c-style cast.
OS only accept following 
 raw_ostream &operator<<(unsigned long N);
  raw_ostream &operator<<(long N);
  raw_ostream &operator<<(unsigned long long N);
  raw_ostream &operator<<(long long N);
  raw_ostream &operator<<(const void *P);

  raw_ostream &operator<<(unsigned int N) {
    return this->operator<<(static_cast<unsigned long>(N));
  }

  raw_ostream &operator<<(int N) {
    return this->operator<<(static_cast<long>(N));
  }

  raw_ostream &operator<<(double N);


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:214
+  PRINTOUTBYTES(1)
+  PRINTBOOL("#", TbTable, isGlobalLinkage);
+  PRINTBOOL(",", TbTable, isOutOfLineEpilogOrPrologue);
----------------
jasonliu wrote:
> Do you need to pass `TbTable` into PRINTBOOL and PRINTGET? It seems you always pass TBTable right now, why not just hardcode it then?
TBVectorExt  uses the macro too


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:356-357
+  OS << "\n";
+  if (End - Address + Index >= 8)
+    OS << "\t\t...\n";
+  else {
----------------
jasonliu wrote:
> `if (End - Address + Index >= 8)`
> Do you actually meant to have `if (End - Address - Index >= 8)` ?
> 
> Also, it might not be safe to skip the bytes printing when the remaining bytes are larger than 8. As the following bytes might not be all zeros. In that case, we should print out the bytes. 
thanks.  done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049