[release/7.0] Fix branch removal in compiler generated code #3161
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
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method. The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method. This does have one possibly negative impact: the issue described in dotnet#2937 is now consistent and happens always. Added tests. Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087 Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar. In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
|
Needs tactics approval |
sbomer
approved these changes
Jan 17, 2023
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.
7.0 port of #3088
Customer impact
In the worst case this case cause the trimmer to crash, preventing the app from being trimmed. In other cases this may lead to incorrect warnings or trimming behavior. The bug is order dependent so it's unpredictable (even though it's deterministic), while workaround may be possible by changing order of things, it would be very fragile.
See reported issue: #3157
Risk
While the change affects lot of places in the trimmer, it's mostly mechanical (see below for the technical description of the change). The main idea is to prevent access to Cecil's instructions list directly and always go through a helper which makes sure that all necessary modifications to the instructions of the method were applied first. As such the risk is relatively low, since it doesn't change how most things work, it only changes order - making sure that body modifications happen before the body is inspected for marking.
Testing
This change added targeted tests which reproed the problem and they cover several variations of the problem. The change has been in main (8.0) for more than a month without any known issues.
I validated that this change fixes the problem reported in #3157
Technical details about the change
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking
This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.
The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.
This does have one possibly negative impact: the issue described in #2937 is now consistent and happens always.
Added tests.
Note that there's still a whole in analysis of compiler generated code around state machines, see #3087
Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.
In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).