This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML][DWARF] Let writeVariableSizedInteger() return Error.
ClosedPublic

Authored by Higuoxing on Jun 16 2020, 1:48 AM.

Details

Summary

This patch helps change the return type of writeVariableSizedInteger() from void to Error.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 16 2020, 1:48 AM

I think the error message "invalid integer write size" makes little sense here. If multiple sections are involved, it's hard to tell which section contains the invalid value. I'm still thinking about improving it.

jhenderson added inline comments.Jun 16 2020, 2:28 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
57

This probably wants to be not_supported because strictly, there's nothing stopping us writing a 3/5/7 etc byte value.

133

Perhaps here and at similar points, it might be worth adding additional context to the message, to address your problem. There are examples of this in various places, such as llvm-readobj, I believe.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
597

perhaps entry -> list

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
411

Similarly, entry -> list here and below.

Higuoxing updated this revision to Diff 271039.Jun 16 2020, 4:18 AM
Higuoxing marked 4 inline comments as done.

Thanks for reviewing!

I added a helper function errorWithContext() which helps add additional context information to the error message.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
133

I'm not sure if I'm doing it in the right way. I added one helper function errorWithContext() which helps add additional information to the error message.

jhenderson added inline comments.Jun 16 2020, 4:57 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
39

This isn't too far off what I had in mind, but is probably more than what you actually need.

Take a look at ELFDumper.cpp in llvm-readobj. You'll see several instances where the function toString is used to convert an error into a string and then this string is used as part of a new error message. For example, in printRelocation you have the following:

Expected<std::pair<const typename ELFT::Sym *, std::string>> Target =
    this->dumper()->getRelocationTarget(SymTab, R);
if (!Target)
  this->reportUniqueWarning(createError(
      "unable to print relocation " + Twine(RelIndex) + " in section " +
      Twine(SecIndex) + ": " + toString(Target.takeError())));

When the Target contains an error, the toString method is used, combined with other context information into a new error message. Consequently, you'd explicitly build up your message with the lower-level error message and higher level things. For example:

if (Error Err = writeVariableSizedInteger(
        Descriptor.Address, Range.AddrSize, OS, DI.IsLittleEndian))
  return createStringError(errc::not_supported, "unable to write debug_aranges address: " + toString(std::move(Err)));
57–60

I'd slightly modify this error to include the size that has been requested.

Higuoxing updated this revision to Diff 271056.Jun 16 2020, 5:20 AM
Higuoxing marked 3 inline comments as done.

Address comments.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
39

Thanks a lot! TIL, toString(Err) marks the error checked!

jhenderson added inline comments.Jun 16 2020, 5:42 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
58

createStringError follows printf style formatting rules, so this can be:

return createStringError(errc::not_supported,
                         "invalid integer write size: %zu", Size);

and then no need to include the header.

134–135

Similarly, consider using %s:

return createStringError(errc::not_supported,
                         "unable to write debug_aranges address: %s",
                             toString(std::move(Err)).c_str());

Same goes below.

Higuoxing updated this revision to Diff 271068.Jun 16 2020, 5:51 AM
Higuoxing marked 2 inline comments as done.

Address comments.

This revision is now accepted and ready to land.Jun 16 2020, 6:09 AM
Harbormaster completed remote builds in B60470: Diff 271068.
This revision was automatically updated to reflect the committed changes.