Skip to content

Enable strict verification of begin_access patterns in all SIL passes. #17534

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 2 commits into from
Jun 28, 2018
Merged

Enable strict verification of begin_access patterns in all SIL passes. #17534

merged 2 commits into from
Jun 28, 2018

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Jun 26, 2018

This also adds handling additional cases like global addressors.

AccessedStorage now properly represents access to global variables, even if they
haven't been fully optimized down to global_addr instructions.

This is essential for optimizing dynamic exclusivity checks. As a
verified SIL property, all access to globals and class properties
needs to be identifiable.
@atrick
Copy link
Contributor Author

atrick commented Jun 26, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Jun 26, 2018

@swift-ci source compatibility test.

@atrick
Copy link
Contributor Author

atrick commented Jun 27, 2018

@swift-ci test source compatibility.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 36566b3ee4c5e5649b2b126a77c525636d00c19e

Ensure that all formal access follows recognizable patterns
at all points in the SIL pipeline.

This is important to run acccess enforcement optimization late in the pipeline.
@atrick
Copy link
Contributor Author

atrick commented Jun 28, 2018

@swift-ci test.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 36566b3ee4c5e5649b2b126a77c525636d00c19e

@atrick
Copy link
Contributor Author

atrick commented Jun 28, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Jun 28, 2018

@shajrawi, @devincoughlin, @rjmccall. These changes were reviewed in an earlier PR. The only thing that changed since then is that DiagnoseStaticExclusivity now ignores access markers with "unsafe" enforcement. Unsafe accesses are not required to be from identifiable addresses. e.g. in lldb we simply load from a RawPointer. That can't be allowed with enforced access.

This works around failures caused by the original PR, which prevented DiagnoseStaticExclusivity from bailing out on unrecognized patterns. This pass, along with any optimistic exclusivity optimizations, must be sound. They cannot allow for any unknown or unrecognized access patterns. Otherwise future changes to SIL could weaken exclusivity with no way to catch the problem.

@atrick atrick merged commit 8d41d6e into swiftlang:master Jun 28, 2018
@rjmccall
Copy link
Contributor

This makes sense to me, but I don't understand how it's not causing major problems with addressors and materializeForSet, since we don't emit begin_access [unsafe] after the pointer_to_address instructions in them.

@atrick
Copy link
Contributor Author

atrick commented Jun 28, 2018

@rjmccall Yeah, it's a problem if we inline nested static access on an inout.
But I think your fix takes care of that nicely.
67fe633

@rjmccall
Copy link
Contributor

Okay. Do you want to take that and actually do the test updates? I just wrote it up to unblock work on my PR, which doesn't have a known timeline yet.

@atrick
Copy link
Contributor Author

atrick commented Jun 28, 2018

I already stole your commit and disabled the SIL verifier check on trunk. So I'll be pushing your commit before reenabling the verification.

@atrick atrick deleted the accessverify branch February 22, 2019 17:40
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