The problem with `is_escaping_closure` was that it didn't consume its operand and therefore reference count checks were unreliable.
For example, copy-propagation could break it.
As this instruction was always used together with an immediately following `destroy_value` of the closure, it makes sense to combine both into a `destroy_not_escaped_closure`.
It
1. checks the reference count and returns true if it is 1
2. consumes and destroys the operand
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
Fixes rdar://128981120 (Crash when inout arg captured through some
closures? (llvm::all_of(apply->getUses(), hasExpectedUsesOfNoEscapePartialApply)
&& "noescape partial_apply has unexpected use!"))
I was originally hoping to reuse mark_must_check for multiple types of checkers.
In practice, this is not what happened... so giving it a name specifically to do
with non copyable types makes more sense and makes the code clearer.
Just a pure rename.
It is legal for a `move_value` to appear as a transitive user of a
`partial_apply`. Look through those like the other ownership
instructions.
rdar://107963619
Although nonescaping closures are representationally trivial pointers to their
on-stack context, it is useful to model them as borrowing their captures, which
allows for checking correct use of move-only values across the closure, and
lets us model the lifetime dependence between a closure and its captures without
an ad-hoc web of `mark_dependence` instructions.
During ownership elimination, We eliminate copy/destroy_value instructions and
end the partial_apply's lifetime with an explicit dealloc_stack as before,
for compatibility with existing IRGen and non-OSSA aware passes.
Handle recursive non-escaping local functions.
Previously, it was thought that recursion would force a closure to be
escaping. This is not necessarilly true.
Update AccessEnforcementSelection to conservatively handle closure cycles.
Fixes rdar://88726092 (Compiler hangs when building)
This patch replace all in-memory objects of DebugValueAddrInst with
DebugValueInst + op_deref, and duplicates logics that handles
DebugValueAddrInst with the latter. All related check in the tests
have been updated as well.
Note that this patch neither remove the DebugValueAddrInst class nor
remove `debug_value_addr` syntax in the test inputs.
An error found in DiagnoseInvalidEscapingCaptures can indicate invalid
SIL, which will cause DiagnoseStaticExclusivity to assert during SIL
verification. When the source-level closure captures an inout
argument, it appears in SIL to be a non-escaping closure. The SIL
verification then fails because the "nonescaping" closure actually
escapes.
Ensure that capture diagnostics run on closures before exclusivity
enforcement runs on the parent function. Bypass the SIL verification
assert if a diagnostic error was found.
Fixes rdar://75364904 (Crash with assertion `noescape partial_apply
has unexpected use`)
* add is_escaping_closure as allowed instruction
* don't restrict the uses of copy_value to be a store
Fixes a verification crash (only in assert builds).
https://bugs.swift.org/browse/SR-14394
rdar://75740622
...and avoid reallocation.
This is immediately necessary for LICM, in addition to its current
uses. I suspect this could be used by many passes that work with
addresses. RLE/DSE should absolutely migrate to it.
Potentially source breaking: SR-11700 Diagnose exclusivity violations
with Dictionary.subscript._modify:
Exclusivity violations within code that computes the `default`
argument during Dictionary access are now diagnosed.
```swift
struct Container {
static let defaultKey = 0
var dictionary = [defaultKey:0]
mutating func incrementValue(at key: Int) {
dictionary[key, default: dictionary[Container.defaultKey]!] += 1
}
}
error: overlapping accesses to 'self.dictionary', but modification requires exclusive access; consider copying to a local variable
dictionary[key, default: dictionary[Container.defaultKey]!] += 1
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: conflicting access is here
dictionary[key, default: dictionary[Container.defaultKey]!] += 1
~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
```
This reworks the logic so that four problems end up being fixed:
Fixes three problems related to coroutines:
(1) DiagnoseStaticExclusivity must consider begin_apply as a user of accessed variables. This was an undefined behavior hole in the diagnostics.
(2) AccessedSummaryAnalysis should consider begin_apply as a user of accessed arguments. This does not show up in practice because coroutines don't capture things.
(3) AccessedSummaryAnalysis must consider begin_apply a valid user of
noescape closures.
And fixes one problem related to resilience:
(4) AccessedSummaryAnalysis must conservatively consider arguments to external functions.
Fixes <rdar://problem/56378713> Investigate why AccessSummaryAnalysis is crashing
This improves on the previous situation:
- The request ensures that the backing storage for lazy properties
and property wrappers gets synthesized first; previously it was
only somewhat guaranteed by callers.
- Instead of returning a range this just returns an ArrayRef,
which simplifies clients.
- Indexing into the ArrayRef is O(1), which addresses some FIXMEs
in the SIL optimizer.
* Teach findAccessedStorage about 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.
* Add stronger SILVerifier support for formal access.
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.
The compile-time exclusivity diagnostics explicitly allow conflicting accesses
to a struct when it can prove that the two accesses are used to project addresses
for separate stored properties. Unfortunately, the logic that detects this special
case gets confused by Thread Sanitizer's SIL-level instrumentation. This causes
the exclusivity diagnostics to have false positives when TSan is enabled.
To fix this, teach the AccessSummaryAnalysis to ignore TSan builtins when
determining whether an access has a single projected subpath.
rdar://problem/40455335
The pattern we see for noescape closure passed to objective c is different:
There is the additional without actually escaping closure sentinel.
rdar://39682865
introduce a common superclass, SILNode.
This is in preparation for allowing instructions to have multiple
results. It is also a somewhat more elegant representation for
instructions that have zero results. Instructions that are known
to have exactly one result inherit from a class, SingleValueInstruction,
that subclasses both ValueBase and SILInstruction. Some care must be
taken when working with SILNode pointers and testing for equality;
please see the comment on SILNode for more information.
A number of SIL passes needed to be updated in order to handle this
new distinction between SIL values and SIL instructions.
Note that the SIL parser is now stricter about not trying to assign
a result value from an instruction (like 'return' or 'strong_retain')
that does not produce any.
The etymology of these terms isn't about race, but "black" = "blocked"
and "white" = "allowed" isn't really a good look these days. In most
cases we weren't using these terms particularly precisely anyway, so
the rephrasing is actually an improvement.
Make the static enforcement of accesses in noescape closures stored-property
sensitive. This will relax the existing enforcement so that the following is
not diagnosed:
struct MyStruct {
var x = X()
var y = Y()
mutating
func foo() {
x.mutatesAndTakesClosure() {
_ = y.read() // no-warning
}
}
}
To do this, update the access summary analysis to summarize accesses to
subpaths of a capture.
rdar://problem/32987932
Make the static enforcement of accesses in noescape closures stored-property
sensitive. This will relax the existing enforcement so that the following is
not diagnosed:
struct MyStruct {
var x = X()
var y = Y()
mutating
func foo() {
x.mutatesAndTakesClosure() {
_ = y.read()
}
}
}
To do this, update the access summary analysis to be stored-property sensitive.
rdar://problem/32987932
I’m totally disabling this assert in 4.0. Tracking down a compiler crash for
each obscure case of missing access marker is not a good use of time since we’re
not actually fixing these cases when we find them anyway. Instead, post 4.0, we
will catch all of these missing cases by implementing strong SIL
verification. Much more SILGen work is really required to fully implement
exclusivity diagnostics. SIL verification will be necessary to drive that.
Fixes <rdar://problem/33024357> AccessSummaryAnalysis assert "Unrecognized
argument use" building source compatibility siesta project.
Mark Lacey caught this bug in the assertion logic that looks for expected SIL
patterns involving non-escaping closures. I broadened it to allow multiple use,
but it was only verifying the first use. This is NFC w.r.t. our automated
testing because that never hits the multiple-use case.
This is a same-day fix for a typo introduced here:
commit c2c55eea12
Author: Andrew Trick <atrick@apple.com>
Date: Wed Jun 21 16:08:06 2017
AccessSummaryAnalysis: handle @convention(block) in nested nonescape closures.
This analysis has a whitelist to ensure that we aren't missing any SIL
patterns and failing to enforce some cases.
There is a special case involving nested non-escaping closures being passed as a
block argument. Whitelist this very special case even though we don't enforce
it because the corresponding diagnostics pass also doesn't enforce it.
Use the AccessSummaryAnalysis to statically enforce exclusive access for
noescape closures passed as arguments to functions.
We will now diagnose when a function is passed a noescape closure that begins
an access on capture when that same capture already has a conflicting access
in progress at the time the function is applied.
The interprocedural analysis is not yet stored-property sensitive (unlike the
intraprocedural analysis), so this will report violations on accesses to
separate stored properties of the same struct.
rdar://problem/32020710
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.
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.
