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

Jlalond · 2025-01-27T23:00:03Z

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.

Instead, we only use the title when it's the first message, removing the duplicate title problem.

llvmbot · 2025-01-27T23:00:40Z

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

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.

Instead, we only use the title when it's the first message, removing the duplicate title problem.

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

2 Files Affected:

(modified) lldb/include/lldb/Core/DebuggerEvents.h (+10-6)
(modified) lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py (+5)

diff --git a/lldb/include/lldb/Core/DebuggerEvents.h b/lldb/include/lldb/Core/DebuggerEvents.h
index 49a4ecf8e537e3..ca06ee835fde38 100644
--- a/lldb/include/lldb/Core/DebuggerEvents.h
+++ b/lldb/include/lldb/Core/DebuggerEvents.h
@@ -44,12 +44,16 @@ 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;
+    // 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();
   }
   const std::string &GetTitle() const { return m_title; }
   const std::string &GetDetails() const { return m_details; }
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 36c0cef9c47143..945c3f7633364c 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)

walter-erquinigo · 2025-01-27T23:09:35Z

lldb/include/lldb/Core/DebuggerEvents.h

-    }
-    return message;
+    // Only put the title in the message of the progress create event.
+    if (m_completed == 0) {


can you create some using/defines for constants like this 0? otherwise it's hard to read

clayborg · 2025-01-27T23:47:19Z

lldb/include/lldb/Core/DebuggerEvents.h

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


We can't change the way that progress events get reported. It will work for DAP nicely, but not for anyone else. We are removing the title from any subsequent notications and that will affect everyone.

I would propose adding a new call like SBDebugger::GetProgressFromEvent(...) where we can get just the detail and the title as separate strings. Maybe:

const char *SBDebugger::GetProgressFromEvent(const lldb::SBEvent &event, uint64_t &progress_id, uint64_t &completed, uint64_t &total, bool &is_debugger_specific, const char **detail);

And if detail is NULL, we return the combined string of title + ": " + detail and if it is non NULL, the return value is the title only, and the detail gets filled into detail. Or we can make accessors for the title and detail like:

const char *SBDebugger::GetProgressTitleFromEvent(const lldb::SBEvent &event); const char *SBDebugger::GetProgressDetailFromEvent(const lldb::SBEvent &event);

Any thoughts on which approach?

The current API for const char * SBDebugger::GetProgressFromEvent(...) is causing some memory bloat for any users as each return value, which is the <title> + ":" + <detail> is being constified via ConstString so that we can return a const char *. Currently I am not sure of how many Progress objects update the detail and return millions of updates, but if they did, that will cause use to constify all of the <title> + ":" + <detail> strings, which is bad. So maybe a new API should be something like:

bool SBDebugger::GetProgressFromEvent( const lldb::SBEvent &event, uint64_t &progress_id, uint64_t &completed, uint64_t &total, bool &is_debugger_specific, char *title, size_t max_title_length, char *detail, size_t max_detail_length);

Then we can call strncpy on the Progress::m_title and Progress::m_detail as needed and avoid constifying the progress strings and bloating up the const string pool.

Might be good to allow title and detail to be NULL and not filled in if they are NULL.

Can we use SBDebugger::GetProgressDataFromEvent which returns SBStructuredData with all the fields and then construct the message in lldb-dap?

Can we use SBDebugger::GetProgressDataFromEvent which returns SBStructuredData with all the fields and then construct the message in lldb-dap?

This was going to be my alternative proposal. I assumed this wouldn't fly because I didn't know how many consumers need a workflow that doesn't follow the DAP rules. But based on the conversation I had with Greg offline the DAP progress events needs to handle it's own branching. I think the best way is returning a structured data to give us flexibility in the future.

Agreed, that method was added exactly for that reason: so we didn't need to keep adding overloads to GetProgressFromEvent every time we added a new field. FWIW I personally think it's reasonable to construct the string in lldb-dap.

lldb/tools/lldb-dap/lldb-dap.cpp

JDevlieghere · 2025-02-07T21:24:23Z

lldb/tools/lldb-dap/lldb-dap.cpp

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


Suggested change

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;

lldb::SBStructuredData keyValue = data.GetValueForKey(key);

if (!keyValue)

return std::string();

const size_t length = keyValue.GetStringValue(nullptr, 0);

std::string str(length + 1, 0);

value.GetStringValue(&str[0], length);

return str;

Not really, you can avoid the resize with the ctor but otherwise that's as good as it gets (without the logging).

JDevlieghere · 2025-02-07T21:28:27Z

lldb/tools/lldb-dap/lldb-dap.cpp

+        uint64_t progress_id = GetUintFromStructuredData(data, "progress_id");
+        uint64_t completed = GetUintFromStructuredData(data, "completed");
+        uint64_t total = GetUintFromStructuredData(data, "total");


Suggested change

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

Nit: I'd make these const to make it clear that we're not going to change these, as opposed to the message that we're building. I'd also move details up here so that all the message building is co-located.

JDevlieghere · 2025-02-07T21:29:26Z

lldb/tools/lldb-dap/lldb-dap.cpp

+        if (message.length() > 0 && progress_id > 0 && total >= 0 &&
+            completed >= 0)


total and completed are unsigned so isn't total >= 0 && completed >= 0 always true?

Jlalond · 2025-02-18T19:35:31Z

@JDevlieghere Cleaned up as you suggested, sorry about the initial rough state but I mostly wanted a sanity check if I was overlooking anything.

github-actions · 2025-02-18T19:39:09Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clayborg

So this is changing lldb-dap progress reporting so the first reporting sends the title + detail, and subsequent entries sent just the details. How does this look in the progress API that is shown by VS Code? Does the detail just show up somewhere and remove the title?

clayborg · 2025-02-19T18:40:51Z

lldb/tools/lldb-dap/lldb-dap.cpp

+        if (completed == 0) {
+          const std::string title = GetStringFromStructuredData(data, "title");
+          message += title;
+          message += ": ";


We should check if details contains anything before appending ": "

clayborg · 2025-02-19T18:43:17Z

lldb/tools/lldb-dap/lldb-dap.cpp

+          message += ": ";
+        }
+
+        message += details;


What if details is empty? We should send just the title again in that case instead of nothing? This really depends on how the progress reporting looks in VS code.

Jlalond · 2025-02-19T21:24:34Z

@clayborg

DAP with a detail and a total

Indeterminate with an increment detail

Then with an indeterminate progress, but no increment detail

For number 2 I think I found another bug for indeterminate messages not updating the detail correctly, but I will address this in a future patch.

lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py

clayborg · 2025-02-20T01:02:46Z

lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py

+
+        # 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:

clayborg · 2025-02-20T01:03:15Z

lldb/test/API/tools/lldb-dap/progress/Progress_emitter.py

@@ -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:


Default will be None if not set now, so this should be:

if total is None:

clayborg · 2025-02-20T01:05:02Z

lldb/tools/lldb-dap/lldb-dap.cpp

+        // the title and the detail as a single message.
+        if (completed == 0 && total == 1) {
+          const std::string message =
+              GetStringFromStructuredData(data, "message");


is the "message" value a combined string if title + detail?

Yes, message is title and detail

clayborg · 2025-02-20T01:07:43Z

lldb/tools/lldb-dap/lldb-dap.cpp

+        // 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) {


restructure as:

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 = ... } else { // This progress is deterministic and will receive updates. On the first event... } }

clayborg · 2025-02-20T01:10:21Z

lldb/tools/lldb-dap/lldb-dap.cpp

+          // We don't check if message has any contents, because we still want
+          // to broadcast a status update.


Lets document what VS code expects a bit more to make it clear. Something like:

// 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.

clayborg · 2025-02-20T01:12:01Z

lldb/tools/lldb-dap/lldb-dap.cpp

+          // 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.


Lets explain this better so any others making modifications will know what is going on. Make it clear that the first string we send to VS code when creating a new progress will be copied into the progress window title, and any subsequent detail strings will get appended to the initial title we provide to VS code.

github-actions · 2025-02-20T22:51:54Z

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

lldb/source/API/SBProgress.cpp

lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D68927453

…arios

…eck to sbprogress description check

… update the doc string

lldb/bindings/interface/SBProgressDocstrings.i

clayborg · 2025-03-03T23:49:57Z

lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py

+        expected_not_in_message=None,
+        only_verify_first_update=False,
+    ):
+        self.dap_server.wait_for_event("progressEnd", 5)


If a server is running multiple tests at once, there may be some delays. If you are sending 3 events, one second apart, I would give this timeout a larger value to ensure we don't timeout on busy machines. Something like 10 or 15 seconds at least.

lldb/bindings/interface/SBProgressDocstrings.i

@clayborg

This patch adds a finalize method which destroys the underlying RAII SBProgress. My primary motivation for this is so I can write better tests that are non-flaky, but after discussing with @clayborg in my DAP message improvement patch (llvm#124648) this is probably an essential API despite that I originally argued it wasn't.

…4648) In my last DAP patch (llvm#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)

Jlalond requested review from walter-erquinigo and clayborg January 27, 2025 23:00

Jlalond requested a review from JDevlieghere as a code owner January 27, 2025 23:00

llvmbot added the lldb label Jan 27, 2025

Jlalond changed the title ~~[LLDB-DAP] Sbdebugger progress no title prefix~~ [LLDB-DAP] SBDebugger don't prefix title on progress updates Jan 27, 2025

walter-erquinigo reviewed Jan 27, 2025

View reviewed changes

clayborg requested changes Jan 27, 2025

View reviewed changes

Jlalond commented Jan 30, 2025

View reviewed changes

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

Jlalond commented Jan 30, 2025

View reviewed changes

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

JDevlieghere added the lldb-dap label Jan 31, 2025

JDevlieghere reviewed Feb 7, 2025

View reviewed changes

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from ca2f7e3 to 19a03d8 Compare February 18, 2025 19:34

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from 19a03d8 to fbe04c1 Compare February 18, 2025 19:40

JDevlieghere approved these changes Feb 18, 2025

View reviewed changes

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch 2 times, most recently from 6e20c5e to f79d7bd Compare February 18, 2025 22:33

clayborg reviewed Feb 19, 2025

View reviewed changes

clayborg requested changes Feb 19, 2025

View reviewed changes

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from 11fd267 to d96b308 Compare February 19, 2025 21:09

clayborg requested changes Feb 20, 2025

View reviewed changes

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from afe6168 to 8616ea3 Compare February 20, 2025 17:38

clayborg requested changes Feb 26, 2025

View reviewed changes

lldb/source/API/SBProgress.cpp Outdated Show resolved Hide resolved

lldb/test/API/tools/lldb-dap/progress/TestDAP_Progress.py Outdated Show resolved Hide resolved

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch 2 times, most recently from 681352c to 6d4b3aa Compare February 26, 2025 20:22

Jlalond added 10 commits March 3, 2025 14:47

Only include title on the first message

51420c9

Add comment explaining if and add a test

dde0774

Migrate to structured data

cd19400

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D68927453

Feedback from Jonas, and cleanup dev logging

b6b0866

Add new test cases, and refactor to support TITLE: DETAIL in all scen…

9ae2fbb

…arios

Test refactor and comment improvements based on Greg's feedback

ddab9fb

Fix tests, correct for new behavior

5e94d10

Python formatting

682eb63

Refactor tests to be concise and add progresse end, add additional ch…

bb079b2

…eck to sbprogress description check

Add Finalize to the test to ensure we always get the end message, and…

cee7540

… update the doc string

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from 6d4b3aa to cee7540 Compare March 3, 2025 23:08

Cleanup test of print statements, run formatter

41b9d7d

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from b0731ca to 41b9d7d Compare March 3, 2025 23:17

Add some basic code to the summary

458707c

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from e762fb3 to 458707c Compare March 3, 2025 23:24

Fix non-deterministic title case

adb146f

clayborg requested changes Mar 3, 2025

View reviewed changes

Greg comment feedback

f2c98a2

clayborg requested changes Mar 4, 2025

View reviewed changes

Add and remove lines as recommended

c7a5f3f

Jlalond force-pushed the sbdebugger-progress-no-title-prefix branch from 19eb994 to c7a5f3f Compare March 4, 2025 22:29

clayborg approved these changes Mar 5, 2025

View reviewed changes

Jlalond merged commit 02f024c into llvm:main Mar 5, 2025
10 checks passed

Jlalond deleted the sbdebugger-progress-no-title-prefix branch March 7, 2025 19:19

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

		if (message.length() > 0 && progress_id > 0 && total >= 0 &&
		completed >= 0)

		// We don't check if message has any contents, because we still want
		// to broadcast a status update.

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

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

Uh oh!

Conversation

Jlalond commented Jan 27, 2025

Uh oh!

llvmbot commented Jan 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond commented Feb 18, 2025

Uh oh!

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

Uh oh!

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 18, 2025 •

edited

Loading

github-actions bot commented Feb 20, 2025 •

edited

Loading