We package all isolation history nodes from a single instruction by placing a
sequence boundary at the bottom. When ever we pop, we actually pop a PartitionOp
at a time meaning that we pop until we see a SequenceBoundary. Thus the sequence
boundary will always be the last element visited when popping meaning that it is
a convenient place to stick the SILLocation associated with the entire
PartitionOp. As a benefit, there was some unused space in IsolationHistory::Node
for that case since we were not using the std::variant field at all.
This means that I added an IsolationHistory field to Partition. Just upstreaming
the beginning part of this work. I added some unittests to exercise the code as
well. NOTE: This means that I did need to begin tracking an
IsolationHistoryFactory and propagating IsolationHistory in the pass
itself... but we do not use it for anything.
A quick overview of the design.
IsolationHistory is the head of an immutable directed acyclic graph. It is
actually represented as an immutable linked list with a special node that ties
in extra children nodes. The users of the information are expected to get a
SmallVectorImpl and process those sibling nodes afterwards. The reason why we
use an immutable approach is that it fits well with the problem and saves space
since different partitions could be pointing at the same linked list
node. Operations occur on an isolation history by pushing/popping nodes. It is
assumed that the user will push nodes in batches with a sequence boundary at the
bottom of the addition which signals to stop processing nodes.
Tieing this together, each Partition within it contains an IsolationHistory. As
the PartitionOpEvaluator applies PartitionOps to Partition in
PartitionOpEvaluator::apply, the evaluator also updates the isolation history in
the partition by first pushing a SequenceBoundary node and then pushing nodes
that will undo the operation that it is performing. This information is used by
the method Partition::popHistory. This pops linked list nodes from its history,
performing the operation in reverse until it hits a SequenceBoundary node.
This allows for one to rewind Partition history. And if one stashes an isolation
history as a target, one can even unwind a partition to what its state was at a
specific transfer point or earlier. Once we are at that point, we can begin
going one node back at a time and see when two values that we are searching for
no longer are apart of the same region. That is a place where we want to emit a
diagnostic. We then process until we find for both of our values history points
where they were the immediate reason why the two regions merge.
rdar://123479934
This should be NFC since the only case where I used this was with self... and I
found another way of doing that using the API I added in the previous commit.
I fixed a bunch of small issues around here that resulted in a bunch of radars
being fixed. Specifically:
1. I made it so that we treat function_refs that are from an actor isolated
function as actor isolated instead of sendable.
2. I made it so that autoclosures which return global actor isolated functions
are treated as producing a global actor isolated function.
3. I made it so that we properly handle SILGen code patterns produced by
Sendable GlobalActor isolated things.
rdar://125452372
rdar://121954871
rdar://121955895
rdar://122692698
ActorIsolation already has a "I have no value case": unspecified. Lets just use
that.
Just a mistake I made that I am trying to fix before anything further depends on
this code.
To squelch errors, we need access to functionality not available in the
unittests. The unittests do not require this functionality anyways, so just
disable squelching during the unittests.
To be more specific this means that either:
1. The use is actually isolated to the same actor. This could mean that the
use is global actor isolated to the same function.
2. The use is nonisolated but is executing within a function that is globally
isolated to the same isolation domain.
rdar://123474616
This issue can come up when a value is initially statically disconnected, but
after we performed dataflow, we discovered that it was actually actor isolated
at the transfer point, implying that we are not actually transferring.
Example:
```swift
@MainActor func testGlobalAndGlobalIsolatedPartialApplyMatch2() {
var ns = (NonSendableKlass(), NonSendableKlass())
// Regions: (ns.0, ns.1), {(mainActorIsolatedGlobal), @MainActor}
ns.0 = mainActorIsolatedGlobal
// Regions: {(ns.0, ns.1, mainActorIsolatedGlobal), @MainActor}
// This is not a transfer since ns is already main actor isolated.
let _ = { @MainActor in
print(ns)
}
useValue(ns)
}
```
To do this, I also added to SILFunction an actor isolation that SILGen puts on
the SILFunction during pre function visitation. We don't print it or serialize
it for now.
rdar://123474616
Just making PartitionUtils.h a little easier to walk through by moving more of
the impl into the .cpp file. This reduces the header from ~1500 lines to ~950
lines which is more managable. This is especially important since I am going
to be adding IsolationHistory to the header file which will expand it even
further.
As an example of the change:
- // expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
+ // expected-note @-1 {{transferring disconnected 'x' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
Part of the reason I am doing this is that I am going to be ensuring that we
handle a bunch more cases and I wanted to fix this diagnostic before I added
more incaranations of it to the tests.
Long term I would like to get region analysis and transfer non sendable out of
the business of directly interpreting the AST... but if we have to do it now, I
would rather us do it through a helper struct. At least the helper struct can be
modified later to work with additional SIL concurrency support when it is added.
This is in preparation for beginning to track an IsolationRegionInfo in
TransferringOperand.
Just splitting this into a separate commit to make it easier to read.
I need to start tracking the dynamic IsolationRegionInfo for the transferring
operand so I can ignore uses that are part of the same
IsolationRegionInfo. IsolationRegionInfo doesn't fit into a pointer, so just to
keep things the same, I am going to just allocate it.
This is an initial staging commit that tests out the bump ptr allocating without
expanding the type yet.
I also eliminated the very basic "why is this task isolated" part of the warning
in favor of the larger, better, region history impl that I am going to land
soon. The diagnostic wasn't that good in the first place and also was fiddly. So
I removed it for now.
rdar://124960994
I am going to need this so that I can use it when evaluating partition
ops. Specifically, so I can when I find two values that do not merge correctly,
emit an error.
Now that we actually know the region that non transferrable things belong to, we
can use this information to give a better diagnostic here.
A really nice effect of this is that we now emit that actor isolated parameters
are actually actor isolated instead of task isolated.
In a subsequent commit, this is going to let me begin handling parameters with
actor regions in a nice way (and standardize all of the errors).
This is meant to be a refactoring commit that uses the current tests in tree to
make sure I did it correctly, so no tests need to be updated.
This involved fixing a few different bugs.
1. We were just performing dataflow by setting that only the initial block needs
to be updated. This means that if there isn’t anything in the initial dataflow
block, we won’t visit any successor blocks. Instead the correct thing to do here
is to visit all blocks in the initial round.
2. I also needed to fix a separate issue where we were updating our union-find
data structure incorrectly as found by an assert on transfernonsendable.swift
that was triggered once I fixed 1. Put simply, we needed to set a new max label
+ 1 when our new max element is less than or equal to the old max label + 1…
before we just did less than so if we had a new max element that is the same as
our fresh label, we wouldn’t increment the fresh label.
rdar://119584497
I left them as friends since that was in the original code. There isn't a reason
to do this and break the encapsulation of Partition. I just added reasonable
helpers that give PartitionOpEvaluator all of the functionality it needs.
Previously I avoided doing this since the only problem would be that in a case
where we had two transfer instructions that were in an if-else block, we would
just emit an error for one:
```swift
if boolValue {
transfer(x)
} else {
transfer(x) // Only emit error for this transfer!
}
useValue(x)
```
Now that we are tracking at the transfer point if any element in the transfer
was captured in a closure, this becomes an actual semantic issue since if we
track the transfer instruction that isn't reachable from the closure capture, we
will not become more pessimistic:
```swift
if boolValue {
closure = { useInOut(&x) }
transfer(x)
} else {
transfer(x)
}
// Since we grab from the else block, sendableField is allowed to be accessed
// since we do not track that x was captured by reference in a closure.
x.sendableField
useValue(x)
```
To be truly safe, we need to emit both errors.
rdar://119048779
If the var is captured in a closure before it is transferred, it is not safe to
access the Sendable field since we may race on accessing the field with an
assignment to the field in another concurrency domain.
rdar://115124361
I am not sure if this would ever actually ever cause a bug... but we should be
consistent. The issue here is that in the evaluator switch when processing
transfers in certain cases when we emitted an early error due to an actor
derived value we were performing a transfer and other times we weren't. With
this patch:
1. We now consistently do not actually transfer the value if it is actor
derived. It is always illegal to transfer an actor derived value... so we know
that if we always just error on transferring it, we have a safe model.
2. The switch we breaking so that we could do an assert that we canonicalized
the modified PartitionOp. Rather than do that and allow for people to
potentially return, I moved the assert into a SWIFT_DEFER and added an assert in
the continuation of the switch. This means that if an author ever breaks out of
the switch, they will hit the assert implying that the author in all cases must
explicitly return within the case statement guaranteeing this mistake does not
happen again.
Specifically:
1. If the value is transferred such that it becomes part of an actor region, the
value is permanently part of the actor region as one would normally have.
2. If the value is just used in an async let or is used by a nonisolated async
function within the async let then while the async let is alive it cannot be
used. But once the async let has been awaited upon, we allow for it to be used
again.
rdar://117506395
This is another NFC refactor in preparation for changing how we emit
errors. Specifically, we need access to not only the instruction, but also the
specific operand that the transfer occurs at. This ensures that we can look up
the specific type information later when we emit an error rather than tracking
this information throughout the entire pass.
getExprForPartitionOp(...) just returned the expression from the loc of op.currentInst:
SILInstruction *sourceInstr = op.getSourceInst(/*assertNonNull=*/true);
Expr *expr = sourceInstr->getLoc().getAsASTNode<Expr>();
Instead of mucking around with exprs, just use the SILLocation from the
SILInstruction.
I also changed how we unique transfer instructions to just use the transfer
instruction itself instead of the AST/Expr of the transfer instruction.
Was experimenting with making PartitionOps a noncopyable type and I discovered
these places where we copy PartitionOps when we could use a const reference. It
is good not to copy PartitionOps since they potentially contain a heap allocated
array.
Sadly, my change to make PartitionOps noncopyable will have to wait until a
forthcoming commit here I overhaul how we emit errors since that older code
copies PartitionOps a lot and I would rather just delete that code and then fix
PartitionOps. But these are on the surface safe changes that makes sense to get
in separately to make that next patch easier to review.
What this does is really split the one dataflow we are performing into two
dataflows we perform at the same time. The first dataflow is the region dataflow
that we already have with transferring never occurring. The second dataflow is a
simple gen/kill dataflow where we gen on a transfer instruction and kill on
AssignFresh. What it tracks are regions where a specific element is transferred
and propagates the region until the element is given a new value. This of course
means that once the dataflow has converged, we have to emit an error not if the
value was transferred, but if any value in its region was transferred.
Specifically:
1. I changed Partition::apply so that it has an emitLog flag. The reason why I
did this is we run apply in a few different situations sometimes when we want to
emit logging other times when we really don't. For instance, we want to emit
logging when walking instructions and updating the entry partition. On the other
hand, we do not want to emit logging if we apply a value to a partition while
attempting to determine why an error needed to be emitted.
2. When we create an assign partition op and we see that our destination and
source are the same representative, we do not create the actual assign. Before
we did not log this so it looked like there was a logic error that was stopping
us from emitting a partition op when visiting said instructions. Now, we emit a
small logging message so it isn't possible to be confused.
3. Since I am adding another parameter to Partition::apply, I decided to
refactor Partition::apply to be in a separate PartitionOpEvaluator data
structure that contains the options that we used to pass into Partition::apply.
This prevents any mistakes around configuring Partition::apply since the fields
provide nice names/common sense default values.
This was a piece of code that I added early while adding better unittesting for
Partition. Instead in that patch I added a forward declared class in Partition
called PartitionTester that could implement getRegion. I just forgot to remove
this.
We were performing a union on the intersection of the lhs/rhs but were dropping
the parts of lhs/rhs that were in the symmetric difference of the two sets.
Without this, we would not diagnose cases like this where we had elements on the
lhs/rhs that were not in the intersection.
```
var closure: () -> () = {}
await transferToMain(closure)
if await booleanFlag {
closure = {
print(self.klass)
}
} else {
closure = {}
}
// At this point we would lose closure since they were different elements
await transferToMain(closure) // We wouldn't error on this!
```
rdar://117437059
