[Lldb-commits] [lldb] [lldb] Improve isolation between Process plugins and OS plugins (PR #125302)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

<details>
<summary>Changes</summary>

Generally speaking, process plugins (e.g. ProcessGDBRemote) should not be aware 
of OS plugin threads. However, ProcessGDBRemote attempts to check for the 
existence of OS threads when calculating stop info. When OS threads are 
present, it sets the stop info directly on the OS plugin thread and leaves the 
ThreadGDBRemote without a StopInfo.

This is problematic for a few reasons:

1. No other process plugins do this, as they shouldn't. They should set the 
stop info for their own process threads, and let the abstractions built on top 
propagate StopInfos.

2. This conflicts with the expectations of ThreadMemory, which checks for the 
backing threads's info, and then attempts to propagate it (in the future, it 
should probably ask the plugin itself too...). We see this happening in the 
code below. The `if` condition will not trigger, because `backing_stop_info_sp` 
will be null (remember, ProcessGDB remote is ignoring its own threads), and 
then this method returns false.

```
bool ThreadMemory::CalculateStopInfo() {
...
  lldb::StopInfoSP backing_stop_info_sp(
      m_backing_thread_sp-&gt;GetPrivateStopInfo());
  if (backing_stop_info_sp &amp;&amp;
      backing_stop_info_sp-&gt;IsValidForOperatingSystemThread(*this)) {
    backing_stop_info_sp-&gt;SetThread(shared_from_this());
```

```
Thread::GetPrivateStopInfo
...
        if (!CalculateStopInfo())
          SetStopInfo(StopInfoSP());
```

To solve this, we change ProcessGDB remote so that it does the principled 
thing: it now only sets the stop info of its own threads. This change by itself 
breaks the tests TestPythonOSPlugin.py and TestOSPluginStepping.py and probably 
explains why ProcessGDB had originally "violated" this isolation of layers.

To make this work, BreakpointSites must be aware of BackingThreads when 
answering the question: "Is this breakpoint valid for this thread?". Why? 
Breakpoints are created on top of the OS threads (that's what the user sees), 
but breakpoints are hit by process threads. In the presence of OS threads, a 
TID-specific breakpoint is valid for a process thread if it is backing an OS 
thread with that TID.

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


2 Files Affected:

- (modified) lldb/source/Breakpoint/BreakpointSite.cpp (+3) 
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (-4) 


``````````diff
diff --git a/lldb/source/Breakpoint/BreakpointSite.cpp 
b/lldb/source/Breakpoint/BreakpointSite.cpp
index 9700a57d3346e0..8b964c5711468c 100644
--- a/lldb/source/Breakpoint/BreakpointSite.cpp
+++ b/lldb/source/Breakpoint/BreakpointSite.cpp
@@ -12,6 +12,7 @@
 
 #include "lldb/Breakpoint/Breakpoint.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
+#include "lldb/Target/Thread.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
@@ -161,6 +162,8 @@ BreakpointLocationSP 
BreakpointSite::GetConstituentAtIndex(size_t index) {
 
 bool BreakpointSite::ValidForThisThread(Thread &thread) {
   std::lock_guard<std::recursive_mutex> guard(m_constituents_mutex);
+  if (ThreadSP backed_thread = thread.GetBackedThread())
+    return m_constituents.ValidForThisThread(*backed_thread);
   return m_constituents.ValidForThisThread(thread);
 }
 
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp 
b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 21a0fa283644d6..07b4470d061918 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1726,10 +1726,6 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
 
   if (!thread_sp->StopInfoIsUpToDate()) {
     thread_sp->SetStopInfo(StopInfoSP());
-    // If there's a memory thread backed by this thread, we need to use it to
-    // calculate StopInfo.
-    if (ThreadSP memory_thread_sp = m_thread_list.GetBackingThread(thread_sp))
-      thread_sp = memory_thread_sp;
 
     if (exc_type != 0) {
       // For thread plan async interrupt, creating stop info on the

``````````

</details>


https://github.com/llvm/llvm-project/pull/125302
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits