Skip to content

[LLDB-DAP] Send Progress update message over DAP #123837

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 5 commits into from
Jan 22, 2025

Conversation

Jlalond
Copy link
Contributor

@Jlalond Jlalond commented Jan 21, 2025

When testing my SBProgress DAP PR (#123826), I noticed Progress update messages aren't sent over DAP. This patch adds the lldb progress event's message to the body when sent over DAP.

Before
image

Now
image

Tested with my progress tester command, testing 10 events 5 seconds apart 1-10

@llvmbot
Copy link
Member

llvmbot commented Jan 21, 2025

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

When testing my SBProgress DAP PR (#123826), I noticed Progress update messages aren't sent over DAP. This patch adds the lldb progress event's message to the body when sent over DAP.

Before
image

Now
image

Tested with my progress tester command, testing 10 events 5 seconds apart 1-10


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

2 Files Affected:

  • (modified) lldb/tools/lldb-dap/ProgressEvent.cpp (+10-5)
  • (modified) lldb/tools/lldb-dap/ProgressEvent.h (+2-1)
diff --git a/lldb/tools/lldb-dap/ProgressEvent.cpp b/lldb/tools/lldb-dap/ProgressEvent.cpp
index 0dcc2ee81001d5..7807ecfb6c34d0 100644
--- a/lldb/tools/lldb-dap/ProgressEvent.cpp
+++ b/lldb/tools/lldb-dap/ProgressEvent.cpp
@@ -117,6 +117,10 @@ json::Value ProgressEvent::ToJSON() const {
     body.try_emplace("cancellable", false);
   }
 
+  if (m_event_type == progressUpdate) {
+    EmplaceSafeString(body, "message", m_message);
+  }
+
   std::string timestamp(llvm::formatv("{0:f9}", m_creation_time.count()));
   EmplaceSafeString(body, "timestamp", timestamp);
 
@@ -164,10 +168,11 @@ const ProgressEvent &ProgressEventManager::GetMostRecentEvent() const {
   return m_last_update_event ? *m_last_update_event : m_start_event;
 }
 
-void ProgressEventManager::Update(uint64_t progress_id, uint64_t completed,
-                                  uint64_t total) {
-  if (std::optional<ProgressEvent> event = ProgressEvent::Create(
-          progress_id, std::nullopt, completed, total, &GetMostRecentEvent())) {
+void ProgressEventManager::Update(uint64_t progress_id, const char *message,
+                                  uint64_t completed, uint64_t total) {
+  if (std::optional<ProgressEvent> event =
+          ProgressEvent::Create(progress_id, StringRef(message), completed,
+                                total, &GetMostRecentEvent())) {
     if (event->GetEventType() == progressEnd)
       m_finished = true;
 
@@ -227,7 +232,7 @@ void ProgressEventReporter::Push(uint64_t progress_id, const char *message,
       m_unreported_start_events.push(event_manager);
     }
   } else {
-    it->second->Update(progress_id, completed, total);
+    it->second->Update(progress_id, message, completed, total);
     if (it->second->Finished())
       m_event_managers.erase(it);
   }
diff --git a/lldb/tools/lldb-dap/ProgressEvent.h b/lldb/tools/lldb-dap/ProgressEvent.h
index 72317b879c803a..8494221890a1c8 100644
--- a/lldb/tools/lldb-dap/ProgressEvent.h
+++ b/lldb/tools/lldb-dap/ProgressEvent.h
@@ -99,7 +99,8 @@ class ProgressEventManager {
 
   /// Receive a new progress event for the start event and try to report it if
   /// appropriate.
-  void Update(uint64_t progress_id, uint64_t completed, uint64_t total);
+  void Update(uint64_t progress_id, const char *message, uint64_t completed,
+              uint64_t total);
 
   /// \return
   ///     \b true if a \a progressEnd event has been notified. There's no

@Jlalond
Copy link
Contributor Author

Jlalond commented Jan 21, 2025

CC: @youngdfb

Copy link
Contributor

@jeffreytan81 jeffreytan81 left a comment

Choose a reason for hiding this comment

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

Can you add a DAP test to ensure progress event does contain the message you update in SBProgress?

@Jlalond
Copy link
Contributor Author

Jlalond commented Jan 21, 2025

@jeffreytan81 Sure, can you point me towards some existing test cases?

@Jlalond Jlalond force-pushed the dap-progress-event-update-message branch from 49551ed to a4446e0 Compare January 21, 2025 23:20
@Jlalond Jlalond force-pushed the dap-progress-event-update-message branch from a4446e0 to 56846b2 Compare January 22, 2025 19:58
Copy link

github-actions bot commented Jan 22, 2025

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


self.dap_server.wait_for_event("progressEnd", 15)
# Expect at least a start, an update, and end event
# However because the progress is an RAII object and we can't guaruntee
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean "progress is not a RAII object"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but I believe because we'll free that memory when the python object is destroyed we can't depend on the deterministic behavior of the RAII object.

@Jlalond Jlalond merged commit a939a9f into llvm:main Jan 22, 2025
5 of 6 checks passed
JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request Jan 27, 2025
When testing my SBProgress DAP PR (llvm#123826), I noticed Progress update
messages aren't sent over DAP. This patch adds the lldb progress event's
message to the body when sent over DAP.

Before

![image](https://github.com/user-attachments/assets/404adaa8-b784-4f23-895f-cd3625fdafad)

Now

![image](https://github.com/user-attachments/assets/eb1c3235-0936-4e36-96e5-0a0ee60dabb8)

Tested with my [progress tester
command](https://gist.github.com/Jlalond/48d85e75a91f7a137e3142e6a13d0947),
testing 10 events 5 seconds apart 1-10

(cherry picked from commit a939a9f)
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Jan 27, 2025
…a939a9fd53d9

[🍒 swift/release/6.1] [LLDB-DAP] Send Progress update message over DAP (llvm#123837)
Jlalond added a commit that referenced this pull request Mar 5, 2025
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)
@Jlalond Jlalond deleted the dap-progress-event-update-message branch March 7, 2025 19:20
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
…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 added a commit that referenced this pull request Mar 27, 2025
…LI (#133309)

In the original SBProgress patch, #123837, I didn't ensure the debugger
was broadcasting these events to the CLI as SBProgress has far been
focused on DAP. We had an internal ask to have SBProgress events
broadcasted to the CLI so this patch addresses that.

<img width="387" alt="image"
src="https://github.com/user-attachments/assets/5eb93a46-1db6-4d46-a6b7-2b2f9bbe71db"
/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants