Skip to content

Add AsyncVariantLookup::ReturnDroppingThunk for parallel RDT lookup in FindOrCreateAssociatedMethodDesc - #129442

Merged
VSadov merged 11 commits into
mainfrom
copilot/enable-async-method-thunk-test
Jul 3, 2026
Merged

Add AsyncVariantLookup::ReturnDroppingThunk for parallel RDT lookup in FindOrCreateAssociatedMethodDesc#129442
VSadov merged 11 commits into
mainfrom
copilot/enable-async-method-thunk-test

Conversation

CopilotAI commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Fixes: #127197

Basically, allow parallel lookups for ReturnDroppingThunk as the need may arise if covariant override is a GVM that needs to lookup for a specialized RDT during GVM resolution.

Initial assumption (and assert) that noone will ever need to lookup for ReturnDroppingThunk is only partially correct.
We will not lookup for an RDT starting from other kind of variant, that part is correct, but we may need to lookup for an RDT, starting from a prototype RDT if something else needs changing - like generic substitution.


The assert !pPrimaryMD->IsReturnDroppingThunk() in FindOrCreateAssociatedMethodDesc fires for generic virtual methods with covariant return, when the primary MethodDesc is itself a ReturnDroppingThunk (RDT) and we need to look up a parallel RDT that differs only in something other than async kind (e.g. generic arguments). The instantiated-method-desc hash table also keyed on a single bool isAsyncVariant, which cannot distinguish a regular async variant from an RDT (both have IsAsyncVariantMethod() == true).

Changes

  • AsyncVariantLookup::ReturnDroppingThunk enum value (method.hpp) — matched only by methods where IsReturnDroppingThunk() is true. Existing Async continues to match only regular async variants (not RDTs).

  • MethodDesc::GetAsyncVariantLookup() helper — returns the lookup value matching a method's own async kind (Ordinary/Async/ReturnDroppingThunk).

  • Convenience overload of FindOrCreateAssociatedMethodDesc — replaces the _ASSERTE(!pPrimaryMD->IsReturnDroppingThunk()) with a 3-way selection of the variant lookup based on the primary MD:

    AsyncVariantLookup variantLookup = pPrimaryMD->IsReturnDroppingThunk() ? AsyncVariantLookup::ReturnDroppingThunk :
    pPrimaryMD->IsAsyncVariantMethod() ? AsyncVariantLookup::Async :
    AsyncVariantLookup::Ordinary;
  • Hash table API widenedInstMethodHashTable::FindMethodDesc and InstantiatedMethodDesc::FindLoadedInstantiatedMethodDesc now take AsyncVariantLookup instead of bool isAsyncVariant; ContainsMethodDesc passes pMD->GetAsyncVariantLookup(). All 9 call sites in genmeth.cpp updated. This is required because a bool key cannot disambiguate regular-async from RDT entries that hash to the same bucket — without it, GetAsyncVariant() on an RDT can return the RDT itself.

Notes

  • The original assertion no longer fires; GetAsyncVariant() on an RDT now correctly returns its parallel regular-async variant.

Note

This PR description and the changes were generated with assistance from GitHub Copilot.

CopilotAI requested review from Copilot and removed request for CopilotJune 15, 2026 20:59
CopilotAI linked an issue Jun 15, 2026 that may be closed by this pull request
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

CopilotAI requested review from Copilot and removed request for CopilotJune 15, 2026 21:51
CopilotAI changed the title [WIP] Fix generic virtual method test for CoreCLRAdd AsyncVariantLookup::ReturnDroppingThunk for parallel RDT lookup in FindOrCreateAssociatedMethodDescJun 15, 2026
CopilotAI requested a review from VSadovJune 15, 2026 21:52
CopilotAI review requested due to automatic review settings June 16, 2026 02:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates CoreCLR’s async-variant method lookup so that ReturnDroppingThunk (RDT) methods can be distinguished from “regular” async variants during FindOrCreateAssociatedMethodDesc / instantiated-method-desc hashtable lookups, preventing incorrect matches when multiple async-kind variants share the same method identity apart from async kind.

Changes:

  • Adds AsyncVariantLookup::ReturnDroppingThunk plus MethodDesc::GetAsyncVariantLookup() and updates matching logic (MatchesAsyncVariantLookup) to disambiguate regular async variants vs RDTs.
  • Widens instantiated-method-desc lookup APIs from bool isAsyncVariant to AsyncVariantLookup and updates call sites accordingly.
  • Modifies the covariant-return async test by removing an ActiveIssue skip annotation on TestGenericVirtualMethod.

Reviewed changes

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

Show a summary per file
FileDescription
src/tests/async/covariant-return/covariant-returns.csRemoves the ActiveIssue gate for TestGenericVirtualMethod, re-enabling the test.
src/coreclr/vm/method.hppIntroduces AsyncVariantLookup::ReturnDroppingThunk, adds GetAsyncVariantLookup(), and refines async-kind matching logic.
src/coreclr/vm/instmethhash.hUpdates InstMethodHashTable::FindMethodDesc signature to accept AsyncVariantLookup (and adds a forward declaration).
src/coreclr/vm/instmethhash.cppUses MatchesAsyncVariantLookup + GetAsyncVariantLookup() to filter and validate hashtable entries.
src/coreclr/vm/genmeth.cppPlumbs AsyncVariantLookup through instantiated method desc lookups/creation paths.

Comment threadsrc/tests/async/covariant-return/covariant-returns.cs
Comment threadsrc/coreclr/vm/method.hpp Outdated
Comment threadsrc/coreclr/vm/instmethhash.h Outdated
@VSadov

Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment threadsrc/coreclr/vm/method.hpp Outdated
Comment threadsrc/tests/async/covariant-return/covariant-returns.csproj Outdated
Comment threadsrc/tests/async/covariant-return/covariant-returns.cs

CopilotAI commented Jun 30, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot resolve the merge conflicts in this pull request

Resolved. The single conflict was in src/coreclr/vm/genmeth.cppNewInstantiatedMethodDesc was refactored in main to move the lock acquisition to the top of the function (collapsing two FindLoadedInstantiatedMethodDesc calls into one). The auto-merge had already correctly applied our GetAsyncVariantLookup() change to that remaining call; the only conflict marker was in the else if (getWrappedCode) setup block, which was resolved by taking main's two-line version (SetupSharedMethodInstantiation + assert). All other PR changes (method.hpp, instmethhash.h, instmethhash.cpp, test files) merged cleanly.

Comment threadsrc/coreclr/vm/method.hpp Outdated
CopilotAI requested review from Copilot and removed request for CopilotJune 30, 2026 17:59
@VSadov
VSadov marked this pull request as ready for review July 2, 2026 22:14
CopilotAI review requested due to automatic review settings July 2, 2026 22:14

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment threadsrc/coreclr/vm/genmeth.cpp
Comment threadsrc/coreclr/vm/method.hpp
CopilotAI requested review from Copilot and removed request for CopilotJuly 2, 2026 22:21
@VSadov
VSadov requested a review from jakobbotschJuly 2, 2026 22:36
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotJuly 2, 2026 22:38
CopilotAI requested a review from VSadovJuly 2, 2026 22:39
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
CopilotAI requested review from Copilot and removed request for CopilotJuly 2, 2026 22:47
@VSadov
VSadov merged commit d972fb1 into mainJul 3, 2026
115 of 117 checks passed
@VSadov
VSadov deleted the copilot/enable-async-method-thunk-test branch July 3, 2026 15:17
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-preview7 milestone Jul 4, 2026
eiriktsarpalis pushed a commit that referenced this pull request Jul 15, 2026
…n FindOrCreateAssociatedMethodDesc (#129442)
Fixes: #127197
Basically, allow parallel lookups for ReturnDroppingThunk as the need
may arise if covariant override is a GVM that needs to lookup for a
specialized RDT during GVM resolution.
Initial assumption (and assert) that noone will ever need to lookup for
ReturnDroppingThunk is only partially correct.
We will not lookup for an RDT starting from other kind of variant, that
part is correct, but we may need to lookup for an RDT, starting from a
prototype RDT if something else needs changing - like generic
substitution.
---
The assert `!pPrimaryMD->IsReturnDroppingThunk()` in
`FindOrCreateAssociatedMethodDesc` fires for generic virtual methods
with covariant return, when the primary `MethodDesc` is itself a
`ReturnDroppingThunk` (RDT) and we need to look up a parallel RDT that
differs only in something other than async kind (e.g. generic
arguments). The instantiated-method-desc hash table also keyed on a
single `bool isAsyncVariant`, which cannot distinguish a regular async
variant from an RDT (both have `IsAsyncVariantMethod() == true`).
## Changes
- **`AsyncVariantLookup::ReturnDroppingThunk` enum value**
(`method.hpp`) — matched only by methods where `IsReturnDroppingThunk()`
is true. Existing `Async` continues to match only regular async variants
(not RDTs).
- **`MethodDesc::GetAsyncVariantLookup()` helper** — returns the lookup
value matching a method's own async kind
(`Ordinary`/`Async`/`ReturnDroppingThunk`).
- **Convenience overload of `FindOrCreateAssociatedMethodDesc`** —
replaces the `_ASSERTE(!pPrimaryMD->IsReturnDroppingThunk())` with a
3-way selection of the variant lookup based on the primary MD:
```cpp
AsyncVariantLookup variantLookup = pPrimaryMD->IsReturnDroppingThunk() ?
AsyncVariantLookup::ReturnDroppingThunk :
pPrimaryMD->IsAsyncVariantMethod() ? AsyncVariantLookup::Async :
AsyncVariantLookup::Ordinary;
```
- **Hash table API widened** — `InstMethodHashTable::FindMethodDesc` and
`InstantiatedMethodDesc::FindLoadedInstantiatedMethodDesc` now take
`AsyncVariantLookup` instead of `bool isAsyncVariant`;
`ContainsMethodDesc` passes `pMD->GetAsyncVariantLookup()`. All 9 call
sites in `genmeth.cpp` updated. This is required because a `bool` key
cannot disambiguate regular-async from RDT entries that hash to the same
bucket — without it, `GetAsyncVariant()` on an RDT can return the RDT
itself.
## Notes
- The original assertion no longer fires; `GetAsyncVariant()` on an RDT
now correctly returns its parallel regular-async variant.
> [!NOTE]
> This PR description and the changes were generated with assistance
from GitHub Copilot.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
Co-authored-by: Vladimir Sadov <vsadov@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 4, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Runtime async] Generic virtual method test failing on CoreCLR

4 participants

@VSadov@jakobbotsch