Repository navigation
[release/9.0] Fix a case in MethodImpl overriding which wasn't handled as expected in ilc.exe for native aot - #107409
Merged
jeffschwMSFT merged 6 commits intoSep 5, 2024
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
…in ilc.exe for native aot - This was causing real C# applications to fail to behave correctly on NativeAOT builds - Enable testing for covariant byref returns on nativeaot (split testing up so that the tests do not expect TypeLoadException, which NativeAOT doesn't reliably generate) - Fix implementation of SynthesizedPgoIncompatible project file flag for test script generation - Put copy of attributetesting.il test into the managed type system unit test suite - Add regression test of issue noted in #96175 into managed type system unit test suite - Update workflow documentation to include a better path to finding details on how to run CoreCLR and Libraries tests for Native AOT Fixes #96175
…e for todo comments
Copy link
Copy Markdown
Contributor
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
Sorry, something went wrong.
jeffschwMSFT
approved these changes
Sep 5, 2024
jeffschwMSFT
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
approved. please get a code review. we can merge when ready
Sorry, something went wrong.
agocke
approved these changes
Sep 5, 2024
agocke
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
LGTM. I'll keep an eye on CI
Sorry, something went wrong.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Reviewers
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #106716 to release/9.0
/cc @davidwrighton
Customer Impact
This bug was found when people internally and externally began using the covariant returns feature which makes use of MethodImpl metadata records for overrides on virtual methods in ways that are not done with the C# compiler under any other circumstance.
Regression
This is not a regression, but merely a newly important bug, which has become important as customers have started using covariant methods.
Testing
The existing testbed for covariant returns would have caught this issue, but it was not supported on NativeAOT, as it mingled tests which tested the TypeLoadException throwing paths and the normal execution paths. This fix was tested by changing the splitting the tests into tests which throw and tests which test correct behavior. In addition, a unit test specific to this issue was added to the type system unit test suite, and the most critical of the newly enabled tests was added to the type system unit test suite.
Risk
Medium. The virtual method resolution algorithm in the managed type system is notoriously complex, and used in crossgen2 as well as native aot. The fix taken was minimal, and did not attempt to address all possible issues in MethodImpl resolution, but instead was limited to ensuring that the code patterns generated by the C# compiler worked correctly. The medium level of risk is what caused the fairly extensive test effort put in as part of this change.