[ios-clr] Deduplicate CreateThread event when a thread runs both R2R and interpreted code#130127
[ios-clr] Deduplicate CreateThread event when a thread runs both R2R and interpreted code#130127kotlarmilos wants to merge 2 commits into
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent duplicate managed debugger CreateThread / thread-attach announcements when a thread executes both ReadyToRun (R2R) and interpreted code, by introducing a per-thread “already sent” flag and short-circuiting the redundant path. It also includes an additional (seemingly separate) change that updates the ReadyToRun stripped-IL sentinel and adds a runtime fail-fast with a clearer message when that sentinel is encountered.
Changes:
- Add
Thread::TSNC_DebuggerThreadStartSentand use it to deduplicateDebugger::ThreadStartedand avoid redundant interpreter-side synchronization. - Change the stripped-IL sentinel body emitted by Crossgen2 from
ldnull; throwto an invalid opcode (0xFE 0x24) and update tests accordingly. - Detect the stripped-IL sentinel at runtime and fail-fast with a specific diagnostic message instead of crashing later with less actionable failures.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/threads.h | Introduces a new ThreadStateNoConcurrency bit to record whether the debugger thread-start event was already sent. |
| src/coreclr/debug/ee/debugger.cpp | Uses the new per-thread bit to prevent duplicate thread-attach IPC events and to skip redundant interpreter entry work. |
| src/coreclr/vm/prestub.cpp | Adds runtime detection/fail-fast for the stripped-IL sentinel body when IL is unexpectedly required. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/CopiedMethodILNode.cs | Changes the Crossgen2 stripped-IL sentinel body to an invalid opcode sequence. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RResultChecker.cs | Updates test logic/messages to match the new stripped-IL sentinel sequence. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
…h R2R and interpreted code PR dotnet#129389 added a second debugger CreateThread announcement for interpreted threads, because the trace-call mechanism that normally announces a thread never fires for purely interpreted code. When corelib is compiled as R2R, for example in Debug on Apple mobile, both announcements happen for the same thread. The precompiled corelib code announces the thread once during startup, and the interpreter announces it again when it reaches interpreted user code. This produces a duplicate thread-attach event and an extra debuggee pause. This change makes the announcement happen at most once per thread using a new per-thread flag named TSNC_DebuggerThreadStartSent. ThreadStarted checks and sets the flag under the ThreadStore lock, so whichever path arrives first sends the event and the second one does nothing. SendCreateThreadAtInterpreterEntry also checks the flag before it sets up the send-event context, which lets the redundant interpreter announcement skip the unnecessary synchronization pause. The flag is only written from ThreadStarted, which always runs on the thread being announced, so it follows the existing single-writer rule for these flags. Reusing the free 0x00000001 slot is safe because no DAC or cDAC contract depends on these flag values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
71e59f0 to
7a34f60
Compare
kotlarmilos
changed the title
|
LGTM |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "ec5bcd7dfc9dda2ba6263e54d73e97ba65b9a3ce",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "57fc9dc8acd841ecffb733faf8def2cd96fadfd0",
"last_reviewed_commit": "ec5bcd7dfc9dda2ba6263e54d73e97ba65b9a3ce",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "57fc9dc8acd841ecffb733faf8def2cd96fadfd0",
"last_recorded_worker_run_id": "29680702935",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec5bcd7dfc9dda2ba6263e54d73e97ba65b9a3ce",
"review_id": 4730522898
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: PR #129389 added a second debugger CreateThread announcement for interpreted threads because the trace-call mechanism that normally announces a thread never fires for purely interpreted code. When corelib is R2R-compiled (e.g. Debug on Apple mobile), a thread that runs both precompiled and interpreted code is announced twice: once at startup via the DebuggerThreadStarter patch and again when the interpreter reaches user code. This produces a duplicate DB_IPCE_THREAD_ATTACH event and an extra debuggee pause. The motivation is clear and legitimate.
Approach: A previously-unused Thread non-cacheable state bit is repurposed as TSNC_DebuggerThreadStartSent (0x00000001). Debugger::ThreadStarted now returns early if the bit is set, and otherwise sets it before sending the attach event; Debugger::SendCreateThreadAtInterpreterEntry checks the same bit up front so the redundant interpreter announcement also skips the synchronization pause (PollWaitingForHelper / SENDIPCEVENT_BEGIN). The approach is minimal and idiomatic for this file.
Summary: The change is correct and low-risk. Both paths that call ThreadStarted execute on the thread being announced: the DebuggerThreadStarter::SendEvent patch path (controller.cpp) and SendCreateThreadAtInterpreterEntry (invoked from Thread::GetOrCreateInterpThreadContext on this). The bit is written only inside ThreadStarted, which asserts ThreadHoldsLock(), so the read-then-set is serialized under the ThreadStore lock and respects the existing single-writer convention for TSNC flags. The early check in SendCreateThreadAtInterpreterEntry is an unsynchronized read, but a benign one: the only cost of a stale false read is falling through to ThreadStarted, which re-checks authoritatively under the lock. Repurposing the comment-marked unused 0x00000001 bit is appropriate and the XML-free comment accurately documents its meaning.
No actionable issues were found. One non-blocking observation for the author's awareness: the flag is never cleared, so it persists for the lifetime of the Thread. This is correct for the debugger-attached lifetime, and CoreCLR reattach re-enumerates existing threads through DAC rather than replaying ThreadStarted, so a persistently-set bit does not suppress a needed announcement on reattach. No change is required.
Verdict: LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 61.4 AIC · ⌖ 10.5 AIC · ⊞ 10K
Description
PR #129389 added a second debugger CreateThread announcement for interpreted threads, because the trace-call mechanism that normally announces a thread never fires for purely interpreted code. When corelib is compiled as R2R, for example in Debug on Apple mobile, both announcements happen for the same thread. The precompiled corelib code announces the thread once during startup, and the interpreter announces it again when it reaches interpreted user code. This produces a duplicate thread-attach event and an extra debuggee pause. This change makes the announcement happen at most once per thread using a new per-thread flag named TSNC_DebuggerThreadStartSent.
ThreadStarted checks and sets the flag under the ThreadStore lock, so whichever path arrives first sends the event and the second one does nothing. SendCreateThreadAtInterpreterEntry also checks the flag before it sets up the send-event context, which lets the redundant interpreter announcement skip the unnecessary synchronization pause. The flag is only written from ThreadStarted, which always runs on the thread being announced, so it follows the existing single-writer rule for these flags.