Skip to content

Fix findAccessedStorage to handle cyclic phis. #21665

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
merged 1 commit into from
Jan 8, 2019
Merged

Fix findAccessedStorage to handle cyclic phis. #21665

merged 1 commit into from
Jan 8, 2019

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Jan 6, 2019

Fixes infinite recursion in findAccessedStorge. This routine was not
designed to be recursive, but recursion was recently added as a hack
for address-type phis. Naturally, phis can be cyclic, so this was not
a correct workaround.

This is only a problem with enforce-exclusivity=checked, with which
findAccessedStorge is invoked after arbitrary optimization passes that
introduce address-type phis. Ultimately, address phis will be
disallowed in SIL, but some optimizer passes must be fixed first.

Fixes rdar://problem/47059671 swiftlang-1001.0.31.11 root: swiftc crashes

@atrick
Copy link
Contributor Author

atrick commented Jan 6, 2019

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Jan 6, 2019

@swift-ci test source compatibility.

@atrick atrick requested a review from shajrawi January 6, 2019 07:54
@atrick
Copy link
Contributor Author

atrick commented Jan 6, 2019

@shajrawi I'm still writing a unit test, but the code can be reviewed in the meantime.

@atrick
Copy link
Contributor Author

atrick commented Jan 7, 2019

@shajrawi I included the unit test.

@atrick
Copy link
Contributor Author

atrick commented Jan 7, 2019

@swift-ci smoke test.

Copy link

@shajrawi shajrawi left a comment

Choose a reason for hiding this comment

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

a couple of typos but overall LGTM!

namespace {
// The result of an accessed storage query. A complete result produces an
// AccessedStorage object, which may or may not be valid. An incomplete result
// produces an SILValue representing the source address for use with deeper
Copy link

Choose a reason for hiding this comment

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

:s/an SILValue/a SILValue/

// The result of an accessed storage query. A complete result produces an
// AccessedStorage object, which may or may not be valid. An incomplete result
// produces an SILValue representing the source address for use with deeper
// queries. The source address is not necessarilly a SIL address type because
Copy link

Choose a reason for hiding this comment

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

:s/necessarilly/necessarily/

return AccessedStorageResult::incomplete(operAddr);
}
return AccessedStorage();

Copy link

Choose a reason for hiding this comment

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

The following huge switch case works / I don't clearly see a better option, but I can see potential future bugs if someone adds new (relevant) SIL instructions / does not update it.

Fixes infinite recursion in findAccessedStorge. This routine was not
designed to be recursive, but recursion was recently added as a hack
for address-type phis. Naturally, phis can be cyclic, so this was not
a correct workaround.

This is only a problem with enforce-exclusivity=checked, with which
findAccessedStorge is invoked after arbitrary optimization passes that
introduce address-type phis. Ultimately, address phis will be
disallowed in SIL, but some optimizer passes must be fixed first.

Fixes <rdar://problem/47059671> swiftlang-1001.0.31.11 root: swiftc crashes
@atrick
Copy link
Contributor Author

atrick commented Jan 7, 2019

@swift-ci smoke test.

@atrick
Copy link
Contributor Author

atrick commented Jan 7, 2019

@swift-ci smoke test OS X platform.

@atrick atrick merged commit 8350d2b into swiftlang:master Jan 8, 2019
@atrick atrick deleted the fix-access-phi branch February 22, 2019 17:25
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