[release/8.0] [mono][interp] Fix type of args when inlining method #102801
Conversation
This file contains 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
The vars allocated from pushing values on the execution stack might not reflect exactly the actual type of the var. Consider this pattern: condbr BB0 newobj Derived // push var0 of type Derived br BB1 BB0: newobj Base // push var1 of type Base // here we will end up inserting a `mov var1 -> var0` BB1: // top of stack will be seen as being var0 call Because we first reach BB1 with the stack contents of var0, BB1 will end up accessing top of the stack as var0. However the type of var0 at this point is not Derived, since it can also be a Base object. We currently don't update the type of var0, but just update the type information of the top of stack entry when entering BB1. When inlining, after this commit, we use the type information from the stack, rather than the type of the var present on the stack.
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
kotlarmilos
approved these changes
May 29, 2024
lewing
added
Servicing-approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #102570 to release/8.0-staging. Fix for #102046
Customer Impact
When using interpreter, inlining a method that has its argument the result of a ternary conditional operator, could lead to an assumption that the argument is of the wrong type. This is a regression from NET7 because in this release we started to devirtualize calls based on the type of objects and when inlining we attempt to get the real type of arguments from the parent method. This issue was reported by customer in BlazorWasm with a simple repro consisting of a list of items so it looks like it can be easily hit. Since reproduction of this issue relies on inlining to happen, it was probably noticed this late in the release cycle since it needs for the problematic method to be tiered up.
Testing
Tested the fix locally on top of 8.0 release branch with the customer provided sample. CI PR testing without issues.
Risk
Low. There are other more conservative fixes like obtaining the type of the arg from the inlined method signature (like it was done in .NET7) or completely disabling devirtualization. In my opinion the fix in this PR is simple enough that reverting behavior is not necessary.