Skip to content

[4.0] [sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values. #10806

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 Jul 7, 2017

[sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values.

partial_apply is a confusing instruction since it:

  1. Is printed with a function signature.
  2. Takes in some arguments of the same type as the underlying types of the given function signatures.
  3. Always takes in those arguments at +1 regardless of the convention printed on the partial apply.

Eventually we will split the partial apply representation so that the box is
represnted explicitly separately from the function signature, eliminating this
confusion.

The problem that we ran into here is that we were not treating @in values from
an alloc_stack or @in_guaranteed at all correctly. The reason why the tests did
not catch this is that a seperate sil_combine optimization that eliminates
trivially dead live ranges was eliminting the alloc_stack of the @in value in
our test. In contrast, the @in_guaranteed case was actually never tested at all.

I added tests for all of these conventions and in addition added a special mode
to SILCombine that stops the alloc_stack eliminating during testing.

rdar://32887993

…r all non-trivial captured values.

partial_apply is a confusing instruction since it:

1. Is printed with a function signature.
2. Takes in some arguments of the same type as the underlying types of the given function signatures.
3. Always takes in those arguments at +1 regardless of the convention printed on the partial apply.

Eventually we will split the partial apply representation so that the box is
represnted explicitly separately from the function signature, eliminating this
confusion.

The problem that we ran into here is that we were not treating @in values from
an alloc_stack or @in_guaranteed at all correctly. The reason why the tests did
not catch this is that a seperate sil_combine optimization that eliminates
trivially dead live ranges was eliminting the alloc_stack of the @in value in
our test. In contrast, the @in_guaranteed case was actually never tested at all.

I added tests for all of these conventions and in addition added a special mode
to SILCombine that stops the alloc_stack eliminating during testing.

rdar://32887993
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

@swift-ci test

@gottesmm gottesmm requested a review from atrick July 7, 2017 00:51
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.

LGTM

@swift-ci
Copy link
Contributor

swift-ci commented Jul 7, 2017

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 5af7650
Test requested by - @gottesmm

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

The Linux test failed due to an unrelated issue. Rerunning the test.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

@swift-ci test linux platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

Thanks Andy!

@gottesmm gottesmm added this to the Swift 4.0 milestone Jul 7, 2017
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

Explanation: The problem that we ran into here is that when deleting dead closures, we were not treating @in and @in_guaranteed closure arguments from an alloc_stack. In both cases, we would have leaked values.
Scope: We perform dead closure elimination during Mandatory Inlining. This means that the leaked values could cause leaks in SourceKit since SourceKit runs Mandatory Inlining.
Radar (and possibly SR Issue): rdar://32887993
Risk: Minimal
Testing: I added exhaustive tests for the various categories of arguments to a partial apply and whether or not they come from an alloc_stack or not.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

Mmm... made a mistake in my CCC. Sigh. I came up with the SourceKit thing at the last minute as I was typing. It was a mistake. Fixing the CCC.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

Explanation: The problem that we ran into here is that when deleting dead closures, we were not treating @in and @in_guaranteed closure arguments from an alloc_stack. In both cases, we would have leaked values.
Scope: This could cause leaks in user programs.
Radar (and possibly SR Issue): rdar://32887993
Risk: Minimal
Testing: I added exhaustive tests for the various categories of arguments to a partial apply and whether or not they come from an alloc_stack or not.

@gottesmm gottesmm changed the title [sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values. [4.0] [sil-combine] When deleting a dead partial_apply, insert a destroy for all non-trivial captured values. Jul 7, 2017
@gottesmm
Copy link
Contributor Author

gottesmm commented Jul 7, 2017

Approved by Bob via email. Merging!

@gottesmm gottesmm merged commit 34a5f03 into swiftlang:swift-4.0-branch Jul 7, 2017
@gottesmm gottesmm deleted the swift-4.0-branchrdar32887993 branch July 7, 2017 19:37
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