[5.1] [mandatory-inlining] Fix lifetime extension error. #26784
Merged
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.
This bug is caused by a quirk in the API of the linear lifetime
checker. Specifically, even though valueHasLinearLifetime is passed a SILValue
(the value whose lifetime one is checking), really it doesnt care about that
value (except for error diagnostics). Really it just cares about the parent
block of the value since it assumes that the value is guaranteed to dominate all
uses.
This creates a footgun when if one is writing code using "generic ossa/non-ossa"
routines on SILBuilder (the emit*Operation methods), if one in non-ossa code
calls that function, it returns the input value of the strong_retain. This
causes the linear lifetime error, to use the parent block of the argument of the
retain, instead of the parent block of the retain itself. This then causes it to
find the wrong leaking blocks and thus insert destroys in the wrong places.
I fix this problem in this commit by noting that the partial apply is our
original insertion point for the copy, so of course it is going to be in the
same block. So I changed the linear lifetime checker to check for leaks with
respect to the partial applies result.
In a subsequent commit, I am going to add a new API on top of this that is based
around the use of the value by the partial apply (maybe
extendLifetimeFromUseToInsertionPoint?). By using the use, it will express in
code more clearly what is happening here and will insert the copy for you.
rdar://54234011
(cherry picked from commit 5294e51)
Explanation:
This fixes a bug in Mandatory Inlining that should be relatively hard to hit, but that if it is hit results in us inserting destroys in the wrong place. The fix is simple and well understand so the pay off should be high and the risk very low.
Issue: rdar://54234011
Scope: B/c this involves mandatory inlining if we miscompile this will hit both -Onone and -O code, so it is important.
Risk: Very Low. The change is simple and well understand.
Testing: Added compiler regression tests.
Reviewed by: @atrick
Resolves: rdar://54234011