[lldb][Target] Clear selected frame index after a StopInfo::PerformAction

[Michael137]

The motivation for this patch is that StopInfo::GetSuggestedStackFrameIndex would not take effect for InstrumentationRuntimeStopInfo (which we plan to implement in #133079). This was happening because the instrumentation runtime plugins would run utility expressions as part of the stop that would set the m_selected_frame_idx. This means SelectMostRelevantFrame was never called, and we would not be able to report the selected frame via the StopInfo object.

This patch makes sure we clear the m_selected_frame_idx to allow GetSuggestedStackFrameIndex to take effect, regardless of what the frame recognizers choose to do.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

The motivation for this patch is that StopInfo::GetSuggestedStackFrameIndex would not take effect for InstrumentationRuntimeStopInfo (which we plan to implement in #133079). This was happening because the instrumentation runtime plugins would run utility expressions as part of the stop that would set the m_selected_frame_idx. This means SelectMostRelevantFrame was never called, and we would not be able to report the selected frame via the StopInfo object.

This patch makes sure we clear the m_selected_frame_idx to allow GetSuggestedStackFrameIndex to take effect, regardless of what the frame recognizers choose to do.

---

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

4 Files Affected:

• (modified) lldb/include/lldb/Target/StackFrameList.h (+3)
• (modified) lldb/include/lldb/Target/Thread.h (+5)
• (modified) lldb/source/Target/Process.cpp (+2)
• (modified) lldb/source/Target/StackFrameList.cpp (+4)

diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index 8a66296346f2d..d805b644b0b31 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -46,6 +46,9 @@ class StackFrameList {
   /// Mark a stack frame as the currently selected frame and return its index.
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
+  /// Resets the selected frame index of this object.
+  void ClearSelectedFrameIndex();
+
   /// Get the currently selected frame index.
   /// We should only call SelectMostRelevantFrame if (a) the user hasn't already
   /// selected a frame, and (b) if this really is a user facing
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 1d1e3dcfc1dc6..9eb9cda983c4a 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -480,6 +480,11 @@ class Thread : public std::enable_shared_from_this<Thread>,
   bool SetSelectedFrameByIndexNoisily(uint32_t frame_idx,
                                       Stream &output_stream);
 
+  /// Resets the selected frame index of this object.
+  void ClearSelectedFrameIndex() {
+    return GetStackFrameList()->ClearSelectedFrameIndex();
+  }
+
   void SetDefaultFileAndLineToSelectedFrame() {
     GetStackFrameList()->SetDefaultFileAndLineToSelectedFrame();
   }
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f2f5598f0ab53..6843d5b220742 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4257,6 +4257,8 @@ bool Process::ProcessEventData::ShouldStop(Event *event_ptr,
         // appropriately. We also need to stop processing actions, since they
         // aren't expecting the target to be running.
 
+        thread_sp->ClearSelectedFrameIndex();
+
         // FIXME: we might have run.
         if (stop_info_sp->HasTargetRunSinceMe()) {
           SetRestarted(true);
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 9c6208e9e0a65..3592c3c03db74 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -936,3 +936,7 @@ size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame,
   strm.IndentLess();
   return num_frames_displayed;
 }
+
+void StackFrameList::ClearSelectedFrameIndex() {
+  m_selected_frame_idx.reset();
+}

[github-actions]

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

[jimingham]

In StackFrameList::Clear we take the m_list_mutex before clearing the selected frame (among other things). Should we also do that here?

[Michael137]

hmmmm good question. GetSelectedFrameIndex is not taking the mutex either. On first glance I would've thought the mutex is there to just protect the m_frames. There's a separate m_inlined_depth_mutex that protects the corresponding member (for performance reason based on the comment next to it).

But if ClearSelectedFrameIndex and GetSelectedFrameIndex can be called concurrently (which is probably the case) they should be synchronized. Do we have concerns with using the list mutex for that?

[Michael137]

GetSelectedFrameIndex modifies m_selected_frame_idx. So concurrent access would already be an issue there. Maybe we're just getting away with it now (because it just doesn't happen much in practice)?

[jimingham]

This is a shared mutex so it can't be recursive, so we'd need to watch out for that.

[Michael137]

Hmm we also write to m_selected_frame_idx in StackFrameList::SetSelectedFrame, which holds a shared_lock. I think after the refactor to make the lock non-recursive, we lost some of protection against concurrent modifications to m_selected_frame_idx. Probably because it wasn't ever clear what m_list_mutex is supposed to protect (the entire class or just the frames collection).

And we end up calling GetSelectedFrameIndex from SetSelectedFrame, so we would need to recursively lock. I could work around this by introducing a separate mutex for m_selected_frame_idx, which I'm not a big fan of either tbh because that feels like a a step towards deadlocks. Wdyt?

[jimingham]

lldb really only should have an opinion about the selected frame index when a process stops, before control gets handed back to the user. After that, it's up to them to change it whenever they feel like it. So maybe we don't need to be as protective of this ivar?

[jimingham]

I'm not sure what we'd do if two parts of lldb decided they wanted to be in control of the selected frame index on stop. That seems like a bad idea, but I'm not sure the right way to fix it would be just to lock the selected frame index just at the point of setting it. There would need to be some explicit negotiation for who gets to control this.

[Michael137]

I see, yea it would be pretty unusual for two bits of LLDB to try to control this. Could ProcessEventData::ShouldStop be called concurrently and access the same StackFrameList somehow?

I added your explanation as to why we might not need synchronization to the member documentation.

[jimingham]

Other than the question about mutexes, this looks good to me.