Skip to content

Fix RuntimeMethodHandle cache key for async variants#128432

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-runtime-method-handle-interning
Open

Fix RuntimeMethodHandle cache key for async variants#128432
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-runtime-method-handle-interning

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 21, 2026

Fixes #128286
Fixes #128291

NativeAOT TypeLoader was interning dynamically created RuntimeMethodHandles without including the async-variant bit in the key. This could cause async/non-async lookups for the same method components to alias to one cached handle and resolve the wrong method variant; this change makes async variant part of handle identity and re-enables the affected System.Text.Json source-gen test project.

  • TypeLoader runtime method handle key

    • Extended RuntimeMethodHandleKey to include isAsyncVariant in identity.
    • Updated constructor and key creation to pass/store isAsyncVariant.
    • Updated Equals to require matching async-variant state.
    • Updated GetHashCode to include async-variant state via combined hashing.
  • NativeAOT libraries test project inclusion

    • Removed the NativeAOT exclusion entry for:
      • System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj
    • Kept unrelated platform-specific exclusions intact.
// before: async flag omitted from interning key
RuntimeMethodHandleKey key = new RuntimeMethodHandleKey(declaringTypeHandle, handle, genericMethodArgs);

// after: async flag is part of key identity
RuntimeMethodHandleKey key = new RuntimeMethodHandleKey(declaringTypeHandle, handle, genericMethodArgs, isAsyncVariant);
Original prompt

Fix NativeAOT RuntimeMethodHandle interning for runtime async variants and re-enable the affected System.Text.Json source generation test project.

Context:

  • Issue: [ci-scan] Test failure: System.Text.Json.SourceGeneration.Roslyn4.4.Tests Access Violation crash under NativeAOT #128286
  • The failing NativeAOT outerloop test project is System.Text.Json.SourceGeneration.Roslyn4.4.Tests.
  • Investigation points to a TypeLoader cache bug: dynamically-created RuntimeMethodHandles are interned by RuntimeMethodHandleKey, but the key currently includes declaring type, method handle, and generic method arguments only. It does not include the runtime async variant bit, even though GetRuntimeMethodHandleForComponents(..., bool isAsyncVariant) stores that bit in MethodHandleInfo.IsAsyncVariant, and RuntimeMethodHandle.Equals compares _numGenericArgsAndFlag, which includes RuntimeMethodHandleConstants.IsAsyncVariant.
  • This means requesting the async and non-async variants of the same method can return the same cached RuntimeMethodHandle, depending on cache population order. Later, GetMethodDescForRuntimeMethodHandle reads the stale/mismatched IsAsyncVariant bit and resolves the wrong method variant.

Required changes:

  1. In src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.LdTokenResultLookup.cs, update RuntimeMethodHandleKey so it includes isAsyncVariant as part of identity:
    • Add a private bool field, e.g. _isAsyncVariant.
    • Update the constructor to accept and store bool isAsyncVariant.
    • Update Equals(RuntimeMethodHandleKey other) to compare the async variant flag in addition to declaring type, method handle, and generic args.
    • Update GetHashCode() so async and non-async variants produce different hashes.
    • Update GetRuntimeMethodHandleForComponents to pass isAsyncVariant when constructing the key.
  2. Add focused test coverage if there is an existing suitable NativeAOT/type loader test location. The test should verify that requesting otherwise-identical runtime method handles with isAsyncVariant: false and isAsyncVariant: true does not return the same handle and preserves the requested async bit. If direct test coverage is impractical because this is internal TypeLoader-only functionality, document why in the PR description and rely on existing System.Text.Json NativeAOT test coverage being re-enabled.
  3. Remove the relevant test project exclusion in src/libraries/tests.proj:
<!-- https://github.com/dotnet/runtime/issues/128286 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Text.Json\tests\System.Text.Json.SourceGeneration.Tests\System.Text.Json.SourceGeneration.Roslyn4.4.Tests.csproj" />

Validation expectations:

  • Build affected NativeAOT TypeLoader/CoreLib projects if feasible.
  • Run or document the most relevant tests. At minimum, ensure the exclusion is removed so System.Text.Json.SourceGeneration.Roslyn4.4.Tests runs again in the libraries test project.

PR title should be concise, e.g. Fix RuntimeMethodHandle cache key for async variants.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: I'm thinking about the theory of @jakobbotch - this could be the case where we're doing dispatch using a RuntimeMethodHandle that was created dynamically using the type loader in System.Private.TypeLoader. could there be a bug in how we're creating these RuntimeMethodHandles or comparing them (missing the runtime async flag on the handle)?

Assistant: Yes — that theory looks very plausible. I found at least one concrete hole in the dynamic RuntimeMethodHandle path: the TypeLoader cache key ignores isAsyncVariant, even though the allocated MethodHandleInfo stores it and RuntimeMethodHandle.Equals compares it.

The suspicious code is in TypeLoaderEnvironment.LdTokenResultLookup.cs:

private struct RuntimeMethodHandleKey : IEquatable<RuntimeMethodHandleKey>
{
    private RuntimeTypeHandle _declaringType;
    private MethodHandle _handle;
    private RuntimeTypeHandle[] _genericArgs;

    public RuntimeMethodHandleKey(RuntimeTypeHandle declaringType, MethodHandle handle, RuntimeTypeHandle[] genericArgs)
    {
        _declaringType = declaringType;
        _handle = handle;
        _genericArgs = genericArgs;
    }
}

But the creation API accepts isAsyncVariant and stores it into MethodHandleInfo:

public unsafe RuntimeMe...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/865c991f-a15c-4cca-9b66-845450dd5e89

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 21, 2026 07:41
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/865c991f-a15c-4cca-9b66-845450dd5e89

Co-authored-by: MichalStrehovsky <13110571+MichalStrehovsky@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot May 21, 2026 07:45
Copilot AI changed the title [WIP] Fix NativeAOT RuntimeMethodHandle interning for async variants Fix RuntimeMethodHandle cache key for async variants May 21, 2026
Copilot AI requested a review from MichalStrehovsky May 21, 2026 07:47
@MichalStrehovsky
Copy link
Copy Markdown
Member

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 NativeAOT’s TypeLoader RuntimeMethodHandle interning to treat the “async variant” bit as part of a handle’s identity, preventing cache aliasing between async/non-async variants. It also re-enables the previously excluded System.Text.Json.SourceGeneration.Roslyn4.4.Tests project for most NativeAOT configurations by removing the broad exclusion while keeping existing OS/arch-specific exclusions.

Changes:

  • Extended RuntimeMethodHandleKey to include the async-variant flag in equality and hashing, and passed the flag when creating interning keys.
  • Updated hashing logic for RuntimeMethodHandleKey to incorporate the async-variant flag.
  • Removed the unconditional NativeAOT test exclusion for System.Text.Json.SourceGeneration.Roslyn4.4.Tests (while retaining existing conditional exclusions, e.g., OS/arch-specific cases).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.LdTokenResultLookup.cs Adds isAsyncVariant into the runtime method handle interning key (equals/hash + key construction).
src/libraries/tests.proj Re-enables System.Text.Json.SourceGeneration.Roslyn4.4.Tests for NativeAOT by removing the broad exclusion entry.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 21, 2026 21:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 2 out of 2 changed files in this pull request and generated 1 comment.

Comment on lines +113 to +115
if (_isAsyncVariant)
hashCode ^= unchecked((int)0x9e3779b9);
return hashCode;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment