-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[wasm][coreclr] Fix methodesc reset for dynamic methods #122444
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
[wasm][coreclr] Fix methodesc reset for dynamic methods #122444
Conversation
Fix dotnet#122419 Make sure we don't enter the branch for rental methods in other cases when using portable entrypoints.
|
Tagging subscribers to this area: @mangod9 |
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 PR fixes a bug in the MethodDesc::Reset() function that affected WebAssembly and CoreCLR when using portable entrypoints. The fix ensures that the "rental methods" code path (for reflection.emit dynamic methods) is only entered when appropriate, preventing non-rental methods from incorrectly executing that branch.
Key Changes:
- Modified conditional compilation logic to add an explicit check for reflection emit modules when portable entrypoints are enabled
- Changed from unconditional execution of the rental methods block to conditional execution based on
IsReflectionEmit()
|
Rental methods were .NET Framework feature. They do not exist in .NET Core. The support for the rental methods should be unreachable code. I think the right fix is to change this to just: |
jkotas
left a comment
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, assuming there are no regressions in the CI
|
The CI fails in LibraryImportGeneratorTests on Linux x64/arm64 both Azure Linux and Alpine with It fails on Windows too, but the assert is not logged into the console. |
|
It may be unrelated though |
|
Ah, yes, it is unrelated, it is #122447 |
Fix #122419
Make sure we don't enter the branch for rental methods in other cases when using portable entrypoints.