Page MenuHomePhabricator

[llvm-readobj] Improve error message for for --string-dump

Authored by StephenTozer on Mar 28 2019, 10:03 AM.



Fixes bug 40630:

This patch changes the error message when the section specified by
--string-dump cannot be found by including the name of the section in
the error message and changing the prefix text to not imply that the
file itself was invalid. As part of this change some uses of
std::error_code have been replaced with the llvm Error class to better
encapsulate the error info (rather than passing File strings around),
and the WithColor class replaces string literal error prefixes.

Diff Detail

Event Timeline

StephenTozer created this revision.Mar 28 2019, 10:03 AM

I've added a few more reviewers who have been reviewing stuff in this area recently. It would be good to get some other comments on the interface changes with regards to error message printing.


I assume this is testing the file name? If so could you do similar to what is done above, e.g. in basic.test.


I think you'd be better off using createStringError() here, which takes variadic arguments and formats in printf style.


Nit: Could you move this above the reportError immediately above, so that it lines up well in diffs with the old version, please?

rupprecht marked 2 inline comments as done.Mar 28 2019, 1:35 PM

I just have a couple nits on top of what James said, but overall this is already an improvement. Thanks!


Whatever is throwing this error should probably be creating a FileError
(I'm fine if this part is in a separate patch tho)


... completely unrelated to this patch, but is this even necessary? I thought stderr was always unbuffered. There will never be anything to flush.


This can go on the previous line, i.e. << Msg << "\n";


I'm guessing this is actually necessary in order to delegate to reportError(StringRef, Error)

grimar added inline comments.Mar 29 2019, 2:55 AM

It is strange to me to see the quotes wrapping the file name here.
I think tools usually do not do that?


You renamed EC->Err here, but the code below still use EC and this renaming
does not seem necessary (it looks as NFC). Not sure why it was done?

StephenTozer marked 2 inline comments as done.Mar 29 2019, 3:20 AM
StephenTozer added inline comments.

The SecName variable in this case is a StringRef, which doesn't work as nicely with printf-style formatting: it has an internal char*, but this is not necessarily null terminated, so would need to be converted to an intermediate string variable to be compatible. Since it has built-in formatting with formatv, I thought that the more appropriate choice.


The move is messy, but since this function now calls the other reportError function it has to be below it. The two alternatives to changing the function order would be to declare the above function earlier in this .cpp file, or declare it in the .h file. The latter seems like a bad idea to me, since it doesn't need to be made available outside of this file. I thought inserting an early declaration for one of the 3 overloads in the .cpp file was messier than changing the order, but if preserving the diff is more important then I can do that instead.

StephenTozer marked an inline comment as done.Mar 29 2019, 3:22 AM
StephenTozer added inline comments.

Leftover change from a previous edit of the function that was undone; the reason behind the rename however is that "EC" is short for Error Code; "Err" is the generic name for Error variables.

StephenTozer marked an inline comment as done.Mar 29 2019, 3:26 AM
StephenTozer added inline comments.

The FileError class automatically wraps the file name in quotes when it produces a message output, while the old std::error_code errors left it entirely up to the error handler.

Minor code cleanups.

jhenderson accepted this revision.Mar 29 2019, 4:10 AM

LGTM, but please wait for another person to confirm approval.

This revision is now accepted and ready to land.Mar 29 2019, 4:10 AM
grimar accepted this revision.Mar 29 2019, 4:32 AM

LGTM with a nit.


The code is still different from original, why?
(I am not against this renaming, but ideally, such things should be done in an NFC commits I think.)
So if there are no reasons for that, please revert.

grimar added inline comments.Mar 29 2019, 4:43 AM

Sorry, I was not clear I guess.

  1. I am not against the renaming EC->Err in a different patch/commit.
  2. The current change affects how the functor is called (i.e. removes the reference), what is also seems like an improvement, but it is not relative to this patch still.

Remove unnecessary/unrelated changes.

Still LGTM, thanks for the update!

Higuoxing accepted this revision.Mar 29 2019, 6:29 AM
sbc100 added inline comments.Mar 29 2019, 8:38 AM

FWIW gnu objdump puts the filename in quotes in error messages too:

$ objdump -s /dev/null
objdump: Warning: '/dev/null' is not an ordinary file
This revision was automatically updated to reflect the committed changes.

Fixes an Object test that I missed in the last revision.