Skip to content

[lldb] Expose structured errors in SBError #120784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

adrian-prantl
Copy link
Collaborator

Building on top of previous work that exposed expression diagnostics via SBCommandReturnObject, this patch generalizes the support to expose any SBError as machine-readable structured data. One use-case of this is to allow IDEs to better visualize expression diagnostics.

rdar://139997604

@llvmbot
Copy link
Member

llvmbot commented Dec 20, 2024

@llvm/pr-subscribers-lldb

Author: Adrian Prantl (adrian-prantl)

Changes

Building on top of previous work that exposed expression diagnostics via SBCommandReturnObject, this patch generalizes the support to expose any SBError as machine-readable structured data. One use-case of this is to allow IDEs to better visualize expression diagnostics.

rdar://139997604


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

9 Files Affected:

  • (modified) lldb/include/lldb/API/SBError.h (+5)
  • (modified) lldb/include/lldb/API/SBStructuredData.h (+1)
  • (modified) lldb/include/lldb/Utility/DiagnosticsRendering.h (+10-4)
  • (modified) lldb/include/lldb/Utility/Status.h (+6)
  • (modified) lldb/source/API/SBError.cpp (+14)
  • (modified) lldb/source/Interpreter/CommandReturnObject.cpp (+1-51)
  • (modified) lldb/source/Utility/DiagnosticsRendering.cpp (+40)
  • (modified) lldb/source/Utility/Status.cpp (+24)
  • (modified) lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py (+47-28)
diff --git a/lldb/include/lldb/API/SBError.h b/lldb/include/lldb/API/SBError.h
index 9f55f92131c06e..58b45ebd793af5 100644
--- a/lldb/include/lldb/API/SBError.h
+++ b/lldb/include/lldb/API/SBError.h
@@ -44,8 +44,13 @@ class LLDB_API SBError {
 
   bool Success() const;
 
+  /// Get the error code.
   uint32_t GetError() const;
 
+  /// Get the error in machine-readable form. Particularly useful for
+  /// compiler diagnostics.
+  SBStructuredData GetErrorData() const;
+
   lldb::ErrorType GetType() const;
 
   void SetError(uint32_t err, lldb::ErrorType type);
diff --git a/lldb/include/lldb/API/SBStructuredData.h b/lldb/include/lldb/API/SBStructuredData.h
index c0d214a7374c65..f96e169f236edd 100644
--- a/lldb/include/lldb/API/SBStructuredData.h
+++ b/lldb/include/lldb/API/SBStructuredData.h
@@ -115,6 +115,7 @@ class SBStructuredData {
   friend class SBLaunchInfo;
   friend class SBDebugger;
   friend class SBFrame;
+  friend class SBError;
   friend class SBTarget;
   friend class SBProcess;
   friend class SBThread;
diff --git a/lldb/include/lldb/Utility/DiagnosticsRendering.h b/lldb/include/lldb/Utility/DiagnosticsRendering.h
index 33aded602e3a77..e5f35c4611c20a 100644
--- a/lldb/include/lldb/Utility/DiagnosticsRendering.h
+++ b/lldb/include/lldb/Utility/DiagnosticsRendering.h
@@ -57,6 +57,13 @@ struct DiagnosticDetail {
   std::string rendered;
 };
 
+StructuredData::ObjectSP Serialize(llvm::ArrayRef<DiagnosticDetail> details);
+
+void RenderDiagnosticDetails(Stream &stream,
+                             std::optional<uint16_t> offset_in_command,
+                             bool show_inline,
+                             llvm::ArrayRef<DiagnosticDetail> details);
+
 class DiagnosticError
     : public llvm::ErrorInfo<DiagnosticError, CloneableECError> {
 public:
@@ -64,12 +71,11 @@ class DiagnosticError
   DiagnosticError(std::error_code ec) : ErrorInfo(ec) {}
   lldb::ErrorType GetErrorType() const override;
   virtual llvm::ArrayRef<DiagnosticDetail> GetDetails() const = 0;
+  StructuredData::ObjectSP GetErrorData() const override {
+    return Serialize(GetDetails());
+  }
   static char ID;
 };
 
-void RenderDiagnosticDetails(Stream &stream,
-                             std::optional<uint16_t> offset_in_command,
-                             bool show_inline,
-                             llvm::ArrayRef<DiagnosticDetail> details);
 } // namespace lldb_private
 #endif
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index b8993dff3bb18a..681a56b7a155e1 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -10,6 +10,7 @@
 #define LLDB_UTILITY_STATUS_H
 
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/StringRef.h"
@@ -38,6 +39,7 @@ class CloneableError
   CloneableError() : ErrorInfo() {}
   virtual std::unique_ptr<CloneableError> Clone() const = 0;
   virtual lldb::ErrorType GetErrorType() const = 0;
+  virtual StructuredData::ObjectSP GetErrorData() const = 0;
   static char ID;
 };
 
@@ -49,6 +51,7 @@ class CloneableECError
   std::error_code convertToErrorCode() const override { return EC; }
   void log(llvm::raw_ostream &OS) const override { OS << EC.message(); }
   lldb::ErrorType GetErrorType() const override;
+  virtual StructuredData::ObjectSP GetErrorData() const override;
   static char ID;
 
 protected:
@@ -183,6 +186,9 @@ class Status {
   ///     NULL otherwise.
   const char *AsCString(const char *default_error_str = "unknown error") const;
 
+  /// Get the error in machine-readable form.
+  StructuredData::ObjectSP GetErrorData() const;
+
   /// Clear the object state.
   ///
   /// Reverts the state of this object to contain a generic success value and
diff --git a/lldb/source/API/SBError.cpp b/lldb/source/API/SBError.cpp
index 31964931649db3..8c9e15736d25b1 100644
--- a/lldb/source/API/SBError.cpp
+++ b/lldb/source/API/SBError.cpp
@@ -9,6 +9,8 @@
 #include "lldb/API/SBError.h"
 #include "Utils.h"
 #include "lldb/API/SBStream.h"
+#include "lldb/API/SBStructuredData.h"
+#include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Utility/Instrumentation.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/VASPrintf.h"
@@ -97,6 +99,18 @@ uint32_t SBError::GetError() const {
   return err;
 }
 
+SBStructuredData SBError::GetErrorData() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBStructuredData sb_data;
+  if (!m_opaque_up)
+    return sb_data;
+
+  StructuredData::ObjectSP data(m_opaque_up->GetErrorData());
+  sb_data.m_impl_up->SetObjectSP(data);
+  return sb_data;
+}
+
 ErrorType SBError::GetType() const {
   LLDB_INSTRUMENT_VA(this);
 
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp b/lldb/source/Interpreter/CommandReturnObject.cpp
index 2776efbb5ee36d..b99b2bc7b36ce4 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -169,57 +169,7 @@ std::string CommandReturnObject::GetErrorString(bool with_diagnostics) {
 }
 
 StructuredData::ObjectSP CommandReturnObject::GetErrorData() {
-  auto make_array = []() { return std::make_unique<StructuredData::Array>(); };
-  auto make_bool = [](bool b) {
-    return std::make_unique<StructuredData::Boolean>(b);
-  };
-  auto make_dict = []() {
-    return std::make_unique<StructuredData::Dictionary>();
-  };
-  auto make_int = [](unsigned i) {
-    return std::make_unique<StructuredData::UnsignedInteger>(i);
-  };
-  auto make_string = [](llvm::StringRef s) {
-    return std::make_unique<StructuredData::String>(s);
-  };
-  auto dict_up = make_dict();
-  dict_up->AddItem("version", make_int(1));
-  auto array_up = make_array();
-  for (const DiagnosticDetail &diag : m_diagnostics) {
-    auto detail_up = make_dict();
-    if (auto &sloc = diag.source_location) {
-      auto sloc_up = make_dict();
-      sloc_up->AddItem("file", make_string(sloc->file.GetPath()));
-      sloc_up->AddItem("line", make_int(sloc->line));
-      sloc_up->AddItem("length", make_int(sloc->length));
-      sloc_up->AddItem("hidden", make_bool(sloc->hidden));
-      sloc_up->AddItem("in_user_input", make_bool(sloc->in_user_input));
-      detail_up->AddItem("source_location", std::move(sloc_up));
-    }
-    llvm::StringRef severity = "unknown";
-    switch (diag.severity) {
-    case lldb::eSeverityError:
-      severity = "error";
-      break;
-    case lldb::eSeverityWarning:
-      severity = "warning";
-      break;
-    case lldb::eSeverityInfo:
-      severity = "note";
-      break;
-    }
-    detail_up->AddItem("severity", make_string(severity));
-    detail_up->AddItem("message", make_string(diag.message));
-    detail_up->AddItem("rendered", make_string(diag.rendered));
-    array_up->AddItem(std::move(detail_up));
-  }
-  dict_up->AddItem("details", std::move(array_up));
-  if (auto stream_sp = m_err_stream.GetStreamAtIndex(eStreamStringIndex)) {
-    auto text = std::static_pointer_cast<StreamString>(stream_sp)->GetString();
-    if (!text.empty())
-      dict_up->AddItem("text", make_string(text));
-  }
-  return dict_up;
+  return Serialize(m_diagnostics);
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
diff --git a/lldb/source/Utility/DiagnosticsRendering.cpp b/lldb/source/Utility/DiagnosticsRendering.cpp
index f5aa27baadfef8..368e2199b749fb 100644
--- a/lldb/source/Utility/DiagnosticsRendering.cpp
+++ b/lldb/source/Utility/DiagnosticsRendering.cpp
@@ -20,6 +20,46 @@ lldb::ErrorType DiagnosticError::GetErrorType() const {
   return lldb::eErrorTypeExpression;
 }
 
+StructuredData::ObjectSP Serialize(llvm::ArrayRef<DiagnosticDetail> details) {
+  auto make_array = []() { return std::make_unique<StructuredData::Array>(); };
+  auto make_dict = []() {
+    return std::make_unique<StructuredData::Dictionary>();
+  };
+  auto dict_up = make_dict();
+  dict_up->AddIntegerItem("version", 1u);
+  auto array_up = make_array();
+  for (const DiagnosticDetail &diag : details) {
+    auto detail_up = make_dict();
+    if (auto &sloc = diag.source_location) {
+      auto sloc_up = make_dict();
+      sloc_up->AddStringItem("file", sloc->file.GetPath());
+      sloc_up->AddIntegerItem("line", sloc->line);
+      sloc_up->AddIntegerItem("length", sloc->length);
+      sloc_up->AddBooleanItem("hidden", sloc->hidden);
+      sloc_up->AddBooleanItem("in_user_input", sloc->in_user_input);
+      detail_up->AddItem("source_location", std::move(sloc_up));
+    }
+    llvm::StringRef severity = "unknown";
+    switch (diag.severity) {
+    case lldb::eSeverityError:
+      severity = "error";
+      break;
+    case lldb::eSeverityWarning:
+      severity = "warning";
+      break;
+    case lldb::eSeverityInfo:
+      severity = "note";
+      break;
+    }
+    detail_up->AddStringItem("severity", severity);
+    detail_up->AddStringItem("message", diag.message);
+    detail_up->AddStringItem("rendered", diag.rendered);
+    array_up->AddItem(std::move(detail_up));
+  }
+  dict_up->AddItem("details", std::move(array_up));
+  return dict_up;
+}
+
 static llvm::raw_ostream &PrintSeverity(Stream &stream,
                                         lldb::Severity severity) {
   llvm::HighlightColor color;
diff --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index 5757935fb86228..9e7e34e6a3ae2e 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -252,6 +252,30 @@ lldb::ErrorType Win32Error::GetErrorType() const {
   return lldb::eErrorTypeWin32;
 }
 
+StructuredData::ObjectSP Status::GetErrorData() const {
+  auto dict_up = std::make_unique<StructuredData::Dictionary>();
+  auto array_up = std::make_unique<StructuredData::Array>();
+  llvm::visitErrors(m_error, [&](const llvm::ErrorInfoBase &error) {
+    if (error.isA<CloneableError>())
+      array_up->AddItem(
+          static_cast<const CloneableError &>(error).GetErrorData());
+    else
+      array_up->AddStringItem(error.message());
+  });
+  dict_up->AddIntegerItem("version", 1u);
+  dict_up->AddIntegerItem("type", (unsigned)GetType());
+  dict_up->AddItem("errors", std::move(array_up));
+  return dict_up;
+}
+
+StructuredData::ObjectSP CloneableECError::GetErrorData() const {
+  auto dict_up = std::make_unique<StructuredData::Dictionary>();
+  dict_up->AddIntegerItem("version", 1u);
+  dict_up->AddIntegerItem("error_code", EC.value());
+  dict_up->AddStringItem("message", message());
+  return dict_up;
+}
+
 ErrorType Status::GetType() const {
   ErrorType result = eErrorTypeInvalid;
   llvm::visitErrors(m_error, [&](const llvm::ErrorInfoBase &error) {
diff --git a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
index 0806776aa6eb03..be1ddac5f75941 100644
--- a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
+++ b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
@@ -207,37 +207,56 @@ def test_command_expr_sbdata(self):
         (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
             self, "// Break here", self.main_source_spec
         )
+
+        def check_error(diags):
+            # Version.
+            version = diags.GetValueForKey("version")
+            self.assertEqual(version.GetIntegerValue(), 1)
+
+            details = diags.GetValueForKey("details")
+
+            # Detail 1/2: undeclared 'a'
+            diag = details.GetItemAtIndex(0)
+
+            severity = diag.GetValueForKey("severity")
+            message = diag.GetValueForKey("message")
+            rendered = diag.GetValueForKey("rendered")
+            sloc = diag.GetValueForKey("source_location")
+            filename = sloc.GetValueForKey("file")
+            hidden = sloc.GetValueForKey("hidden")
+            in_user_input = sloc.GetValueForKey("in_user_input")
+
+            self.assertEqual(str(severity), "error")
+            self.assertIn("undeclared identifier 'a'", str(message))
+            # The rendered string should contain the source file.
+            self.assertIn("user expression", str(rendered))
+            self.assertIn("user expression", str(filename))
+            self.assertFalse(hidden.GetBooleanValue())
+            self.assertTrue(in_user_input.GetBooleanValue())
+
+            # Detail 1/2: undeclared 'b'
+            diag = details.GetItemAtIndex(1)
+            message = diag.GetValueForKey("message")
+            self.assertIn("undeclared identifier 'b'", str(message))
+
+        # Test diagnostics in CommandReturnObject
         interp = self.dbg.GetCommandInterpreter()
         cro = lldb.SBCommandReturnObject()
         interp.HandleCommand("expression -- a+b", cro)
 
         diags = cro.GetErrorData()
-        # Version.
-        version = diags.GetValueForKey("version")
-        self.assertEqual(version.GetIntegerValue(), 1)
+        check_error(diags)
 
-        details = diags.GetValueForKey("details")
-
-        # Detail 1/2: undeclared 'a'
-        diag = details.GetItemAtIndex(0)
-
-        severity = diag.GetValueForKey("severity")
-        message = diag.GetValueForKey("message")
-        rendered = diag.GetValueForKey("rendered")
-        sloc = diag.GetValueForKey("source_location")
-        filename = sloc.GetValueForKey("file")
-        hidden = sloc.GetValueForKey("hidden")
-        in_user_input = sloc.GetValueForKey("in_user_input")
-
-        self.assertEqual(str(severity), "error")
-        self.assertIn("undeclared identifier 'a'", str(message))
-        # The rendered string should contain the source file.
-        self.assertIn("user expression", str(rendered))
-        self.assertIn("user expression", str(filename))
-        self.assertFalse(hidden.GetBooleanValue())
-        self.assertTrue(in_user_input.GetBooleanValue())
-
-        # Detail 1/2: undeclared 'b'
-        diag = details.GetItemAtIndex(1)
-        message = diag.GetValueForKey("message")
-        self.assertIn("undeclared identifier 'b'", str(message))
+        # Test diagnostics in SBError
+        frame = thread.GetSelectedFrame()
+        value = frame.EvaluateExpression("a+b")
+        error = value.GetError()
+        self.assertTrue(error.Fail())
+        self.assertEquals(error.GetType(), lldb.eErrorTypeExpression)
+        data = error.GetErrorData()
+        version = data.GetValueForKey("version")
+        self.assertEqual(version.GetIntegerValue(), 1)
+        err_ty = data.GetValueForKey("type")
+        self.assertEqual(err_ty.GetIntegerValue(), 4)
+        diags = data.GetValueForKey("errors").GetItemAtIndex(0)
+        check_error(diags)

Copy link

github-actions bot commented Dec 20, 2024

✅ With the latest revision this PR passed the Python code formatter.

@medismailben
Copy link
Member

LGTM. nit: I'd have called the API SBError::GetStructuredError.

Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM module naming.

Building on top of previous work that exposed expression diagnostics
via SBCommandReturnObject, this patch generalizes the support to
expose any SBError as machine-readable structured data. One use-case
of this is to allow IDEs to better visualize expression diagnostics.

rdar://139997604
@adrian-prantl
Copy link
Collaborator Author

LGTM. nit: I'd have called the API SBError::GetStructuredError.

I have no strong feelings about this — I chose GetErrorData, because that's what SBStructuredData SBCommandReturnObject::GetErrorData() is called.

@adrian-prantl adrian-prantl merged commit d9cc37f into llvm:main Dec 20, 2024
7 checks passed
@slydiman
Copy link
Contributor

slydiman commented Dec 20, 2024

The test lldb-api::TestExprDiagnostics.py is broken after this patch.
https://lab.llvm.org/buildbot/#/builders/195/builds/2683

Traceback (most recent call last):
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1770, in test_method
    return attrvalue(self)
           ^^^^^^^^^^^^^^^
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py", line 255, in test_command_expr_sbdata
    self.assertEquals(error.GetType(), lldb.eErrorTypeExpression)
    ^^^^^^^^^^^^^^^^^
AttributeError: 'ExprDiagnosticsTestCase' object has no attribute 'assertEquals'. Did you mean: 'assertEqual'?

adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Dec 21, 2024
Building on top of previous work that exposed expression diagnostics via
SBCommandReturnObject, this patch generalizes the support to expose any
SBError as machine-readable structured data. One use-case of this is to
allow IDEs to better visualize expression diagnostics.

rdar://139997604
(cherry picked from commit d9cc37f)
adrian-prantl added a commit to swiftlang/llvm-project that referenced this pull request Dec 21, 2024
…-6.1-lldb-Expose-structured-errors-in-SBError-120784

[Cherry-pick into swift/release/6.1] [lldb] Expose structured errors in SBError (llvm#120784)
@slydiman
Copy link
Contributor

Please fix the lldb-api::TestExprDiagnostics.py test ASAP.

carlocab added a commit to carlocab/llvm-project that referenced this pull request Dec 22, 2024
Fixes

    Traceback (most recent call last):
      File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1770, in test_method
        return attrvalue(self)
               ^^^^^^^^^^^^^^^
      File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py", line 255, in test_command_expr_sbdata
        self.assertEquals(error.GetType(), lldb.eErrorTypeExpression)
        ^^^^^^^^^^^^^^^^^
    AttributeError: 'ExprDiagnosticsTestCase' object has no attribute 'assertEquals'. Did you mean: 'assertEqual'?

`assertEqual` is a method inherited from `unittest.TestCase`.

See llvm#120784 and llvm#120784 (comment)
@carlocab
Copy link
Member

Opened #120901 to fix the test failure.

carlocab added a commit that referenced this pull request Dec 23, 2024
Fixes

    Traceback (most recent call last):
File
"/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py",
line 1770, in test_method
        return attrvalue(self)
               ^^^^^^^^^^^^^^^
File
"/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py",
line 255, in test_command_expr_sbdata
        self.assertEquals(error.GetType(), lldb.eErrorTypeExpression)
        ^^^^^^^^^^^^^^^^^
AttributeError: 'ExprDiagnosticsTestCase' object has no attribute
'assertEquals'. Did you mean: 'assertEqual'?

`assertEqual` is a method inherited from `unittest.TestCase`.

See #120784 and
#120784 (comment)
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…20901)

Fixes

    Traceback (most recent call last):
File
"/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py",
line 1770, in test_method
        return attrvalue(self)
               ^^^^^^^^^^^^^^^
File
"/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py",
line 255, in test_command_expr_sbdata
        self.assertEquals(error.GetType(), lldb.eErrorTypeExpression)
        ^^^^^^^^^^^^^^^^^
AttributeError: 'ExprDiagnosticsTestCase' object has no attribute
'assertEquals'. Did you mean: 'assertEqual'?

`assertEqual` is a method inherited from `unittest.TestCase`.

See #120784 and
llvm/llvm-project#120784 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants