Skip to content

[temp-rvalue-opt] Disable handling of load [take] in temp-rvalue-elim. #34327

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 Oct 15, 2020

This pass was attempting to handle cases where we use load [take] +
destroy_value instead of a destroy_addr since we have a loadable value. There
are a few issues around how this was done, so for 5.3, we are just going to turn
off the optimization.

rdar://70356725


CCC

Explanation: This patch turns off an optimization in
TempRValueOpt that could cause miscompiles such as
leaks/use-after-frees. Note that we haven't seen this in practice
from actual Swift programs, we have only seen it in SIL code
snippets. So we are disabling this out of an abundance of
caution.

Scope: This miscompile could result in memory unsafety.

SR Issue: rdar://70356725

Risk: None. This is just turning off the optimization using a 2
line fix. We are going to do a fuller fix on main.

Testing: Added compiler tests that validate the optimization has
been disabled.

Reviewer: @eeckstein

@gottesmm gottesmm requested a review from a team as a code owner October 15, 2020 23:22
@gottesmm
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 275dc426ef38c171aa00ee32fd5c0e6712237a1b

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 275dc426ef38c171aa00ee32fd5c0e6712237a1b

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

You have to update the temp_rvalue_opt_ossa.sil test

This pass was attempting to handle cases where we use load [take] +
destroy_value instead of a destroy_addr since we have a loadable value. There
are a few issues around how this was done, so for 5.3, we are just going to turn
off the optimization.

rdar://70356725
@gottesmm gottesmm force-pushed the release/5.3/rdar70356725 branch from 275dc42 to d0b0600 Compare October 19, 2020 19:30
@gottesmm
Copy link
Contributor Author

@swift-ci test

1 similar comment
@gottesmm
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

LGTM!

@gottesmm
Copy link
Contributor Author

@swift-ci nominate

@tkremenek tkremenek merged commit aee95e6 into swiftlang:release/5.3-20201012 Oct 21, 2020
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.

4 participants