This is an archive of the discontinued LLVM Phabricator instance.

Improve error handling in llvm-dwarfdump
ClosedPublic

Authored by aprantl on Jun 17 2021, 12:52 PM.

Details

Summary

Without this patch we're only showing a generic error message derived from the error code to the end user.

rdar://79378794

Diff Detail

Event Timeline

aprantl created this revision.Jun 17 2021, 12:52 PM
aprantl requested review of this revision.Jun 17 2021, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 12:52 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
jhenderson added inline comments.Jun 18 2021, 12:16 AM
llvm/test/tools/llvm-dwarfdump/X86/lc_malformed.test
1 ↗(On Diff #352826)

This test looks somewhat tangential to the code change?

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
273

I think the code would be a lot simpler if you switched which is the "base" error reporting function. If I'm not mistaken, you can use toString to convert an Error into an error message. You could convert an error code to an Error and report things that way. Approximate code:

static void error(StringRef Prefix, Error Err) {
  if (!Err)
    return;
  WithColor::error() << Prefix << ": " << toString(std::move(Err)) << "\n";
  exit(1);  
}

static void error(StringRef Prefix, std::error_code EC) {
  error(errorCodeToError(EC));
}
JDevlieghere added inline comments.Jun 21 2021, 9:45 AM
llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
273

I was going to suggest the same thing, so +1 from me

aprantl added inline comments.Jun 22 2021, 2:56 PM
llvm/test/tools/llvm-dwarfdump/X86/lc_malformed.test
2 ↗(On Diff #352826)

Without the patch you would see Invalid data was encountered while parsing the file.

aprantl updated this revision to Diff 353814.Jun 22 2021, 4:20 PM

Thanks, this is much cleaner!

It wasn't clear to me that toString was able to deal with BinaryError.

jhenderson accepted this revision.Jun 23 2021, 12:14 AM

LGTM. If you want to bring back the test in a separate patch, that's absolutely fine (it sounds like there is some missing test coverage from what you said).

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
270

Nit: looks like there's some trailing whitespace here.

This revision is now accepted and ready to land.Jun 23 2021, 12:14 AM

LGTM. If you want to bring back the test in a separate patch,

I forgot to git-add it before uploading the patch....

This revision was landed with ongoing or failed builds.Jun 23 2021, 10:44 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 23 2021, 11:27 AM

This breaks check-llvm http://45.33.8.238/linux/49596/step_12.txt

Please take a look, and please revert if it takes a bit to fix.