This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Remove Error.cpp,.h and drop dependencies in the code.
ClosedPublic

Authored by grimar on Aug 28 2020, 3:41 AM.

Details

Summary

We have Error.cpp/.h which contains some code for working with error codes.
In fact we use Error/Expected<> almost everywhere already and we can get rid
of these files.

Note: a few places in the code used readobj specific error codes,
e.g. return readobj_error::unknown_symbol. But these codes are never really used,
i.e. the code checks the fact of a success/error call only.
So I've changes them to return inconvertibleErrorCode() for now.
It seems that these places probably should be converted to use Error/Expected<>.

Rebased on top of D86771.

Diff Detail

Event Timeline

grimar created this revision.Aug 28 2020, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 3:41 AM
grimar requested review of this revision.Aug 28 2020, 3:41 AM
sbc100 added inline comments.Aug 28 2020, 4:09 AM
llvm/tools/llvm-readobj/llvm-readobj.cpp
640

Should we assert or llvm_unreachable here then? Since it should be impossible to get here because the above "case" statement should handle all valid file types?

grimar added inline comments.Aug 28 2020, 5:08 AM
llvm/tools/llvm-readobj/llvm-readobj.cpp
640

I don't think that it is right to use llvm_unreachable, because I guess it might be possible that one day a new object format appears, but will be unsupported by llvm-readelf. We probably don't want to fail in this case, but want to show an error message I think.

Regarding adding an assert, I don't know. I don't have any stong feeling about it.

jhenderson added inline comments.Sep 1 2020, 1:08 AM
llvm/tools/llvm-readobj/llvm-readobj.cpp
582

This doesn't look clang-formatted.

589–590

I think the message should be: "foo.o has an unrecognized file type" (or possibly even "unsupported" instead of "unrecognized").

640

I think llvm_unreachable is the right thing here. Currently, it should be impossible to reach this code (hence "unreachable"). The only way that it would be reachable is if somebody adds a new variety of Binary without properly adding support for it everywhere. This is a programmer error, so assert/llvm_unreachable is the way to report this. Since this code can't be executed under current state, llvm_unreachable is more appropriate than assert (which is better used for checking pre/post conditions).

If one day a user does add a new file type, which shouldn't be supported by llvm-readobj, they can change to reporting a normal error as you are currently doing. This would be a conscious decision on their part, to not (at that point) add the support.

grimar updated this revision to Diff 289133.Sep 1 2020, 4:24 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/llvm-readobj.cpp
640

OK, I've used llvm_unreachable.

This revision is now accepted and ready to land.Sep 1 2020, 4:44 AM
llvm/tools/llvm-readobj/ELFDumper.cpp