[lldb] Store expression evaluator diagnostics in an llvm::Error (NFC)

This patch is a reworking of Pete Lawrence's (@PortalPete) proposal
for better expression evaluator error messages:
#80938

This patch is preparatory patch for improving the rendering of
expression evaluator diagnostics. Currently diagnostics are rendered
into a string and the command interpreter layer then textually parses
words like "error:" to (sometimes) color the output accordingly. In
order to enable user interfaces to do better with diagnostics, we need
to store them in a machine-readable fromat. This patch does this by
adding a new llvm::Error kind wrapping a DiagnosticDetail struct that
is used when the error type is eErrorTypeExpression. Multiple
diagnostics are modeled using llvm::ErrorList.

Right now the extra information is not used by the CommandInterpreter,
this will be added in a follow-up patch!

Author: adrian-prantl

lldb/include/lldb/Expression/DiagnosticManager.h

@@ -12,6 +12,9 @@
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-types.h"
 
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Status.h"
+
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -20,6 +23,53 @@
 
 namespace lldb_private {
 
+/// A compiler-independent representation of a Diagnostic. Expression
+/// evaluation failures often have more than one diagnostic that a UI
+/// layer might want to render differently, for example to colorize
+/// it.
+///
+/// Running example:
+///   (lldb) expr 1+foo
+///   error: <user expression 0>:1:3: use of undeclared identifier 'foo'
+///   1+foo
+///     ^
+struct DiagnosticDetail {
+  struct SourceLocation {
+    FileSpec file;
+    unsigned line = 0;
+    uint16_t column = 0;
+    uint16_t length = 0;
+    bool in_user_input = false;
+  };
+  /// Contains {{}, 1, 3, 3, true} in the example above.
+  std::optional<SourceLocation> source_location;
+  /// Contains eSeverityError in the example above.
+  lldb::Severity severity = lldb::eSeverityInfo;
+  /// Contains "use of undeclared identifier 'x'" in the example above.
+  std::string message;
+  /// Contains the fully rendered error message.
+  std::string rendered;
+};
+
+/// An llvm::Error used to communicate diagnostics in Status. Multiple
+/// diagnostics may be chained in an llvm::ErrorList.
+class ExpressionError
+    : public llvm::ErrorInfo<ExpressionError, ExpressionErrorBase> {
+  std::string m_message;
+  std::vector<DiagnosticDetail> m_details;
+
+public:
+  static char ID;
+  using llvm::ErrorInfo<ExpressionError, ExpressionErrorBase>::ErrorInfo;
+  ExpressionError(lldb::ExpressionResults result, std::string msg,
+                  std::vector<DiagnosticDetail> details = {});
+  std::string message() const override;
+  llvm::ArrayRef<DiagnosticDetail> GetDetail() const { return m_details; }
+  std::error_code convertToErrorCode() const override;
+  void log(llvm::raw_ostream &OS) const override;
+  std::unique_ptr<CloneableError> Clone() const override;
+};
+
 enum DiagnosticOrigin {
   eDiagnosticOriginUnknown = 0,
   eDiagnosticOriginLLDB,
@@ -49,37 +99,28 @@ class Diagnostic {
     }
   }
 
-  Diagnostic(llvm::StringRef message, lldb::Severity severity,
-             DiagnosticOrigin origin, uint32_t compiler_id)
-      : m_message(message), m_severity(severity), m_origin(origin),
-        m_compiler_id(compiler_id) {}
-
-  Diagnostic(const Diagnostic &rhs)
-      : m_message(rhs.m_message), m_severity(rhs.m_severity),
-        m_origin(rhs.m_origin), m_compiler_id(rhs.m_compiler_id) {}
+  Diagnostic(DiagnosticOrigin origin, uint32_t compiler_id,
+             DiagnosticDetail detail)
+      : m_origin(origin), m_compiler_id(compiler_id), m_detail(detail) {}
 
   virtual ~Diagnostic() = default;
 
   virtual bool HasFixIts() const { return false; }
 
-  lldb::Severity GetSeverity() const { return m_severity; }
+  lldb::Severity GetSeverity() const { return m_detail.severity; }
 
   uint32_t GetCompilerID() const { return m_compiler_id; }
 
-  llvm::StringRef GetMessage() const { return m_message; }
+  llvm::StringRef GetMessage() const { return m_detail.message; }
+  const DiagnosticDetail &GetDetail() const { return m_detail; }
 
-  void AppendMessage(llvm::StringRef message,
-                     bool precede_with_newline = true) {
-    if (precede_with_newline)
-      m_message.push_back('\n');
-    m_message += message;
-  }
+  void AppendMessage(llvm::StringRef message, bool precede_with_newline = true);
 
 protected:
-  std::string m_message;
-  lldb::Severity m_severity;
   DiagnosticOrigin m_origin;
-  uint32_t m_compiler_id; // Compiler-specific diagnostic ID
+  /// Compiler-specific diagnostic ID.
+  uint32_t m_compiler_id;
+  DiagnosticDetail m_detail;
 };
 
 typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList;
@@ -102,10 +143,7 @@ class DiagnosticManager {
 
   void AddDiagnostic(llvm::StringRef message, lldb::Severity severity,
                      DiagnosticOrigin origin,
-                     uint32_t compiler_id = LLDB_INVALID_COMPILER_ID) {
-    m_diagnostics.emplace_back(
-        std::make_unique<Diagnostic>(message, severity, origin, compiler_id));
-  }
+                     uint32_t compiler_id = LLDB_INVALID_COMPILER_ID);
 
   void AddDiagnostic(std::unique_ptr<Diagnostic> diagnostic) {
     if (diagnostic)
@@ -130,6 +168,11 @@ class DiagnosticManager {
       m_diagnostics.back()->AppendMessage(str);
   }
 
+  /// Copies the diagnostics into an llvm::Error{List}.
+  /// The first diagnostic wraps \c result.
+  llvm::Error GetAsError(lldb::ExpressionResults result,
+                         llvm::Twine message = {}) const;
+
   // Returns a string containing errors in this format:
   //
   // "error: error text\n

lldb/include/lldb/Utility/Status.h

@@ -75,18 +75,13 @@ class Win32Error : public llvm::ErrorInfo<Win32Error, CloneableECError> {
   static char ID;
 };
 
-class ExpressionError
-    : public llvm::ErrorInfo<ExpressionError, CloneableECError> {
+class ExpressionErrorBase
+    : public llvm::ErrorInfo<ExpressionErrorBase, CloneableECError> {
 public:
-  using llvm::ErrorInfo<ExpressionError, CloneableECError>::ErrorInfo;
-  ExpressionError(std::error_code ec, std::string msg = {})
-      : ErrorInfo(ec), m_string(msg) {}
-  std::unique_ptr<CloneableError> Clone() const override;
-  std::string message() const override { return m_string; }
+  using llvm::ErrorInfo<ExpressionErrorBase, CloneableECError>::ErrorInfo;
+  ExpressionErrorBase(std::error_code ec, std::string msg = {})
+      : ErrorInfo(ec) {}
   static char ID;
-
-protected:
-  std::string m_string;
 };
 
 /// \class Status Status.h "lldb/Utility/Status.h" An error handling class.
@@ -160,9 +155,6 @@ class Status {
     return Status(llvm::formatv(format, std::forward<Args>(args)...));
   }
 
-  static Status FromExpressionError(lldb::ExpressionResults result,
-                                    std::string msg);
-
   /// Set the current error to errno.
   ///
   /// Update the error value to be \c errno and update the type to be \c
@@ -175,8 +167,11 @@ class Status {
   /// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
   static Status FromError(llvm::Error error);
 
-  /// FIXME: Replace this with a takeError() method.
+  /// FIXME: Replace all uses with takeError() instead.
   llvm::Error ToError() const;
+
+  llvm::Error takeError() { return std::move(m_error); }
+
   /// Don't call this function in new code. Instead, redesign the API
   /// to use llvm::Expected instead of Status.
   Status Clone() const { return Status(ToError()); }

lldb/source/Breakpoint/BreakpointLocation.cpp

@@ -264,9 +264,10 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx,
     if (!m_user_expression_sp->Parse(diagnostics, exe_ctx,
                                      eExecutionPolicyOnlyWhenNeeded, true,
                                      false)) {
-      error = Status::FromErrorStringWithFormat(
-          "Couldn't parse conditional expression:\n%s",
-          diagnostics.GetString().c_str());
+      error = Status::FromError(
+          diagnostics.GetAsError(lldb::eExpressionParseError,
+                                 "Couldn't parse conditional expression:"));
+
       m_user_expression_sp.reset();
       return true;
     }
@@ -324,8 +325,8 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx,
     }
   } else {
     ret = false;
-    error = Status::FromErrorStringWithFormat(
-        "Couldn't execute expression:\n%s", diagnostics.GetString().c_str());
+    error = Status::FromError(diagnostics.GetAsError(
+        lldb::eExpressionParseError, "Couldn't execute expression:"));
   }
 
   return ret;

lldb/source/Expression/DiagnosticManager.cpp

@@ -14,23 +14,29 @@
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb_private;
+char ExpressionError::ID;
 
-void DiagnosticManager::Dump(Log *log) {
-  if (!log)
-    return;
-
-  std::string str = GetString();
-
-  // GetString() puts a separator after each diagnostic. We want to remove the
-  // last '\n' because log->PutCString will add one for us.
-
-  if (str.size() && str.back() == '\n') {
-    str.pop_back();
+/// A std::error_code category for eErrorTypeExpression.
+class ExpressionCategory : public std::error_category {
+  const char *name() const noexcept override {
+    return "LLDBExpressionCategory";
   }
-
-  log->PutCString(str.c_str());
+  std::string message(int __ev) const override {
+    return ExpressionResultAsCString(
+        static_cast<lldb::ExpressionResults>(__ev));
+  };
+};
+ExpressionCategory &expression_category() {
+  static ExpressionCategory g_expression_category;
+  return g_expression_category;
 }
 
+ExpressionError::ExpressionError(lldb::ExpressionResults result,
+                                 std::string msg,
+                                 std::vector<DiagnosticDetail> details)
+    : ErrorInfo(std::error_code(result, expression_category())), m_message(msg),
+      m_details(details) {}
+
 static const char *StringForSeverity(lldb::Severity severity) {
   switch (severity) {
   // this should be exhaustive
@@ -44,9 +50,33 @@ static const char *StringForSeverity(lldb::Severity severity) {
   llvm_unreachable("switch needs another case for lldb::Severity enum");
 }
 
+std::string ExpressionError::message() const {
+  std::string str;
+  {
+    llvm::raw_string_ostream os(str);
+    if (!m_message.empty())
+      os << m_message << '\n';
+    for (const auto &detail : m_details)
+      os << StringForSeverity(detail.severity) << detail.rendered << '\n';
+  }
+  return str;
+}
+
+std::error_code ExpressionError::convertToErrorCode() const {
+  return llvm::inconvertibleErrorCode();
+}
+
+void ExpressionError::log(llvm::raw_ostream &OS) const { OS << message(); }
+
+std::unique_ptr<CloneableError> ExpressionError::Clone() const {
+  return std::make_unique<ExpressionError>(
+      (lldb::ExpressionResults)convertToErrorCode().value(), m_message,
+      m_details);
+}
+
 std::string DiagnosticManager::GetString(char separator) {
-  std::string ret;
-  llvm::raw_string_ostream stream(ret);
+  std::string str;
+  llvm::raw_string_ostream stream(str);
 
   for (const auto &diagnostic : Diagnostics()) {
     llvm::StringRef severity = StringForSeverity(diagnostic->GetSeverity());
@@ -61,8 +91,39 @@ std::string DiagnosticManager::GetString(char separator) {
       stream << message.drop_front(severity_pos + severity.size());
     stream << separator;
   }
+  return str;
+}
+
+void DiagnosticManager::Dump(Log *log) {
+  if (!log)
+    return;
 
-  return ret;
+  std::string str = GetString();
+
+  // We want to remove the last '\n' because log->PutCString will add
+  // one for us.
+
+  if (!str.empty() && str.back() == '\n')
+    str.pop_back();
+
+  log->PutString(str);
+}
+
+llvm::Error DiagnosticManager::GetAsError(lldb::ExpressionResults result,
+                                          llvm::Twine message) const {
+  std::vector<DiagnosticDetail> details;
+  for (const auto &diag : m_diagnostics)
+    details.push_back(diag->GetDetail());
+  return llvm::make_error<ExpressionError>(result, message.str(), details);
+}
+
+void DiagnosticManager::AddDiagnostic(llvm::StringRef message,
+                                      lldb::Severity severity,
+                                      DiagnosticOrigin origin,
+                                      uint32_t compiler_id) {
+  m_diagnostics.emplace_back(std::make_unique<Diagnostic>(
+      origin, compiler_id,
+      DiagnosticDetail{{}, severity, message.str(), message.str()}));
 }
 
 size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format,
@@ -85,3 +146,13 @@ void DiagnosticManager::PutString(lldb::Severity severity,
     return;
   AddDiagnostic(str, severity, eDiagnosticOriginLLDB);
 }
+
+void Diagnostic::AppendMessage(llvm::StringRef message,
+                               bool precede_with_newline) {
+  if (precede_with_newline) {
+    m_detail.message.push_back('\n');
+    m_detail.rendered.push_back('\n');
+  }
+  m_detail.message += message;
+  m_detail.rendered += message;
+}

lldb/source/Expression/ExpressionParser.cpp

@@ -63,9 +63,8 @@ ExpressionParser::RunStaticInitializers(IRExecutionUnitSP &execution_unit_sp,
             exe_ctx, call_static_initializer, options, execution_errors);
 
     if (results != eExpressionCompleted) {
-      err = Status::FromErrorStringWithFormat(
-          "couldn't run static initializer: %s",
-          execution_errors.GetString().c_str());
+      err = Status::FromError(execution_errors.GetAsError(
+          lldb::eExpressionSetupError, "couldn't run static initializer:"));
       return err;
     }
   }