WIP: Prevent VGHM from moving guards #7411
Draft
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
In some cases the VGHM opt was moving code above a runtime guard allowing a privatized argument load to read past the end of the object bounds. This privatized temp slot was marked collectible causing the GC to assert when the loaded value was not a valid object reference. The issue the assert is reporting would not cause any runtime behaviour issues as the guard would have prevented any of the inlined code from executing and therefore the temp would never have been used, but allowing the invalid load to happen could cause a SEGV if the load offset caused an access passed the end of the heap. This fix simply disables guard motion in this opt to prevent this problem from occurring. It's likely that move HCR guards within the bounds of GC points is fine, but performance testing will be done to insure that the cost of disabling guard motion even for HCR guards has no major performance effect.
|
My testing has uncovered a number of failures that might be caused by this fix, so for now I need to analyse those issues and see if any changes to the fix is needed. |
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.
In some cases the VGHM opt was moving code above a runtime guard allowing a privatized argument load to read past the end of the object bounds. This privatized temp slot was marked collectible causing the GC to assert when the loaded value was not a valid object reference. The issue the assert is reporting would not cause any runtime behaviour issues as the guard would have prevented any of the inlined code from executing and therefore the temp would never have been used, but allowing the invalid load to happen could cause a SEGV if the load offset caused an access passed the end of the heap. This fix simply disables guard motion in this opt to prevent this problem from occurring. It's likely that move HCR guards within the bounds of GC points is fine, but performance testing will be done to insure that the cost of disabling guard motion even for HCR guards has no major performance effect.