[cherry-pick] Fixes for StopReason propagation in the presence of OS plugins #9945
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ationship (llvm#125300) This enables finding the backed thread from the backing thread without going through the thread list, and it will be useful for subsequent commits. (cherry picked from commit 90a51a4)
…#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. (cherry picked from commit 79e804b)
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a cherry-pick of two commits from upstream LLDB:
llvm#125300
llvm#125302