This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Refactor DWARF classes to use unified error reporting. NFC.
ClosedPublic

Authored by vleschuk on Jul 28 2018, 11:04 PM.

Details

Summary

DWARF-related classes in lib/DebugInfo/DWARF contained duplicating code for creating StringError instances, like:

template <typename... Ts>                                                      
static Error createError(char const *Fmt, const Ts &... Vals) {                
  std::string Buffer;                                                          
  raw_string_ostream Stream(Buffer);                                           
  Stream << format(Fmt, Vals...);                                              
  return make_error<StringError>(Stream.str(), inconvertibleErrorCode());
}

Similar function was placed in Support lib in https://reviews.llvm.org/D49824 .

This revision makes DWARF classes use this function instead of their local implementation of it.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk created this revision.Jul 28 2018, 11:04 PM
vleschuk added inline comments.Jul 28 2018, 11:10 PM
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
27 ↗(On Diff #157883)

@wolfgangp I have compiled new code with gcc (7.2.1) and it compiles correctly and passes all tests. Could you please let me know if we can safely get rid of this workaround here?

JDevlieghere added inline comments.Jul 29 2018, 8:20 AM
include/llvm/DebugInfo/DWARF/DWARFListTable.h
213 ↗(On Diff #157883)

Although having an inconvertibleErrorCode (as per Lang's comment in the other diff) prevents conversion between errors and error codes, I'm not convinced it's actually needed here. If we don't need the conversion in libDebugInfo (which I think we don't) I'd rather have an inconvertible one instead of whichever is close enough.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
27 ↗(On Diff #157883)

We officially support GCC 4.8.0 and later, so whether it compiles with 7.2.1 doesn't really tell us much. I believe we have bots running pretty old versions of GCC so you could try removing and see if any of them are failing (or you get complaints from downstream users).

vleschuk marked 2 inline comments as done.Jul 30 2018, 2:47 AM
vleschuk added inline comments.
include/llvm/DebugInfo/DWARF/DWARFListTable.h
213 ↗(On Diff #157883)

During https://reviews.llvm.org/D49676 it was decided to use convertible error codes in such situations. I think we should have consistent behavior along code sections (in this case style should be similar in all DebugInfo/DWARF* classes).

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
27 ↗(On Diff #157883)

I have tested it, build fails if we just remove this part from code, however everything works fine if we fully apply the patch from this revision and replace calls to createError() with the createStringError(). Tested with clang 6.0, gcc 4.8, 4.9 and 7.2.

vleschuk marked 4 inline comments as done.Jul 30 2018, 2:48 AM
wolfgangp added inline comments.Jul 30 2018, 9:19 AM
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
27 ↗(On Diff #157883)

I have compiled new code with gcc (7.2.1) and it compiles correctly and passes all tests.
Could you please let me know if we can safely get rid of this workaround here?

It certainly looks like after your changes the workaround is no longer needed since we don't need the explicit specialization of 'createError' any more.

vleschuk marked an inline comment as done.Jul 30 2018, 11:03 AM
vleschuk added inline comments.
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
27 ↗(On Diff #157883)

Thanks a lot for confirmation!

vleschuk marked an inline comment as done.Jul 31 2018, 7:38 AM

Does anyone have more questions/suggestions/objections on this revision?

JDevlieghere added inline comments.Jul 31 2018, 7:47 AM
include/llvm/DebugInfo/DWARF/DWARFListTable.h
213 ↗(On Diff #157883)

I'm probably overlooking it, but where exactly in D49676 was this decided? I don't mind per se, but I'd like to understand why we're doing this for future code reviews.

vleschuk added inline comments.Jul 31 2018, 11:09 AM
include/llvm/DebugInfo/DWARF/DWARFListTable.h
213 ↗(On Diff #157883)

Ping. Any questions?

JDevlieghere accepted this revision.Aug 2 2018, 2:46 AM

In the differential you linked we agreed to have the API include an error code as an argument. That still leaves us the option to pass an inconvertibleErrorCode() as the first argument.

Anyway, since nobody else seems to mind the switch to actual error codes, this LGTM. Thanks for taking the time to work on this, Victor!

This revision is now accepted and ready to land.Aug 2 2018, 2:46 AM
jhenderson requested changes to this revision.Aug 2 2018, 3:29 AM
jhenderson added a reviewer: jhenderson.
jhenderson added inline comments.
lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
398–399 ↗(On Diff #157883)

Nit: The indentation looks a bit weird here. Is this how clang-format formats this?

516 ↗(On Diff #157883)

No. Don't use this errc value here. file_exists is for files, as the name implies. This should be invalid_argument again, I guess (there isn't a code for "malformed input file").

617 ↗(On Diff #157883)

You missed an inconvertibleErrorCode here...

Actually, looking at the function itself, it can NEVER fail (always returns true), so this is technically dead code. I'd consider changing the signature of the function to return void. But that should be a separate change. If you are going to do this, I'm okay with it continuing to be inconvertibleErrorCode, but otherwise, maybe io_error?

lib/DebugInfo/DWARF/DWARFContext.cpp
1583 ↗(On Diff #157883)

This should be invalid_argument, looking at the lookupTarget function (which actually should just return an Error itself rather than setting a string...)

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
51 ↗(On Diff #157883)

I didn't know about this errc value. Good spot! Maybe it's worth using in one or two other cases where the input is malformed? What do you think?

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
629 ↗(On Diff #157883)

This might want to be illegal byte sequence.

837 ↗(On Diff #157883)

This should be illegal_byte_sequence, I reckon.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
93 ↗(On Diff #157883)

Maybe not_supported here?

probinson added inline comments.Aug 2 2018, 8:04 AM
lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
617 ↗(On Diff #157883)

Re. extractValue() there's a place in DWARFDie.cpp that asserts the return value is true; but that's because DIEs come from a section that has actually been parsed already. Other places that use forms to interpret data are less (ahem) assertive, and this code isn't relying on a pre-parsed section, so I think returning an error would be appropriate.

I just noticed that llvmObject has an object_error equivalent to std::errc, with a make_error_code method allowing us to turn them into std::error_code for use with the new function. In particular, there are unexpected_eof and parser_failed errors, which might be more appropriate for certain situations in this file. What do you think?

Hi @vleschuk, I just wanted to quickly check-in on this to see if you need any help with making further improvements as suggested?

Hi @vleschuk, I just wanted to quickly check-in on this to see if you need any help with making further improvements as suggested?

Hello, sorry, had a delay with them, will update the review today/tomorrow.

vleschuk marked 9 inline comments as done.Aug 19 2018, 3:26 AM
vleschuk added inline comments.
lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
617 ↗(On Diff #157883)

Ok, using io_error for now.

lib/DebugInfo/DWARF/DWARFContext.cpp
1583 ↗(On Diff #157883)

looking at the lookupTarget function (which actually should just return an Error itself rather than setting a string...)

However return result of lookupTarget is used below. Changing its behavior is not related to this change... Setting invalid_argument.

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
51 ↗(On Diff #157883)

Yep, used it for "Incorrectly terminated.." and "Section too small". Thanks for the tip.

vleschuk marked 2 inline comments as done.EditedAug 19 2018, 3:32 AM

I just noticed that llvmObject has an object_error equivalent to std::errc, with a make_error_code method allowing us to turn them into std::error_code for use with the new function. In particular, there are unexpected_eof and parser_failed errors, which might be more appropriate for certain situations in this file. What do you think?

unexpected_eof is not very appropriate, when sections are too small I used illegal_byte_sequence. Maybe we could use parser_failed to replace some invalid_argument's. But won't that enum number concur with some other? I suggest we leave it for separate change.

vleschuk updated this revision to Diff 161393.Aug 19 2018, 3:46 AM
  • Changed error codes to more appropriate ones
  • Fixed formatting

@jhenderson Please take a look when you have time.

jhenderson accepted this revision.Aug 20 2018, 2:00 AM

LGTM, from my point of view now.

include/llvm/DebugInfo/DWARF/DWARFListTable.h
226 ↗(On Diff #161393)

This is probably another good place illegal_byte_sequence error.

lib/DebugInfo/DWARF/DWARFContext.cpp
1583 ↗(On Diff #157883)

Sorry, yeah, I meant an Expected. But no need to change that in this change.

This revision was automatically updated to reflect the committed changes.