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

[adrian-prantl]

…NFC]

This patch is the first patch in a series reworking of Pete Lawrence's (@PortalPete) amazing 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!

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Adrian Prantl (adrian-prantl)

Changes

…NFC]

This patch is the first patch in a series reworking of Pete Lawrence's (@PortalPete) amazing 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 Status::Detail vector to Status that is used when the error type is eErrorTypeExpression. I tried my best to minimize the overhead this adds to Status in all other use-cases.

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

---

Full diff: https://github.com/llvm/llvm-project/pull/106442.diff

9 Files Affected:

• (modified) lldb/include/lldb/Expression/DiagnosticManager.h (+22-28)
• (modified) lldb/include/lldb/Utility/Status.h (+48-2)
• (modified) lldb/source/Expression/DiagnosticManager.cpp (+27)
• (modified) lldb/source/Expression/UserExpression.cpp (+13-13)
• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h (+2-3)
• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+40-7)
• (modified) lldb/source/Utility/Status.cpp (+21-4)
• (modified) lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py (+1-1)
• (modified) lldb/unittests/Expression/DiagnosticManagerTest.cpp (+6-6)

diff --git a/lldb/include/lldb/Expression/DiagnosticManager.h b/lldb/include/lldb/Expression/DiagnosticManager.h
index d49b7c99b114fb..252492ce8f776a 100644
--- a/lldb/include/lldb/Expression/DiagnosticManager.h
+++ b/lldb/include/lldb/Expression/DiagnosticManager.h
@@ -12,6 +12,8 @@
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-types.h"
 
+#include "lldb/Utility/Status.h"
+
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 
@@ -49,37 +51,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,
+             Status::Detail 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; }
+  Status::Detail 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;
+  Status::Detail m_detail;
 };
 
 typedef std::vector<std::unique_ptr<Diagnostic>> DiagnosticList;
@@ -102,10 +95,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,13 +120,17 @@ class DiagnosticManager {
       m_diagnostics.back()->AppendMessage(str);
   }
 
-  // Returns a string containing errors in this format:
-  //
-  // "error: error text\n
-  // warning: warning text\n
-  // remark text\n"
+  /// Returns a string containing errors in this format:
+  ///
+  ///     "error: error text\n
+  ///     warning: warning text\n
+  ///     remark text\n"
+  LLVM_DEPRECATED("Use GetAsStatus instead", "GetAsStatus()")
   std::string GetString(char separator = '\n');
 
+  /// Copies the diagnostics into an lldb::Status.
+  Status GetAsStatus(lldb::ExpressionResults result);
+
   void Dump(Log *log);
 
   const std::string &GetFixedExpression() { return m_fixed_expression; }
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index b304291ffae00e..5a9683e09c6e53 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILITY_STATUS_H
 #define LLDB_UTILITY_STATUS_H
 
+#include "lldb/Utility/FileSpec.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/StringRef.h"
@@ -47,7 +48,34 @@ class Status {
   /// into ValueType.
   typedef uint32_t ValueType;
 
-  Status();
+  /// A customizable "detail" for an error. For example, expression
+  /// evaluation failures often have more than one diagnostic that the
+  /// UI layer might want to render differently.
+  ///
+  /// Running example:
+  ///   (lldb) expr 1+x
+  ///   error: <user expression 0>:1:3: use of undeclared identifier 'foo'
+  ///   1+foo
+  ///     ^
+  struct Detail {
+    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;
+  };
+
+  Status() = default;
 
   /// Initialize the error object with a generic success value.
   ///
@@ -57,7 +85,8 @@ class Status {
   /// \param[in] type
   ///     The type for \a err.
   explicit Status(ValueType err, lldb::ErrorType type = lldb::eErrorTypeGeneric,
-                  std::string msg = {});
+                  std::string msg = {})
+      : m_code(err), m_type(type), m_string(std::move(msg)) {}
 
   Status(std::error_code EC);
 
@@ -83,6 +112,12 @@ class Status {
     return Status(result, lldb::eErrorTypeExpression, msg);
   }
 
+  static Status
+  FromExpressionErrorDetails(lldb::ExpressionResults result,
+                             llvm::SmallVectorImpl<Detail> &&details) {
+    return Status(result, lldb::eErrorTypeExpression, std::move(details));
+  }
+
   /// Set the current error to errno.
   ///
   /// Update the error value to be \c errno and update the type to be \c
@@ -144,13 +179,24 @@ class Status {
   ///     success (non-erro), \b false otherwise.
   bool Success() const;
 
+  /// Get the list of details for this error. If this is non-empty,
+  /// clients can use this to render more appealing error messages
+  /// from the details. If you just want a pre-rendered string, use
+  /// AsCString() instead. Currently details are only used for
+  /// eErrorTypeExpression.
+  llvm::ArrayRef<Detail> GetDetails() const;
+
 protected:
+  /// Separate so the main constructor doesn't need to deal with the array.
+  Status(ValueType err, lldb::ErrorType type,
+         llvm::SmallVectorImpl<Detail> &&details);
   /// Status code as an integer value.
   ValueType m_code = 0;
   /// The type of the above error code.
   lldb::ErrorType m_type = lldb::eErrorTypeInvalid;
   /// A string representation of the error code.
   mutable std::string m_string;
+  llvm::SmallVector<Detail, 0> m_details;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Expression/DiagnosticManager.cpp b/lldb/source/Expression/DiagnosticManager.cpp
index a8330138f3d53b..5c20eba1dbd07e 100644
--- a/lldb/source/Expression/DiagnosticManager.cpp
+++ b/lldb/source/Expression/DiagnosticManager.cpp
@@ -65,6 +65,23 @@ std::string DiagnosticManager::GetString(char separator) {
   return ret;
 }
 
+Status DiagnosticManager::GetAsStatus(lldb::ExpressionResults result) {
+  llvm::SmallVector<Status::Detail, 0> details;
+  details.reserve(m_diagnostics.size());
+  for (const auto &diagnostic : m_diagnostics)
+    details.push_back(diagnostic->GetDetail());
+  return Status::FromExpressionErrorDetails(result, std::move(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,
+      Status::Detail{{}, severity, message.str(), message.str()}));
+}
+
 size_t DiagnosticManager::Printf(lldb::Severity severity, const char *format,
                                  ...) {
   StreamString ss;
@@ -85,3 +102,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;
+}
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index c2889e4c986bfe..159714489ad111 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/lldb/source/Expression/UserExpression.cpp
@@ -328,18 +328,19 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
     }
 
     if (!parse_success) {
-      std::string msg;
-      {
-        llvm::raw_string_ostream os(msg);
-        if (!diagnostic_manager.Diagnostics().empty())
-          os << diagnostic_manager.GetString();
-        else
-          os << "expression failed to parse (no further compiler diagnostics)";
-        if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
-            !fixed_expression->empty())
-          os << "\nfixed expression suggested:\n  " << *fixed_expression;
+      if (target->GetEnableNotifyAboutFixIts() && fixed_expression &&
+          !fixed_expression->empty()) {
+        std::string fixit =
+            "fixed expression suggested:\n  " + *fixed_expression;
+        diagnostic_manager.AddDiagnostic(fixit, lldb::eSeverityInfo,
+                                         eDiagnosticOriginLLDB);
       }
-      error = Status::FromExpressionError(execution_results, msg);
+      if (!diagnostic_manager.Diagnostics().size())
+        error = Status::FromExpressionError(
+            execution_results,
+            "expression failed to parse (no further compiler diagnostics)");
+      else
+        error = diagnostic_manager.GetAsStatus(execution_results);
     }
   }
 
@@ -384,8 +385,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
           error = Status::FromExpressionError(
               execution_results, "expression failed to execute, unknown error");
         else
-          error = Status::FromExpressionError(execution_results,
-                                              diagnostic_manager.GetString());
+          error = diagnostic_manager.GetAsStatus(execution_results);
       } else {
         if (expr_result) {
           result_valobj_sp = expr_result->GetValueObject();
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
index 21abd71cc34eeb..f6e635afbf68ca 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
@@ -29,9 +29,8 @@ class ClangDiagnostic : public Diagnostic {
     return diag->getKind() == eDiagnosticOriginClang;
   }
 
-  ClangDiagnostic(llvm::StringRef message, lldb::Severity severity,
-                  uint32_t compiler_id)
-      : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {}
+  ClangDiagnostic(Status::Detail detail, uint32_t compiler_id)
+      : Diagnostic(eDiagnosticOriginClang, compiler_id, detail) {}
 
   ~ClangDiagnostic() override = default;
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 90f26de939a478..6b524250bbf4de 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -26,6 +26,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
+#include "clang/Frontend/TextDiagnostic.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/Preprocessor.h"
@@ -212,20 +213,20 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
     m_passthrough->HandleDiagnostic(DiagLevel, Info);
     m_os->flush();
 
-    lldb::Severity severity;
+    Status::Detail detail;
     bool make_new_diagnostic = true;
 
     switch (DiagLevel) {
     case DiagnosticsEngine::Level::Fatal:
     case DiagnosticsEngine::Level::Error:
-      severity = lldb::eSeverityError;
+      detail.severity = lldb::eSeverityError;
       break;
     case DiagnosticsEngine::Level::Warning:
-      severity = lldb::eSeverityWarning;
+      detail.severity = lldb::eSeverityWarning;
       break;
     case DiagnosticsEngine::Level::Remark:
     case DiagnosticsEngine::Level::Ignored:
-      severity = lldb::eSeverityInfo;
+      detail.severity = lldb::eSeverityInfo;
       break;
     case DiagnosticsEngine::Level::Note:
       m_manager->AppendMessageToDiagnostic(m_output);
@@ -254,14 +255,46 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
       std::string stripped_output =
           std::string(llvm::StringRef(m_output).trim());
 
-      auto new_diagnostic = std::make_unique<ClangDiagnostic>(
-          stripped_output, severity, Info.getID());
+      // Translate the source location.
+      if (Info.hasSourceManager()) {
+        Status::Detail::SourceLocation loc;
+        clang::SourceManager &sm = Info.getSourceManager();
+        const clang::SourceLocation sloc = Info.getLocation();
+        if (sloc.isValid()) {
+          const clang::FullSourceLoc fsloc(sloc, sm);
+          clang::PresumedLoc PLoc = fsloc.getPresumedLoc(true);
+          StringRef filename =
+              PLoc.isValid() ? PLoc.getFilename() : StringRef{};
+          loc.file = FileSpec(filename);
+          loc.line = fsloc.getSpellingLineNumber();
+          loc.column = fsloc.getSpellingColumnNumber();
+          // A heuristic detecting the #line 1 "<user expression 1>".
+          loc.in_user_input = filename.starts_with("<user expression ");
+          // Find the range of the primary location.
+          for (const auto &range : Info.getRanges()) {
+            if (range.getBegin() == sloc) {
+              // FIXME: This is probably not handling wide characters correctly.
+              unsigned end_col = sm.getSpellingColumnNumber(range.getEnd());
+              if (end_col > loc.column)
+                loc.length = end_col - loc.column;
+              break;
+            }
+          }
+          detail.source_location = loc;
+        }
+      }
+      llvm::SmallString<0> msg;
+      Info.FormatDiagnostic(msg);
+      detail.message = msg.str();
+      detail.rendered = stripped_output;
+      auto new_diagnostic =
+          std::make_unique<ClangDiagnostic>(detail, Info.getID());
 
       // Don't store away warning fixits, since the compiler doesn't have
       // enough context in an expression for the warning to be useful.
       // FIXME: Should we try to filter out FixIts that apply to our generated
       // code, and not the user's expression?
-      if (severity == lldb::eSeverityError)
+      if (detail.severity == lldb::eSeverityError)
         AddAllFixIts(new_diagnostic.get(), Info);
 
       m_manager->AddDiagnostic(std::move(new_diagnostic));
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 7260b7b3e0a03e..3b552e5540df61 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -37,10 +37,9 @@ class raw_ostream;
 using namespace lldb;
 using namespace lldb_private;
 
-Status::Status() {}
-
-Status::Status(ValueType err, ErrorType type, std::string msg)
-    : m_code(err), m_type(type), m_string(std::move(msg)) {}
+Status::Status(ValueType err, ErrorType type,
+               llvm::SmallVectorImpl<Status::Detail> &&details)
+    : m_code(err), m_type(type), m_details(std::move(details)) {}
 
 // This logic is confusing because c++ calls the traditional (posix) errno codes
 // "generic errors", while we use the term "generic" to mean completely
@@ -133,6 +132,19 @@ static std::string RetrieveWin32ErrorString(uint32_t error_code) {
 }
 #endif
 
+static const char *StringForSeverity(lldb::Severity severity) {
+  switch (severity) {
+  // this should be exhaustive
+  case lldb::eSeverityError:
+    return "error: ";
+  case lldb::eSeverityWarning:
+    return "warning: ";
+  case lldb::eSeverityInfo:
+    return "";
+  }
+  llvm_unreachable("switch needs another case for lldb::Severity enum");
+}
+
 // Get the error value as a NULL C string. The error string will be fetched and
 // cached on demand. The cached error string value will remain until the error
 // value is changed or cleared.
@@ -159,6 +171,11 @@ const char *Status::AsCString(const char *default_error_str) const {
 #endif
       break;
 
+    case eErrorTypeExpression: {
+      llvm::raw_string_ostream stream(m_string);
+      for (const auto &detail : m_details)
+        stream << StringForSeverity(detail.severity) << detail.rendered << '\n';
+    } break;
     default:
       break;
     }
diff --git a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
index 620b6e44fc852a..36e302be2525b5 100644
--- a/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
+++ b/lldb/test/API/lang/objc/modules-compile-error/TestModulesCompileError.py
@@ -21,7 +21,7 @@ def test(self):
             "expr @import LLDBTestModule",
             error=True,
             substrs=[
-                "module.h:4:1: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
+                "module.h:4:1: error: use of undeclared identifier 'syntax_error_for_lldb_to_find'",
                 "syntax_error_for_lldb_to_find // comment that tests source printing",
                 "could not build module 'LLDBTestModule'",
             ],
diff --git a/lldb/unittests/Expression/DiagnosticManagerTest.cpp b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
index 05fe7c164d6818..bf554020ad8924 100644
--- a/lldb/unittests/Expression/DiagnosticManagerTest.cpp
+++ b/lldb/unittests/Expression/DiagnosticManagerTest.cpp
@@ -19,8 +19,8 @@ class FixItDiag : public Diagnostic {
 
 public:
   FixItDiag(llvm::StringRef msg, bool has_fixits)
-      : Diagnostic(msg, lldb::eSeverityError,
-                   DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id),
+      : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+                   Status::Detail{{}, lldb::eSeverityError, msg.str(), {}}),
         m_has_fixits(has_fixits) {}
   bool HasFixIts() const override { return m_has_fixits; }
 };
@@ -30,8 +30,8 @@ namespace {
 class TextDiag : public Diagnostic {
 public:
   TextDiag(llvm::StringRef msg, lldb::Severity severity)
-      : Diagnostic(msg, severity, DiagnosticOrigin::eDiagnosticOriginLLDB,
-                   custom_diag_id) {}
+      : Diagnostic(DiagnosticOrigin::eDiagnosticOriginLLDB, custom_diag_id,
+                   Status::Detail{{}, severity, msg.str(), {}}) {}
 };
 } // namespace
 
@@ -42,8 +42,8 @@ TEST(DiagnosticManagerTest, AddDiagnostic) {
   std::string msg = "foo bar has happened";
   lldb::Severity severity = lldb::eSeverityError;
   DiagnosticOrigin origin = DiagnosticOrigin::eDiagnosticOriginLLDB;
-  auto diag =
-      std::make_unique<Diagnostic>(msg, severity, origin, custom_diag_id);
+  auto diag = std::make_unique<Diagnostic>(
+      origin, custom_diag_id, Status::Detail{{}, severity, msg, {}});
   mgr.AddDiagnostic(std::move(diag));
   EXPECT_EQ(1U, mgr.Diagnostics().size());
   const Diagnostic *got = mgr.Diagnostics().front().get();

[jimingham]

"Detail" seems too generic a name for what is a very "compiler error" centric data structure in Status. But if it's ExprDetail, then the next time someone wants to add richer info about an error, would we have them add an OtherErrorDetail structure? Maybe the latter question can be pushed off till there IS a second usage. But it still bugs me that this claims to more generality than it has.

[adrian-prantl]

FYI, this is where this is going: #106470

[labath]

The llvm::Error class already supports this kind of metadata (and in a much better way IMO). Given that moving to llvm::Error is our long term goal, would it be possible to port enough APIs so that it's possible to pass an unmodified Error object from the producer to the place that needs to consume it?

[adrian-prantl]

The llvm::Error class already supports this kind of metadata (and in a much better way IMO). Given that moving to llvm::Error is our long term goal, would it be possible to port enough APIs so that it's possible to pass an unmodified Error object from the producer to the place that needs to consume it?

@labath We can refactor lldb::Status to be a wrapper around llvm::Error (with the goal of eventually removing lldb::Status once all uses have been migrated to check errors). But then this Detail class would still need to exist, just wrapped inside of an ErrorBase subclass. Here's a very rough sketch:

// eErrorTypeGeneric -> StringError
// eErrorTypeMachKernel -> new subclass ...
// etc ...
class ExpressionError : public llvm::ErrorBase {
  std::vector<Detail> m_details;
  lldb::ExpressionResults m_code;
  ...
}

class Status {
   llvm::Error m_error;

public:
  operator=(other) { LOG_ERROR("dropping error"); m_error = other.m_error; }
  ...
  rest of interface remains same for now ...
}

And then we could gradually port APIs from Status to Expected.

Is that what you have in mind?

[labath]

Not really (although this might work as well). What I meant was that, AIUI you're doing this so that you can plumb structured error information from one place (clang expression parser?) to another place (the thing which generates the expression errors -- I guess somewhere CommandObjectExpression?). That could be done by creating a (custom) llvm::Error in the first place, and consuming it in the second one and by making sure that all of the functions along that path don't lose this information by converting the error to a lldb_private::Status. I haven't looked at how hard much changes would that require, but I'm hoping that will be a single relatively well defined path that can be ported with reasonable effort. Even if there's e.g. a virtual function somewhere along the path, we don't necessarily have to port all implementations of that function immediately. We could just port the ones we need, and provide a shim so that the other implementations can remain unchanged...

[adrian-prantl]

Not really (although this might work as well). What I meant was that, AIUI you're doing this so that you can plumb structured error information from one place (clang expression parser?) to another place (the thing which generates the expression errors -- I guess somewhere CommandObjectExpression?).

Yes, just that it's a generalized data structure for all expression parser plugins.

That could be done by creating a (custom) llvm::Error in the first place, and consuming it in the second one and by making sure that all of the functions along that path don't lose this information by converting the error to a lldb_private::Status.

This would only work if Status actually stored an error, like I outlined above. The conversion from structured error into text needs to happen at the UI layer. For example, because only it knows indentation and coloring.

Oh.. you mean converting the entire path from the expression evaluator to CommandObject from Status to llvm::Error! Yes. That would work. Since I have a few hours today, I'm going to attempt what I outlined above and then later change the APIs from Status to Expected.

[adrian-prantl]

Rebased on top of #106774!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[adrian-prantl]

Rebased once more. I think this should address all the high-level comments.