[lldb] Emit diagnostics as "important" output #137280

JDevlieghere · 2025-04-25T01:00:29Z

Handle diagnostics events from the debugger (i.e. asynchronous errors and warnings) in the lldb-dap event handler and emit them as "important" output.

llvmbot · 2025-04-25T01:01:03Z

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Handle diagnostics events from the debugger (i.e. asynchronous errors and warnings) in the lldb-dap event handler and emit them as "important" output.

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

9 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+1)
(modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+11-2)
(modified) lldb/test/API/tools/lldb-dap/console/TestDAP_console.py (+17)
(added) lldb/test/API/tools/lldb-dap/console/minidump.yaml (+24)
(modified) lldb/tools/lldb-dap/DAP.cpp (+3)
(modified) lldb/tools/lldb-dap/DAP.h (+1-1)
(modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (+13)
(modified) lldb/tools/lldb-dap/LLDBUtils.cpp (+14)
(modified) lldb/tools/lldb-dap/LLDBUtils.h (+3)

diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index dadf6b1f8774c..5acec92ef235b 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -227,6 +227,7 @@ def handle_recv_packet(self, packet):
                     self.output[category] += output
                 else:
                     self.output[category] = output
+                print(category)
                 self.output_condition.notify()
                 self.output_condition.release()
                 # no need to add 'output' event packets to our packets list
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index b5b55b336d535..5f90830a34059 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -217,16 +217,25 @@ def get_stdout(self, timeout=0.0):
     def get_console(self, timeout=0.0):
         return self.dap_server.get_output("console", timeout=timeout)
 
+    def get_important(self, timeout=0.0):
+        return self.dap_server.get_output("important", timeout=timeout)
+
+    def collect_stdout(self, timeout_secs, pattern=None):
+        return self.dap_server.collect_output(
+            "stdout", timeout_secs=timeout_secs, pattern=pattern
+        )
+
     def collect_console(self, timeout_secs, pattern=None):
         return self.dap_server.collect_output(
             "console", timeout_secs=timeout_secs, pattern=pattern
         )
 
-    def collect_stdout(self, timeout_secs, pattern=None):
+    def collect_important(self, timeout_secs, pattern=None):
         return self.dap_server.collect_output(
-            "stdout", timeout_secs=timeout_secs, pattern=pattern
+            "important", timeout_secs=timeout_secs, pattern=pattern
         )
 
+
     def get_local_as_int(self, name, threadId=None):
         value = self.dap_server.get_local_variable_value(name, threadId=threadId)
         # 'value' may have the variable value and summary.
diff --git a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
index 8bbab8d0991de..b07c4f871d73b 100644
--- a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
+++ b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
@@ -164,3 +164,20 @@ def test_exit_status_message_ok(self):
             console_output,
             "Exit status does not contain message 'exited with status'",
         )
+
+    def test_diagnositcs(self):
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        core = self.getBuildArtifact("minidump.core")
+        self.yaml2obj("minidump.yaml", core)
+        self.dap_server.request_evaluate(
+            f"target create --core  {core}", context="repl"
+        )
+
+        output = self.get_important()
+        self.assertIn(
+            "warning: unable to retrieve process ID from minidump file",
+            output,
+            "diagnostic found in important output",
+        )
diff --git a/lldb/test/API/tools/lldb-dap/console/minidump.yaml b/lldb/test/API/tools/lldb-dap/console/minidump.yaml
new file mode 100644
index 0000000000000..42f0a23702950
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/console/minidump.yaml
@@ -0,0 +1,24 @@
+--- !minidump
+Streams:
+  - Type:            ThreadList
+    Threads:
+      - Thread Id:       0x00003E81
+        Context:         0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B0010000000000033000000000000000000000006020100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000010A234EBFC7F000010A234EBFC7F00000000000000000000F09C34EBFC7F0000C0A91ABCE97F00000000000000000000A0163FBCE97F00004602000000000000921C40000000000030A434EBFC7F000000000000000000000000000000000000C61D4000000000007F0300000000000000000000000000000000000000000000801F0000FFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF25252525252525252525252525252525000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+        Stack:
+          Start of Memory Range: 0x00007FFCEB34A000
+          Content:         ''
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x0000000000400000
+        Size of Image:   0x00017000
+        Module Name:     'a.out'
+        CodeView Record: ''
+  - Type:            SystemInfo
+    Processor Arch:  AMD64
+    Platform ID:     Linux
+    CSD Version:     'Linux 3.13'
+    CPU:
+      Vendor ID:       GenuineIntel
+      Version Info:    0x00000000
+      Feature Info:    0x00000000
+...
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 134762711b89d..09f7e817707bc 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -342,6 +342,9 @@ void DAP::SendOutput(OutputType o, const llvm::StringRef output) {
   case OutputType::Console:
     category = "console";
     break;
+  case OutputType::Important:
+    category = "important";
+    break;
   case OutputType::Stdout:
     category = "stdout";
     break;
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 727e5c00623e8..e8673705119e4 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -67,7 +67,7 @@ typedef llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>
 using AdapterFeature = protocol::AdapterFeature;
 using ClientFeature = protocol::ClientFeature;
 
-enum class OutputType { Console, Stdout, Stderr, Telemetry };
+enum class OutputType { Console, Important, Stdout, Stderr, Telemetry };
 
 /// Buffer size for handling output events.
 constexpr uint64_t OutputBufferSize = (1u << 12);
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index b4250cd6becb3..56089fb08b2ea 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -9,6 +9,7 @@
 #include "DAP.h"
 #include "EventHelper.h"
 #include "JSONUtils.h"
+#include "LLDBUtils.h"
 #include "Protocol/ProtocolRequests.h"
 #include "RequestHandler.h"
 #include "lldb/API/SBEvent.h"
@@ -117,6 +118,9 @@ static void EventThreadFunction(DAP &dap) {
   lldb::SBEvent event;
   lldb::SBListener listener = dap.debugger.GetListener();
   dap.broadcaster.AddListener(listener, eBroadcastBitStopEventThread);
+  dap.debugger.GetBroadcaster().AddListener(
+      listener, lldb::SBDebugger::eBroadcastBitError |
+                    lldb::SBDebugger::eBroadcastBitWarning);
   bool done = false;
   while (!done) {
     if (listener.WaitForEvent(1, event)) {
@@ -222,6 +226,15 @@ static void EventThreadFunction(DAP &dap) {
             dap.SendJSON(llvm::json::Value(std::move(bp_event)));
           }
         }
+      } else if (event_mask & eBroadcastBitError ||
+                 event_mask & eBroadcastBitWarning) {
+        SBStructuredData data = SBDebugger::GetDiagnosticFromEvent(event);
+        if (!data.IsValid())
+          continue;
+        std::string type = GetStringValue(data.GetValueForKey("type"));
+        std::string message = GetStringValue(data.GetValueForKey("message"));
+        dap.SendOutput(OutputType::Important,
+                       llvm::formatv("{0}: {1}", type, message).str());
       } else if (event.BroadcasterMatchesRef(dap.broadcaster)) {
         if (event_mask & eBroadcastBitStopEventThread) {
           done = true;
diff --git a/lldb/tools/lldb-dap/LLDBUtils.cpp b/lldb/tools/lldb-dap/LLDBUtils.cpp
index a27beff0b030d..0b3d416ecbe3d 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.cpp
+++ b/lldb/tools/lldb-dap/LLDBUtils.cpp
@@ -10,6 +10,7 @@
 #include "DAP.h"
 #include "JSONUtils.h"
 #include "lldb/API/SBStringList.h"
+#include "lldb/API/SBStructuredData.h"
 
 #include <mutex>
 
@@ -172,4 +173,17 @@ llvm::Error ToError(const lldb::SBError &error) {
       error.GetCString());
 }
 
+std::string GetStringValue(const lldb::SBStructuredData &data) {
+  if (!data.IsValid())
+    return "";
+
+  const size_t str_length = data.GetStringValue(nullptr, 0);
+  if (!str_length)
+    return "";
+
+  std::string str(str_length, 0);
+  data.GetStringValue(&str[0], str_length + 1);
+  return str;
+}
+
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/LLDBUtils.h b/lldb/tools/lldb-dap/LLDBUtils.h
index 2c57847303cb3..7d51427123c0f 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.h
+++ b/lldb/tools/lldb-dap/LLDBUtils.h
@@ -159,6 +159,9 @@ GetEnvironmentFromArguments(const llvm::json::Object &arguments);
 /// Take ownership of the stored error.
 llvm::Error ToError(const lldb::SBError &error);
 
+/// Provides the string value if this data structure is a string type.
+std::string GetStringValue(const lldb::SBStructuredData &data);
+
 } // namespace lldb_dap
 
 #endif

JDevlieghere · 2025-04-25T01:01:54Z

lldb/tools/lldb-dap/LLDBUtils.h

@@ -159,6 +159,9 @@ GetEnvironmentFromArguments(const llvm::json::Object &arguments);
 /// Take ownership of the stored error.
 llvm::Error ToError(const lldb::SBError &error);

+/// Provides the string value if this data structure is a string type.
+std::string GetStringValue(const lldb::SBStructuredData &data);


There's a few more places that can use this helper. I'm planning on addressing that in a follow-up PR.

github-actions · 2025-04-25T01:04:32Z

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

labath · 2025-04-25T06:41:01Z

Will this actually result in a popup window as #137002 (comment) implies? I'm worried that some of these messages might end up too annoying if the user has explicitly acknowledge each of them.

da-viper · 2025-04-25T11:08:00Z

@labath this is what it looks like at the bottom right

labath · 2025-04-25T11:23:26Z

I see, so it's one of those popups that don't require interaction and disappear on their own after a while. That's better, though I'm still somewhat worried, as these errors tend to be bursty.

Like, if there's something wrong with the debug info, we could get dozens (or more) of errors of the kind I just removed in #132395. I see they're currently being deduped, but the implementation is rather naive -- a hash of the error message, which won't help since a lot of these messages contain debug info offsets and stuff. However, I suppose we could improve this logic if that becomes a problem, so I guess I'd be fine with this.

da-viper · 2025-04-25T11:32:31Z

this does not catch

llvm-project/lldb/tools/lldb-dap/DAP.cpp

Lines 215 to 219 in d775b91

    
           if (auto Error = err.RedirectTo(overrideErr, [this](llvm::StringRef output) { 
        
                 SendOutput(OutputType::Stderr, output); 
        
               })) 
        
             return Error;

for example if you add the initcommand of breakpoint set --file=main.c --line=2 --command "bogus" it is not sent to the important category

da-viper · 2025-04-25T11:36:46Z

Maybe we limit it to only Errors.
as on linux if lldb is not built with lzma support it will spam warning messages like.

warning: 45F7FBFE-9455-A458-4A50-347C4A5BC883/libsync.so No LZMA support found for reading .gnu_debugdata section
warning: 8E2F4272-9D82-2CEF-A46D-C69DF3FEC722/liblog.so No LZMA support found for reading .gnu_debugdata section

lldb/test/API/tools/lldb-dap/console/TestDAP_console.py

labath · 2025-04-25T11:50:50Z

this does not catch

llvm-project/lldb/tools/lldb-dap/DAP.cpp

Lines 215 to 219 in d775b91

if (auto Error = err.RedirectTo(overrideErr, [this](llvm::StringRef output) {

SendOutput(OutputType::Stderr, output);

}))

return Error;

for example if you add the initcommand of breakpoint set --file=main.c --line=2 --command "bogus" it is not sent to the important category

I don't think we want to redirect all of stderr here -- it's too easy to write something to it accidentally (e.g. a forgotten debug print in a python data formatter). I can imagine emitting a debugger diagnostic in cases like this, though I think that's a job for a different patch.

Maybe we limit it to only Errors. as on linux if lldb is not built with lzma support it will spam warning messages like.
warning: 45F7FBFE-9455-A458-4A50-347C4A5BC883/libsync.so No LZMA support found for reading .gnu_debugdata section
warning: 8E2F4272-9D82-2CEF-A46D-C69DF3FEC722/liblog.so No LZMA support found for reading .gnu_debugdata section

Yeah, this is a good example of the bursty behavior. Limiting it only to errors might be a good idea, though I currentl feel like the distinction between errors and warnings is pretty arbitrary. Like, I don't see why this should be "only" a warning but the message in #132395 an error. However, this would at least give us an easy way to silence the most egregious offenders.

I also think this LZMA message is important enough to see -- but once, not once for each module. So I can imagine a setup where the first of these messages would cause a popup and the rest would be squirrelled away somewhere -- but for that we'd need a better mechanism to track these.

lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp

JDevlieghere · 2025-04-25T15:53:04Z

I also think this LZMA message is important enough to see -- but once, not once for each module. So I can imagine a setup where the first of these messages would cause a popup and the rest would be squirrelled away somewhere -- but for that we'd need a better mechanism to track these.

The methods to emit diagnostics (ReportError, ReportWarning) already take an optional std::once_flag. We have a somewhat similar way of making these errors and warnings very visible in Xcode, so making sure they're not too spammy is something we also care about. I remember several patches (maybe some on the Swift fork) to improve this. In other words, if this needs improving, let's do it at the source.

ashgti

LGTM

Handle diagnostics events from the debugger (i.e. asynchronous errors and warnings) in the lldb-dap event handler and emit them as "important" output.

JDevlieghere requested review from ashgti, vogelsgesang and da-viper April 25, 2025 01:00

llvmbot added lldb lldb-dap labels Apr 25, 2025

JDevlieghere force-pushed the lldb-dap-diagnostics branch from f426ab0 to c2eab3f Compare April 25, 2025 01:01

JDevlieghere commented Apr 25, 2025

View reviewed changes

da-viper reviewed Apr 25, 2025

View reviewed changes

lldb/test/API/tools/lldb-dap/console/TestDAP_console.py Show resolved Hide resolved

ashgti approved these changes Apr 25, 2025

View reviewed changes

lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp Outdated Show resolved Hide resolved

ashgti approved these changes Apr 25, 2025

View reviewed changes

JDevlieghere added 3 commits April 25, 2025 10:01

[lldb] Emit diagnostics as "important" output

93f3b75

Handle diagnostics events from the debugger (i.e. asynchronous errors and warnings) in the lldb-dap event handler and emit them as "important" output.

Appease the formatter

c7180ff

Use unqualified eBroadcastBitError and eBroadcastBitWarning

36842d3

JDevlieghere force-pushed the lldb-dap-diagnostics branch from a822059 to 36842d3 Compare April 25, 2025 17:01

Use std::string::data

9aa318e

JDevlieghere merged commit d0ce861 into llvm:main Apr 25, 2025
7 of 9 checks passed

JDevlieghere deleted the lldb-dap-diagnostics branch April 25, 2025 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Emit diagnostics as "important" output #137280

[lldb] Emit diagnostics as "important" output #137280

Uh oh!

JDevlieghere commented Apr 25, 2025

Uh oh!

llvmbot commented Apr 25, 2025

Uh oh!

JDevlieghere Apr 25, 2025

Uh oh!

github-actions bot commented Apr 25, 2025 •

edited

Loading

Uh oh!

labath commented Apr 25, 2025

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

labath commented Apr 25, 2025 •

edited

Loading

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

Uh oh!

labath commented Apr 25, 2025

Uh oh!

Uh oh!

JDevlieghere commented Apr 25, 2025

Uh oh!

ashgti left a comment

Uh oh!

Uh oh!

Uh oh!

[lldb] Emit diagnostics as "important" output #137280

[lldb] Emit diagnostics as "important" output #137280

Uh oh!

Conversation

JDevlieghere commented Apr 25, 2025

Uh oh!

llvmbot commented Apr 25, 2025

Uh oh!

JDevlieghere Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath commented Apr 25, 2025

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

labath commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

da-viper commented Apr 25, 2025

Uh oh!

Uh oh!

labath commented Apr 25, 2025

Uh oh!

Uh oh!

JDevlieghere commented Apr 25, 2025

Uh oh!

ashgti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 25, 2025 •

edited

Loading

labath commented Apr 25, 2025 •

edited

Loading