[lldb] Add Status::Detail to store expression evaluator diagnostics [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 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!

Author: adrian-prantl

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; }

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

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;
+}

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();

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;
 

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));