[Async v2] Implement async method variant handling in AddMethod and UpdateMethod#125397
[Async v2] Implement async method variant handling in AddMethod and UpdateMethod#125397tommcdon wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR implements runtime-async method variant support in the Edit-and-Continue (EnC) path by extending EEClass::AddMethodDesc/EEClass::AddMethod to carry async metadata (flags + optional alternate signature) and by updating EnC method update logic to consider async method variants.
Changes:
- Extend
EEClass::AddMethodDescto accept async flags and an optional async-variant signature, and plumb that intoMethodTableBuilder::InitMethodDesc. - Add async return-type classification and async-variant creation logic to
EEClass::AddMethod. - Update
EditAndContinueModule::UpdateMethodto also reset the entrypoint for the method’s async counterpart.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/coreclr/vm/encee.cpp | Resets entrypoints for async counterparts during EnC method updates. |
| src/coreclr/vm/class.h | Extends AddMethodDesc signature to accept async flags and optional async signature. |
| src/coreclr/vm/class.cpp | Implements async return classification and passes async flags/signature into EnC-added MethodDesc creation. |
You can also share your feedback on Copilot code review. Take the survey.
| MethodDesc* pNewMDUnused; | ||
| if (FAILED(AddMethodDesc(pMTMaybe, methodDef, dwImplFlags, dwMemberAttrs, &pNewMDUnused))) | ||
| if (FAILED(AddMethodDesc(pMTMaybe, methodDef, dwImplFlags, dwMemberAttrs, | ||
| primaryAsyncFlags, NULL, 0, &pNewMDUnused))) |
There was a problem hiding this comment.
When the added method is Task/ValueTask-returning and the owning type is a generic type definition, the code updates existing instantiated MethodTables by calling AddMethodDesc only for the primary variant. If an async variant is required, it also needs to be added for each existing instantiation; otherwise GetAsyncOtherVariant / GetParallelMethodDesc(... AsyncOtherVariant) on those instantiations can fail to find the counterpart (since it enumerates introduced methods on that MethodTable).
| primaryAsyncFlags, NULL, 0, &pNewMDUnused))) | |
| 0, NULL, 0, &pNewMDUnused))) |
| } | ||
| else if (IsMiAsync(dwImplFlags)) | ||
| { | ||
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) |
There was a problem hiding this comment.
For IsMiAsync methods that are not Task/ValueTask-returning, MethodTableBuilder enforces that these infrastructure async methods can only be declared in the system module (otherwise it throws BADFORMAT). EEClass::AddMethod currently sets AsyncMethodFlags::AsyncCall without any equivalent validation, which would let EnC introduce invalid async-call methods on user types. Consider mirroring the loader check and rejecting such edits when the declaring type isn't implemented in the system module.
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) | |
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) | |
| // These infrastructure async methods are only permitted on types implemented in the system module, | |
| // mirroring the validation performed by MethodTableBuilder during normal type loading. | |
| if (!pModule->IsSystem()) | |
| { | |
| LOG((LF_ENC, LL_INFO100, | |
| "EEClass::AddMethod rejecting infrastructure async method (methodDef: 0x%08x) on non-system module\n", | |
| methodDef)); | |
| return COR_E_BADIMAGEFORMAT; | |
| } |
| if (pMethod->HasAsyncOtherVariant()) | ||
| { | ||
| MethodDesc* pAsyncOther = pMethod->GetAsyncOtherVariantNoCreate(); | ||
| if (pAsyncOther != NULL) | ||
| { | ||
| pAsyncOther->ResetCodeEntryPointForEnC(); | ||
| } |
There was a problem hiding this comment.
GetAsyncOtherVariantNoCreate() does not appear to exist anywhere in the repo (only reference is this new call), so this will not compile. If the intent is to avoid GC/allocation in this GC_NOTRIGGER path, use an existing no-create API (e.g., FindOrCreateAssociatedMethodDesc(... allowCreate: FALSE, AsyncOtherVariant)), or add a GetAsyncOtherVariantNoCreate helper alongside GetAsyncVariantNoCreate.
| bool isAsyncTaskReturning = IsMiAsync(dwImplFlags) && IsTaskReturning(returnKind); | ||
|
|
||
| // Create the primary MethodDesc (task-returning variant for async, or the only variant for non-async) | ||
| AsyncMethodFlags primaryAsyncFlags = AsyncMethodFlags::None; | ||
| if (isAsyncTaskReturning) | ||
| { | ||
| // For IsMiAsync + task-returning: the task-returning variant is a thunk, | ||
| // the real IL body belongs to the async variant. | ||
| primaryAsyncFlags = AsyncMethodFlags::ReturnsTaskOrValueTask | AsyncMethodFlags::Thunk; | ||
| } | ||
| else if (IsMiAsync(dwImplFlags)) | ||
| { | ||
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) | ||
| primaryAsyncFlags = AsyncMethodFlags::AsyncCall; |
There was a problem hiding this comment.
Async variant handling here only triggers when IsMiAsync(dwImplFlags) is set, but MethodTableBuilder creates Task/ValueTask async variants for any Task/ValueTask-returning method (and uses IsMiAsync only to decide which side is the thunk). As written, adding a normal Task/Task<T>/ValueTask/ValueTask<T> method via EnC would skip setting ReturnsTaskOrValueTask and would not create the async variant, diverging from the normal loader behavior.
| bool isAsyncTaskReturning = IsMiAsync(dwImplFlags) && IsTaskReturning(returnKind); | |
| // Create the primary MethodDesc (task-returning variant for async, or the only variant for non-async) | |
| AsyncMethodFlags primaryAsyncFlags = AsyncMethodFlags::None; | |
| if (isAsyncTaskReturning) | |
| { | |
| // For IsMiAsync + task-returning: the task-returning variant is a thunk, | |
| // the real IL body belongs to the async variant. | |
| primaryAsyncFlags = AsyncMethodFlags::ReturnsTaskOrValueTask | AsyncMethodFlags::Thunk; | |
| } | |
| else if (IsMiAsync(dwImplFlags)) | |
| { | |
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) | |
| primaryAsyncFlags = AsyncMethodFlags::AsyncCall; | |
| const bool returnsTaskOrValueTask = IsTaskReturning(returnKind); | |
| const bool isMiAsync = IsMiAsync(dwImplFlags); | |
| // Create the primary MethodDesc (task-returning variant for async, or the only variant for non-async) | |
| AsyncMethodFlags primaryAsyncFlags = AsyncMethodFlags::None; | |
| // Any Task/ValueTask-returning method should participate in async variant handling, | |
| // matching MethodTableBuilder behavior. IsMiAsync only determines which side is the thunk. | |
| if (returnsTaskOrValueTask) | |
| { | |
| primaryAsyncFlags |= AsyncMethodFlags::ReturnsTaskOrValueTask; | |
| } | |
| if (isMiAsync) | |
| { | |
| if (returnsTaskOrValueTask) | |
| { | |
| // For IsMiAsync + task-returning: the task-returning variant is a thunk, | |
| // the real IL body belongs to the async variant. | |
| primaryAsyncFlags |= AsyncMethodFlags::Thunk; | |
| } | |
| else | |
| { | |
| // IsMiAsync but not task-returning: infrastructure async method (e.g. Await helpers) | |
| primaryAsyncFlags |= AsyncMethodFlags::AsyncCall; | |
| } |
|
|
||
| if (returnKind == MethodReturnKind::NonGenericTaskReturningMethod) | ||
| { | ||
| // "... Task ... Method(args);" → "... void ... Method(args);" |
There was a problem hiding this comment.
This is duplicating code from regular type loader. Can it be refactored to avoid the duplication?
This change implements a TODO item in
EEClass::AddMethodDescto support Runtime Async