[lldb] Fix ThreadPlanStepOut::DoPlanExplainsStop inspection of BreakpointSite (llvm#169799)

Suppose two threads are performing the exact same step out plan. They
will both have an internal breakpoint set at their parent frame. Now
supposed both of those breakpoints are in the same address (i.e. the
same BreakpointSite).

At the end of `ThreadPlanStepOut::DoPlanExplainsStop`, we see this:

```
// If there was only one owner, then we're done.  But if we also hit
// some user breakpoint on our way out, we should mark ourselves as
// done, but also not claim to explain the stop, since it is more
// important to report the user breakpoint than the step out
// completion.

if (site_sp->GetNumberOfConstituents() == 1)
  return true;
```

In other words, the plan looks at the name number of constituents of the
site to decide whether it explains the stop, the logic being that a
_user_ might have put a breakpoint there. However, the implementation is
not correct; in particular, it will fail in the situation described
above. We should only care about non-internal breakpoints that would
stop for the current thread.

It is tricky to test this, as it depends on the timing of threads, but I
was able to consistently reproduce the issue with a swift program using
concurrency.

rdar://165481473
(cherry picked from commit c0f0936)

Author: felipepiovezan

lldb/include/lldb/Breakpoint/BreakpointSite.h

@@ -156,6 +156,10 @@ class BreakpointSite : public std::enable_shared_from_this<BreakpointSite>,
   ///     would be valid for this thread, false otherwise.
   bool ValidForThisThread(Thread &thread);
 
+  /// Returns true if at least one constituent is both public and valid for
+  /// `thread`.
+  bool ContainsUserBreakpointForThread(Thread &thread);
+
   /// Print a description of this breakpoint site to the stream \a s.
   /// GetDescription tells you about the breakpoint site's constituents. Use
   /// BreakpointSite::Dump(Stream *) to get information about the breakpoint

lldb/source/Breakpoint/BreakpointSite.cpp

@@ -168,6 +168,22 @@ bool BreakpointSite::ValidForThisThread(Thread &thread) {
   return m_constituents.ValidForThisThread(thread);
 }
 
+bool BreakpointSite::ContainsUserBreakpointForThread(Thread &thread) {
+  if (ThreadSP backed_thread = thread.GetBackedThread())
+    return ContainsUserBreakpointForThread(*backed_thread);
+
+  std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
+  for (const BreakpointLocationSP &bp_loc :
+       m_constituents.BreakpointLocations()) {
+    const Breakpoint &bp = bp_loc->GetBreakpoint();
+    if (bp.IsInternal())
+      continue;
+    if (bp_loc->ValidForThisThread(thread))
+      return true;
+  }
+  return false;
+}
+
 void BreakpointSite::BumpHitCounts() {
   std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
   for (BreakpointLocationSP loc_sp : m_constituents.BreakpointLocations()) {

lldb/source/Target/ThreadPlanStepOut.cpp

@@ -390,13 +390,10 @@ bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) {
           }
         }
 
-        // If there was only one owner, then we're done.  But if we also hit
-        // some user breakpoint on our way out, we should mark ourselves as
-        // done, but also not claim to explain the stop, since it is more
-        // important to report the user breakpoint than the step out
-        // completion.
-
-        if (site_sp->GetNumberOfConstituents() == 1)
+        // If the thread also hit a user breakpoint on its way out, the plan is
+        // done but should not claim to explain the stop. It is more important
+        // to report the user breakpoint than the step out completion.
+        if (!site_sp->ContainsUserBreakpointForThread(GetThread()))
           return true;
       }
       return false;