Skip to content

[RuntimeAsync] ilasm/ildasm support for the MethodImpl.Async - #115332

Merged
VSadov merged 3 commits into
dotnet:mainfrom
hez2010:async2-ilasm
May 13, 2025
Merged

[RuntimeAsync] ilasm/ildasm support for the MethodImpl.Async#115332
VSadov merged 3 commits into
dotnet:mainfrom
hez2010:async2-ilasm

Conversation

@hez2010

@hez2010hez2010 commented May 6, 2025

Copy link
Copy Markdown
Contributor

Implementing ilasm and ildasm support for RuntimeAsync following the spec.

Closes#115093

cc: @VSadov

CopilotAI review requested due to automatic review settings May 6, 2025 12:06

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 pull request adds support for the "async" method implementation attribute to both ilasm and ildasm, in accordance with the RuntimeAsync specification. The changes include:

  • In il_kywd.h, a new keyword "async" is declared.
  • In dasm.cpp, the disassembler now checks for the async attribute and prints " async" in the output.

Reviewed Changes

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

FileDescription
src/coreclr/inc/il_kywd.hAdded the "async" keyword definition to support async methods.
src/coreclr/ildasm/dasm.cppUpdated method dump logic to include "async" when the attribute is active.
Files not reviewed (2)
  • src/coreclr/ilasm/asmparse.y: Language not supported
  • src/coreclr/ilasm/prebuilt/asmparse.grammar: Language not supported

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2025
@huoyaoyuan

Copy link
Copy Markdown
Member

Combining MethodImplOptions.Async with MethodImplOptions.Synchronized is invalid.
Applying MethodImplOptions.Async to methods with a byref or ref-like return value is invalid.
Applying MethodImplOptions.Async to vararg methods is invalid.

Does ilasm do this sort of check?

@hez2010

hez2010 commented May 6, 2025

Copy link
Copy Markdown
ContributorAuthor

Does ilasm do this sort of check?

I don't think so. We don't have any of such check in ilasm, and it just emits whatever you write.
For example, today ilasm will happily take a byreflike type for a generic parameter that doesn't accept byreflike types. Such checks are done at the runtime.

@jkotas

Copy link
Copy Markdown
Member

I don't think so. We don't have any of such check in ilasm, and it just emits whatever you write.

Right, it is a by-design feature that ilasm can produce invalid IL.

@VSadov

Copy link
Copy Markdown
Member

This looks like a fairly mechanical change. I am not very familiar with ilasm/ildasm, but supporting async looks very similar to supporting synchronized.

I am wondering - are there any tests for ilasm/ildasm?

@hez2010

Copy link
Copy Markdown
ContributorAuthor

I am wondering - are there any tests for ilasm/ildasm?

almost no. the only ilasm test in https://github.com/dotnet/runtime/tree/main/src/tests/ilasm is to test MethodImpl.AggressiveOptimization. and there's nothing for ildasm.

@jkotas

Copy link
Copy Markdown
Member

We test ilasm/ildasm by roundtripping all runtime test binaries through ildasm/ilasm, and verifying that the result still works. So the test coverage for this will come online as side-effect of having any tests for async.

@VSadovVSadov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jkotas

Copy link
Copy Markdown
Member

This introduced regression dotnet/dotnet#544 (comment) that is breaking the dotnet/dotnet build.

Reverting in #115621 until we figure out what to do about this.

@hez2010

Copy link
Copy Markdown
ContributorAuthor

Seems that we cannot make async a keyword in IL as it may be used as an identifier.
I'll work on a fix by making async a non-keyword in IL.

@VSadov

Copy link
Copy Markdown
Member

I wonder if we could just treat it as numerical value if ilasm can take methodimpl as a number.
It would not be pretty, but might be ok for IL

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 15, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ILTools-coreclrcommunity-contributionIndicates that the PR has been added by a community memberruntime-async

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RuntimeAsync] Implement ilasm/ildasm support for the MethodImpl.Async

6 participants

@hez2010@huoyaoyuan@jkotas@VSadov@am11