[Error] Add FileError helper; upgrade StringError behavior

[aganea]

Summary

Introducing a new error helper, FileError; and upgrading StringError.

FileError

is meant to encapsulate both an Error and a file name/path. It should be used in cases where an Error occurs deep down the call chain, and we want to return it to the caller along with the file name.

Previously, the caller had to know the file name, and in some cases it was unavailable. An example of this could be the loading of a COFF object (OBJ). Those files kinds can contain an indirection to another PDB or OBJ (precompiled headers) file. If that indirection fails to load for some reason, the caller might not have that indirect file name in hand. For those cases, a FileError would be inserted at the right place in the call chain.

Currently FileError logs with the following format: '{file}': {error}, such as: 'test.bin': no such file or directory

StringError

was updated to display the error messages in different ways. These can be:

1. display the error_code message, and convert to the same error_code (ECError behavior)
2. display an arbitrary string, and convert to a provided error_code (current StringError behavior)
3. display both an error_code message and a string, in this order; and convert to the same error_code

These behaviors can be triggered depending on the constructor. The goal is to use StringError as a base class, when a library needs to provide a explicit Error type.
The following illustrates these three cases:

return make_error<CodeViewError>(cv_error_code::corrupt_record); // case 1

return make_error<CodeViewError>(cv_error_code::corrupt_record, "Invalid frame data record format!"); // case 2

return make_error<CodeViewError>("Invalid type index"); // case 3

...CodeViewError being a subtype of StringError only with a minimal implementation:

class CodeViewError : public ErrorInfo<CodeViewError, StringError> {
public:
  using ErrorInfo<CodeViewError, StringError>::ErrorInfo; // inherit constructors
  CodeViewError(const Twine &S) : ErrorInfo(S, cv_error_code::unspecified) {}
  static char ID;
};

---

Usages are illustrated in D50664 (pending).

Tested with asserts enabled to ensure all Errors are checked in the unit tests.

[zturner]

Is it possible for

include/llvm/Support/Error.h

1217–1218 ↗

(On Diff #160905)

I wonder if this should have a more general name. There's nothing really specific to Debug Info in this implementation, it's basically just a wrapper around a std::error_code and std::string

1248 ↗

(On Diff #160905)

I wonder if we can get away with removing the EC template argument. A std::error_condition is essentially a wrapper around an error code and an error category, so ultimately when we call back into the category to format we're losing the info about the error code type anyway. It doesn't matter what the enum type is, the only thing that matters is that we know which class can format it.

So I think we can just make this an int.

Furthermore, I wonder if we need the ManagedStatic at all. All of these categories are cheap to construct, and I think they are all default constructible anyway, so perhaps our convertToErrorCode() function could just look like:

return std::error_code(Code, ECat{});

In all existing use cases, we never pass the same category with a different error code type, so we already havea 1:1 mapping anyway, and I don't think this is too restrictive. What do you think?

[aganea]

include/llvm/Support/Error.h

1217–1218 ↗

(On Diff #160905)

Will do. This new DebugErrorInfo class ressembles StringError, however the behavior for printing the message is different.

• StringError only prints the message
• DebugErrorInfo prints the message if the error_code is unspecified; otherwise it prints the associated error_code message, along with the user-provided message.
• Similarily, ECError is in the same problem domain as the two above, except that it always prints its associated error_code message.

I'll see if I can work out a more elegant solution, without adding a new class.

1248 ↗

(On Diff #160905)

Agreed, see above comment.

As for convertToErrorCode(), category objects cannot be created on the fly, the reference must be pointing to a singleton, as stated here and here.

The following shows how std::error_category shouldn't be used:

enum class WrongCatError {
  TestError = 1,
};

class WrongErrorCategory final : public std::error_category {
public:
  const char *name() const noexcept override { return "wrong"; }
  std::string message(int condition) const override { return ""; }
};

} // namespace

namespace std {
template <> struct is_error_code_enum<WrongCatError> : std::true_type {};
} // namespace std

namespace {

std::error_code make_error_code(WrongCatError Error) {
  auto E = std::error_code(static_cast<int>(Error), WrongErrorCategory()); // category is a temporary
  return E;
}

std::error_code SubFn() {
  return WrongCatError::TestError; // implicit call to make_error_code in
                                   // std::error_code's constructor
}

std::error_code SubFn2() {
  int a[200] = {0};
  return SubFn();
}

TEST(Error, WrongCategoryUsage) {
  auto E1 = SubFn();
  auto E2 = SubFn2();
  EXPECT_NE(E1, E2); // we fail because WrongErrorCategory is passed as a temporary
}

An example of such misuse can be found in clang/trunk/lib/Frontend/PrecompiledPreamble.cpp:

std::error_code clang::make_error_code(BuildPreambleError Error) {
  return std::error_code(static_cast<int>(Error), BuildPreambleErrorCategory());
}

The standard says that std::error_category shall compare by pointer, which of course wouldn't work in this case. The std::error_code object ends up here with a dangling pointer (at least in my VS2017 15.8 implementation of STL).

Unfortunately, the issue comes from the API to std::error_code which is misleading and cannot express the singleton storage requirement:

class error_code {
...
error_code( int ec, const error_category& ecat ); // 'ecat' is a reference to an error_category that we won't modify
...
}

...when in reality the API contract should say something more like:

class error_code {
...
error_code( int ec, const error_category static& ecat ); // 'ecat' would be a reference to an static error_category that we won't modify
...
}

...which we cannot write evidently.

This issue is vaguely described in P0824R1.

[aganea]

Updated following previous comments:

• Removed DebugErrorInfo and merged behavior into StringError.
• Moved back ManagedStatic<> into .cpp implementation(s) - see ErrorTest.cpp for example.
• Updated summary with use-cases.

[zturner]

Sorry for the delay, fell off my radar. LGTM

[aganea]

include/llvm/Support/Error.h

1143 ↗

(On Diff #162936)

I've noticed @lhames mentions here that usage of inconvertibleErrorCode() shall be explicit. I'll change that. Please let me know if there's anything else.

[aganea]

Thank you Zachary.

llvm/trunk/include/llvm/Support/Error.h

 /// that it has a void return type, and requires all errors to be handled and
 /// no new errors be returned. It prevents errors (assuming they can all be
 /// handled) from having to be bubbled all the way to the top-level.
 ///
 /// *All* Error instances must be checked before destruction, even if
 /// they're moved-assigned or constructed from Success values that have already
 /// been checked. This enforces checking through all levels of the call stack.
 class LLVM_NODISCARD Error {
-  // ErrorList needs to be able to yank ErrorInfoBase pointers out of this
-  // class to add to the error list.
+  // Both ErrorList and FileError need to be able to yank ErrorInfoBase
+  // pointers out of this class to add to the error list.
   friend class ErrorList;
+  friend class FileError;
 
   // handleErrors needs to be able to set the Checked flag.
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
 
   // Expected<T> needs to be able to steal the payload when constructed from an
   // error.
   template <typename T> friend class Expected;

 ///   ....
 /// };
 ///
 /// This class provides an implementation of the ErrorInfoBase::kind
 /// method, which is used by the Error RTTI system.
 template <typename ThisErrT, typename ParentErrT = ErrorInfoBase>
 class ErrorInfo : public ParentErrT {
 public:
+  using ParentErrT::ParentErrT; // inherit constructors
+
   static const void *classID() { return &ThisErrT::ID; }
 
   const void *dynamicClassID() const override { return &ThisErrT::ID; }
 
   bool isA(const void *const ClassID) const override {
     return ClassID == classID() || ParentErrT::isA(ClassID);
   }
 };
template <typename T> ErrorOr<T> expectedToErrorOr(Expected<T> &&E) {
   return std::move(*E);
 }
 
 /// This class wraps a string in an Error.
 ///
 /// StringError is useful in cases where the client is not expected to be able
 /// to consume the specific error message programmatically (for example, if the
 /// error message is to be presented to the user).
+///
+/// StringError can also be used when additional information is to be printed
+/// along with a error_code message. Depending on the constructor called, this
+/// class can either display:
+///    1. the error_code message (ECError behavior)
+///    2. a string
+///    3. the error_code message and a string
+///
+/// These behaviors are useful when subtyping is required; for example, when a
+/// specific library needs an explicit error type. In the example below,
+/// PDBError is derived from StringError:
+///
+///   @code{.cpp}
+///   Expected<int> foo() {
+///      return llvm::make_error<PDBError>(pdb_error_code::dia_failed_loading,
+///                                        "Additional information");
+///   }
+///   @endcode
+///
 class StringError : public ErrorInfo<StringError> {
 public:
   static char ID;
 
+  // Prints EC + S and converts to EC
+  StringError(std::error_code EC, const Twine &S = Twine());
+
+  // Prints S and converts to EC
   StringError(const Twine &S, std::error_code EC);
 
   void log(raw_ostream &OS) const override;
   std::error_code convertToErrorCode() const override;
 
   const std::string &getMessage() const { return Msg; }
 
 private:
   std::string Msg;
   std::error_code EC;
+  const bool PrintMsgOnly = false;
 };
 
 /// Create formatted StringError object.
 template <typename... Ts>
 Error createStringError(std::error_code EC, char const *Fmt,
                         const Ts &... Vals) {
   std::string Buffer;
   raw_string_ostream Stream(Buffer);
   Stream << format(Fmt, Vals...);
   return make_error<StringError>(Stream.str(), EC);
 }
 
 Error createStringError(std::error_code EC, char const *Msg);
 
+/// This class wraps a filename and another Error.
+///
+/// In some cases, an error needs to live along a 'source' name, in order to
+/// show more detailed information to the user.
+class FileError final : public ErrorInfo<FileError> {
+
+  template <class Err>
+  friend Error createFileError(
+      std::string, Err,
+      typename std::enable_if<std::is_base_of<Error, Err>::value &&
+                              !std::is_base_of<ErrorSuccess, Err>::value>::type
+          *);
+
+public:
+  void log(raw_ostream &OS) const override {
+    assert(Err && !FileName.empty() && "Trying to log after takeError().");
+    OS << "'" << FileName << "': ";
+    Err->log(OS);
+  }
+
+  Error takeError() { return Error(std::move(Err)); }
+
+  std::error_code convertToErrorCode() const override;
+
+  // Used by ErrorInfo::classID.
+  static char ID;
+
+private:
+  FileError(std::string F, std::unique_ptr<ErrorInfoBase> E) {
+    assert(E && "Cannot create FileError from Error success value.");
+    assert(!F.empty() &&
+           "The file name provided to FileError must not be empty.");
+    FileName = F;
+    Err = std::move(E);
+  }
+
+  static Error build(std::string F, Error E) {
+    return Error(std::unique_ptr<FileError>(new FileError(F, E.takePayload())));
+  }
+
+  std::string FileName;
+  std::unique_ptr<ErrorInfoBase> Err;
+};
+
+/// Concatenate a source file path and/or name with an Error. The resulting
+/// Error is unchecked.
+template <class Err>
+inline Error createFileError(
+    std::string F, Err E,
+    typename std::enable_if<std::is_base_of<Error, Err>::value &&
+                            !std::is_base_of<ErrorSuccess, Err>::value>::type
+        * = nullptr) {
+  return FileError::build(F, std::move(E));
+}
+
 /// Helper for check-and-exit error handling.
 ///
 /// For tool use only. NOT FOR USE IN LIBRARY CODE.
 ///
 class ExitOnError {
 public:
   /// Create an error on exit helper.
   ExitOnError(std::string Banner = "", int DefaultErrorExitCode = 1)

llvm/trunk/lib/Support/Error.cpp

 #include <system_error>
 
 using namespace llvm;
 
 namespace {
 
   enum class ErrorErrorCode : int {
     MultipleErrors = 1,
+    FileError,
     InconvertibleError
   };
 
   // FIXME: This class is only here to support the transition to llvm::Error. It
   // will be removed once this transition is complete. Clients should prefer to
   // deal with the Error value directly, rather than converting to error_code.
   class ErrorErrorCategory : public std::error_category {
   public:
     const char *name() const noexcept override { return "Error"; }
 
     std::string message(int condition) const override {
       switch (static_cast<ErrorErrorCode>(condition)) {
       case ErrorErrorCode::MultipleErrors:
         return "Multiple errors";
       case ErrorErrorCode::InconvertibleError:
         return "Inconvertible error value. An error has occurred that could "
                "not be converted to a known std::error_code. Please file a "
                "bug.";
+      case ErrorErrorCode::FileError:
+          return "A file error occurred.";
       }
       llvm_unreachable("Unhandled error code");
     }
   };
 
 }
 
 static ManagedStatic<ErrorErrorCategory> ErrorErrorCat;
 
 namespace llvm {
 
 void ErrorInfoBase::anchor() {}
 char ErrorInfoBase::ID = 0;
 char ErrorList::ID = 0;
 char ECError::ID = 0;
 char StringError::ID = 0;
+char FileError::ID = 0;
 
 void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner) {
   if (!E)
     return;
   OS << ErrorBanner;
   handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
     EI.log(OS);
     OS << "\n";
   });
 }
 
 
 std::error_code ErrorList::convertToErrorCode() const {
   return std::error_code(static_cast<int>(ErrorErrorCode::MultipleErrors),
                          *ErrorErrorCat);
 }
 
 std::error_code inconvertibleErrorCode() {
   return std::error_code(static_cast<int>(ErrorErrorCode::InconvertibleError),
                          *ErrorErrorCat);
 }
 
+std::error_code FileError::convertToErrorCode() const {
+  return std::error_code(static_cast<int>(ErrorErrorCode::FileError),
+                         *ErrorErrorCat);
+}
+
 Error errorCodeToError(std::error_code EC) {
   if (!EC)
     return Error::success();
   return Error(llvm::make_unique<ECError>(ECError(EC)));
 }
 
 std::error_code errorToErrorCode(Error Err) {
   std::error_code EC;
if (getPtr())
     getPtr()->log(dbgs());
   else
     dbgs() << "Error value was Success. (Note: Success values must still be "
               "checked prior to being destroyed).\n";
   abort();
 }
 #endif
 
-StringError::StringError(const Twine &S, std::error_code EC)
+StringError::StringError(std::error_code EC, const Twine &S)
     : Msg(S.str()), EC(EC) {}
 
-void StringError::log(raw_ostream &OS) const { OS << Msg; }
+StringError::StringError(const Twine &S, std::error_code EC)
+    : Msg(S.str()), EC(EC), PrintMsgOnly(true) {}
+
+void StringError::log(raw_ostream &OS) const {
+  if (PrintMsgOnly) {
+    OS << Msg;
+  } else {
+    OS << EC.message();
+    if (!Msg.empty())
+      OS << (" " + Msg);
+  }
+}
 
 std::error_code StringError::convertToErrorCode() const {
   return EC;
 }
 
 Error createStringError(std::error_code EC, char const *Msg) {
   return make_error<StringError>(Msg, EC);
 }

llvm/trunk/unittests/Support/ErrorTest.cpp

 //===----- unittests/ErrorTest.cpp - Error.h tests ------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Error.h"
 #include "llvm-c/Error.h"
 
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 #include <memory>
 
 using namespace llvm;
 
 namespace {

 
 // Test that success values implicitly convert to false, and don't cause crashes
 // once they've been implicitly converted.
 TEST(Error, CheckedSuccess) {
   Error E = Error::success();
   EXPECT_FALSE(E) << "Unexpected error while testing Error 'Success'";
 }
 
-// Test that unchecked succes values cause an abort.
+// Test that unchecked success values cause an abort.
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
 TEST(Error, UncheckedSuccess) {
   EXPECT_DEATH({ Error E = Error::success(); },
                "Program aborted due to an unhandled Error:")
       << "Unchecked Error Succes value did not cause abort()";
 }
 #endif
 
handleAllErrors(
     },
     [&](CustomError &CE) {
       GotCE = true;
     });
   EXPECT_TRUE(GotCSE) << "Failed to round-trip ErrorList via C API";
   EXPECT_TRUE(GotCE) << "Failed to round-trip ErrorList via C API";
 }
 
+TEST(Error, FileErrorTest) {
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+    EXPECT_DEATH(
+      {
+        Error S = Error::success();
+        createFileError("file.bin", std::move(S));
+      },
+      "");
+#endif
+  Error E1 = make_error<CustomError>(1);
+  Error FE1 = createFileError("file.bin", std::move(E1));
+  EXPECT_EQ(toString(std::move(FE1)).compare("'file.bin': CustomError {1}"), 0);
+
+  Error E2 = make_error<CustomError>(2);
+  Error FE2 = createFileError("file.bin", std::move(E2));
+  handleAllErrors(std::move(FE2), [](const FileError &F) {
+    EXPECT_EQ(F.message().compare("'file.bin': CustomError {2}"), 0);
+  });
+
+  Error E3 = make_error<CustomError>(3);
+  Error FE3 = createFileError("file.bin", std::move(E3));
+  auto E31 = handleErrors(std::move(FE3), [](std::unique_ptr<FileError> F) {
+    return F->takeError();
+  });
+  handleAllErrors(std::move(E31), [](const CustomError &C) {
+    EXPECT_EQ(C.message().compare("CustomError {3}"), 0);
+  });
+
+  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);
+}
+
+enum class test_error_code {
+  unspecified = 1,
+  error_1,
+  error_2,
+};
+
 } // end anon namespace
+
+namespace std {
+    template <>
+    struct is_error_code_enum<test_error_code> : std::true_type {};
+} // namespace std
+
+namespace {
+
+const std::error_category &TErrorCategory();
+
+inline std::error_code make_error_code(test_error_code E) {
+    return std::error_code(static_cast<int>(E), TErrorCategory());
+}
+
+class TestDebugError : public ErrorInfo<TestDebugError, StringError> {
+public:
+    using ErrorInfo<TestDebugError, StringError >::ErrorInfo; // inherit constructors
+    TestDebugError(const Twine &S) : ErrorInfo(S, test_error_code::unspecified) {}
+    static char ID;
+};
+
+class TestErrorCategory : public std::error_category {
+public:
+  const char *name() const noexcept override { return "error"; }
+  std::string message(int Condition) const override {
+    switch (static_cast<test_error_code>(Condition)) {
+    case test_error_code::unspecified:
+      return "An unknown error has occurred.";
+    case test_error_code::error_1:
+      return "Error 1.";
+    case test_error_code::error_2:
+      return "Error 2.";
+    }
+    llvm_unreachable("Unrecognized test_error_code");
+  }
+};
+
+static llvm::ManagedStatic<TestErrorCategory> TestErrCategory;
+const std::error_category &TErrorCategory() { return *TestErrCategory; }
+
+char TestDebugError::ID;
+
+TEST(Error, SubtypeStringErrorTest) {
+  auto E1 = make_error<TestDebugError>(test_error_code::error_1);
+  EXPECT_EQ(toString(std::move(E1)).compare("Error 1."), 0);
+
+  auto E2 = make_error<TestDebugError>(test_error_code::error_1,
+                                       "Detailed information");
+  EXPECT_EQ(toString(std::move(E2)).compare("Error 1. Detailed information"),
+            0);
+
+  auto E3 = make_error<TestDebugError>(test_error_code::error_2);
+  handleAllErrors(std::move(E3), [](const TestDebugError &F) {
+    EXPECT_EQ(F.message().compare("Error 2."), 0);
+  });
+
+  auto E4 = joinErrors(make_error<TestDebugError>(test_error_code::error_1,
+                                                  "Detailed information"),
+                       make_error<TestDebugError>(test_error_code::error_2));
+  EXPECT_EQ(toString(std::move(E4))
+                .compare("Error 1. Detailed information\n"
+                         "Error 2."),
+            0);
+}
+
+} // namespace