Ensure consistent drop for panicking drop in hint::select_unpredictable #145174
Conversation
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
There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having any drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly. The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body.
rustbot
added
S-waiting-on-review
197g
changed the title
This comment has been minimized.
This comment has been minimized.
It seems important for LLVM that we select on the values by-value instead of reading and have no intermediate store. So make sure the guards selects both potential drops but defers the return value to the second selection. Since the two options alias we use raw mutable pointers instead of mutable references as before.
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.
There are a few alternatives to the implementation. The principal problem is that the selected value must be owned (in the sense of having a drop flag of sorts) when the unselected value is dropped, such that panic unwind goes through the drop of both. This ownership must then be passed on in return when the drop went smoothly.
The basic way of achieving this is by extracting the selected value first, at the cost of relying on the optimizer a little more for detecting the copy as constructing the return value despite having a place in the body. Unfortunately, that causes LLVM to discard the !unpredictable annotation (for some reason that is beyond my comprehension of LLVM).
Extract from the build log showing an unannotated select being used
So instead, this PR includes a guard to drop the selected
MaybeUnit<T>which is active only for the section where the unselected value is dropped. That leaves the code for selecting the result intact leading to the expected ir. That complicates the 'unselection' process a little bit since we require both values as a result of that intrinsic call. Since the arguments alias, this portion as well as the drop guard uses raw pointers.Closes: #145148
Prior: #139977