-
Notifications
You must be signed in to change notification settings - Fork 5k
[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
Conversation
There was a problem hiding this 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
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. |
Right, it is a by-design feature that ilasm can produce invalid IL. |
This looks like a fairly mechanical change. I am not very familiar with ilasm/ildasm, but supporting 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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
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. |
Seems that we cannot make |
I wonder if we could just treat it as numerical value if ilasm can take methodimpl as a number. |
Implementing ilasm and ildasm support for RuntimeAsync following the spec.
Closes #115093
cc: @VSadov