Complete the transition to the full translateSILMultiAssign signature by
updating all remaining call sites to pass explicit indirect results. We just
pass through mechanically an empty ArrayRef<Operand *>, so nothing semantic is
changing.
Arguably some of these entrypoints should use actual indirect parameters
(looking at use store_borrow), but this at least makes progress.
Previously, region-based isolation was not diagnosing cases where a non-Sendable
value projected from a Sendable base that was MainActor isolated. For example:
```swift
@MainActor
struct MainActorBox {
var value: NonSendableKlass?
mutating func take() -> sending NonSendableKlass {
if let value {
self.value = nil
return value // Should warn: main actor-isolated value cannot be 'sending'
} else {
preconditionFailure("Consumed twice")
}
}
}
```
This was caused by two issues:
1. A logic bug in `AddressBaseComputingVisitor::visitAll` where we overwrote an
already-found Sendable value when recursing. The visitor should only record
the first Sendable value found, not subsequent ones. This caused us to
incorrectly return the `@MainActor` `MainActorBox` as the base instead of
emitting a require for the projected value.
2. struct_element_addr being transparent unconditionally causing us to not even
emit a require at all.
This commit has two parts:
1. I fixed the first two issues by fixing the logic bug and by making
struct_element_addr only transparent if its operand and result are
non-Sendable. I added logic that we have used in other such cases to handle
non-Sendable operand/Sendable result as well as Sendable operand and
non-Sendable result. I then added the same support for tuple_element_addr and
unchecked_take_enum_data_addr since they suffered from a similar problem.
2. Adding the logic to handle variants where the operand was non-Sendable and
the result was Sendable, caused a bunch of tests to break so I had to fix
that.
To fix the broken test cases, I had to make the compiler smarter about how it
was inserting this require. To do this:
1. I checked if the access base of the projection was actually immutable or if
we are an alloc_stack that there was a prefix path in the projection path of
immutable projections that end in the alloc_stack. In such a case, we know
that we do not need to require that the operand is available in the current
isolation domain since we will never write to that piece of memory and once
we have loaded the result from memory, we know that the value is Sendable so
nothing bad can happen.
2. If the access base of the projection was mutable, I used the same logic that
we used for alloc_box that were non-Sendable that stored a Sendable thing by
changing operand to be a `require [mutable_base_of_sending]` on the result of
the projection instead of requiring the projection's operand. This ensures
that we handled important flow sensitive cases.
rdar://169626088
For stores to unaliased, non-aggregate-projected destinations, switch from
translateSILAssignDirect(destValue, src) to translateSILAssignIndirect(dest, src).
This passes the Operand* rather than just the SILValue, enabling proper tracking
of the destination address in region analysis.
First in a series migrating instruction handlers to indirect assignment.
NOTE: I originally thought that pack_element_set would also have this property,
but alloc_pack today is always assumed to be to be MayAlias and thus we emit a
merge. I think this is fine since one can never actually assign over a pack
variable like one can other things (that is I think they are generally just used
for marshalling and the like). If I am wrong, I can just fix it later.
rdar://156024613
https://github.com/swiftlang/swift/issues/83121
Explicitly pass an empty indirect results array to translateSILMultiAssign when
translating partial_apply instructions, since partial_apply does not produce
indirect results. This transitions the call site from the backward-compatible
overload to the full signature introduced in bc8eadad12f.
Add two template overloads of translateSILAssignIndirect that wrap
translateSILMultiAssign for the common case of assigning to a single
indirect parameter destination:
1. A generic template accepting any collection of source operands
2. A specialization for a single source operand
These simplify call sites that only need to handle one destination.
Add a new IndirectResultsRangeTy template parameter and indirectResultAddresses
parameter to translateSILMultiAssign, allowing it to process both direct result
values and indirect result operands.
I also added overload that takes only direct results for backward
compatibility. I am going to get rid of this eventually once we have finished
the transition.
It previously just accepted ArrayRef<TrackableValue>. I am going to be using
this with a transform + concat range in a subsequent change to pass a combined
ArrayRef<TrackableValue> + ArrayRef<std::pair<Operand, TrackableValue>>.second.
Since after address lowering, `Borrow` can remain loadable with a known-
layout address-only referent, we need instructions that handle three
forms:
- borrow and referent are both loadable values
- borrow is a value, but referent is address-only
- borrow and referent are both address-only
begin_dealloc_ref is only lookthrough its first parameter. We want to treat the
other parameter as just a require. This hit an assert in the SIL test cases in
the next commit.
Split the `Assign` partition operation into `AssignDirect` and `AssignIndirect`
to properly handle the case where a value in memory is overwritten.
Previously, when a store instruction overwrote a value in memory (e.g.,
`store %new to [assign] %addr`), we would simply update %addr's region to
match %new's region. This caused us to lose information about the original
value that was in %addr, which could include actor or task isolation info.
For example:
%addr = alloc_stack
store %task_local to [init] %addr // %addr's region is task-isolated
store %new_value to [assign] %addr // Previously: lost task isolation!
Now, `AssignIndirect` creates a new representative value (identified by the
store's destination operand) for the overwritten value and merges it into
the destination's region before reassigning. This preserves the region's
isolation properties.
`AssignDirect` continues to be used for direct results where the SSA value
itself is new (e.g., struct_extract results, pack_element_get addresses).
The actual implementation change for AssignIndirect will come in a follow-up
commit. This commit only refactors code in preparation for that.
This commit systematically replaces all calls to `SILIsolationInfo::isNonSendableType(type, fn)`
and `SILIsolationInfo::isSendableType(type, fn)` with their value-based equivalents
`SILIsolationInfo::isNonSendable(value)` and `SILIsolationInfo::isSendable(value)`.
This refactoring enables more precise Sendability analysis for captured values
in closures, which is a prerequisite for treating inferred-immutable weak
captures as Sendable, a modification I will be making a subsequent commit.
I made the type-based `isSendableType(type, fn)` methods private to prevent
future misuse. The only place where isSendableType was needed to be used outside
of SILIsolationInfo itself was when checking the fields of a box. Rather than
exposing the API for that one purpose, I added two APIs specifically for that
use case.
This instruction can be used to disable ownership verification on it's result and
will be allowed only in raw SIL.
Sometimes SILGen can produce invalid ownership SSA, that cannot be resolved until
mandatory passes run. We have a few ways to piecewise disable verification.
With unchecked_ownership instruction we can provide a uniform way to disable ownership
verification for a value.
This instruction converts Builtin.ImplicitActor to Optional<any Actor>. In the
process of doing so, it masks out the bits we may have stolen from the witness
table pointer of Builtin.ImplicitActor. The bits that we mask out are the bottom
two bits of the top nibble of the TBI space on platforms that support TBI (that
is bit 60,61 on arm64). On platforms that do not support TBI, we just use the
bottom two tagged pointer bits (0,1).
By using an instruction, we avoid having to represent the bitmasking that we are
performing at the SIL level and can instead just make the emission of the
bitmasking an IRGen detail. It also allows us to move detection if we are
compiling for AArch64 to be an IRGen flag instead of a LangOpts flag.
The instruction is a guaranteed forwarding instruction since we want to treat
its result as a borrowed projection from the Builtin.ImplicitActor.
Specifically, this code was added because otherwise we would in swift 5 +
strict-concurrency mode emit two warnings, one at the AST level and one at the
SIL level. Once we are in swift-6 mode, this does not happen since we stop
compiling at the AST level since we will emit the AST level diagnostic as an
error.
To do this, we tried to pattern match what the AST was erroring upon and treat
the parameter as disconnected instead of being isolated. Sadly, this resulted in
us treating certain closure cases incorrectly and not emit a diagnostic
(creating a concurrency hole).
Given that this behavior results in a bad diagnostic only to avoid emitting two
diagnostics in a mode which is not going to last forever... it really doesn't
make sense to keep it. We really need a better way to handle these sorts of
issues. Perhaps a special semantic parameter put on the function that squelches
certain errors. But that is something for another day. The specific case it
messes up is:
```
class NonSendable {
func action() async {}
}
@MainActor
final class Foo {
let value = NonSendable()
func perform() {
Task { [value] in
await value.action() // Should emit error but do not.
}
}
}
```
In this case, we think that value is sending... when it isnt and we should emit
an error.
rdar://146378329
We previously were not "unsending" inout sending parameters after sending them
so they could not be used again in the caller and could not be forwarded into
other 'inout sending' parameters. While looking at the code I
realized it was pretty obtuse/confusing so I cleaned up the logic and fixed a
few other issues in the process.
Now we follow the following pattern in the non-isolation crossing case:
1. We first require the callee operand.
2. We then merge/require all of the non-explicitly sent parameters.
3. We then through all of the parameters and require/send all of the sending parameters.
4. At the end of processing, we unsend all of the sending parameters that were
'inout sending' parameters.
In the case of isolation crossing applies we:
1. Require all parameters that are not explicitly marked as sending and then
send them all at once. We are really just saving a little work by not merging
them into one large region and then requiring/sending that region once.
2. Then for each sending parameter, we require/send them one by one interleaving
the requires/sends. This ensures that if a value is passed to different
explicitly sending parameters, we get an error.
3. Then once we have finished processing results, we perform an undo send on all
of the 'inout sending' params.
rdar://154440896
We want 'inout sending' parameters to have the semantics that not only are they
disconnected on return from the function but additionally they are guaranteed to
be in their own disconnected region on return. This implies that we must emit
errors when an 'inout sending' parameter or any element that is in the same
region as the current value within an 'inout sending' parameter is
returned. This commit contains a new diagnostic for RegionIsolation that adds
specific logic for detecting and emitting errors in these situations.
To implement this, we introduce 3 new diagnostics with each individual
diagnostic being slightly different to reflect the various ways that this error
can come up in source:
* Returning 'inout sending' directly:
```swift
func returnInOutSendingDirectly(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
return x // expected-warning {{cannot return 'inout sending' parameter 'x' from global function 'returnInOutSendingDirectly'}}
// expected-note @-1 {{returning 'x' risks concurrent access since caller assumes that 'x' and the result of global function 'returnInOutSendingDirectly' can be safely sent to different isolation domains}}
}
```
* Returning a value in the same region as an 'inout sending' parameter. E.x.:
```swift
func returnInOutSendingRegionVar(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
var y = x
y = x
return y // expected-warning {{cannot return 'y' from global function 'returnInOutSendingRegionVar'}}
// expected-note @-1 {{returning 'y' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' and the result of global function 'returnInOutSendingRegionVar' can be safely sent to different isolation domains}}
}
```
* Returning the result of a function or computed property that is in the same
region as the 'inout parameter'.
```swift
func returnInOutSendingViaHelper(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return useNonSendableKlassAndReturn(y) // expected-warning {{cannot return result of global function 'useNonSendableKlassAndReturn' from global function 'returnInOutSendingViaHelper'}}
// expected-note @-1 {{returning result of global function 'useNonSendableKlassAndReturn' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' and the result of global function 'returnInOutSendingViaHelper' can be safely sent to different isolation domains}}
}
```
Additionally, I had to introduce a specific variant for each of these
diagnostics for cases where due to us being in a method, we are actually in our
caller causing the 'inout sending' parameter to be in the same region as an
actor isolated value:
* Returning 'inout sending' directly:
```swift
extension MyActor {
func returnInOutSendingDirectly(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
return x // expected-warning {{cannot return 'inout sending' parameter 'x' from instance method 'returnInOutSendingDirectly'}}
// expected-note @-1 {{returning 'x' risks concurrent access since caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingDirectly' is 'self'-isolated}}
}
}
```
* Returning a value in the same region as an 'inout sending' parameter. E.x.:
```swift
extension MyActor {
func returnInOutSendingRegionLet(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return y // expected-warning {{cannot return 'y' from instance method 'returnInOutSendingRegionLet'}}
// expected-note @-1 {{returning 'y' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingRegionLet' is 'self'-isolated}}
}
}
```
* Returning the result of a function or computed property that is in the same region as the 'inout parameter'.
```swift
extension MyActor {
func returnInOutSendingViaHelper(_ x: inout sending NonSendableKlass) -> NonSendableKlass {
let y = x
return useNonSendableKlassAndReturn(y) // expected-warning {{cannot return result of global function 'useNonSendableKlassAndReturn' from instance method 'returnInOutSendingViaHelper'; this is an error in the Swift 6 language mode}}
// expected-note @-1 {{returning result of global function 'useNonSendableKlassAndReturn' risks concurrent access to 'inout sending' parameter 'x' since the caller assumes that 'x' is not actor-isolated and the result of instance method 'returnInOutSendingViaHelper' is 'self'-isolated}}
}
}
```
To implement this, I used two different approaches depending on whether or not
the returned value was generic or not.
* Concrete
In the case where we had a concrete value, I was able to in simple cases emit
diagnostics based off of the values returned by the return inst. In cases where
we phied together results due to multiple results in the same function, we
determine which of the incoming phied values caused the error by grabbing the
exit partition information of each of the incoming value predecessors and seeing
if an InOutSendingAtFunctionExit would emit an error.
* Generic
In the case of generic code, it is a little more interesting since the result is
a value stored in an our parameter instead of being a value directly returned by
a return inst. To work around this, I use PrunedLiveness to determine the last
values stored into the out parameter in the function to avoid having to do a
full dataflow. Then I take the exit blocks where we assign each of those values
and run the same check as we do in the direct phi case to emit the appropriate
error.
rdar://152454571
Centralize the logic for figuring out the conformances for the various
init_existential* instructions in a SILIsolationInfo static method, and
always go through that when handling "assign" semantics. This way, we
can use CONSTANT_TRANSLATION again for these instructions, or a simpler
decision process between Assign and LookThrough.
The actually undoes a small change made earlier when we stopped looking
through `init_existential_value` instructions. Now we do when there are
no isolated conformances.
Better match the style of SILIsolationInfo by moving the code for determining
SILIsolationInfo from conformances or dynamic casts to existentials into
static `getXYZ` methods on SILIsolationInfo.
Other than adding an assertion regarding disconnected regions, no
intended functionality change.
When we introduce isolation due to a (potential) isolated conformance,
keep track of the protocol to which the conformance could be
introduced. Use this information for two reasons:
1. Downgrade the error to a warning in Swift < 7, because we are newly
diagnosing these
2. Add a note indicating where the isolated conformance could be introduced.
When a conformance becomes part of a value, and that conformance could
potentially be isolated, the value cannot leave that particular
isolation domain. For example, if we perform a runtime lookup for a
conformance to P as part of a dynamic cast `as? any P`, the
conformance to P used in the cast could be isolated. Therefore, it is
not safe to transfer the resulting value to another concurrency domain.
Model this in region analysis by considering whether instructions that
add conformances could end up introducing isolated conformances. In
such cases, merge the regions with either the isolation of the
conformance itself (if known) or with the region of the task (making
them task-isolated). This prevents such values from being sent.
Note that `@concurrent` functions, which never dynamically execute on
an actor, cannot pick up isolated conformances.
Fixes issue #82550 / rdar://154437489
Otherwise, depending on the exact value that we perform the underlying look up
at... we will get different underlying values. To see this consider the
following SIL:
```sil
%1 = alloc_stack $MyEnum<T>
copy_addr %0 to [init] %1
%2 = unchecked_take_enum_data_addr %1, #MyEnum.some!enumelt
%3 = load [take] %2
%4 = project_box %3, 0
%5 = load_borrow %4
%6 = copy_value %5
```
If one were to perform an underlying object query on %4 or %3, one would get
back an underlying object of %1. In contrast, if one performed the same
operation on %5, then one would get back %3. The reason why this happens is that
we first see we have an object but that it is from a load_borrow so we need to
look through the load_borrow and perform the address underlying value
computation. When we do that, we find project_box to be the value. project_box
is special since it is the only address base we ever look through since from an
underlying object perspective, we want to consider the box to be the underlying
object rather than the projection. So thus we see that the result of the
underlying address computation is that the underlying address is from a load
[take]. Since we then pass in load [take] recursively into the underlying value
object computation, we just return load [take]. In contrast, the correct
behavior is to do the more general recurse that recognizes that we have a load
[take] and that we need to look through it and perform the address computation.
rdar://151598281
It derives the address of the first element of a vector, i.e. a `Builtin.FixedArray`, from the address of the vector itself.
Addresses of other vector elements can then be derived with `index_addr`.
