Skip to content

Commit

Permalink
[lldb] Improve isolation between Process plugins and OS plugins (llvm…
Browse files Browse the repository at this point in the history
…#125302)

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->GetPrivateStopInfo());
  if (backing_stop_info_sp &&
      backing_stop_info_sp->IsValidForOperatingSystemThread(*this)) {
    backing_stop_info_sp->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.
  • Loading branch information
felipepiovezan authored Feb 3, 2025
1 parent 1abe7e8 commit 79e804b
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 4 deletions.
3 changes: 3 additions & 0 deletions lldb/source/Breakpoint/BreakpointSite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down
4 changes: 0 additions & 4 deletions lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 = thread_sp->GetBackedThread())
thread_sp = memory_thread_sp;

if (exc_type != 0) {
// For thread plan async interrupt, creating stop info on the
Expand Down
21 changes: 21 additions & 0 deletions lldb/unittests/OperatingSystem/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
add_lldb_unittest(ProcessGdbRemoteTests
GDBRemoteClientBaseTest.cpp
GDBRemoteCommunicationClientTest.cpp
GDBRemoteCommunicationServerLLGSTest.cpp
GDBRemoteCommunicationServerTest.cpp
GDBRemoteCommunicationTest.cpp
GDBRemoteTestUtils.cpp

LINK_LIBS
LLVMTestingSupport
lldbCore
lldbHost
lldbInterpreter
lldbPluginProcessUtility
lldbSymbol
lldbTarget
lldbValueObject

LINK_COMPONENTS
Support
)
59 changes: 59 additions & 0 deletions lldb/unittests/OperatingSystem/OperatingSystemPlugin.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
//===-- OperatingSystemPlugin.h ---------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "lldb/Core/PluginManager.h"
#include "lldb/Target/OperatingSystem.h"
#include "lldb/Target/Thread.h"
#include "lldb/Target/ThreadList.h"

/// An operating system plugin that does nothing: simply keeps the thread lists
/// as they are.
class OperatingSystemIdentityMap : public lldb_private::OperatingSystem {
public:
OperatingSystemIdentityMap(lldb_private::Process *process)
: OperatingSystem(process) {}

static OperatingSystem *CreateInstance(lldb_private::Process *process,
bool force) {
return new OperatingSystemIdentityMap(process);
}
static llvm::StringRef GetPluginNameStatic() { return "identity map"; }
static llvm::StringRef GetPluginDescriptionStatic() { return ""; }

static void Initialize() {
lldb_private::PluginManager::RegisterPlugin(GetPluginNameStatic(),
GetPluginDescriptionStatic(),
CreateInstance, nullptr);
}
static void Terminate() {
lldb_private::PluginManager::UnregisterPlugin(CreateInstance);
}
llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }

// Simply adds the threads from real_thread_list into new_thread_list.
bool UpdateThreadList(lldb_private::ThreadList &old_thread_list,
lldb_private::ThreadList &real_thread_list,
lldb_private::ThreadList &new_thread_list) override {
for (const auto &real_thread : real_thread_list.Threads())
new_thread_list.AddThread(real_thread);
return true;
}

void ThreadWasSelected(lldb_private::Thread *thread) override {}

lldb::RegisterContextSP
CreateRegisterContextForThread(lldb_private::Thread *thread,
lldb::addr_t reg_data_addr) override {
return thread->GetRegisterContext();
}

lldb::StopInfoSP
CreateThreadStopReason(lldb_private::Thread *thread) override {
return thread->GetStopInfo();
}
};
10 changes: 10 additions & 0 deletions lldb/unittests/OperatingSystem/TestThreadSpecificBreakpoints.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//===-- OperatingSystemPlugin.h ---------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "OperatingSystemPlugin.h"
LLDB_PLUGIN_DEFINE(OperatingSystemIdentityMap)

0 comments on commit 79e804b

Please sign in to comment.