[LLDB-DAP] SBDebugger don't prefix title on progress updates (#124648)

Jlalond · web-flow · commit 02f024ca9740 · 2025-03-05T12:02:44.000-08:00
In my last DAP patch (#123837), we piped the DAP update message into the update event. However, we had the title embedded into the update message. This makes sense for progress Start, but makes the update message look pretty wonky. ![image](https://github.com/user-attachments/assets/9f6083d1-fc50-455c-a1a7-a2f9bdaba22e) Instead, we only use the title when it's the first message, removing the duplicate title problem. ![image](https://github.com/user-attachments/assets/ee7aefd1-1852-46f7-94bc-84b8faef6dac)
diff --git a/lldb/bindings/interface/SBProgressDocstrings.i b/lldb/bindings/interface/SBProgressDocstrings.i
@@ -11,7 +11,48 @@ The Progress class helps make sure that progress is correctly reported
 and will always send an initial progress update, updates when
 Progress::Increment() is called, and also will make sure that a progress
 completed update is reported even if the user doesn't explicitly cause one
-to be sent.") lldb::SBProgress;
+to be sent.
+
+Progress can either be deterministic, incrementing up to a known total or non-deterministic
+with an unbounded total. Deterministic is better if you know the items of work in advance, but non-deterministic
+exposes a way to update a user during a long running process that work is taking place.
+
+For all progresses the details provided in the constructor will be sent until an increment detail
+is provided. This detail will also continue to be broadcasted on any subsequent update that doesn't
+specify a new detail. Some implementations differ on throttling updates and this behavior differs primarily
+if the progress is deterministic or non-deterministic. For DAP, non-deterministic update messages have a higher
+throttling rate than deterministic ones.
+
+Below are examples in Python for deterministic and non-deterministic progresses.
+
+    deterministic_progress1 = lldb.SBProgress('Deterministic Progress', 'Detail', 3, lldb.SBDebugger) 
+    for i in range(3): 
+        deterministic_progress1.Increment(1, f'Update {i}') 
+    # The call to Finalize() is a no-op as we already incremented the right amount of 
+    # times and caused the end event to be sent. 
+    deterministic_progress1.Finalize()
+
+    deterministic_progress2 = lldb.SBProgress('Deterministic Progress', 'Detail', 10, lldb.SBDebugger) 
+    for i in range(3): 
+        deterministic_progress2.Increment(1, f'Update {i}')      
+    # Cause the progress end event to be sent even if we didn't increment the right 
+    # number of times. Real world examples would be in a try-finally block to ensure
+    # progress clean-up.
+    deterministic_progress2.Finalize() 
+
+If you don't call Finalize() when the progress is not done, the progress object will eventually get
+garbage collected by the Python runtime, the end event will eventually get sent, but it is best not to 
+rely on the garbage collection when using lldb.SBProgress.
+
+Non-deterministic progresses behave the same, but omit the total in the constructor.
+
+    non_deterministic_progress = lldb.SBProgress('Non deterministic progress, 'Detail', lldb.SBDebugger)
+    for i in range(10):
+        non_deterministic_progress.Increment(1)
+    # Explicitly send a progressEnd, otherwise this will be sent
+    # when the python runtime cleans up this object.
+    non_deterministic_progress.Finalize()
+") lldb::SBProgress;    
 
 %feature("docstring",
 "Finalize the SBProgress, which will cause a progress end event to be emitted. This 
diff --git a/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py b/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py
@@ -37,7 +37,11 @@ def create_options(cls):
         )
 
         parser.add_option(
-            "--total", dest="total", help="Total to count up.", type="int"
+            "--total",
+            dest="total",
+            help="Total items in this progress object. When this option is not specified, this will be an indeterminate progress.",
+            type="int",
+            default=None,
         )
 
         parser.add_option(
@@ -47,6 +51,14 @@ def create_options(cls):
             type="int",
         )
 
+        parser.add_option(
+            "--no-details",
+            dest="no_details",
+            help="Do not display details",
+            action="store_true",
+            default=False,
+        )
+
         return parser
 
     def get_short_help(self):
@@ -68,12 +80,30 @@ def __call__(self, debugger, command, exe_ctx, result):
             return
 
         total = cmd_options.total
-        progress = lldb.SBProgress("Progress tester", "Detail", total, debugger)
+        if total is None:
+            progress = lldb.SBProgress(
+                "Progress tester", "Initial Indeterminate Detail", debugger
+            )
+        else:
+            progress = lldb.SBProgress(
+                "Progress tester", "Initial Detail", total, debugger
+            )
+
+        # Check to see if total is set to None to indicate an indeterminate progress
+        # then default to 10 steps.
+        if total is None:
+            total = 10
 
         for i in range(1, total):
-            progress.Increment(1, f"Step {i}")
+            if cmd_options.no_details:
+                progress.Increment(1)
+            else:
+                progress.Increment(1, f"Step {i}")
             time.sleep(cmd_options.seconds)
 
+        # Not required for deterministic progress, but required for indeterminate progress.
+        progress.Finalize()
+
 
 def __lldb_init_module(debugger, dict):
     # Register all classes that have a register_lldb_command method
diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -4,21 +4,54 @@
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
+import json
 import os
 import time
 
 import lldbdap_testcase
 
 
 class TestDAP_progress(lldbdap_testcase.DAPTestCaseBase):
+    def verify_progress_events(
+        self,
+        expected_title,
+        expected_message=None,
+        expected_not_in_message=None,
+        only_verify_first_update=False,
+    ):
+        self.dap_server.wait_for_event("progressEnd", 15)
+        self.assertTrue(len(self.dap_server.progress_events) > 0)
+        start_found = False
+        update_found = False
+        end_found = False
+        for event in self.dap_server.progress_events:
+            event_type = event["event"]
+            if "progressStart" in event_type:
+                title = event["body"]["title"]
+                self.assertIn(expected_title, title)
+                start_found = True
+            if "progressUpdate" in event_type:
+                message = event["body"]["message"]
+                if only_verify_first_update and update_found:
+                    continue
+                if expected_message is not None:
+                    self.assertIn(expected_message, message)
+                if expected_not_in_message is not None:
+                    self.assertNotIn(expected_not_in_message, message)
+                update_found = True
+            if "progressEnd" in event_type:
+                end_found = True
+
+        self.assertTrue(start_found)
+        self.assertTrue(update_found)
+        self.assertTrue(end_found)
+
     @skipIfWindows
     def test_output(self):
         program = self.getBuildArtifact("a.out")
         self.build_and_launch(program)
         progress_emitter = os.path.join(os.getcwd(), "Progress_emitter.py")
-        print(f"Progress emitter path: {progress_emitter}")
         source = "main.cpp"
-        # Set breakpoint in the thread function so we can step the threads
         breakpoint_ids = self.set_source_breakpoints(
             source, [line_number(source, "// break here")]
         )
@@ -30,20 +63,73 @@ def test_output(self):
             "`test-progress --total 3 --seconds 1", context="repl"
         )
 
-        self.dap_server.wait_for_event("progressEnd", 15)
-        # Expect at least a start, an update, and end event
-        # However because the underlying Progress instance is an RAII object and we can't guaruntee
-        # it's deterministic destruction in the python API, we verify just start and update
-        # otherwise this test could be flakey.
-        self.assertTrue(len(self.dap_server.progress_events) > 0)
-        start_found = False
-        update_found = False
-        for event in self.dap_server.progress_events:
-            event_type = event["event"]
-            if "progressStart" in event_type:
-                start_found = True
-            if "progressUpdate" in event_type:
-                update_found = True
+        self.verify_progress_events(
+            expected_title="Progress tester",
+            expected_not_in_message="Progress tester",
+        )
 
-        self.assertTrue(start_found)
-        self.assertTrue(update_found)
+    @skipIfWindows
+    def test_output_nodetails(self):
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        progress_emitter = os.path.join(os.getcwd(), "Progress_emitter.py")
+        source = "main.cpp"
+        breakpoint_ids = self.set_source_breakpoints(
+            source, [line_number(source, "// break here")]
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+        self.dap_server.request_evaluate(
+            f"`command script import {progress_emitter}", context="repl"
+        )
+        self.dap_server.request_evaluate(
+            "`test-progress --total 3 --seconds 1 --no-details", context="repl"
+        )
+
+        self.verify_progress_events(
+            expected_title="Progress tester",
+            expected_message="Initial Detail",
+        )
+
+    @skipIfWindows
+    def test_output_indeterminate(self):
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        progress_emitter = os.path.join(os.getcwd(), "Progress_emitter.py")
+        source = "main.cpp"
+        breakpoint_ids = self.set_source_breakpoints(
+            source, [line_number(source, "// break here")]
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+        self.dap_server.request_evaluate(
+            f"`command script import {progress_emitter}", context="repl"
+        )
+        self.dap_server.request_evaluate("`test-progress --seconds 1", context="repl")
+
+        self.verify_progress_events(
+            expected_title="Progress tester: Initial Indeterminate Detail",
+            expected_message="Step 1",
+            only_verify_first_update=True,
+        )
+
+    @skipIfWindows
+    def test_output_nodetails_indeterminate(self):
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        progress_emitter = os.path.join(os.getcwd(), "Progress_emitter.py")
+        source = "main.cpp"
+        breakpoint_ids = self.set_source_breakpoints(
+            source, [line_number(source, "// break here")]
+        )
+        self.dap_server.request_evaluate(
+            f"`command script import {progress_emitter}", context="repl"
+        )
+
+        self.dap_server.request_evaluate(
+            "`test-progress --seconds 1 --no-details", context="repl"
+        )
+
+        self.verify_progress_events(
+            expected_title="Progress tester: Initial Indeterminate Detail",
+            expected_message="Initial Indeterminate Detail",
+            only_verify_first_update=True,
+        )
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -18,8 +18,32 @@ using namespace lldb;
 
 namespace lldb_dap {
 
-static void ProgressEventThreadFunction(DAP &dap) {
-  llvm::set_thread_name(dap.name + ".progress_handler");
+static std::string GetStringFromStructuredData(lldb::SBStructuredData &data,
+                                               const char *key) {
+  lldb::SBStructuredData keyValue = data.GetValueForKey(key);
+  if (!keyValue)
+    return std::string();
+
+  const size_t length = keyValue.GetStringValue(nullptr, 0);
+
+  if (length == 0)
+    return std::string();
+
+  std::string str(length + 1, 0);
+  keyValue.GetStringValue(&str[0], length + 1);
+  return str;
+}
+
+static uint64_t GetUintFromStructuredData(lldb::SBStructuredData &data,
+                                          const char *key) {
+  lldb::SBStructuredData keyValue = data.GetValueForKey(key);
+
+  if (!keyValue.IsValid())
+    return 0;
+  return keyValue.GetUnsignedIntegerValue();
+}
+
+void ProgressEventThreadFunction(DAP &dap) {
   lldb::SBListener listener("lldb-dap.progress.listener");
   dap.debugger.GetBroadcaster().AddListener(
       listener, lldb::SBDebugger::eBroadcastBitProgress |
@@ -35,14 +59,47 @@ static void ProgressEventThreadFunction(DAP &dap) {
           done = true;
         }
       } else {
-        uint64_t progress_id = 0;
-        uint64_t completed = 0;
-        uint64_t total = 0;
-        bool is_debugger_specific = false;
-        const char *message = lldb::SBDebugger::GetProgressFromEvent(
-            event, progress_id, completed, total, is_debugger_specific);
-        if (message)
-          dap.SendProgressEvent(progress_id, message, completed, total);
+        lldb::SBStructuredData data =
+            lldb::SBDebugger::GetProgressDataFromEvent(event);
+
+        const uint64_t progress_id =
+            GetUintFromStructuredData(data, "progress_id");
+        const uint64_t completed = GetUintFromStructuredData(data, "completed");
+        const uint64_t total = GetUintFromStructuredData(data, "total");
+        const std::string details =
+            GetStringFromStructuredData(data, "details");
+
+        if (completed == 0) {
+          if (total == UINT64_MAX) {
+            // This progress is non deterministic and won't get updated until it
+            // is completed. Send the "message" which will be the combined title
+            // and detail. The only other progress event for thus
+            // non-deterministic progress will be the completed event So there
+            // will be no need to update the detail.
+            const std::string message =
+                GetStringFromStructuredData(data, "message");
+            dap.SendProgressEvent(progress_id, message.c_str(), completed,
+                                  total);
+          } else {
+            // This progress is deterministic and will receive updates,
+            // on the progress creation event VSCode will save the message in
+            // the create packet and use that as the title, so we send just the
+            // title in the progressCreate packet followed immediately by a
+            // detail packet, if there is any detail.
+            const std::string title =
+                GetStringFromStructuredData(data, "title");
+            dap.SendProgressEvent(progress_id, title.c_str(), completed, total);
+            if (!details.empty())
+              dap.SendProgressEvent(progress_id, details.c_str(), completed,
+                                    total);
+          }
+        } else {
+          // This progress event is either the end of the progress dialog, or an
+          // update with possible detail. The "detail" string we send to VS Code
+          // will be appended to the progress dialog's initial text from when it
+          // was created.
+          dap.SendProgressEvent(progress_id, details.c_str(), completed, total);
+        }
       }
     }
   }
