Skip to content

[5.1] [mandatory-inlining] Fix lifetime extension error. #26784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Aug 22, 2019

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

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)
@gottesmm gottesmm requested a review from atrick August 22, 2019 19:51
@gottesmm gottesmm requested a review from a team as a code owner August 22, 2019 19:51
@gottesmm
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gottesmm gottesmm changed the title [mandatory-inlining] Fix lifetime extension error. [5.1] [mandatory-inlining] Fix lifetime extension error. Aug 22, 2019
@gottesmm
Copy link
Contributor Author

@swift-ci nominate

@gottesmm gottesmm merged commit e90298c into swiftlang:swift-5.1-branch Aug 27, 2019
@gottesmm gottesmm deleted the swift-5.1-branch-rdar54234011 branch August 27, 2019 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants