Skip to content

Conversation

@eduardo-vp
Copy link
Member

@eduardo-vp eduardo-vp commented Nov 13, 2025

The method matches EmitAsyncMethodThunk in CoreCLR.

Copilot AI review requested due to automatic review settings November 13, 2025 00:51
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 13, 2025
@eduardo-vp eduardo-vp added runtime-async and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 13, 2025
Copilot finished reviewing on behalf of eduardo-vp November 13, 2025 01:04
Copy link
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 implements async method thunks in Native AOT by replacing a ThrowNotSupportedException placeholder with actual IL generation code in the EmitAsyncMethodThunk method. The implementation mirrors the existing CoreCLR VM C++ implementation to ensure consistency across runtimes.

Key Changes

  • Removed the NotSupportedException throw and implemented full IL generation for async method thunks
  • Added logic to handle both ValueTask and Task return types (generic and non-generic variants)
  • Implemented instantiation handling for generic types and methods
  • Added cross-reference comments between C++ and C# implementations

Reviewed Changes

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

File Description
src/coreclr/vm/asyncthunks.cpp Updated comment capitalization and added cross-reference comment to C# implementation
src/coreclr/tools/Common/TypeSystem/IL/Stubs/AsyncThunks.cs Implemented full async method thunk IL generation matching CoreCLR VM behavior, handling ValueTask/Task paths with completion checking and awaiting logic

@dotnet-policy-service
Copy link
Contributor

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

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

This looks good to me, however the failure in the Checked leg is real. You'll need to wait for #121573 to clear it.

@MichalStrehovsky MichalStrehovsky added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 13, 2025
@MichalStrehovsky MichalStrehovsky removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 13, 2025
@eduardo-vp eduardo-vp enabled auto-merge (squash) November 13, 2025 22:11
@eduardo-vp eduardo-vp merged commit 6b5b847 into dotnet:main Nov 14, 2025
95 of 99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants