Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 13, 2025

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented May 6, 2025

Implementing ilasm and ildasm support for RuntimeAsync following the spec.

Closes #115093

cc: @VSadov

@Copilot Copilot AI review requested due to automatic review settings May 6, 2025 12:06
@ghost ghost added the area-ILTools-coreclr label May 6, 2025
Copy link
Contributor

@Copilot 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 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.

File Description
src/coreclr/inc/il_kywd.h Added the "async" keyword definition to support async methods.
src/coreclr/ildasm/dasm.cpp Updated 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-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 6, 2025
@huoyaoyuan
Copy link
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
Copy link
Contributor Author

hez2010 commented May 6, 2025

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
Member

jkotas commented May 6, 2025

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
Member

VSadov commented May 13, 2025

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
Contributor Author

hez2010 commented May 13, 2025

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
Member

jkotas commented May 13, 2025

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.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@VSadov VSadov merged commit 660f22a into dotnet:main May 13, 2025
113 checks passed
jkotas added a commit that referenced this pull request May 15, 2025
@jkotas
Copy link
Member

jkotas commented May 15, 2025

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
Contributor Author

hez2010 commented May 15, 2025

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
Member

VSadov commented May 15, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ILTools-coreclr community-contribution Indicates that the PR has been added by a community member runtime-async
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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