Also, the store_borrow work in the previous patch caused some additional issues
to crop up. I fixed them in this PR and added some tests in the process.
We can't really treat them as always-initialized because that makes move checking
think that there's a value to destroy even on initialization, causing deinits to
run on uninitialized memory. Remove my previous hack, and use a `zeroInitializer`
to initialize the value state when emitting `init`, which is where we really need
the bootstrapping-into-initialized behavior. rdar://113057256
We want these to be borrowed in most cases and to create an appropriate onion
wrapping. Since we are doing this in more cases now, we fix a bunch of cases
where we used to be forced to insert a copy since a coroutine or access would
end too early.
llvm::SmallSetVector changed semantics
(https://reviews.llvm.org/D152497) resulting in build failures in Swift.
The old semantics allowed usage of types that did not have an
`operator==` because `SmallDenseSet` uses `DenseSetInfo<T>::isEqual` to
determine equality. The new implementation switched to using
`std::find`, which internally uses `operator==`. This type is used
pretty frequently with `swift::Type`, which intentionally deletes
`operator==` as it is not the canonical type and therefore cannot be
compared in normal circumstances.
This patch adds a new type-alias to the Swift namespace that provides
the old semantic behavior for `SmallSetVector`. I've also gone through
and replaced usages of `llvm::SmallSetVector` with the
`Swift::SmallSetVector` in places where we're storing a type that
doesn't implement or explicitly deletes `operator==`. The changes to
`llvm::SmallSetVector` should improve compile-time performance, so I
left the `llvm::SmallSetVector` where possible.
This attribute can be attached to a noncopyable struct to specify that its
storage is raw, meaning the type definition is (with some limitations)
able to do as it pleases with the storage. This provides a basis for
implementing types for things like atomics, locks, and data structures
that use inline storage to store conditionally-initialized values.
The example in `test/Prototypes/UnfairLock.swift` demonstrates the use
of a raw layout type to wrap Darwin's `os_unfair_lock` APIs, allowing
a lock value to be stored inside of classes or other types without
needing a separate allocation, and using the borrow model to enforce
safe access to lock-guarded storage.
Originally, we were relying on capture info to determine if we needed to insert
this mark_must_check. This ignored that the way that we are handling
escaping/non-escaping is something that is approximated in the AST but actually
determined at SIL level. With that in mind, rather than relying on the capture
info here, just rely on us having an inout argument. The later SIL level
checking for inout escapes is able to handle mark_must_check and knows how to
turn off noncopyable errors in the closure where we detect the error to prevent
us from emitting further errors due to the mark_must_check here.
I discovered this while playing with the previous commit.
rdar://112555589
Without this, we emit a copy of noncopyable type error since we do not insert a
mark_must_check on lazily initialized global initializers.
rdar://111402912
Both of these can cause us to insert destroy_addr in the wrong locations.
1. The first causes us to insert destroys for parts of values that are not
actually on the boundary since we didn't use our mask and instead used all of
the liveness information.
2. We were merging successor information using '&=' instead of '|=. This caused
a problem if we had multiple regions for the same successor. In such a case, we
would not have anything in common for the regions causing us to not have any
bits in common, resulting in us inserting too many destroy_addr instead of
skipping as we were supposed to.
rdar://112434492
This is similar to our ban on partial consuming a value for this release. The
reason for this is that, one can achieve a similar affect as partial consumption
via a consumption of the entire value and then a partial reinitialization. Example:
```swift
struct X : ~Copyable { var i = 5, var i2 = Klass() }
var x = X()
_ = consume x
x.i = 5
```
in the case above, we now have a value that is in a partially initialized state.
We still allow for move only types to have their fields initialized as long as
there is an intervening init.
rdar://111498740
Previously, when doing this I could just use the terminator both in cases of
inout and for class field/global accesses... but after some recent changes to
field sensitive pruned liveness, this seems to no longer work. So just do the
right thing and use the field access.
The specific bug was that we would introduce a destroy_addr along one of the
paths where the value wasn't defined resulting in a dominance error.
I added a SIL test that shows this fixes the issue, a swift test, and also an
Interpreter test that validates the correctness.
rdar://111659649
We previously were emitting a consuming partial_apply diagnostic. I had to
reformulate slightly the way we pattern match the diagnostics to make sure that
we get the proper no consume diagnostic.
rdar://111461837
The reason why I am doing this is that this was not part of the original
evolution proposal (it was called an extension) and after some discussion it was
realized that partial consumption would benefit from discussion on the forums.
rdar://111353459
In the next commit, I am modifying the move only checker to ensure that we
always error when partially invalidating. While doing this I discovered that
these copies were getting in the way of emitting good diagnostics in that case.
The address checker records instructions that initialize fields in its
initInsts map. Previously, that map mapped from an instruction to a
range of fields of the type. But an instruction can initialize multiple
discontiguous fields of a single value. (Indeed an attempt to add a
second range that was initialized by an already initializing
instruction--even if it were overlapping or adjacent--would have no
effect and the map wouldn't be updated.) Here, this is fixed by fixing
the representation and updating the storage whenver a new range is seen
to be initialized by the instruction. As in
https://github.com/apple/swift/pull/66728 , a SmallBitVector is the
representation chosen.
rdar://111391893
In preparation for having a third instance getting or creating the bits
affected by an instruction, introduce a typealias for maps
SILInstruction->SmallBitVector and a helper function that returns
a reference to the SmallBitVector for an instruction and two others that
set into those bits the bits from another bit vector or from a range.
The address checker records instructions that reinit fields in its
reinitInsts map. Previously, that map mapped from an instruction to a
range of fields of the type. But an instruction can use multiple
discontiguous fields of a single value. (Indeed an attempt to add a
second range that was reinit'd by an already reinit'ing
instruction--even if it were overlapping or adjacent--would have no
effect and the map wouldn't be updated.) Here, this is fixed by fixing
the representation and updating the storage whenver a new range is seen
to be reinit'd by the instruction. As in
https://github.com/apple/swift/pull/66728 , a SmallBitVector is the
representation chosen.
rdar://111356251
This is phase-1 of switching from llvm::Optional to std::optional in the
next rebranch. llvm::Optional was removed from upstream LLVM, so we need
to migrate off rather soon. On Darwin, std::optional, and llvm::Optional
have the same layout, so we don't need to be as concerned about ABI
beyond the name mangling. `llvm::Optional` is only returned from one
function in
```
getStandardTypeSubst(StringRef TypeName,
bool allowConcurrencyManglings);
```
It's the return value, so it should not impact the mangling of the
function, and the layout is the same as `std::optional`, so it should be
mostly okay. This function doesn't appear to have users, and the ABI was
already broken 2 years ago for concurrency and no one seemed to notice
so this should be "okay".
I'm doing the migration incrementally so that folks working on main can
cherry-pick back to the release/5.9 branch. Once 5.9 is done and locked
away, then we can go through and finish the replacement. Since `None`
and `Optional` show up in contexts where they are not `llvm::None` and
`llvm::Optional`, I'm preparing the work now by going through and
removing the namespace unwrapping and making the `llvm` namespace
explicit. This should make it fairly mechanical to go through and
replace llvm::Optional with std::optional, and llvm::None with
std::nullopt. It's also a change that can be brought onto the
release/5.9 with minimal impact. This should be an NFC change.
During initializeLiveness, all blocks which contain destroys are to be
recorded in consuming blocks. Previously, however, certain reinits were
not being added. Here, all reinits are correctly added.
The multi-def algorithm is based off the single-def algorithm. However,
it differs from it in needing to handle "liveness holes"--uses before defs.
When identifying blocks which are originally live, the algorithm starts
from consuming blocks and walks backwards until a condition is met.
Previously, that condition was finding blocks which were originally
live. That makes sense in the single-def case: if a block is originally
live, either it was already walked through or else it was one of the
blocks discovered by liveness. In either case, its predecessors have
already been visited if appropriate.
However, in the multi-def case, this condition is too stringent. It
fails to account for the possibility of a block with has a "liveness
hole". Only stop the backwards walk if the predecessor not only (1) was
in originally live but additionally (2) "kills liveness"--doesn't have a
use before a def.
rdar://111221183
Like a nested partial_apply reabstraction, but without the thunk, these change
the representation of the function but can't escape it without breaking the
nonescaping closure promotion. rdar://111060678
Specifically, we were calling process on a visitor that was already moved. To
fix this I created a separate state structure that the visitor initializes and
moved the post-processing step onto the state data structure.
This seems to not be causing any problems today, but could in the future.
rdar://111060475
SILGen introduces a copy of the capture, because the semantics of escaping partial_apply's
requires the closure to take ownership of the parameters. We don't know when a closure is
strictly nonescaping or its final lifetime until ClosureLifetimeFixup runs, but that replaces
the consume of the copy with a borrow of the copy normally, hoping later passes fix it up.
We can't wait that long for move-only types, which can't be copied, so try to remove the
copy up front when the copy lives long enough and has no interfering uses other than the
partial_apply. rdar://110137169
Track liveness of self so we don't accidentally think that such types
can be memberwise reinitialized.
Fixes rdar://110232973 ([move-only] Checker should distinguish in
between field of single field struct vs parent field itself (was:
mutation of field in noncopyable struct should not trigger deinit))
The address checker records uses in its livenessUses map. Previously,
that map mapped from an instruction to a range of fields of the type.
But an instruction can use multiple discontiguous fields of a single
value. Here, such instructions are properly recorded by fixing the map
to store a bit vector for each instruction.
rdar://110676577
Previously, the checker inserted destroys after each last use. Here,
extend the lifetimes of fields as far as possible within their original
(unchecked) limits.
rdar://99681073
FieldSensitivePrunedLiveness is used as a vectorization of
PrunedLiveness. An instance of FSPL with N elements needs to be able to
represent the same states as N instances of PL.
Previously, it failed to do that in two significant ways:
(1) It attempted to save space for which elements were live by using
a range. This failed to account for instructions which are users of
non-contiguous fields of an aggregate.
apply(
@owned (struct_element_addr %s, #S.f1),
@owned (struct_element_addr %s, #S.f3)
)
(2) It used a single bit to represent whether the instruction was
consuming. This failed to account for instructions which consumed
some fields and borrowed others.
apply(
@owned (struct_element_addr %s, #S.f1),
@guaranteed (struct_element_addr %s, #S.f2)
)
The fix for (1) is to use a bit vector to represent which elements
are used by the instruction. The fix for (2) is to use a second bit
vector to represent which elements are _consumed_ by the instruction.
Adapted the move-checker to use the new representation.
rdar://110909290
The value `self` is mutable (i.e., var-bound) in
a `consuming` method. Since you're allowed to
reinitialize a var after consuming, that means
you were also naturally allowed to reinitialize
self after `discard self`. But that capability was
not intended; after you discard self you shouldn't
be reinitializing it, as that's probably a mistake.
This change makes reinitialization of `self`
reachable from a `discard self` statement an error.
rdar://106098163
As part of SE-390, you're required to write either:
- `consume self`
- pass self as a `consuming` parameter to a function
- `discard self`
before the function ends in a context that contains a
`discard self` somewhere. This prevents people from accidentally
invoking the deinit due to implicit destruction of `self` before
exiting the function.
rdar://106099027
Before the previous commit, we didn't see load [take] very often since it occurs
mostly on temporaries where we treat the copy_addr as the relevant take. Now
that we are checking temporaries though, we need to support this behavior.
Change SILGen to emit the `debug_value` instruction using the original inout
parameter address, instead of the `mark_must_check` inserted for move-only
parameters, because code in the MoveOnlyAddressChecker did not expect to
find the debug_value anywhere but on the original address. Update move-only
diagnostics so that they pick up the declaration name for a memory location
from any debug_value instruction if there are more than one. rdar://109740281
After a value is consumed, we emit a `debug_value undef` to indicate that the
variable value is no longer valid to the debugger. However, once a value is
reassigned, it becomes valid again, so emit a `debug_value %original_address` to
reassociate the variable with the valid memory location. rdar://109218404
