[lldb] Broadcast eBroadcastBitStackChanged when frame providers change

[vogelsgesang]

We want to reload the call stack whenever the frame providers are updated. To do so, we now emit a eBroadcastBitStackChanged on all threads whenever any changes to the frame providers take place.

I found this very useful while iterating on a frame provider in lldb-dap. So far, the new frame provider only took effect after continuing execution. Now the backtrace in VS-Code gets refreshed immediately upon running target frame-provider add.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Adrian Vogelsgesang (vogelsgesang)

Changes

We want to reload the call stack whenever the frame providers are updated. To do so, we now emit a eBroadcastBitStackChanged on all threads whenever any changes to the frame providers take place.

I found this very useful while iterating on a frame provider in lldb-dap. So far, the new frame provider only took effect after continuing execution. Now the back trace in VS-Code gets refreshed immediately upon running target frame-provider add.

---

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

2 Files Affected:

• (modified) lldb/include/lldb/Target/Target.h (+5)
• (modified) lldb/source/Target/Target.cpp (+35-24)

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 812a638910b3b..6d1f5f83d153b 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -776,6 +776,11 @@ class Target : public std::enable_shared_from_this<Target>,
   const llvm::DenseMap<uint32_t, ScriptedFrameProviderDescriptor> &
   GetScriptedFrameProviderDescriptors() const;
 
+protected:
+  /// Notify all threads that the stack traces might have changed.
+  void NotifyThreadsOfChangedFrameProviders();
+
+public:
   // This part handles the breakpoints.
 
   BreakpointList &GetBreakpointList(bool internal = false);
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 2305f1019ea4f..fe8e41ec4d825 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3725,47 +3725,43 @@ llvm::Expected<uint32_t> Target::AddScriptedFrameProviderDescriptor(
   if (!descriptor.IsValid())
     return llvm::createStringError("invalid frame provider descriptor");
 
+  uint32_t descriptor_id = descriptor.GetID();
+
   llvm::StringRef name = descriptor.GetName();
   if (name.empty())
     return llvm::createStringError(
         "frame provider descriptor has no class name");
 
-  std::lock_guard<std::recursive_mutex> guard(
-      m_frame_provider_descriptors_mutex);
-
-  uint32_t descriptor_id = descriptor.GetID();
-  m_frame_provider_descriptors[descriptor_id] = descriptor;
+  {
+    std::unique_lock<std::recursive_mutex> guard(m_frame_provider_descriptors_mutex);
+    m_frame_provider_descriptors[descriptor_id] = descriptor;
+  }
 
-  // Clear frame providers on existing threads so they reload with new config.
-  if (ProcessSP process_sp = GetProcessSP())
-    for (ThreadSP thread_sp : process_sp->Threads())
-      thread_sp->ClearScriptedFrameProvider();
+  NotifyThreadsOfChangedFrameProviders();
 
   return descriptor_id;
 }
 
 bool Target::RemoveScriptedFrameProviderDescriptor(uint32_t id) {
-  std::lock_guard<std::recursive_mutex> guard(
-      m_frame_provider_descriptors_mutex);
-  bool removed = m_frame_provider_descriptors.erase(id);
-
-  if (removed)
-    if (ProcessSP process_sp = GetProcessSP())
-      for (ThreadSP thread_sp : process_sp->Threads())
-        thread_sp->ClearScriptedFrameProvider();
+  bool removed;
+  {
+    std::lock_guard<std::recursive_mutex> guard(
+        m_frame_provider_descriptors_mutex);
+    removed = m_frame_provider_descriptors.erase(id);
+  }
 
+  if (removed) NotifyThreadsOfChangedFrameProviders();
   return removed;
 }
 
 void Target::ClearScriptedFrameProviderDescriptors() {
-  std::lock_guard<std::recursive_mutex> guard(
-      m_frame_provider_descriptors_mutex);
-
-  m_frame_provider_descriptors.clear();
+  {
+    std::lock_guard<std::recursive_mutex> guard(
+        m_frame_provider_descriptors_mutex);
+    m_frame_provider_descriptors.clear();
+  }
 
-  if (ProcessSP process_sp = GetProcessSP())
-    for (ThreadSP thread_sp : process_sp->Threads())
-      thread_sp->ClearScriptedFrameProvider();
+  NotifyThreadsOfChangedFrameProviders();
 }
 
 const llvm::DenseMap<uint32_t, ScriptedFrameProviderDescriptor> &
@@ -3775,6 +3771,21 @@ Target::GetScriptedFrameProviderDescriptors() const {
   return m_frame_provider_descriptors;
 }
 
+void Target::NotifyThreadsOfChangedFrameProviders() {
+  ProcessSP process_sp = GetProcessSP();
+  if (!process_sp)
+    return;
+  for (ThreadSP thread_sp : process_sp->Threads()) {
+    // Clear frame providers on existing threads so they reload with new config.
+    thread_sp->ClearScriptedFrameProvider();
+    // Notify threads that the stack traces might have changed.
+    if (EventTypeHasListeners(Thread::eBroadcastBitStackChanged)) {
+      auto data_sp = std::make_shared<Thread::ThreadEventData>(thread_sp);
+      thread_sp->BroadcastEvent(Thread::eBroadcastBitStackChanged, data_sp);
+    }
+  }
+}
+
 void Target::FinalizeFileActions(ProcessLaunchInfo &info) {
   Log *log = GetLog(LLDBLog::Process);
 

[github-actions]

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

[jimingham]

This looks fine to me, except that it is a little odd to have a function called NotifyThreadsOfChangedFrameProviders... that does more than notify, it also clears the cached providers. A function called NotifyWhatever should just notify.

[vogelsgesang]

except that it is a little odd to have a function called NotifyThreadsOfChangedFrameProviders... that does more than notify, it also clears the cached providers. A function called NotifyWhatever should just notify.

LLM-generated proposals:

• InvalidateThreadFrameProviders() — "invalidate" implies clearing cache and forcing reload
• RefreshThreadsForChangedFrameProviders() — "refresh" implies clearing and reloading
• ReloadThreadFrameProviders() — "reload" implies clearing and reloading

Any preference? Otherwise, I would probably go with InvalidateThreadFrameProviders

[medismailben]

LGTM with comments. Please write a test for this to make sure we receive the event as expected.

[github-actions]

🐧 Linux x64 Test Results

• 33302 tests passed
• 512 tests skipped

✅ The build succeeded and all tests passed.

[vogelsgesang]

ok, AI failed at generating a correct test case. I will need some time to understand how to use SBListeners from Python

[github-actions]

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

[vogelsgesang]

/cherry-pick 943782b

[llvmbot]

/cherry-pick 943782b

Error: Command failed due to missing milestone.

[vogelsgesang]

/cherry-pick 943782b

[llvmbot]

Failed to cherry-pick: 943782b

https://github.com/llvm/llvm-project/actions/runs/21727862504

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[vogelsgesang]

Ok, takes some rebasing effort to backport this.
@medismailben do you think we should even backport this?
If so, I could do that some time next week