This is an archive of the discontinued LLVM Phabricator instance.

[Error] Add source location to Errors
AbandonedPublic

Authored by hintonda on Nov 14 2019, 11:16 AM.

Details

Reviewers
lhames
beanz
Summary

This patch adds an llvm_make_error macro and SlocErrorBuilder
class to add source location to Errors gated on
LLVM_ENABLE_ABI_BREAKING_CHECKS.

Adding source location is an opt-in change. To opt-in, simply
replace llvm::make_error with the macro llvm_make_error.

Event Timeline

hintonda created this revision.Nov 14 2019, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2019, 11:16 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
hintonda updated this revision to Diff 229397.Nov 14 2019, 1:41 PM
  • Remove SourceLocationError class and reuse FileError.
hintonda retitled this revision from [Error] Add SourceLocationError to [Error] Add source location macro.Nov 14 2019, 1:43 PM
hintonda edited the summary of this revision. (Show Details)

I'm not sure I follow this. My understanding is that FileError was designed to make it easy to attach a source location to an error generated while operating on a file, but this patch is adapting it to track the LLVM source location that generated the error, right?

I don't think we want to do that -- it makes it very difficult to reason about what FileError's fields mean.

I'm not sure I follow this. My understanding is that FileError was designed to make it easy to attach a source location to an error generated while operating on a file, but this patch is adapting it to track the LLVM source location that generated the error, right?

Yes.

I don't think we want to do that -- it makes it very difficult to reason about what FileError's fields mean.

In my initial commit, I had added a new class specifically for this, SourceLocationError, but @beanz convinced me to reuse FileError, since the only difference was the message formatting.

Should I add back SourceLocationError?

In my initial commit, I had added a new class specifically for this, SourceLocationError, but @beanz convinced me to reuse FileError, since the only difference was the message formatting.

Should I add back SourceLocationError?

We definitely shouldn't re-use FileError, but I'm not sure we want to use SourceLocationError either: On the one hand it gets you the line info, on the other hand it means that your errors have different types in debug builds vs non-debug builds. This could lead to some pretty subtle issues when mixing debug and non-debug code, or trying to reproduce issues from release builds on debug builds.

What if we used a side table and stored the line info for every in-flight error? (We could condition this on LLVM_ENABLE_EXPENSIVE_CHECKS if it proved to be expensive, but I seriously doubt it would be: the cost is marginal and only paid for failure values, not success values).

The hard part would be automating the printing of this source location information. Right now that would require cooperation from every ErrorInfo subclass, since right now rendering is fully controlled by the user supplied log method. We could make the log method private and force everyone to use a new stream operator like this:

DenseMap<ErrorInfoBase*, std::pair<const char*, unsigned>> ErrorOriginSourceLoc;

raw_ostream &operator<<(raw_ostream &OS, const ErrorInfoBase &EIB) {
  EIB.log(OS);
#ifndef NDEBUG
#ifdef ENABLE_EXPENSIVE_CHECKS
  auto LocI = ErrorOriginSourceLoc.find(&EIB);
  if (LocI != ErrorOriginSourceLoc.end()) {
    OS << " (Error originated at " << LocI->second.first << ":" << LocI->second.second << ")";
  }
#endif // ENABLE_EXPENSIVE_CHECKS
#endif // NDEBUG
  return OS; 
}

That way we're free to decorate user supplied log messages in debug builds in any way we want (in this case by adding the source location information).

hintonda updated this revision to Diff 230589.Nov 21 2019, 10:00 PM

Switch to builder.

hintonda retitled this revision from [Error] Add source location macro to [Error] Add source location to Errors.Nov 21 2019, 10:01 PM
hintonda edited the summary of this revision. (Show Details)
hintonda abandoned this revision.Apr 26 2020, 9:22 AM