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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please make sure you've run clang-format and fix all clang-tidy warnings in your new code.



================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:114
+
+std::string AddOffsetToValueInString(const std::string &StrMsg,
+                                     unsigned Offset) {
----------------
Can this function be static too?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:126
+    std::string NumString = StrMsg.substr(PrePos, Pos - PrePos);
+    int num = std::stoi(NumString, 0, 16);
+    std::stringstream stream;
----------------
Please address these clang-tidy warnings by following LLVM coding standards.


================
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";
----------------
DiggerLin wrote:
> 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.
> 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.

You can follow this same technique to not print the file name too. Why don't you want the file name in the message though? If the error stream is being redirected to a file, you won't know where the warning is coming from if you are working with multiple files or archives.

Also, please fix the other whitespace and grammar issues, as I requested in my previous comment.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:202
+  PRINTOUTBYTES(1)
+  OS << "# Version = " << (int)TbTable.getVersion() << "\n";
+
----------------
DiggerLin wrote:
> 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);
`raw_ostream &operator<<(unsigned char C);` exists. Take a look at `raw_ostream.h`. However, it may print the value as a character, not as an integer. How about using the `format` function to format your strings, using printf formatting, and PRIx8 to ensure the right type is being used without the need for casting? DWARFDebugLine.cpp has some good examples of that sort of thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049