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`.
The specific issue was when we were walking instructions looking to see if there
was a partial apply escaping instruction, we were not including the user
itself. That means that if the user was the partial apply escaping instruction,
we would return that no escape occured.
rdar://149414471
While bringing up @rjmccall on rbi, our discussions showed that the name
functionArgPartition was misleading to someone who hadn't worked on the pass
before. It became clear that initialEntryBlockPartition would be a better name
that would make it clearer/easy to understand.
I am doing this so I can mark requires as being on a mutable non-Sendable base
from a Sendable value.
I also took this as an opportunity to compress the size of PartitionOp to be 24
bytes instead of 40 bytes.
There are a few major changes here:
1. We now return a TrackableValue from getTrackableValue() if we have either a
non-Sendable value or a non-Sendable base. This means that we /will/ return
TrackableValues that may have a Sendable value or a Sendable base. To make it
easier to work with this, I moved the isSendable check and the do I have a base
check into PartitionOpBuilder. So, most of the actual code around emitting
values does not need to reason about this. They can just call addRequire or
addSend and pass in either TrackableValue::value or TrackableValue::base without
needing to check if the former is non-Sendable or if the latter is non-Sendable
and non-nil.
2. I searched all of the places where we were grabbing trackable values and
inserted require checks for the base value as appropriate.
Both of these together have prevented the code from becoming too heavy.
This fixes https://forums.swift.org/t/lets-debug-missing-rbi-data-race-diagnostics/78910
rdar://149019222
Previously, when we saw any Sendable type and attempted to look up an underlying
tracked value, we just bailed. This caused an issue in situations like the
following where we need to emit an error:
```swift
func test() {
var x = 5
Task.detached { x += 1 }
print(x)
}
```
The problem with the above example is that despite value in x being Sendable,
'x' is actually in a non-Sendable box. We are passing that non-Sendable box into
the detached task by reference causing a race against the read from the
non-Sendable box later in the function. In SE-0414, this is explicitly banned in
the section called "Accessing Sendable fields of non-Sendable types after weak
transferring". In this example, the box is the non-Sendable type and the value
stored in the box is the Sendable field.
To properly represent this, we need to change how the underlying object part of
our layering returns underlying objects and vends TrackableValues to the actual
analysis for addresses. NOTE: We leave the current behavior alone for SIL
objects.
By doing this, in situations like the above, despite have a Sendable value (the
integer), we are able to ensure that we require that the non-Sendable box
containing the integer is not used after we have sent it into the other Task
despite us not actually using the box directly.
Below I describe the representation change in more detail and describe the
various cases here. In this commit, I only change the representation and do not
actually use the new base information. I do that in the next commit to make this
change easier for others to read and review. I made sure that change was NFC by
leaving RegionAnalysis.cpp:727 returning an optional.none if the value found was
a Sendable value.
----
The way we modify the representation is that we instead of just returning a
single TrackedValue return a pair of tracked values, one for the base and one
for the "value". We return this pair in what is labeled a
"TrackableValueLookupResult":
```c++
struct TrackableValueLookupResult {
TrackableValue value;
std::optional<TrackableValue> base;
TrackableValueLookupResult(TrackableValue value)
: value(value), base() {}
TrackableValueLookupResult(TrackableValue value, TrackableValue base)
: value(value), base(base) {}
};
```
In the case where we are accessing a projection path out of a non-Sendable type
that contains all non-Sendable fields, we do not do anything different than we
did previously. We just walk up from use->def until we find the access path base
which we use as the representative of the leaf of the chain and return
TrackableValueLookupResult(access path base).
In the case where we are accessing a Sendable leaf type projected from a
non-Sendable base, we store the leaf type as our value and return the actual
non-Sendable base in TrackableValueLookupResult. Importantly this ensures that
even though our Sendable value will be ignored by the rest of the analysis, the
rest of the analysis will ensure that our base is required if our base is a var
that had been escaped into a closure by reference.
In the case where we are accessing a non-Sendable leaf type projected from a
Sendable type (which we may have continued to be projected subsequently out of
additional Sendable types or a non-Sendable type), we make the last type on the
projection path before the Sendable type, the value of the leaf type. We return
the eventual access path base as our underlying value base. The logic here is
that since we are dealing with access paths, our access path can only consist of
projections into a recursive value type (e.x.: struct/tuple/enum... never a
class). The minute that we hit a pointer or a class, we will no longer be along
the access path since we will be traversing a non-contiguous piece of
memory (consider a class vs the class's storage) and the traversal from use->def
will stop. Thus, we know that there are only two ways we can get a field in that
value type to be Sendable and have a non-Sendable field:
1. The struct can be @unchecked Sendable. In such a case, we want to treat the
leaf field as part of its own disconnected region.
2. The struct can be global actor isolated. In such a case, we want to treat the
leaf field as part of the global actor's region rather than whatever actor.
The reason why we return the eventual access path base as our tracked value base
is that we want to ensure that if the var value had been escaped by reference,
we can require that the var not be sent since we are going to attempt to access
state from the var in order to get the global actor guarded struct that we are
going to attempt to extract our non-Sendable leaf value out of.
I also added some basic tests of its functionality. I am doing this in
preparation for making some more invasive changes to getTrackableValue and I
want to be able to test it out very specifically in SIL.
Due to compile time issues, I added a cache into
getUnderlyingTrackedValue(). This caused an iterator invalidation issue when we
recursed in the case when we had an underlying object since we would recurse
into getUnderlyingTrackedValue() instead of getUnderlyingTrackedValueHelper()
potentially causing us to cache another value and thus causing the underlying
DenseMap to expand. Now we instead just call getUnderlyingTrackedValueHelper()
so that we avoid the invalidation issue. This may cause us to use slightly more
compile time but we are still only ever going to compute the underlying value
once for any specific value.
Specifically,
1. UseDefChainVisitor::actorIsolation is dead. I removed it to prevent any
confusion/thoughts that it actually found isolation. I also removed it from
UnderlyingTrackedValue since that was the only place where we were using it. I
left UnderlyingTrackedValue there in case I need to add additional state there
in the future.
2. Now that UseDefChainVisitor is only used to find the base of a value (while
not looking through Sendable addresses), I renamed it to
AddressBaseComputingVisitor.
3. I renamed UseDefChainVisitor::isMerge to isProjectedFromAggregate. That is
actually what we use it for. I also added a comment explaining what it is used
for.
It appears that we can end up breaking this assertion when inlining
SIL from modules with strict concurrency enabled into modules that
don't. That's not a assertion-worth condition.
In these cases, we want to lookthrough so we propagate through
nonisolated(unsafe) and make it easier to discover that we are processing
keypaths (the reason I am making this change).
From talking with @dgregor, it became clear that this comment was easily
interpreted as saying that AssignFresh always introduced a disconnected value...
which is not the case. Instead, AssignFresh just introduces a new value that
could have any form of isolation. The actual isolation of the value is assigned
via tryToTrackValue and eventually SILIsolationInfo::get().
Introduce a new experimental feature StrictSendableMetatypes that stops
treating all metatypes as `Sendable`. Instead, metatypes of generic
parameters and existentials are only considered Sendable if their
corresponding instance types are guaranteed to be Sendable.
Start with enforcing this property within region isolation. Track
metatype creation instructions and put them in the task's isolation
domain, so that transferring them into another isolation domain
produces a diagnostic. As an example:
func f<T: P>(_: T.Type) {
let x: P.Type = T.self
Task.detached {
x.someStaticMethod() // oops, T.Type is not Sendable
}
}
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
This is used for synthetic uses like _ = x that do not act as a true use but
instead only suppress unused variable warnings. This patch just adds the
instruction.
Eventually, we can use it to move the unused variable warning from Sema to SIL
slimmming the type checker down a little bit... but for now I am using it so
that other diagnostic passes can have a SIL instruction (with SIL location) so
that we can emit diagnostics on code like _ = x. Today we just do not emit
anything at all for that case so a diagnostic SIL pass would not see any
instruction that it could emit a diagnostic upon. In the next patch of this
series, I am going to add SILGen support to do that.
In terms of the test suite the only difference is that we allow for non-Sendable
types to be returned from nonisolated functions. This is safe due to the rules
of rbi. We do still error when we return non-Sendable functions across isolation
boundaries though.
The reason that I am doing this now is that I am implementing a prototype that
allows for nonisolated functions to inherit isolation from their caller. This
would have required me to implement support both in Sema for results and
arguments in SIL. Rather than implement results in Sema, I just finished the
work of transitioning the result checking out of Sema and into SIL. The actual
prototype will land in a subsequent change.
rdar://127477211
TLDR: Was looking at some performance traces and saw that we need to cache the
result of this value.
----
Specifically, I noticed that we were spending a lot of time computing this
operation. When I looked at the code I saw that we already had a cache along the
relevant code paths... but the cache was from equivalence class representative
-> state. Before we hit that cache, we were performing the work to map the value
to the equivalence class representative... so the work to perform the relevant
lookup from value -> state (which goes through the equivalence class
representative) was not just a hash table lookup. This operation makes it
cheaper by making it two cache lookups.
It may be possible to make this cheaper by redoing the actual mapping of
information so that we can go straight from value to state. I think it would be
slightly different since we would probably need to represent the state in a
separate array and map with indices... which is really just a more efficient
hash table. We could also use malloc/etc but lets not even talk about that.
rdar://139520959
I am going to be adding more functionality to this that moves a bit of the
utilities code into it. So it really makes sense to move it to the top of the
file closer to that code. I am doing this separately to make the other
refactoring easier to see in the diff.
