Skip to content

[4.0] [Exclusivity] Relax intra-procedural diagnostics on separate stored structs #10360

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

devincoughlin
Copy link
Contributor

This is a series of commits that relaxes the intra-procedural static enforcement of exclusive access so that we no longer diagnose on accesses to separate stored structs of the same property.

https://bugs.swift.org/browse/SR-5119
rdar://problem/31909639

This PR includes the AccessSummaryAnalysis, although the only thing it is used for in this PR is the shared IndexTrie to represent projection paths. This PR doesn't actually run any of the inter procedural closure analysis. That will be CCC'd to swift-4.0 in a different in a different PR.

Make helper functions static and avoid defining one except when assertions
are enabled.
Relax the static enforcement of exclusive access so that we no longer diagnose
on accesses to separate stored structs of the same property:

takesInout(&s.f1, &s.f2) // no-warning

And perform the analogous relaxation for tuple elements.

To do this, track for each begin_access the projection path from that
access and record the read and write-like modifications on a per-subpath
basis.

We still warn if the there are conflicting accesses on subpaths where one is
the prefix of another.

This commit leaves the diagnostic text in a not-so-good shape since we refer
to the DescriptiveDeclKind of the access even describing a subpath.

I'll fix that up in a later commit that changes only diagnostic text.

https://bugs.swift.org/browse/SR-5119
rdar://problem/31909639
Remove the descriptive decl kind (since with subpaths it is not correct and
cannot represent a tuple element) and change "simultaneous" to "overlapping"
in order to lower the register slightly and avoid connoting threading.

For example, for the following:

   takesTwoInouts(&x.f, &x.f)

the diagnostic will change from

"simultaneous accesses to var 'x.f', but modification requires exclusive access;
consider copying to a local variable"

to

"overlapping accesses to 'x.f', but modification requires exclusive access;
consider copying to a local variable"
Move IndexTrieNode from DeadObjectElimination into its own header. I plan to
use this data structure when diagnosing static violations of exclusive access.
…le args

Add an interprocedural SIL analysis pass that summarizes the accesses that
closures make on their @inout_aliasable captures. This will be used to
statically enforce exclusivity for calls to functions that take noescape
closures.

The analysis summarizes the accesses on each argument independently and
uses the BottomUpIPAnalysis utility class to iterate to a fixed point when
there are cycles in the call graph.

For now, the analysis is not stored-property-sensitive -- that will come in a
later commit.
…ectionPath

IndexTrie is a more light-weight representation and it works well in this case.
This requires recovering the represented sequence from an IndexTrieNode, so
also add a getParent() method.
@devincoughlin devincoughlin changed the title Exclusivity relax separate stored struct 4.0 [4.0] [Exclusivity] Relax intra-procedural diagnostics on separate stored structs Jun 18, 2017
@devincoughlin
Copy link
Contributor Author

@atrick Would you review for CCC? This is all code you have seen before. The only change on cherry-picking was in 6fa2f76 to resolve a merge conflict in diagnoseExclusivityViolation() because of @ahoppen's work to remove getName() on ValueDecl

@devincoughlin
Copy link
Contributor Author

@swift-ci Please test

@devincoughlin
Copy link
Contributor Author

@swift-ci Please test source compatibility

@devincoughlin devincoughlin requested a review from atrick June 18, 2017 08:32
@devincoughlin
Copy link
Contributor Author

Note: The failures on the compatibility suite are unrelated to this PR.

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Looks good for CCC.
Thanks!

@devincoughlin
Copy link
Contributor Author

CCC Information:
Explanation:
Relax static enforcement of exclusive access to allow overlapping access to
separate stored properties of the same struct without a static diagnostic.
For example, with this change static enforcement wil not complaing about:

swap(&s.f, &s.g)

where s is a struct with stored properties f and g.

Scope: This is a persistent source of false positives in the static enforcement.
It is most common in code that calls C functions taking multiple arguments
with inout-to-pointer conversions. It also affects some idioms that use the
new String APIs.

Radar: rdar://problem/31909639
This is also tracked by https://bugs.swift.org/browse/SR-5119

Risk: This is a relatively low risk change. It causes the compiler to warn in
fewer places than it did before.

Testing: We've added the usual regression tests and also confirmed that the
diagnostics remove the issues it previously reported on the Standard Library
and in the Source Compatibility Suite.

@tkremenek tkremenek merged commit 4834dcc into swiftlang:swift-4.0-branch Jun 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.

3 participants