Skip to content

Fix for memory leak when partially applying function with @autoreleased return value #1967

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

slavapestov
Copy link
Contributor

This is a backport of the fix for rdar://problem/24805609 to Swift 2.2.

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@jckarter and @jrose-apple Do you mind reviewing this?

@slavapestov slavapestov added this to the Swift 2.2.x milestone Mar 31, 2016
@jrose-apple
Copy link
Contributor

Your test case doesn't have any partial application in it, does it?

@slavapestov
Copy link
Contributor Author

Unfortunately, it does. Dynamic method calls look up the method, partially apply self, wrap it in an optional, then destructure the optional :)

@slavapestov
Copy link
Contributor Author

I guess I should cook up a SILGen test, though...

@tkremenek
Copy link
Member

@slavapestov do you plan on incorporating a SILGen test into this pull request?

@slavapestov
Copy link
Contributor Author

@tkremenek That sounds like a good idea. I'll cook something up now.

slavapestov and others added 3 commits March 30, 2016 23:09
…d, convert @autoreleased result to @owned

In IRGen, @autoreleased return values are always converted to +1 by
calling objc_retainAutoreleasedReturnValue(), so a partial application
thunk cannot have a result with @autoreleased convention. Just turn
it into @owned instead, since that's what it is, using similar logic
as the @unowned_inner_pointer => @unowned case.

Fixes <rdar://problem/24805609>.
We do clever tricks to avoid having to serialize the full type of a
partial_apply instruction, but those conflicted with our clever tricks
for handling unusual return conventions. Make sure we do all type
calculations for partial_apply in one place. (Credit to Slava for
both introducing the problem and immediately seeing how to fix it.)

This should allow re-application of e6a519f and 86dcce1.
@slavapestov slavapestov force-pushed the partial-apply-fix-2.2 branch from c323ccd to 2d29049 Compare March 31, 2016 06:09
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@tkremenek tkremenek merged commit eda3c8d into swiftlang:swift-2.2-branch Mar 31, 2016
@slavapestov slavapestov deleted the partial-apply-fix-2.2 branch April 5, 2016 07:41
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