Page MenuHomePhabricator

[llvm/Object] - Convert SectionRef::getName() to return Expected<>
ClosedPublic

Authored by grimar on Mon, Aug 12, 7:10 AM.

Details

Summary

SectionRef::getName() returns std::error_code now.
Returning Expected<> instead has multiple benefirs

For example, it forces user to check the error returned.
Also Expected<> may keep a valuable string error message,
that is more useful than having a error code.
(Object\invalid.test was updated to show the new messages printed.)

This patch makes a change for all users to switch to Expected<> version.

Important note: in a few places the error returned was ignored before my changes.
In such places I left them ignored. My intention was to convert the interface
used, and not to improve and/or the existent users in this patch.
(Though I think this is good idea for a follow-ups to revisit such places
and either remove consumeError calls or comment each of them to clarify why
it is OK to have them).

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Mon, Aug 12, 7:10 AM
JDevlieghere added inline comments.Mon, Aug 12, 8:45 AM
include/llvm/Object/ELFObjectFile.h
468 ↗(On Diff #214632)

What if the Expected is in the success state, but the StringRef is empty?

sbc100 accepted this revision.Mon, Aug 12, 2:12 PM
sbc100 added inline comments.
include/llvm/Object/ObjectFile.h
438 ↗(On Diff #214632)

Can you return the result of getSectionName here?

lib/DebugInfo/DWARF/DWARFContext.cpp
1512 ↗(On Diff #214632)

Add TODOs here and below to handle errors?

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
287 ↗(On Diff #214632)

You use ErrorOrName here but NameOrErr here. Presumably we should prefer one or at least be consistent?

lib/Object/ELFObjectFile.cpp
386 ↗(On Diff #214632)

In most other places you use Expected<StringRef> rather than auto. Is there a reason for auto here and not elsewhere?

This revision is now accepted and ready to land.Mon, Aug 12, 2:12 PM
MaskRay accepted this revision.Mon, Aug 12, 11:37 PM
MaskRay added inline comments.
tools/llvm-cov/TestingSupport.cpp
52 ↗(On Diff #214632)

Something like:

 if (Expected<StringRef> NameOrErr = Section.getName())
   SectName = *NameOrErr;
 else {
   consumeError(NameOrErr.takeError());
   return 1;
}
tools/llvm-objdump/MachODump.cpp
7398 ↗(On Diff #214632)

superfluous parentheses

tools/llvm-objdump/llvm-objdump.cpp
932 ↗(On Diff #214632)

superfluous parentheses

tools/llvm-readobj/Win64EHDumper.cpp
308 ↗(On Diff #214632)

if (Expected<StringRef> NameOrErr = Section.getName()) (this is what you do in other files)

tools/llvm-size/llvm-size.cpp
407 ↗(On Diff #214632)

name_or_err->size()

448 ↗(On Diff #214632)

name_or_err->c_str() and delete namestr

grimar updated this revision to Diff 214782.Tue, Aug 13, 1:43 AM
grimar marked 14 inline comments as done.
  • Addressed review comments.
include/llvm/Object/ELFObjectFile.h
468 ↗(On Diff #214632)

Thanks! I fixed this case.

include/llvm/Object/ObjectFile.h
438 ↗(On Diff #214632)

Done, thanks!

lib/DebugInfo/DWARF/DWARFContext.cpp
1512 ↗(On Diff #214632)

I did not add TODOs here or in another places intentionally.
It feels that many places lack the error checking and needs adding "TODOs",
but I am not sure which of them really need it (i.e. sometimes consuming errors is OK).
What I was thinking about is that ideally
each place where we have a consumeError in LLVM should have a
comment clarifying everything is OK and its using was intentional. If not - then it is
an implicit "TODO" that we need to check.

I.e. I would like to not to add "TODOs" in this patch if you and others are OK with that.

lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
287 ↗(On Diff #214632)

Done.

lib/Object/ELFObjectFile.cpp
386 ↗(On Diff #214632)

No. Sometimes LLVM code use auto for expressing the Expected<> return type.
I would prefer to see an explicit types in such situations. Fixed.

tools/llvm-cov/TestingSupport.cpp
52 ↗(On Diff #214632)

This is one line longer, but OK.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 14, 1:46 AM
Herald added a subscriber: seiya. · View Herald Transcript

Sorry, but this patch broke the build (http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16455), I reverted it. Please run check-all before re-committing.

grimar added a comment.EditedWed, Aug 14, 2:09 AM

Sorry, but this patch broke the build (http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/16455), I reverted it. Please run check-all before re-committing.

Hi, I am sorry for the breakage. Did not realize that clang also depends on this API.

(And I think I've reverted it in rL368813 actually.)