Uh oh!
There was an error while loading. Please reload this page.
[clr-interp] Fix EnC failure when adding a generic method to a generic type - #127755
[clr-interp] Fix EnC failure when adding a generic method to a generic type#127755kotlarmilos wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR EnC method addition for generic types so EEClass::AddMethod no longer holds m_AvailableTypesLock while creating MethodDescs for existing generic instantiations. In the VM, this avoids same-thread lock re-entry during generic-method setup on the debugger/EnC path.
Changes:
- Snapshot matching generic instantiations from
GetAvailableParamTypes()while holdingAvailableTypesLock. - Release the lock before calling
AddMethodDescfor each collected instantiation. - Preserve the existing per-instantiation method/async-variant creation flow after the lock is released.
Show a summary per file
| File | Description |
|---|---|
src/coreclr/vm/class.cpp | Changes EEClass::AddMethod to gather matching instantiated MethodTable* entries under the available-types lock, then add new MethodDescs after releasing that lock. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
jkotas
commented
May 4, 2026
I do not see a method with this name. What is actual name of this method? What is the stacktrace that leads to this point? |
| instantiations.Append(pMTMaybe); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
What guarantees that there won't be more instantiations added in the meantime by other threads?
There was a problem hiding this comment.
Good question. The original loop held m_AvailableTypesLock across the whole iteration, which prevented other threads from publishing new instantiations of the same generic type into m_AvailableParamTypes while we were attaching the new method.
I'll update the PR to keep the lock continuously.
There was a problem hiding this comment.
Actually, I updated SetupGenericMethodDefinition skip taking the lock when the current thread already owns it. The lock protects Module.m_GenericParamToDescMap, so a caller that already holds it is in the same protected region.
There was a problem hiding this comment.
Found better fix - mark m_AvailableTypesLock reentrant which matches m_AvailableClassLock, instead of conditionally skipping the inner acquire.
There was a problem hiding this comment.
Copilot pointed out that allowing reentrancy only fixes the self-recursive case and would still trip the lock-level violation when an instantiation lives in a different loader module, so I reverted to collecting the matching instantiations under the lock and releasing it before calling AddMethodDesc.
There is no guarantee that other threads won't insert new instantiations after we release, but they don't need to be in the collected set. We already added the new MethodDesc to the generic type definition itself before the loop starts, so any instantiation built after that point automatically includes the new method through the normal MethodTableBuilder::Build path.
There was a problem hiding this comment.
We already added the new MethodDesc to the generic type definition itself before the loop starts, so any instantiation built after that point automatically includes the new method
There is still a race condition: Some other thread could have started building an instantiation before the metadata edit and it may finish building it only after all this is done. This instantiation would be missing the newly added method.
There was a problem hiding this comment.
Hmm, this is actually a pre-existing bug. This change may make it more likely to be hit (the window where the race condition can happen is larger).
Discussion at #125397 (comment) is related to this. Type loader is designed to create new entities lazily. We try to create MethodDescs eagerly here. It turns out it is hard to do that 100% reliably. The correct fix would be to switch to creating these MethodDesc lazily, but that is likely to come with a bug tail.
The proposed fix should not make it worse.
There was a problem hiding this comment.
Thanks, since it isn't interpreter-specific, I created a tracking issue to decide on approach #127851
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
ad45def to
3999b1eComparekotlarmilos
commented
May 5, 2026
Sorry I put the wrong method name. The actual call is InstantiatedMethodDesc::SetupGenericMethodDefinition Test log: Stack trace: |
3999b1e to
d53448fCompareUh oh!
There was an error while loading. Please reload this page.
Adding a generic method to an existing generic type during Edit and Continue currently fails. EEClass::AddMethod walks every existing instantiation of the type while holding the owning module's CrstAvailableParamTypes, and inside the loop calls AddMethodDesc. For a generic method that path reaches InstantiatedMethodDesc::SetupGenericMethodDefinition which itself takes CrstAvailableParamTypes (potentially on a different module's classloader if the instantiation lives there). Taking the same-level lock again trips the Crst lock-level assert in checked builds and would deadlock in release. Split the walk into two phases. First, under the lock, iterate the type hash and collect the matching MethodTable* instantiations into a local array. Then release the lock and call AddMethodDesc on each collected instantiation. The hash iteration is still protected, and the per-instantiation method setup runs without holding the lock so SetupGenericMethodDefinition can acquire whatever CrstAvailableParamTypes instance it needs without violating lock ordering. This handles both the same-module and cross-loader-module cases. Tests: - CrossPlatformEnC.AddGenericInstanceMethodOnGenericType - CrossPlatformEnC.AddGenericStaticMethodOnGenericType - CrossPlatformEnC.GenericMethodsWithExpressions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d53448f to
90ffacbCompareUh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
| // This code contains a race condition bug. Types that began loading before the metadata edit | ||
| // and are still loading at this point may not get updated. We assume that this issue does not | ||
| // have a meaningful impact on overall metadata update reliability. |
| instantiations.Append(pMTMaybe); | ||
| } | ||
| } | ||
| for (COUNT_T i = 0; i < instantiations.GetCount(); i++) |
| InlineSArray<MethodTable*, 8> instantiations; | ||
| { | ||
| TypeHandle th = pEntry->GetTypeHandle(); | ||
| if (th.IsTypeDesc()) | ||
| continue; | ||
| EETypeHashTable* paramTypes = pMod->GetAvailableParamTypes(); | ||
| CrstHolder ch(pMod->GetClassLoader()->GetAvailableTypesLock()); | ||
kotlarmilos
commented
May 6, 2026
The failures are not interpreter-specific and the work will be tracked in #127851 |
Description
Adding a generic method to an existing generic type during Edit and Continue currently fails.
EEClass::AddMethodtakesm_AvailableTypesLockand then calls into code that needs the same lock to publish the new method's generic parameters.The other lock that protects type-loader tables,
m_AvailableClassLock, is already configured to allow this kind of nested take. Configurem_AvailableTypesLockthe same way. The other places that use this lock take and release it as a single unit, so they are unaffected.Tests: