[RuntimeAsync] Do not fail EnC on ordinary Task-returning methods when async feature is turned on.

[VSadov]

Fixes: #115536

We currently reject EnC updates on anything async-aware if async feature is enabled, including ordinary Task-returning methods. That is a regression and the last remaining failure in existing libraries tests if the feature is turned on.

Note that this fix not supposed to enable support for EnC in the presence of actual runtime async methods (although it may be sufficient). We are not yet looking into that. For the near future it would make sense to just disable EnC when async IL is emitted.

This is just to fix the case when an existing scenario with ordinary task-returning methods is affected if the feature is enabled in the runtime.

NOTE: in this PR the async feature is not enabled.
We can see that the change actually fixes the failure with the feature turned on in #115947

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull Request Overview

Enables Edit-and-Continue (EnC) updates for ordinary Task-returning methods when the async feature is enabled by handling both method variants and removing the previous rejection.

• Introduce GetAsyncOtherVariantNoCreate to fetch the async-related variant without allocation
• Extend ResetCodeEntryPointForEnC to reset the code entry for both Task-returning variants
• Remove the HasAsyncMethodData check in UpdateMethod to allow EnC on async-aware methods

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/coreclr/vm/method.hpp	Add GetAsyncOtherVariantNoCreate for retrieving the peer async variant without creation
src/coreclr/vm/method.cpp	Update ResetCodeEntryPointForEnC to also reset the Task-returning variant
src/coreclr/vm/encee.cpp	Remove the early rejection of async methods in EditAndContinueModule::UpdateMethod

Comments suppressed due to low confidence (2)

src/coreclr/vm/method.cpp:3241

• New EnC behavior for Task-returning methods isn't covered by existing tests—add a test that applies EnC to an ordinary Task-returning method to verify both variants get updated.

if (IsTaskReturningMethod())

src/coreclr/vm/method.cpp:3245

• Calling ResetCodeEntryPointForEnC recursively on the other variant may trigger an assertion or infinite recursion if the peer is itself a thunk; consider guarding this or breaking the recursion.

_ASSERTE(!IsAsyncThunkMethod());

[jkotas]

The thunk should not need to change due to EnC edit. It should be enough to reset the actual method only.

Should the fix rather be at the top of this method? Something like:

if (thunk)
{
    GetActualMethod()->ResetCodeEntryPointForEnC();
   return;
}

[VSadov]

But would the thunk, if already JITed, have captured the address of the old method?

I.E. how the old thunk would know the address of the new method? Are we calling everything via indirection when EnC is enabled?

[VSadov]

But would the thunk, if already JITed, have captured the address of the old method?

I.E. how the old thunk would know the address of the new method? Are we calling everything via indirection when EnC is enabled?

Thinking more about this - updating callers of the changed method would be a more general problem, not only affecting thunks, so perhaps we do call everything via indirection for EnC and updating only the method that changed would be enough.

[VSadov]

Also - the edits would always modify the IL of the actual method, not a thunk.
If the caller always gives us the methoddesc that owns the changed IL, then we would always have the correct method, thus GetActualMethod() would be unnecessary.

There is a possibility of confusion if the EnC code operates in terms of methoddef/token, then the ordinary Task-returning case would still map it to the correct methoddesc, but for runtime async method the methoddef could be resolved to the task-returning thunk.
I am not very familiar with entire way how a code edit ends up getting here.

Perhaps just adding an assert that we should not see a thunk here would be sufficient?

[VSadov]

Basically, the following would be correct:

if (thunk)
{
    GetActualMethod()->ResetCodeEntryPointForEnC();
   return;
}

I just suspect the if (thunk) may always be false because user does not edit thunks.

[VSadov]

By looking at ApplyEditAndContinue it seems like the diff is in terms of tokens, so doing if (thunk) may be necessary.

[jkotas]

we do call everything via indirection for EnC and updating only the method that changed would be enough.

Right, EnC adds indirections everywhere, disables inlining, etc.

[jkotas]

so doing if (thunk) may be necessary.

Right, that's what I thought as well.