Explanation: Unowned result conventions do not work well with OSSA. Retain the
result right after the call when we come out of OSSA so we can treat the
returned value as if it was owned when we do optimizations.
This fix a miscompilation due to the DestroyAddrHoisting pass hoisting destroys
above copies with unowned sources. When the destroyed object was the last
reference to the pointed memory the copy is happening too late resulting in
a use after free.
Issues: rdar://160462854
Original PRs: #84612
Risk: We change where retaining of the unowned return values
happen in the optimization pipeline. It is hard to anticipate all the
possible effects but it should make the optimizer more correct.
Testing: Added a compiler test.
Reviewers: @eeckstein, @atrick
Updates ConsumeOperatorCopyableValuesChecker to identify store_borrow
instructions as a liveness-affecting use so that patterns that would
previously slip through undiagnosed are correctly identified. e.g.
```swift
func use<V>(_ v: borrowing V) {}
func f() {
let a = A()
_ = consume a
use(a) // previously would not be diagnosed
}
```
(cherry picked from commit 754a3007d5)
Specifically for 6.2, we are making optimize hop to executor more conservative
around caller isolation inheriting functions. This means that we are:
1. No longer treating calls to caller isolation inheriting functions as having a
hop in their prologue. In terms of this pass, it means that when determining
dead hop to executors, we no longer think that a caller isolation inheriting
function means that an earlier hop to executor is not required.
2. Treating returns from caller isolation inheriting callees as requiring a
hop. The reason why we are doing this is that we can no longer assume that our
caller will hop after we return.
Post 6.2, there are three main changes we are going to make:
* Forward Dataflow
Caller isolation inheriting functions will no longer be treated as suspension
points meaning that we will be able to propagate hops over them and can assume
that we know the actor that we are on when we enter the function. Practically
this means that trees of calls that involve just nonisolated(nonsending) async
functions will avoid /all/ hop to executor calls since we will be able to
eliminate all of them since the dataflow will just propagate forward from the
entrance that we are already on the actor.
* Backwards Dataflow
A caller isolation inheriting call site will still cause preceding
hop_to_executor functions to be live. This is because we need to ensure that we
are on the caller isolation inheriting actor before we hit the call site. If we
are already on that actor, the hop will be eliminated by the forward pass. But
if the hop has not been eliminated, then the hop must be needed to return us to
the appropriate actor.
We will also keep the behavior that returns from a caller isolation inheriting
function are considered to keep hop to executors alive. If we were able to
propagate to a hop to executor before the return inst with the forward dataflow,
then we know that we are guaranteed to still be on the relevant actor. If the
hop to executor is still there, then we need it to ensure that our caller can
treat the caller isolation inheriting function as a non-suspension point.
rdar://155905383
(cherry picked from commit b3942424c8)
This makes the code easier to write and also prevents any lifetime issues from a
diagnostic outliving the SmallString due to diagnostic transactions.
(cherry picked from commit 010fa39f31)
I am doing this so that I can change how we emit the diagnostics just for
SendNonSendable depending on if NonisolatedNonsendingByDefault is enabled
without touching the rest of the compiler.
This does not actually change any of the actual output though.
(cherry picked from commit 4ce4fc4f95)
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.
(cherry picked from commit 02c34bb830)
No update is needed for the values they produce. This pass should
really be refactored not to crash on instructions that aren't explicitly
listed or at least not to compile if not every instruction is listed.
rdar://155059418
Handle the presence of mark_dependence instructions after a begin_apply.
Fixes a compiler crash:
"copy of noncopyable typed value. This is a compiler bug. ..."
(cherry picked from commit 7a29d9d8b6)
Extract the special pattern matching logic that is otherwise unrelated to the
check() function. This makes it obvious that the implementation was failing to
set the 'changed' flag whenever needed.
(cherry picked from commit c41715ce8c)
The reason I am doing this is that we have gotten reports about certain test
cases where we are emitting errors about self being captured in isolated
closures where the sourceloc is invalid. The reason why this happened is that
the decl returned by getIsolationCrossing did not have a SourceLoc since self
was being used implicitly.
In this commit I fix that issue by using SIL level information instead of AST
level information. This guarantees that we get an appropriate SourceLoc. As an
additional benefit, this fixed some extant errors where due to some sort of bug
in the AST, we were saying that a value was nonisolated when it was actor
isolated in some of the error msgs.
rdar://151955519
(cherry picked from commit f31236931b)
Defer visiting an instruction until its operands have been visited. Otherwise,
this pass will crash during ownership verification with invalid operand
ownership.
Fixes rdar://152879038 ([moveonly] MoveOnlyWrappedTypeEliminator ownership
verifier crashes on @_addressableSelf)
(cherry picked from commit 16fffd1704)
This prevents simplification and SILCombine passes to remove (alive) `mark_dependence_addr`.
The instruction is conceptually equivalent to
```
%v = load %addr
%d = mark_dependence %v on %base
store %d to %addr
```
Therefore the address operand has to be defined as writing to the address.
When the called closure throws an error, it needs to clean up the buffer.
This means that the buffer is uninitialized at this point.
We need an `end_lifetime` so that the move-only checker doesn't insert a wrong `destroy_addr` because it thinks that the buffer is initialized.
Fixes a mis-compile.
rdar://151461109
Apply the MoveOnlyAddressChecker change from
https://github.com/swiftlang/swift/pull/73358 to the
MoveOnly[Value]Checker.
After 7713eef817, before running value
checking, all lifetimes in the function are completed. That doesn't
quite work because lifetime completion expects not to encounter
reborrows or their adjacent phis.
rdar://151325025
Enable walking into `TypeOffsetSizePair`s from an existential into an
archetype. And set the access kind on `open_existential_addr`
instructions which are the sources of rewritten `copy_addr`s to mutable.
rdar://141279635
A metatype need not always come from a metatype instruction. It can come from
a SILArgument. Fix the invalid cast operation in OSLogOptimization.
Fixes rdar://146160325
A trivial store is allowed to occur on an existing live value, and should not
trigger an attempt to destroy the original value completely. Fixes rdar://147791932.
Specifically, we only do this if the base is a let or if it is a var but not
captured by reference.
rdar://149019222
(cherry picked from commit 23b6937cbc)
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.
(cherry picked from commit a045c9880a)
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.
(cherry picked from commit c846c2279e)
Revert "Merge pull request #79707 from DougGregor/transparent-integer-conversions"
This reverts commit 9c2c4ea07f, reversing
changes made to 829e03c104.
When performing a dynamic cast to an existential type that satisfies
(Metatype)Sendable, it is unsafe to allow isolated conformances of any
kind to satisfy protocol requirements for the existential. Identify
these cases and mark the corresponding cast instructions with a new flag,
`[prohibit_isolated_conformances]` that will be used to indicate to the
runtime that isolated conformances need to be rejected.
