[lldb] Remove unfiltered stop reason propagation from StopInfoMachException

[felipepiovezan]

In the presence of OS plugins, StopInfoMachException currently propagates breakpoint stop reasons even if those breakpoints were not intended for a specific thread, effectively removing our ability to set thread-specific breakpoints.

This was originally added in 1, but the motivation provided in the comment does not seem strong enough to remove the ability to set thread-specific breakpoints. The only way to break thread specific breakpoints would be if a user set such a breakpoint and then loaded an OS plugin, a scenario which we would likely not want to support.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

In the presence of OS plugins, StopInfoMachException currently propagates breakpoint stop reasons even if those breakpoints were not intended for a specific thread, effectively removing our ability to set thread-specific breakpoints.

This was originally added in 1, but the motivation provided in the comment does not seem strong enough to remove the ability to set thread-specific breakpoints. The only way to break thread specific breakpoints would be if a user set such a breakpoint and then loaded an OS plugin, a scenario which we would likely not want to support.

---

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

3 Files Affected:

• (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+2-7)
• (modified) lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py (+13)
• (modified) lldb/test/API/functionalities/plugins/python_os_plugin/main.c (+2)

diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index d08e99f4844de3..698ba0f0f720f6 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -780,13 +780,8 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
         // but if it is for another thread, we can just report no reason.  We
         // don't need to worry about stepping over the breakpoint here, that
         // will be taken care of when the thread resumes and notices that
-        // there's a breakpoint under the pc. If we have an operating system
-        // plug-in, we might have set a thread specific breakpoint using the
-        // operating system thread ID, so we can't make any assumptions about
-        // the thread ID so we must always report the breakpoint regardless
-        // of the thread.
-        if (bp_site_sp->ValidForThisThread(thread) ||
-            thread.GetProcess()->GetOperatingSystem() != nullptr)
+        // there's a breakpoint under the pc.
+        if (bp_site_sp->ValidForThisThread(thread))
           return StopInfo::CreateStopReasonWithBreakpointSiteID(
               thread, bp_site_sp->GetID());
         else if (is_trace_if_actual_breakpoint_missing)
diff --git a/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py b/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
index 479c94c2315407..4b8471d1cc32be 100644
--- a/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
+++ b/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
@@ -219,3 +219,16 @@ def run_python_os_step(self):
             6,
             "Make sure we stepped from line 5 to line 6 in main.c",
         )
+
+        thread_bp_number = lldbutil.run_break_set_by_source_regexp(
+            self, "Set tid-specific breakpoint here", num_expected_locations=1
+        )
+        breakpoint = target.FindBreakpointByID(thread_bp_number)
+        # This breakpoint should not be hit.
+        breakpoint.SetThreadID(123)
+        process.Continue()
+
+        frame = thread.GetFrameAtIndex(0)
+        self.assertFalse(
+            frame.IsValid(), "Should not stop, the breakpoint was not for this thread."
+        )
diff --git a/lldb/test/API/functionalities/plugins/python_os_plugin/main.c b/lldb/test/API/functionalities/plugins/python_os_plugin/main.c
index faa6dd58ecd62b..248ecc8f650d2a 100644
--- a/lldb/test/API/functionalities/plugins/python_os_plugin/main.c
+++ b/lldb/test/API/functionalities/plugins/python_os_plugin/main.c
@@ -3,5 +3,7 @@
 int main (int argc, char const *argv[], char const *envp[])
 {
     puts("stop here"); // Set breakpoint here
+    puts("hello");
+    puts("don't stop here on tid-specific breakpoint"); // Set tid-specific breakpoint here
     return 0;
 }

[github-actions]

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

[felipepiovezan]

clang-format on the test is actually problematic for the breakpoint regex, so I am just going to ignore it.

[jasonmolenda]

clang-format on the test is actually problematic for the breakpoint regex, so I am just going to ignore it.

you can do // clang-format off / // clang-format on around it, if you want to bother.

[jasonmolenda]

clang-format on the test is actually problematic for the breakpoint regex, so I am just going to ignore it.

you can do // clang-format off / // clang-format on around it, if you want to bother.

(you could also just set the regex breakpoint on the string you're printing, instead of bothering with the comment)

[felipepiovezan]

clang-format on the test is actually problematic for the breakpoint regex, so I am just going to ignore it.

you can do // clang-format off / // clang-format on around it, if you want to bother.

(you could also just set the regex breakpoint on the string you're printing, instead of bothering with the comment)

Ohh yeah, this is much better actually!

[jimingham]

LGTM. Adding an OS plugin is a user gesture, and changes the view of the threads in the process as its primary action. So I agree it's reasonable that the user who adds the OS plugin would know they have to reset their thread specific breakpoints to handle the new view.