[iOS] Avoid debugger patches in R2R code without dynamic code support - #133577
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa691cda-51f9-484c-a3a4-c6c9fa45a7c2
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa691cda-51f9-484c-a3a4-c6c9fa45a7c2
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
Debugger::MapPatchToDJI can misreport recursive-bind success as a bind failure when BindPatch returns false after recursively activating the patch, potentially causing incorrect breakpoint-set error behavior.
Pull request overview
Prevents the CoreCLR debugger from attempting to apply software-breakpoint patches to ReadyToRun (R2R) code when dynamic code compilation is disabled (notably relevant to iOS/device scenarios where page permission changes can invalidate code signatures).
Changes:
- Add
FEATURE_DYNAMIC_CODE_COMPILED-guarded checks to avoid binding/activating patches into R2R code paths and to cancel an outstanding thread-starter when interpreter entry sends the thread-start event directly. - Change
DebuggerController::ActivatePatchto returnbooland propagate activation failure to callers; remove/unbind patches that cannot be activated. - Make
PatchTraceand related helpers return success/failure based on whether a patch was actually installed.
File summaries
| File | Description |
|---|---|
| src/coreclr/debug/ee/debugger.cpp | Skips mapping deferred patches for R2R code when dynamic code is disabled; propagates activation failures; cancels outstanding thread-starter at interpreter entry. |
| src/coreclr/debug/ee/controller.h | Updates patching APIs to return status (ActivatePatch, AddPatchToStartOfLatestMethod). |
| src/coreclr/debug/ee/controller.cpp | Implements ActivatePatch status return and rejects R2R patch activation when dynamic code is disabled; propagates patch-creation/activation failures through tracing and binding paths. |
Review details
Suppressed comments (1)
src/coreclr/debug/ee/debugger.cpp:4887
- MapPatchToDJI treats BindPatch returning false as an unconditional bind failure, but BindPatch can return false when it bound the patch recursively via GetJitInfo (see BindPatch's "patch bound recursively" path). In that case the patch may already be activated, and returning CORDBG_E_CODE_NOT_AVAILABLE will incorrectly report an unbindable patch and can lead to spurious BreakpointSetError events. Handle the recursive-bind case similarly to AddBindAndActivatePatchForMethodDesc by treating an already-activated patch as success.
if (DebuggerController::BindPatch(dcp, djiTo->m_nativeCodeVersion.GetMethodDesc(), NULL))
{
if (!DebuggerController::ActivatePatch(dcp))
{
DebuggerController::GetPatchTable()->UnbindPatch(dcp);
return CORDBG_E_CODE_NOT_AVAILABLE;
}
LOG((LF_CORDB, LL_INFO1000, "D::MPTDJI Binding went fine!\n" ));
return S_OK;
}
else
{
LOG((LF_CORDB, LL_INFO1000, "D::MPTDJI Binding failed for some reason!\n"));
// Caller can track this HR and send error.
return CORDBG_E_CODE_NOT_AVAILABLE;
}
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Patch activation can still report success after ApplyPatch fails.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The platform guards omit physical tvOS, which shares the signed-code restriction and no-dynamic-code configuration.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/coreclr/debug/ee/controller.cpp:2043
- Keep this debug-only exemption aligned with the R2R rejection policy for tvOS. Once
ActivatePatchrejects tvOS R2R patches, this remainsfalseand valid failures will trip the assertion below.
#if defined(TARGET_IOS) && !defined(FEATURE_DYNAMIC_CODE_COMPILED)
bool isReadyToRunPatchUnsupported =
ExecutionManager::IsReadyToRunCode(dac_cast<PCODE>(dji->m_addrOfCode));
#else
constexpr bool isReadyToRunPatchUnsupported = false;
#endif // TARGET_IOS && !FEATURE_DYNAMIC_CODE_COMPILED
src/coreclr/debug/ee/controller.cpp:2575
- The tvOS no-dynamic-code configuration also needs this preflight. Otherwise resolving debug information for a managed R2R trace can enter recursive patch binding on physical tvOS before the central activation check rejects it.
#if defined(TARGET_IOS) && !defined(FEATURE_DYNAMIC_CODE_COMPILED)
// Resolving debug information can recursively bind pending patches. Reject
// unpatchable R2R code before entering that path.
if (ExecutionManager::IsReadyToRunCode(trace->GetAddress()))
src/coreclr/debug/ee/debugger.cpp:8923
- The same pending
DebuggerThreadStarterleak can occur on tvOS after R2R patch activation is rejected: interpreter entry sends the event directly, but this guard leaves the tvOS starter alive. IncludeTARGET_TVOSwhen canceling it.
#if defined(TARGET_IOS) && !defined(FEATURE_DYNAMIC_CODE_COMPILED)
DebuggerController::CancelOutstandingThreadStarter(pRuntimeThread);
#endif // TARGET_IOS && !FEATURE_DYNAMIC_CODE_COMPILED
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
|
does it make sense to turn this on for wasm targets now too? |
We'll need this backported to .NET 11 branch for iOS so I would rather keep it small if possible. Is debugging of WASM supported in .NET 11? If so, we should test it with this change but I haven't tried it myself yet. We should then implement this #134050 for .NET 12 and extend the support to WASM definitely. Edit: I've removed the iOS ifdef so now it will apply to WASM also, but it is not tested at the moment. |
Wasm debugging is not a thing in .NET 11. You do not need to worry about it. |
0514536 to
4880faf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A newly retained patch pointer may become stale during recursive binding.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
/backport to release/11.0 |
|
Started backporting to |
…ic code support (#134139) Backport of #133577 to release/11.0 /cc @matouskozak ## Customer Impact - [ ] Customer reported - [x] Found internally The debugging on iOS physical devices doesn't work because debugger tries to put an internal breakpoint into the R2R images which triggers a code signing crash on iOS. This means that an iOS app cannot start in debug on physical devices. This PR prevents debugger attempts to put a patch into R2R code on platforms that are build with disabled dynamic code compilation. Currently, only iOS/tvOS are configurations which ship under CoreCLR with dynamic code compilation. ## Regression - [ ] Yes - [x] No ## Testing This was uncovered by CSI manual testing on physical iOS devices (https://devdiv.visualstudio.com/DevDiv/_workitems/edit/3027937). The fix was verified to work in both VS and VS Code debugger on physical iOS devices. ## Risk Low: The biggest change (blocking patches on R2R code) is covered by `#ifndef FEATURE_DYNAMIC_CODE_COMPILED` thus shouldn't affect any other configuration that is supported under debugger. The rest of changes is around propagation of results in case the `ActivatePatch` false. Note, this change can cause debugger issues on windows x64 if build without dynamic code compile as mentioned in #133577 (comment) but we don't ship that configuration anywhere currently. **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Co-authored-by: Matous Kozak <55735845+matouskozak@users.noreply.github.com>
…dotnet#133577) ## Description When CoreCLR runs an R2R + interpreter application on a physical iOS device, `DebuggerThreadStarter` can try to place a software breakpoint in signed R2R code. Making the page writable removes execute permission, and iOS does not allow the runtime to restore it. The process is then terminated with a code-signing invalid-page failure. This change prevents debugger patch activation in R2R code when `FEATURE_DYNAMIC_CODE_COMPILED` is disabled. It also: - propagates patch activation failure instead of reporting success; - removes or unbinds patches that cannot be activated; - keeps deferred R2R patches unbound; - cancels the pending `DebuggerThreadStarter` when interpreter entry sends the thread-start event directly; Breakpoints cannot be activated in R2R methods in this configuration. A direct breakpoint request fails with `CORDBG_E_UNABLE_TO_SET_BREAKPOINT`.
Description
When CoreCLR runs an R2R + interpreter application on a physical iOS device,
DebuggerThreadStartercan try to place a software breakpoint in signed R2R code. Making the page writable removes execute permission, and iOS does not allow the runtime to restore it. The process is then terminated with a code-signing invalid-page failure.This change prevents debugger patch activation in R2R code when
FEATURE_DYNAMIC_CODE_COMPILEDis disabled. It also:DebuggerThreadStarterwhen interpreter entry sends the thread-start event directly;Breakpoints cannot be activated in R2R methods in this configuration. A direct breakpoint request fails with
CORDBG_E_UNABLE_TO_SET_BREAKPOINT.NOTE: This will need to be backported to .NET 11 to fix debugging on iOS physical devices.
Note
This pull request description was created by GitHub Copilot.