[Error] Add source location to Errors

[hintonda]

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.

[hintonda]

• Remove SourceLocationError class and reuse FileError.

[lhames]

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.

[hintonda]

In D70263#1748105, @lhames wrote:

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?

[lhames]

In D70263#1748442, @hintonda wrote:

In D70263#1748105, @lhames wrote:

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]

Switch to builder.

llvm/include/llvm/Support/Error.h

public:
   /// Print an error message to an output stream.
   virtual void log(raw_ostream &OS) const = 0;
 
   /// Return the error message as a string.
   virtual std::string message() const {
     std::string Msg;
     raw_string_ostream OS(Msg);
     log(OS);
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+    if (File)
+      OS << " at " << File << ":" << Line;
+#endif
     return OS.str();
   }
 
   /// Convert this error to a std::error_code.
   ///
   /// This is a temporary crutch to enable interaction with code still
   /// using std::error_code. It will be removed in the future.
   virtual std::error_code convertToErrorCode() const = 0;
virtual bool isA(const void *const ClassID) const {
     return ClassID == classID();
   }
 
   // Check whether this instance is a subclass of ErrorInfoT.
   template <typename ErrorInfoT> bool isA() const {
     return isA(ErrorInfoT::classID());
   }
 
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  void setSourceLocation(const char *file, unsigned line) {
+    File = file;
+    Line = line;
+  }
+#endif
+
 private:
   virtual void anchor();
 
   static char ID;
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  const char *File = nullptr;
+  unsigned Line = 0;
+#endif
 };
 
 /// Lightweight error class with error context and mandatory checking.
 ///
 /// Instances of this class wrap a ErrorInfoBase pointer. Failure states
 /// are represented by setting the pointer to a ErrorInfoBase subclass
 /// instance containing information describing the failure. Success is
 /// represented by a null pointer value.

 inline ErrorSuccess Error::success() { return ErrorSuccess(); }
 
 /// Make a Error instance representing failure using the given error info
 /// type.
 template <typename ErrT, typename... ArgTs> Error make_error(ArgTs &&... Args) {
   return Error(std::make_unique<ErrT>(std::forward<ArgTs>(Args)...));
 }
 
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+namespace detail {
+
+/// Builder class for creating Errors with source locations.
+class SlocErrorBuilder {
+public:
+  SlocErrorBuilder(const char *File, unsigned Line) : File(File), Line(Line) {
+    assert(File && "File cannot be nullptr");
+  }
+
+  /// Make a Error instance representing failure using the given error info
+  /// type with source location.
+  template <typename ErrT, typename... ArgTs> Error make_error(ArgTs &&... Args) {
+    auto Err = std::make_unique<ErrT>(std::forward<ArgTs>(Args)...);
+    Err->setSourceLocation(File, Line);
+    return Error(std::move(Err));
+  }
+
+private:
+  const char *File;
+  unsigned Line;
+};
+} // namespace detail
+
+#define llvm_make_error                                       \
+  ::llvm::detail::SlocErrorBuilder(__FILE__, __LINE__).make_error
+#else
+#define llvm_make_error ::llvm::make_error
+#endif
+
 /// Base class for user error types. Users should declare their error types
 /// like:
 ///
 /// class MyError : public ErrorInfo<MyError> {
 ///   ....
 /// };
 ///
 /// This class provides an implementation of the ErrorInfoBase::kind

llvm/unittests/Support/ErrorTest.cpp

Error FE4 =
       joinErrors(createFileError("file.bin", make_error<CustomError>(41)),
                  createFileError("file2.bin", make_error<CustomError>(42)));
   EXPECT_EQ(toString(std::move(FE4))
                 .compare("'file.bin': CustomError {41}\n"
                          "'file2.bin': CustomError {42}"),
             0);
 }
 
+TEST(Error, SourceLocationErrorTest) {
+  std::string ErrStr;
+  raw_string_ostream OS(ErrStr);
+
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+  // __FILE__ and __LINE__ added
+  int Line = __LINE__;
+  OS << "^CustomError \\{1\\} at " << __FILE__ << ":" << (Line + 2) << "$";
+  auto E = llvm_make_error<CustomError>(1);
+  EXPECT_THAT(toString(std::move(E)), ::testing::ContainsRegex(OS.str()));
+#else
+  // __FILE__ and __LINE_ not added
+  OS << "^CustomError \\{1\\}$";
+  auto E = llvm_make_error<CustomError>(1);
+  EXPECT_THAT(toString(std::move(FE1)), ::testing::ContainsRegex(OS.str()));
+#endif
+}
+
 enum class test_error_code {
   unspecified = 1,
   error_1,
   error_2,
 };
 
 } // end anon namespace