This is an archive of the discontinued LLVM Phabricator instance.

[ProfileData] (llvm) Use Error in InstrProf and Coverage
ClosedPublic

Authored by vsk on May 3 2016, 11:49 PM.

Details

Summary

Transition InstrProf and Coverage over to the stricter Error/Expected interface.

This has several advantages:

  1. It's impossible to ignore hard errors.
  2. It's impossible to ignore breakages in code that *usually* returns "success" values.
  3. It's easier to set breakpoints on specific error conditions (e.g, in the relevant ErrorInfoBase constructor), because we no longer create errors by returning raw enum values.

The only disadvantage I'm aware of is that in the event of an actual error, there is a malloc required.

This patch is motivated by my experiences with maintaining the swift compiler's code coverage functionality. Having stricter error checking would have helped us catch multiple bugs at testing time. Certain hard errors triggered by upstream changes wouldn't have been silently ignored.

I apologize for the large diff (thankfully it's very mechanical). Once I converted InstrProf, I had a hard time getting the Coverage code to work without converting it as well. Since there is no SampleProf <- InstrProf dependency, I will deal with it in a separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 56096.May 3 2016, 11:49 PM
vsk retitled this revision from to [ProfileData] (llvm) Use Error in InstrProf and Coverage.
vsk updated this object.
vsk added reviewers: davidxl, xur.
vsk added a subscriber: lhames.
davidxl added inline comments.May 4 2016, 9:44 AM
include/llvm/ProfileData/InstrProf.h
278 ↗(On Diff #56096)

Should it be templatized and share the def with CoverageMapError?

300 ↗(On Diff #56096)

Document this interface

309 ↗(On Diff #56096)

Document this interface.

vsk marked 2 inline comments as done.May 4 2016, 11:47 AM
vsk added inline comments.
include/llvm/ProfileData/InstrProf.h
278 ↗(On Diff #56096)

Good point, I'll have this in the next revision. @lhames, could something like this be worth pulling into Support/Error.h?

vsk updated this revision to Diff 56184.May 4 2016, 11:48 AM
  • Document two static functions in InstrProfError.
  • Share code between InstrProfError and CoverageMapError.
lhames added a comment.May 4 2016, 3:36 PM

I think EnumBasedError is problematic: at a minimum it needs to implement the convertToErrorCode method, since it's part of the current Error contract. Clients must be allowed to convert to std::error_code to allow interoperability with code that hasn't switched to Error yet. That shouldn't be too much a headache. You just need to create an error category and move your existing to-string methods for the enum over to that. See e.g. lib/Object/Error.cpp for a simple error_category definition.

EnumBasedError probably shouldn't have its own create method either - wherever you're creating a success value you should just use Error::success(). Where you're creating a failure value you should use make_error<ErrT>(<enum value>);

include/llvm/ProfileData/InstrProf.h
303–321 ↗(On Diff #56184)

InstrProfError (and CoverageMappingError) need their own ID members. If they don't have them the Error RTTI will treat them both as EnumBasedErrors.

vsk added a comment.May 4 2016, 10:56 PM

I'll make the suggested fixes to EnumBasedError.

Based on Lang's feedback it seems like this class should live in ProfileData for now, on a trial basis. We could easily revisit this decision once we know more about whether or not each enum value kind should get its own ErrorInfoBase.

include/llvm/ProfileData/InstrProf.h
303–321 ↗(On Diff #56184)

Yikes! Is this true even if I define distinct ID's for them? Specifically:

template <> char EnumBasedError<instrprof_error, InstrProfError>::ID = 0; // InstrProf.cpp
template <> char EnumBasedError<coveragemap_error, CoverageMapError>::ID = 0; // CoverageMapping.cpp
602 ↗(On Diff #56184)

In the event of a large number of overflow errors, the malloc traffic out of this API might be a real performance concern. The same goes for all clients of InstrProfError::merge.

For now, I will re-work this to return a raw instrprof_error. In the future, we may want to change the API again to get the best of both worlds: undroppable errors and no memory overhead.

davidxl added inline comments.May 6 2016, 1:23 PM
include/llvm/ProfileData/InstrProf.h
303–321 ↗(On Diff #56184)

Can you elaborate on the RTTI issue? Also what is the problem that can be caused with this (both Error treated as EnumBasedErrors)?

lhames added inline comments.May 6 2016, 2:26 PM
include/llvm/ProfileData/InstrProf.h
303–321 ↗(On Diff #56184)

My bad! didn't stop to think about EnumBasedError being templated. I think having the ID on EnumBasedError like this should be safe.

davidxl edited edge metadata.May 9 2016, 12:10 PM

Any more update to the patch?

vsk added a comment.May 9 2016, 12:39 PM

Sorry, I've had to deal with some failures in our internal branches. I'll update this patch shortly.

vsk updated this revision to Diff 56760.May 10 2016, 11:07 AM
vsk edited edge metadata.
  • Move EnumBasedError to ProfileCommon.h so SampleProf can reuse it later.
  • Remove EnumBasedError::create(), remove the DerivedError CRTP, and use Error::success()/make_error<> everywhere explicitly.
  • Honor the convertToErrorCode contract.
vsk added a comment.May 10 2016, 11:10 AM

I think I addressed Lang's review feedback.

If http://reviews.llvm.org/D20082 looks good, I'd like to rebase onto it. After that it seems like we'd be in good shape.

include/llvm/ProfileData/InstrProf.h
303–321 ↗(On Diff #56184)

@davidxl Here's an example where this could be a problem. Suppose classes A and B both inherit from class Parent, and that the storage for ID is shared. Then, a "B" error could be handled by a callback for "A" errors.

vsk updated this revision to Diff 56956.May 11 2016, 1:32 PM
  • Rebase onto master. This lets us incorporate SoftInstrProfErrors (see D20082).
vsk updated this revision to Diff 56957.May 11 2016, 1:47 PM
  • Fix a comment. takeError() resets the FirstError field.
  • Remove accidentally added copy constructor and copy assignment method for InstrProfRecord.
davidxl added inline comments.May 12 2016, 11:32 AM
include/llvm/ProfileData/Coverage/CoverageMapping.h
47 ↗(On Diff #56957)

no need for this overrider.

include/llvm/ProfileData/InstrProf.h
259 ↗(On Diff #56957)

why this change?

288 ↗(On Diff #56957)

no need for this override -- define in base.

include/llvm/ProfileData/ProfileCommon.h
41 ↗(On Diff #56957)

The name seems to general. How about just ProfErrorInfoBase

lib/ProfileData/Coverage/CoverageMappingReader.cpp
45 ↗(On Diff #56957)

Can this ever happen?

vsk marked 11 inline comments as done.May 12 2016, 12:17 PM
vsk added inline comments.
include/llvm/ProfileData/InstrProf.h
259 ↗(On Diff #56957)

I did it to catch cases where we treat instrprof_errors as bools or ints. It isn't directly related to this patch so I'll remove the change.

lib/ProfileData/Coverage/CoverageMappingReader.cpp
45 ↗(On Diff #56957)

Yes, thanks for pointing this out. In unittests/ProfileData/CoverageMappingTest.cpp, CoverageMappingReaderMock::readNextRecord() returns instrprof_error::eof instead of coveragemap_error::eof. Fixed in r269324.

vsk updated this revision to Diff 57086.May 12 2016, 12:17 PM
vsk marked 2 inline comments as done.

Address David's review feedback.

  • EnumBasedError -> ProfErrorInfoBase
  • Clean up the log() overrides.
  • Minimize the diff (don't change the "success" values, remove a #include of ManagedStatic.h from InstrProf.h)
davidxl accepted this revision.May 13 2016, 11:13 AM
davidxl edited edge metadata.

lgtm

This revision is now accepted and ready to land.May 13 2016, 11:13 AM
This revision was automatically updated to reflect the committed changes.