Page MenuHomePhabricator

[obj2yaml] - Rework tool's error reporting logic for ELF target.
ClosedPublic

Authored by grimar on Jul 12 2019, 6:19 AM.

Details

Summary

ELF.h contains two getSymbol methods
which seems to be used only from obj2yaml.

One of these methods calls another, which in turn
contains untested error message which doesn't
provide enough information.

Problem is that after improving only just that message,
obj2yaml will not show it,
("Error reading file: yaml: Invalid data was
encountered while parsing the file" message will be shown instead),
because internal errors handling of tool is based on ErrorOr<> class which
stores a error code and as a result can only show a predefined error string, what
actually isn't very useful.

In this patch, I rework obj2yaml's error reporting system
for ELF targets to use Error Expected<> classes.
Also, I improve the error message produced
by getSymbol for demonstration of the new functionality.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 12 2019, 6:19 AM

Happy to see more ErrorOr/std::error_code becoming Error/Expected ๐Ÿ˜Š

include/llvm/Object/ELF.h
364 โ†—(On Diff #209472)

Elf_Sym_Range Symbols = *SymsOrErr;

tools/obj2yaml/elf2yaml.cpp
164 โ†—(On Diff #209472)

return E; to avoid clang -Wpessimizing-move (automatic move from lvalues applies here)

176 โ†—(On Diff #209472)

Y->Sections.emplace_back(*SecOrErr);

We can leverage the ctor of unique_ptr here.

197 โ†—(On Diff #209472)

ditto

204 โ†—(On Diff #209472)

ditto

430 โ†—(On Diff #209472)

return E;

444 โ†—(On Diff #209472)

return E;

grimar updated this revision to Diff 209772.Jul 15 2019, 1:29 AM
grimar marked 8 inline comments as done.
  • Addressed review comments.
tools/obj2yaml/elf2yaml.cpp
164 โ†—(On Diff #209472)

I can't. Otherwise I have the following errors:
(MVSC 2017)

elf2yaml.cpp(164): error C2280: 'llvm::Error::Error(const llvm::Error &)': attempting to reference a deleted function
llvm/Support/Error.h(186): note: see declaration of 'llvm::Error::Error'
llvm/Support/Error.h(186): note: 'llvm::Error::Error(const llvm::Error &)': function was explicitly deleted

When I search for "return std::move(" in LLVMs code, I see many similar places.

MaskRay added inline comments.Jul 15 2019, 1:52 AM
tools/obj2yaml/elf2yaml.cpp
164 โ†—(On Diff #209472)

Sorry, the -Wpessimizing-move warning is on elf2yaml.cpp:394 (where the return type is Error), not here (where the return type is Expected).

grimar updated this revision to Diff 209778.Jul 15 2019, 2:00 AM
grimar marked an inline comment as done.
  • Addressed review comments.
tools/obj2yaml/elf2yaml.cpp
164 โ†—(On Diff #209472)

Fixed, thanks!

MaskRay accepted this revision.Jul 15 2019, 2:44 AM
This revision is now accepted and ready to land.Jul 15 2019, 2:44 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. ยท View Herald TranscriptJul 15 2019, 3:50 AM