Commit b82454c
authored
[Release/10.0] Pass down DJI and MethodDesc to avoid unnecessary PtrHashMap lookup (#126247)
Fixes #126096
main fixes this issue via #123492
- a different approach that's too risky for servicing.
#123651 had tried a targeted
approach and this completes the targeted fix for the path.
# Description
PR #123651 fixed the `GetJitInfo` call in `BindPatch` to avoid a
deadlock-prone `HashMap::LookupValue` -> `GCCoopHackNoThread` ->
`DisablePreemptiveGC` path while holding the DebuggerController lock.
However, the same function has a second call to
`CodeRegionInfo::GetCodeRegionInfo(NULL, NULL, startAddr)` that follows
the exact same deadlock path when `dji` is NULL, `GetCodeRegionInfo`
calls `InitializeFromStartAddress` -> `GetMethodRegionInfo` ->
`EECodeInfo` constructor -> `ReadyToRunJitManager::JitCodeToMethodInfo`
-> `HashMap::LookupValue` -> GC cooperative mode transition.
This one-line fix passes the already-available `info` (DebuggerJitInfo)
and `pMD` (MethodDesc) to `GetCodeRegionInfo`, matching every other
callsite in controller.cpp. When `info` is non-null and has a cached
`m_addrOfCode`, `GetCodeRegionInfo` returns the cached
`m_codeRegionInfo` immediately without constructing an `EECodeInfo`,
avoiding the HashMap and the GC mode transition entirely. The change in
#126249 tries to find other
locations that can face this issue. So far, they are all around the
thread starter added in r2r delay call transitions. This change fixes
most of those cases.
# Customer Impact
Without this fix, customers debugging ReadyToRun applications can
experience debugger hangs. The deadlock occurs when:
1. A thread holds the DebuggerController lock in `BindPatch` (e.g.,
during `MapAndBindFunctionPatches` from `JITComplete` or
`AddBindAndActivateILReplicaPatch`)
2. `GetCodeRegionInfo` constructs an `EECodeInfo` for a R2R method,
entering `HashMap::LookupValue` which transitions to GC cooperative mode
3. A GC or debugger suspension is pending `DisablePreemptiveGC` ->
`RareDisablePreemptiveGC` -> `WaitSuspendEvents` blocks indefinitely
4. Other threads waiting on the DebuggerController lock (or the GC
needing threads at safe points) deadlock
The result is the debugger and IDE hang, requiring force-close and loss
of the debugging session.
# Regression
No. The root cause has existed since ReadyToRun support was added. The
original issue (#123650) notes that timing changes may have made it more
likely to hit recently, but this specific `GetCodeRegionInfo` path has
always been vulnerable. The bug has become a lot more visible in 10.0
nonetheless.
# Testing
- The fix is a single-line change that passes already-available local
variables (`info`, `pMD`) to `GetCodeRegionInfo` instead of `(NULL,
NULL)`, matching all other callsites in controller.cpp (see lines ~6173,
~6219, ~8054 for the same pattern).
- Debugger tests pass with this change.
# Risk
**Very Low.** This is a one-line change that passes two existing local
variables to a function instead of NULL. The `info` and `pMD` variables
are already computed earlier in the same function and used in the lines
immediately surrounding this call. Every other `GetCodeRegionInfo`
callsite in controller.cpp already passes the DJI and MethodDesc. The
only effect is avoiding an unnecessary `EECodeInfo` construction (and
its HashMap lookup) when cached data is available.1 parent a13f1e4 commit b82454c
1 file changed
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1455 | 1455 | | |
1456 | 1456 | | |
1457 | 1457 | | |
1458 | | - | |
| 1458 | + | |
1459 | 1459 | | |
1460 | 1460 | | |
1461 | 1461 | | |
| |||
0 commit comments