Skip to content

[4.0] SILGen: Ease off +0 peepholes for load exprs. #11043

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

Conversation

jckarter
Copy link
Contributor

Explanation: Due to stricter handling of lvalues, peepholes that would attempt to load an rvalue without retaining would lead to use-after-free bugs.

Scope: Regression from 3.1

Issue: rdar://problem/32730865

Risk: Low, removal of a peephole optimization, small risk of performance regressions

Testing: Swift CI, project from radar

Now that we more tightly close formal accesses on lvalues, having LoadExpr and friends try to return a +0 loaded value is unsafe without deeper analysis, since the access will be closed immediately after the load and potentially free temporary memory that might be the only thing keeping the borrowed object alive. Fixes rdar://problem/32730865.

(cherry picked from commit 7705393)
@jckarter
Copy link
Contributor Author

@swift-ci Please test

@jckarter
Copy link
Contributor Author

@swift-ci Please test source compatibility

@jckarter
Copy link
Contributor Author

@swift-ci Please benchmark

@jckarter jckarter changed the title SILGen: Ease off +0 peepholes for load exprs. [4.0] SILGen: Ease off +0 peepholes for load exprs. Jul 19, 2017
@swift-ci
Copy link
Contributor

Build comment file:

Build failed before running benchmark.


@jckarter
Copy link
Contributor Author

Benchmark build failed because it looks like there isn't result history for the 4.0 branch. The PR for master at #11026 has results that are probably in line with what we'd see.

@jckarter jckarter merged commit 451322d into swiftlang:swift-4.0-branch Jul 19, 2017
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.

2 participants