[lldb] Fix ThreadPlanStepOut::DoPlanExplainsStop inspection of BreakpointSite

[felipepiovezan]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

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.

---

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

1 Files Affected:

• (modified) lldb/source/Target/ThreadPlanStepOut.cpp (+20-7)

diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index d49a01bdbcef7..7970e731aa972 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Target/ThreadPlanStepOut.h"
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
@@ -306,6 +307,21 @@ bool ThreadPlanStepOut::ValidatePlan(Stream *error) {
   return true;
 }
 
+/// Returns true if :
+/// 1. All breakpoints in this site are internal, and
+/// 2. All breakpoint locations in this site are NOT valid for `thread`.
+static bool NoUserBreakpointsHere(BreakpointSite &site, Thread &thread) {
+  for (unsigned bp_idx = 0; bp_idx < site.GetNumberOfConstituents(); bp_idx++) {
+    BreakpointLocation &bp_loc = *site.GetConstituentAtIndex(bp_idx);
+    const Breakpoint &bp = bp_loc.GetBreakpoint();
+    if (bp.IsInternal())
+      continue;
+    if (bp_loc.ValidForThisThread(thread))
+      return false;
+  }
+  return true;
+}
+
 bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) {
   // If the step out plan is done, then we just need to step through the
   // inlined frame.
@@ -356,13 +372,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 (NoUserBreakpointsHere(*site_sp, GetThread()))
           return true;
       }
       return false;