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

Jacob Lalonde via lldb-commits lldb-commits at lists.llvm.org
Thu Feb 20 09:36:11 PST 2025


https://github.com/Jlalond updated https://github.com/llvm/llvm-project/pull/124648

>From 12ff645735c1dbf51e58b9f80d4e3e13a0babdf5 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Mon, 27 Jan 2025 13:41:58 -0800
Subject: [PATCH 1/7] Only include title on the first message

---
 lldb/include/lldb/Core/DebuggerEvents.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 49a4ecf8e537e..52e4f77d7637d 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -44,12 +44,15 @@ class ProgressEventData : public EventData {
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
   std::string GetMessage() const {
-    std::string message = m_title;
-    if (!m_details.empty()) {
-      message.append(": ");
-      message.append(m_details);
-    }
-    return message;
+    if (m_completed == 0) {
+      std::string message = m_title;
+      if (!m_details.empty()) {
+        message.append(": ");
+        message.append(m_details);
+      }
+      return message;
+    } else
+      return !m_details.empty() ? m_details : std::string();
   }
   const std::string &GetTitle() const { return m_title; }
   const std::string &GetDetails() const { return m_details; }

>From 82ed76ae3b6bef176bf54dd1031f0cb9c95081c1 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Mon, 27 Jan 2025 14:48:01 -0800
Subject: [PATCH 2/7] Add comment explaining if and add a test

---
 lldb/include/lldb/Core/DebuggerEvents.h                   | 1 +
 lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 52e4f77d7637d..ca06ee835fde3 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -44,6 +44,7 @@ class ProgressEventData : public EventData {
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
   std::string GetMessage() const {
+    // Only put the title in the message of the progress create event.
     if (m_completed == 0) {
       std::string message = m_title;
       if (!m_details.empty()) {
diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
index 36c0cef9c4714..945c3f7633364 100755
--- a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
+++ b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -41,8 +41,13 @@ def test_output(self):
         for event in self.dap_server.progress_events:
             event_type = event["event"]
             if "progressStart" in event_type:
+                title = event["body"]["title"]
+                self.assertIn("Progress tester", title)
                 start_found = True
             if "progressUpdate" in event_type:
+                message = event["body"]["message"]
+                print(f"Progress update: {message}")
+                self.assertNotIn("Progres tester", message)
                 update_found = True
 
         self.assertTrue(start_found)

>From e15090782a93e07e8a260a0594ca02430e68e09f Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 30 Jan 2025 10:04:04 -0800
Subject: [PATCH 3/7] Migrate to structured data

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:


Differential Revision: https://phabricator.intern.facebook.com/D68927453
---
 lldb/include/lldb/Core/DebuggerEvents.h | 16 +++-----
 lldb/tools/lldb-dap/lldb-dap.cpp        | 54 +++++++++++++++++++++----
 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index ca06ee835fde3..49a4ecf8e537e 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -44,16 +44,12 @@ class ProgressEventData : public EventData {
   uint64_t GetCompleted() const { return m_completed; }
   uint64_t GetTotal() const { return m_total; }
   std::string GetMessage() const {
-    // Only put the title in the message of the progress create event.
-    if (m_completed == 0) {
-      std::string message = m_title;
-      if (!m_details.empty()) {
-        message.append(": ");
-        message.append(m_details);
-      }
-      return message;
-    } else
-      return !m_details.empty() ? m_details : std::string();
+    std::string message = m_title;
+    if (!m_details.empty()) {
+      message.append(": ");
+      message.append(m_details);
+    }
+    return message;
   }
   const std::string &GetTitle() const { return m_title; }
   const std::string &GetDetails() const { return m_details; }
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index e323990d8b6ed..4b3460190a7c9 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -55,6 +55,7 @@
 #include <thread>
 #include <vector>
 
+#include <iostream>
 #if defined(_WIN32)
 // We need to #define NOMINMAX in order to skip `min()` and `max()` macro
 // definitions that conflict with other system headers.
@@ -412,6 +413,30 @@ void SendStdOutStdErr(DAP &dap, lldb::SBProcess &process) {
     dap.SendOutput(OutputType::Stderr, llvm::StringRef(buffer, count));
 }
 
+static std::string GetStringFromStructuredData(lldb::SBStructuredData &data,
+                                               const char *key) {
+  lldb::SBStructuredData keyValue = data.GetValueForKey(key);
+  if (!keyValue)
+    return std::string();
+
+  size_t size = keyValue.GetStringValue(nullptr, 0);
+  std::cout << "Size for " << key << " " << size << std::endl;
+  std::string stringValue;
+  stringValue.resize(size);
+  keyValue.GetStringValue(&stringValue[0], size + 1);
+  std::cout << "String value after: " << stringValue << std::endl;
+  return stringValue;
+}
+
+static uint64_t GetUintFromStructuredData(lldb::SBStructuredData &data,
+                                          const char *key) {
+  lldb::SBStructuredData keyValue = data.GetValueForKey(key);
+
+  if (!keyValue.IsValid())
+    return -1;
+  return keyValue.GetUnsignedIntegerValue();
+}
+
 void ProgressEventThreadFunction(DAP &dap) {
   lldb::SBListener listener("lldb-dap.progress.listener");
   dap.debugger.GetBroadcaster().AddListener(
@@ -428,14 +453,27 @@ 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);
+
+        uint64_t progress_id = GetUintFromStructuredData(data, "progress_id");
+        uint64_t completed = GetUintFromStructuredData(data, "completed");
+        uint64_t total = GetUintFromStructuredData(data, "total");
+        std::string message;
+        // Include the title on the first event.
+        if (completed == 0) {
+          std::string title = GetStringFromStructuredData(data, "title");
+          message += title;
+          message += ": ";
+        }
+
+        std::string details = GetStringFromStructuredData(data, "details");
+        message += details;
+        // Verbose check, but we get -1 for the uint64 on failure to read
+        // so we check everything before broadcasting an event.
+        if (message.length() > 0 && progress_id > 0 && total >= 0 &&
+            completed >= 0)
+          dap.SendProgressEvent(progress_id, message.c_str(), completed, total);
       }
     }
   }

>From d616814939c8af59fae20e8994d3af4f37b27232 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Tue, 18 Feb 2025 11:28:58 -0800
Subject: [PATCH 4/7] Feedback from Jonas, and cleanup dev logging

---
 lldb/tools/lldb-dap/lldb-dap.cpp | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 4b3460190a7c9..b08ab08d9a8b3 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -55,7 +55,6 @@
 #include <thread>
 #include <vector>
 
-#include <iostream>
 #if defined(_WIN32)
 // We need to #define NOMINMAX in order to skip `min()` and `max()` macro
 // definitions that conflict with other system headers.
@@ -419,13 +418,10 @@ static std::string GetStringFromStructuredData(lldb::SBStructuredData &data,
   if (!keyValue)
     return std::string();
 
-  size_t size = keyValue.GetStringValue(nullptr, 0);
-  std::cout << "Size for " << key << " " << size << std::endl;
-  std::string stringValue;
-  stringValue.resize(size);
-  keyValue.GetStringValue(&stringValue[0], size + 1);
-  std::cout << "String value after: " << stringValue << std::endl;
-  return stringValue;
+  const size_t length = keyValue.GetStringValue(nullptr, 0);
+  std::string str(length + 1, 0);
+  keyValue.GetStringValue(&str[0], length);
+  return str;
 }
 
 static uint64_t GetUintFromStructuredData(lldb::SBStructuredData &data,
@@ -433,7 +429,7 @@ static uint64_t GetUintFromStructuredData(lldb::SBStructuredData &data,
   lldb::SBStructuredData keyValue = data.GetValueForKey(key);
 
   if (!keyValue.IsValid())
-    return -1;
+    return 0;
   return keyValue.GetUnsignedIntegerValue();
 }
 
@@ -456,23 +452,24 @@ void ProgressEventThreadFunction(DAP &dap) {
         lldb::SBStructuredData data =
             lldb::SBDebugger::GetProgressDataFromEvent(event);
 
-        uint64_t progress_id = GetUintFromStructuredData(data, "progress_id");
-        uint64_t completed = GetUintFromStructuredData(data, "completed");
-        uint64_t total = GetUintFromStructuredData(data, "total");
+        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");
         std::string message;
         // Include the title on the first event.
         if (completed == 0) {
-          std::string title = GetStringFromStructuredData(data, "title");
+          const std::string title = GetStringFromStructuredData(data, "title");
           message += title;
           message += ": ";
         }
 
-        std::string details = GetStringFromStructuredData(data, "details");
         message += details;
         // Verbose check, but we get -1 for the uint64 on failure to read
         // so we check everything before broadcasting an event.
-        if (message.length() > 0 && progress_id > 0 && total >= 0 &&
-            completed >= 0)
+        if (message.length() > 0)
           dap.SendProgressEvent(progress_id, message.c_str(), completed, total);
       }
     }

>From f79d7bdd4a3d4062cc8c144c5bb1b1f824087fc9 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Tue, 18 Feb 2025 14:04:53 -0800
Subject: [PATCH 5/7] Fix test by making sure we include up to the last
 character

---
 lldb/tools/lldb-dap/lldb-dap.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index b08ab08d9a8b3..3805eb3f55879 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -419,8 +419,12 @@ static std::string GetStringFromStructuredData(lldb::SBStructuredData &data,
     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);
+  keyValue.GetStringValue(&str[0], length + 1);
   return str;
 }
 

>From d96b3089088871ebccf7306ac2550222a7efafaa Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 19 Feb 2025 12:49:20 -0800
Subject: [PATCH 6/7] Fix bug in SBProgress, add new test cases, and refactor
 to support TITLE: DETAIL in all scenarios

---
 lldb/source/API/SBProgress.cpp                |   5 +-
 .../lldb-dap/progress/Progress_emitter.py     |  29 +++-
 .../lldb-dap/progress/TestDAP_Progress.py     | 127 +++++++++++++++++-
 lldb/tools/lldb-dap/lldb-dap.cpp              |  29 ++--
 4 files changed, 174 insertions(+), 16 deletions(-)

diff --git a/lldb/source/API/SBProgress.cpp b/lldb/source/API/SBProgress.cpp
index e67e289a60eff..403900aed4a8c 100644
--- a/lldb/source/API/SBProgress.cpp
+++ b/lldb/source/API/SBProgress.cpp
@@ -40,7 +40,10 @@ SBProgress::~SBProgress() = default;
 void SBProgress::Increment(uint64_t amount, const char *description) {
   LLDB_INSTRUMENT_VA(amount, description);
 
-  m_opaque_up->Increment(amount, description);
+  std::optional<std::string> description_opt;
+  if (description)
+    description_opt = description;
+  m_opaque_up->Increment(amount, description_opt);
 }
 
 lldb_private::Progress &SBProgress::ref() const { return *m_opaque_up; }
diff --git a/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py b/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py
index 7f4055cab9ddd..e962b1a10d402 100644
--- 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,10 @@ def create_options(cls):
         )
 
         parser.add_option(
-            "--total", dest="total", help="Total to count up.", type="int"
+            "--total",
+            dest="total",
+            help="Total to count up, use -1 to identify as indeterminate",
+            type="int",
         )
 
         parser.add_option(
@@ -47,6 +50,13 @@ def create_options(cls):
             type="int",
         )
 
+        parser.add_option(
+            "--no-details",
+            dest="no_details",
+            help="Do not display details",
+            action="store_true",
+        )
+
         return parser
 
     def get_short_help(self):
@@ -68,10 +78,23 @@ def __call__(self, debugger, command, exe_ctx, result):
             return
 
         total = cmd_options.total
-        progress = lldb.SBProgress("Progress tester", "Detail", total, debugger)
+        if total == -1:
+            progress = lldb.SBProgress(
+                "Progress tester", "Indeterminate Detail", debugger
+            )
+        else:
+            progress = lldb.SBProgress("Progress tester", "Detail", total, debugger)
+
+        # Check to see if total is set to -1 to indicate an indeterminate progress
+        # then default to 10 steps.
+        if total == -1:
+            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)
 
 
diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
index 945c3f7633364..2611df042f677 100755
--- a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
+++ b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -4,6 +4,7 @@
 
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
+import json
 import os
 import time
 
@@ -18,7 +19,6 @@ def test_output(self):
         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")]
         )
@@ -52,3 +52,128 @@ def test_output(self):
 
         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")
+        print(f"Progress emitter path: {progress_emitter}")
+        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.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:
+                title = event["body"]["title"]
+                self.assertIn("Progress tester", title)
+                start_found = True
+            if "progressUpdate" in event_type:
+                message = event["body"]["message"]
+                # We send an update on first message to set the details, so ignore the first update
+                if not update_found:
+                    self.assertTrue(message == "", message)
+                update_found = True
+
+        self.assertTrue(start_found)
+        self.assertTrue(update_found)
+
+    @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")
+        print(f"Progress emitter path: {progress_emitter}")
+        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 -1 --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:
+                title = event["body"]["title"]
+                self.assertIn("Progress tester", title)
+                start_found = True
+            if "progressUpdate" in event_type:
+                message = event["body"]["message"]
+                print(f"Progress update: {message}")
+                self.assertNotIn("Progres tester", message)
+                update_found = True
+
+        self.assertTrue(start_found)
+        self.assertTrue(update_found)
+
+    @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")
+        print(f"Progress emitter path: {progress_emitter}")
+        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 --total -1 --seconds 1 --no-details", 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:
+                title = event["body"]["title"]
+                self.assertIn("Progress tester", title)
+                start_found = True
+            if "progressUpdate" in event_type:
+                message = event["body"]["message"]
+                # We send an update on first message to set the details, so ignore the first update
+                if not update_found:
+                    self.assertTrue(message == "", message)
+                update_found = True
+
+        self.assertTrue(start_found)
+        self.assertTrue(update_found)
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 3805eb3f55879..ee7614a9e3ca5 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -462,19 +462,26 @@ void ProgressEventThreadFunction(DAP &dap) {
         const uint64_t total = GetUintFromStructuredData(data, "total");
         const std::string details =
             GetStringFromStructuredData(data, "details");
-        std::string message;
-        // Include the title on the first event.
-        if (completed == 0) {
-          const std::string title = GetStringFromStructuredData(data, "title");
-          message += title;
-          message += ": ";
-        }
 
-        message += details;
-        // Verbose check, but we get -1 for the uint64 on failure to read
-        // so we check everything before broadcasting an event.
-        if (message.length() > 0)
+        // If we're only creating and sending on progress event, we send
+        // the title and the detail as a single message.
+        if (completed == 0 && total == 1) {
+          const std::string message =
+              GetStringFromStructuredData(data, "message");
           dap.SendProgressEvent(progress_id, message.c_str(), completed, total);
+        } else if (completed == 0) {
+          // On the first event, send just the title which will be captured by
+          // DAP Then if details is not empty, send a second event with the
+          // 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
+          // We don't check if message has any contents, because we still want
+          // to broadcast a status update.
+          dap.SendProgressEvent(progress_id, details.c_str(), completed, total);
       }
     }
   }

>From afe616849dd7e42abea0e5527d2f5b1ef8b3745b Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 20 Feb 2025 09:35:54 -0800
Subject: [PATCH 7/7] Test refactor and comment improvements based on Greg's
 feedback

---
 .../lldb-dap/progress/Progress_emitter.py     |  5 +-
 .../lldb-dap/progress/TestDAP_Progress.py     |  6 +--
 lldb/tools/lldb-dap/lldb-dap.cpp              | 46 ++++++++++++-------
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py b/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py
index e962b1a10d402..41283da068868 100644
--- a/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py
+++ b/lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py
@@ -41,6 +41,7 @@ def create_options(cls):
             dest="total",
             help="Total to count up, use -1 to identify as indeterminate",
             type="int",
+            default=None,
         )
 
         parser.add_option(
@@ -78,7 +79,7 @@ def __call__(self, debugger, command, exe_ctx, result):
             return
 
         total = cmd_options.total
-        if total == -1:
+        if total is None:
             progress = lldb.SBProgress(
                 "Progress tester", "Indeterminate Detail", debugger
             )
@@ -87,7 +88,7 @@ def __call__(self, debugger, command, exe_ctx, result):
 
         # Check to see if total is set to -1 to indicate an indeterminate progress
         # then default to 10 steps.
-        if total == -1:
+        if total is None:
             total = 10
 
         for i in range(1, total):
diff --git a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
index 2611df042f677..c7579e8074c1d 100755
--- a/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
+++ b/lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py
@@ -109,9 +109,7 @@ def test_output_indeterminate(self):
         self.dap_server.request_evaluate(
             f"`command script import {progress_emitter}", context="repl"
         )
-        self.dap_server.request_evaluate(
-            "`test-progress --total -1 --seconds 1", context="repl"
-        )
+        self.dap_server.request_evaluate("`test-progress --seconds 1", context="repl")
 
         self.dap_server.wait_for_event("progressEnd", 15)
         # Expect at least a start, an update, and end event
@@ -151,7 +149,7 @@ def test_output_nodetails_indeterminate(self):
         )
 
         self.dap_server.request_evaluate(
-            "`test-progress --total -1 --seconds 1 --no-details", context="repl"
+            "`test-progress --seconds 1 --no-details", context="repl"
         )
 
         self.dap_server.wait_for_event("progressEnd", 15)
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index ee7614a9e3ca5..e1f980ff4d8dc 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -463,25 +463,37 @@ void ProgressEventThreadFunction(DAP &dap) {
         const std::string details =
             GetStringFromStructuredData(data, "details");
 
-        // If we're only creating and sending on progress event, we send
-        // the title and the detail as a single message.
-        if (completed == 0 && total == 1) {
-          const std::string message =
-              GetStringFromStructuredData(data, "message");
-          dap.SendProgressEvent(progress_id, message.c_str(), completed, total);
-        } else if (completed == 0) {
-          // On the first event, send just the title which will be captured by
-          // DAP Then if details is not empty, send a second event with the
-          // 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,
+        if (completed == 0) {
+          if (total == 1) {
+            // 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
-          // We don't check if message has any contents, because we still want
-          // to broadcast a status update.
+          } 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);
+        }
       }
     }
   }



More information about the lldb-commits mailing list