CONTEXT: This code works by building up a stack of SIL values of values that
need to be transformed into a StringRef as part of generating our name path and
then as a second phase performs the conversion of those values to StringRef as
we pop from the stack.
This is the first in a string of commits that are going to refactor
VariableNameUtils so that the stack will only contain StringRef instead of SIL
entities. This will be accomplished by moving the SIL value -> StringRef code
from the combining part of the algorithm (where we drain the stack) to the
construction of the stack.
The reason why I am doing this is two fold:
1. By just storing StringRef into the stack I am simplifying the code. Today as
mentioned above in the context, we gather up the SILValue we want to process and
then just convert them to StringRef. This means that any time one has to add a
new instruction, one has to update two different pieces of code. By trafficking
in StringRef instead, one only has to update one piece of code.
2. I want to add some simple code that allows for us to get names from closures
which would require me to recurse. I am nervous about putting
values/instructions from different functions in the same data structure. Today
it is safe, but it is bad practice. Instead, by just using StringRef in the
stack, I can avoid this problem.
The reason that I am changing this code is that getWithIsolationCrossing is a
bad API that was being used to infer actor isolation straight from an ApplyExpr
without adding an actor instance. This can cause us to reject programs
unnecessarily if we in other parts of the code correctly infer the SILValue
actor instance for the isolation.
Rather than allow for that, I am removing this code and I improved the rest of
the pattern matching here to ensure that we handled that with the normal actor
instance inferring code. This will prevent this type of mismerge from happening
by mistake. I fixed up the changes in the test cases.
The only usage of this left is for ApplyIsolationCrossings parsed straight from
SIL that we use only when testing. This is safe since if a test writer is using
the parsed SIL in this manner, they can make sure that mismerges do not happen.
In this part of the code, we are attempting to merge all of the operands into
the same region and then assigning all non-Sendable results of the function to
that same region. The problem that was occuring here was a thinko due to the
control flow of the code here not separating nicely the case of whether or not
we had operands or not. Previously this did not matter, since we just used the
first result in such a case... but since we changed to assign to the first
operand element in some cases, it matters now. To fix this, I split the confused
logic into two different easy to follow control paths... one if we have operands
and one where we do not have an operand. In the case where we have a first
operand, we merge our elements into its region. If we do not have any operands,
then we just perform one large region assign fresh.
This was not exposed by code that used non-coroutines since in SIL only
coroutines today have multiple results.
rdar://132767643
The main changes are:
*) Rewrite everything in swift. So far, parts of memory-behavior analysis were already implemented in swift. Now everything is done in swift and lives in `AliasAnalysis.swift`. This is a big code simplification.
*) Support many more instructions in the memory-behavior analysis - especially OSSA instructions, like `begin_borrow`, `end_borrow`, `store_borrow`, `load_borrow`. The computation of end_borrow effects is now much more precise. Also, partial_apply is now handled more precisely.
*) Simplify and reduce type-based alias analysis (TBAA). The complexity of the old TBAA comes from old days where the language and SIL didn't have strict aliasing and exclusivity rules (e.g. for inout arguments). Now TBAA is only needed for code using unsafe pointers. The new TBAA handles this - and not more. Note that TBAA for classes is already done in `AccessBase.isDistinct`.
*) Handle aliasing in `begin_access [modify]` scopes. We already supported truly immutable scopes like `begin_access [read]` or `ref_element_addr [immutable]`. For `begin_access [modify]` we know that there are no other reads or writes to the access-address within the scope.
*) Don't cache memory-behavior results. It turned out that the hit-miss rate was pretty bad (~ 1:7). The overhead of the cache lookup took as long as recomputing the memory behavior.
Use the more precise areUsesWithinBoundary API (which takes dead-end
blocks into account). This requires first updating liveness with the
newly created destroys.
Just clear all structures in a single method which is called wherever
clearing is done. Fixes a failure to clear discoveredBlocks under
certain circumstances.
The unittests for PartitionUtils pass in mocked operands and instructions that
cannot be dereferenced. Adding this static CRTP helper allows for the unittest
PartitionOpEvaluator subclass to just return false for it instead of
dereferencing operands or instructions. The rest of the evaluators just get to
use the default "normal" implementation that actually accesses program state.
This will let me know the exact source operand used instead of the source value
representative. This will ensure that the name associated with the diagnostic is
not of the representative value, but the actual value that was the source of the
assign.
This is an NFCI commit that is an algebraic refactor.
This is just moving up the declaration in the chain of dependencies so that I
can write logic in PartitionUtils.h using it. I also added entrypoints to lookup
the ReprensetativeValue for our various emitters.
Closures generally only inherit actor instance isolation if they directly
capture state from the actor instance. In this case, for some reason that is not
true, so we hit an assert that assumes that we will only see a global actor
isolated isolation.
Region Isolation should be able to handle code even if the closure isolation
invariant is violated by the frontend. So to do this, I am introducing a new
singleton actor instance to represent the isolation of a defer or closure
created in an actor instance isolated method. The reason why I am using a
singleton is that closures and defer are not methods so we do not actually know
which parameter is 'self' since it isn't in the abi. But we still need some
value to represent the captured values as belonging to. To square this circle, I
just did what we have done in a similar situation where we did not have a value:
(ActorAccessorInit). In that case, we just use a sentinel to represent the
instance (NOTE: This is represented just via a kind so ActorInstances that are
operator== equal will not &value equal since we are just using a kind).
We are already using this routine in other parts of TransferNonSendable to
ensure that we look through common insts that SILGen inserts that do not change
the actual underlying actor instance that we are using. In this case, I added
support for casts, optional formation, optional extraction, existential ref
initialization.
As an example of where this came up is the following test case where we fail to
look through an init_existential_ref.
```swift
public actor MyActor {
private var intDict: [Int: Int] = [:]
public func test() async {
await withTaskGroup(of: Void.self) { taskGroup in
for (_, _) in intDict {}
await taskGroup.waitForAll() // Isolation merge failure happens here
}
}
}
```
I also added the ability to at the SIL level actual test out this merge
condition using the analysis test runner. I used this to validate that this
functionality works as expected in a precise way.
rdar://130113744
Before we wouldn't print them in all situations and even more so a few of the
printing routines did not have it at all. This just adds a centralized
SILIsolationInfo::dumpOptions() method and then goes through all of the printing
helpers and changes them to use them as appropriate.
Given a function or a partial_apply with an isolated parameter, we do not know
immediately what the actual isolation is of the function or partial_apply since
we do not know which instance will be applied to the function or partial_apply.
In this commit, I introduce a new bit into SILIsolationInfo that tracks this
information upon construction and allows for it to merge with ownership that has
the appropriate type and a specific instance. Since the values that created the
two isolations, will be in the same region this should ensure that the value is
only ever in a flow sensitive manner in a region with only one actor instance
(since regions with isolations with differing actor instances are illegal).
Specifically:
1. We error now if one transfers an 'inout sending' parameter and does not
reinitialize it before the end of the function.
2. We error now if one merges an 'inout sending' parameter into an actor
isolated region and do not reinitialize it with a non-actor isolated value
before the end of the function.
rdar://126303739
This asserts only option is an option to make it quicker/easier to triage
unknown pattern match errors by aborting when we emit it (allowing one to
immediately drop into the debugger at that point).
Previously, it only happened for errors in RegionAnalysis not in
TransferNonSendable itself.
From the perspective of the IR, we are changing SILIsolationInfo such that
inferring an actor instance means looking at equivalence classes of values where
we consider operands to look through instructions to be equivalent to their dest
value. The result is that cases where the IR maybe puts in a copy_value or the
like, we consider the copy_value to have the same isolation info as using the
actor directly. This prevents a class of crashes due to merge failings. Example:
```swift
actor MyActor {
init() async {
init(ns: NonSendableKlass) async {
self.k = NonSendableKlass()
self.helper(ns)
}
func helper(_ newK: NonSendableKlass) {}
}
```
Incidently, we already had a failing test case from this behavior rather than
the one that was the original genesis. Specifically:
1. If a function's SILIsolationInfo is the same as the isolation info of a
SILValue, we assume that no transfer actually occurs.
2. Since we were taking too static of a view of actor instances when comparing,
we would think that a SILIsolationInfo of a #isolation parameter to as an
argument would be different than the ambient's function isolation which is also
that same one. So we would emit a transfer non transferrable error if we pass in
any parameters of the ambient function into another isolated function. Example:
```swift
actor Test {
@TaskLocal static var local: Int?
func withTaskLocal(isolation: isolated (any Actor)? = #isolation,
_ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async {
Self.$local.withValue(12) {
// We used to get these errors here since we thought that body's isolation
// was different than the body's isolation.
//
// warning: sending 'body' risks causing data races
// note: actor-isolated 'body' is captured by a actor-isolated closure...
body(NonSendableValue(), isolation)
}
}
}
```
rdar://129400019
This is usually the case. Some examples, where they layout is _not_ dependent:
```
struct S<T> {
var x: Int // no members which depend on T
}
struct S<T> {
var c: SomeClass<T> // a class reference does not depend on the layout of the class
}
```
We view the conversion from a Sendable to a non-Sendable function via
convert function to produce a new fresh sendable value. We should
squelch that error.
TLDR:
The reason why I am doing this is it ensures that temporary store_borrow that we
create when materializing a value before were treated as uses. So we would error
on this:
```swift
@MainActor func transferToMain<T>(_ t: T) async {}
func test() async {
let x = NonSendableKlass()
await transferToMain(x)
await transferToMain(x)
}
```
----
store_borrow is an instruction intended to be used to initialize temporary
alloc_stack with borrows. Since it is a temporary, we do not want to error on
the temporaries initialization... instead, we want to error on the use of the
temporary parameter.
This is achieved by making it so that store_borrow still performs an
assign/merge, but does not require that src/dest be alive. So the regions still
merge (yielding diagnostics for later uses).
It also required me to make it so that PartitionOp::{Assign,Merge} do not
require by default. Instead, we want the individual operations to always emit a
PartitionOp::Require explicitly (which they already did).
One thing to be aware of is that when it comes to diagnostics, we already know
how to find a temporaries original value and how to handle that. So this is the
last part of making store_borrow behave nicely.
rdar://129237675
Enhance the utility with the ability to end lifetimes of lexical values
at indicated instructions, overriding the usual behavior of maintaining
such lifetimes' previous endpoints (modulo non-deinit-barrier
instructions).
Parameterized `extendUnconsumedLiveness` on the ends of interest and the
action to take when visiting the extended boundary and named the
resulting function `visitExtendedUnconsumedBoundary`.
