The verify-di-hole SILVerifier pass is very useful in catching incorrectly set
SILDebugScopes, but it can be very tedious to track down the root cause of a
verification failure. This patch replicates much of its functionality inside an
assertion in SILBuilder, which will result in a backtrace pointing directly to
where the offending scope is being set.
Although nonescaping closures are representationally trivial pointers to their
on-stack context, it is useful to model them as borrowing their captures, which
allows for checking correct use of move-only values across the closure, and
lets us model the lifetime dependence between a closure and its captures without
an ad-hoc web of `mark_dependence` instructions.
During ownership elimination, We eliminate copy/destroy_value instructions and
end the partial_apply's lifetime with an explicit dealloc_stack as before,
for compatibility with existing IRGen and non-OSSA aware passes.
Consider the following example:
```
class Klass {}
@_moveOnly struct Butt {
var k = Klass()
}
func mixedUse(_: inout Butt, _: __owned Butt) {}
func foo() {
var y = Butt()
mixedUse(&y, y)
}
```
In this case, we want to have an exclusivity violation. Before this patch, we
did a by-value load [copy] of y and then performed the inout access. Since the
access scopes did not overlap, we would not get an exclusivity violation.
Additionally, since the checker assumes that exclusivity violations will be
caught in such a situation, we convert the load [copy] to a load [take] causing
a later memory lifetime violation as seen in the following SIL:
```
sil hidden [ossa] @$s4test3fooyyF : $@convention(thin) () -> () {
bb0:
%0 = alloc_stack [lexical] $Butt, var, name "y" // users: %4, %5, %8, %12, %13
%1 = metatype $@thin Butt.Type // user: %3
// function_ref Butt.init()
%2 = function_ref @$s4test4ButtVACycfC : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %3
%3 = apply %2(%1) : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %4
store %3 to [init] %0 : $*Butt // id: %4
%5 = begin_access [modify] [static] %0 : $*Butt // users: %7, %6
%6 = load [take] %5 : $*Butt // user: %10 // <————————— This was a load [copy].
end_access %5 : $*Butt // id: %7
%8 = begin_access [modify] [static] %0 : $*Butt // users: %11, %10
// function_ref mixedUse2(_:_:)
%9 = function_ref @$s4test9mixedUse2yyAA4ButtVz_ADntF : $@convention(thin) (@inout Butt, @owned Butt) -> () // user: %10
%10 = apply %9(%8, %6) : $@convention(thin) (@inout Butt, @owned Butt) -> ()
end_access %8 : $*Butt // id: %11
destroy_addr %0 : $*Butt // id: %12
dealloc_stack %0 : $*Butt // id: %13
%14 = tuple () // user: %15
return %14 : $() // id: %15
} // end sil function '$s4test3fooyyF'
```
Now, instead we create a [consume] access and get the nice exclusivity error we
are looking for.
NOTE: As part of this I needed to tweak the verifier so that [deinit] accesses
are now allowed to have any form of access enforcement before we are in
LoweredSIL. I left in the original verifier error in LoweredSIL and additionally
left in the original error in IRGen. The reason why I am doing this is that I
need the deinit access to represent semantically what consuming from a
ref_element_addr, global, or escaping mutable var look like at the SIL level so
that the move checker can error upon it. Since we will error upon such
consumptions in Canonical SIL, such code patterns will never actually hit
Lowered/IRGen SIL, so it is safe to do so (and the verifier/errors will help us
if we make any mistakes). In the case of a non-escaping var though, we will be
able to use deinit statically and the move checker will make sure that it is not
reused before it is reinitialized.
rdar://101767439
- SILPackType carries whether the elements are stored directly
in the pack, which we're not currently using in the lowering,
but it's probably something we'll want in the final ABI.
Having this also makes it clear that we're doing the right
thing with substitution and element lowering. I also toyed
with making this a scalar type, which made it necessary in
various places, although eventually I pulled back to the
design where we always use packs as addresses.
- Pack boundaries are a core ABI concept, so the lowering has
to wrap parameter pack expansions up as packs. There are huge
unimplemented holes here where the abstraction pattern will
need to tell us how many elements to gather into the pack,
but a naive approach is good enough to get things off the
ground.
- Pack conventions are related to the existing parameter and
result conventions, but they're different on enough grounds
that they deserve to be separated.
I am adding this to make it easy to determine if a SILFunction that is not inout
aliasable is captured. This is useful when emitting certain types of
diagnostics like I need to emit with move only.
The current diagnostic that we are emitting is not perfect, but at least this
prevents us from saying that an error is not occuring here. I am going to file a
bug to track the QoI work of improving the diagnostic.
rdar://103313305
`getValue` -> `value`
`getValueOr` -> `value_or`
`hasValue` -> `has_value`
`map` -> `transform`
The old API will be deprecated in the rebranch.
To avoid merge conflicts, use the new API already in the main branch.
rdar://102362022
Also renaming it to be UncheckedConversionComponent since it's a better name.
As a physical component, we'd run into problems in assignment statements.
The problem was that if we had something like:
```
SomeOtherComponent // first component
GetterSetterComponent
ABISafeConversionComponent // last component
```
When emitting the assignment, we always drill down through all but
the last component by calling `project()` on each one. Then on the last
component, we'd do the actual setting operation. But GetterSetterComponent
cannot be projected when the access is for writing.
So, to work around this I decided to model it as a TranslationComponent, because
those are specifically designed to be handled during an assignment by popping those
off the end of the component sequence, untranslating the value we're about to assign
as we go, until we hit the GetterSetterComponent.
By "untranslating" we're effectively putting Sendable back onto the set's argument
prior to calling set, because the underlying property's type still has `@Sendable`
on it (e.g., it's accessors still have that on its argument type). When
"translating" we're effectively taking Sendable off after reading it.
I think actually works really well and makes much more sense now.
resolves rdar://99619834
Some notes:
1. I added support for both loadable/address only types.
2. These tests are based off of porting the move only object tests for inout,
vars, mutating self, etc.
3. I did not include already written tests for address only types in this
specific merge since I need to change us to borrow move only var like types.
Without that, we get a lot of spurious error msgs and the burden of writing that
is not worth it. So instead in a forthcoming commit where I fix that issue in
SILGen, I will commit the corresponding address only tests for this work.
4. I did not include support for trivial types in this. I am going to do
object/address for that at the same time.
This change ensures all store_borrows are ended with an end_borrow, and uses of the store_borrow
destination are all in the enclosing store_borrow scope and via the store_borrow return address.
Fix tests to reflect new store_borrow pattern
Andy some time ago already created the new API but didn't go through and update
the old occurences. I did that in this PR and then deprecated the old API. The
tree is clean, so I could just remove it, but I decided to be nicer to
downstream people by deprecating it first.
Specifically this means that rather than always being owned, we now have owned
and guaranteed versions of copyable_to_moveonlywrapper. Similar to
moveonlywrapper_to_copyable, one chooses which variant one gets by using
specific SILBuilder APIs:
create{Owned,Guaranteed}CopyableToMoveOnlyWrapperValueInst. It is still
forwarding and the rest of the forwarding APIs work as expected except that the
forwarding ownership is fixed (and an assertion will result if one attempts to
do so).
NOTE: It is assumed that trivial operands are always passed to the owned
variant.
The reason why this is necessary is that the trivial input value doesn't have a
cleanup associated with it, but the result of the instruction is owned. So
otherwise we will not have a cleanup for the result, resulting in an OSSA error.
I discovered this as I began to write some Interpreter tests for move only. This
was triggered by SILGen attempting to pass a non-trivial type to StringBuilder
which is generic, so we needed to materialize.
I also discovered a small bug where we were not properly ignoring class_method
in the move only type eliminator. I just folded the small fix + a test for that
into this commit.
This involved doing the following:
1. Update the move only checker to look for new patterns.
2. Teach emitSemanticStore to use a moveonlywrapper_to_copyable to store moveonly
values into memory. The various checkers will validate that this code is
correct.
3. When emitting an apply, always unwrap move only variables. In the
future, I am going to avoid this if a parameter is explicitly marked as also
being moveonly (e.x.: @moveOnly parameter or @noEscape argument).
4. Convert from moveOnly -> copyable on return inst automatically in SILGen.
5. Fix SILGenLValue emission so we emit an error diagnostic later rather than
crash. This is needed to keep SILGen emitting move only addresses (that is no
implicit copy address only lets) in a form that the move only checker then
will error upon. Without this change, SILGen crashes instead of emitting an
error diagnostic in the following test:
.//test/SILOptimizer/move_only_checker_addressonly_fail.swift
Reduces the number of _ContiguousArrayStorage metadata.
In order to support constant time bridging we do need to set the correct
metadata when we bridge to Objective-C. This is so that the type check
succeeds when bridging back from Objective-C to reuse the storage
instance rather than bridging the elements.
To support dynamically setting the `_ContiguousArrayStorage` element
type i needed to add support for optimizing `alloc_ref_dynamic`
throughout the optimizer.
Possible future improvements:
* Use different metadata such that we can disambiguate native Swift
classes during destruction -- allowing native release rather then unknown
release usage.
* Optimize the newly added semantic function
getContiguousArrayStorageType
rdar://86171143
The reason why I am doing this is that currently checked_cast_br of an AnyObject
may return a different value due to boxing. As an example, this can occur when
performing a checked_cast_br of a __SwiftValue(AnyHashable(Klass())).
To model this in SIL, I added to OwnershipForwardingMixin a bit of information
that states if the instruction directly forwards ownership with a default value
of true. In checked_cast_br's constructor though I specialize this behavior if
the source type is AnyObject and thus mark the checked_cast_br as not directly
forwarding its operand. If an OwnershipForwardingMixin directly forwards
ownership, one can assume that if it forwards ownership, it will always do so in
a way that ensures that each forwarded value is rc-identical to whatever result
it algebraically forwards ownership to. So in the context of checked_cast_br, it
means that the source value is rc-identical to the argument of the success and
default blocks.
I added a verifier check to CheckedCastBr that makes sure that it can never
forward guaranteed ownership and have a source type of AnyObject.
In SemanticARCOpts, I modified the code that builds extended live ranges of
owned values (*) to check if a OwnershipForwardingMixin is directly
forwarding. If it is not directly forwarding, then we treat the use just as an
unknown consuming use. This will then prevent us from converting such an owned
value to guaranteed appropriately in such a case.
I also in SILGen needed to change checked_cast_br emission in SILGenBuilder to
perform an ensurePlusOne on the input operand (converting it to +1 with an
appropriate cleanup) if the source type is an AnyObject. I found this via the
verifier check catching this behavior from SILGen when compiling libswift. This
just shows how important IR verification of invariants can be.
(*) For those unaware, SemanticARCOpts contains a model of an owned value and
all forwarding uses of it called an OwnershipLiveRange.
rdar://85710101
I am purposely doing this in SILGen rather than at the type system level to
avoid having to have to add a bunch of boilerplate to the type system. Instead
of doing that, I am in SILGen checking for the isNoImplicitCopy bit on the
ParamDecl when we emit arguments. At that point, I set on the specific
SILArgument being emitted the bit that it is no implicit copy. In terms of
printing at the SIL level, I just printed it in front of the function argument
type like @owned, e.x.:
func myFunc(_ x: @_noImplicitCopy T) -> T {
...
}
becomes:
bb0(%0 : @noImplicitCopy @owned $T):
Some notes:
* Just to be explicit, I am making it so that no implicit copy parameters by
default are always passed at +1. The reason why I think this makes sense is
that this is the natural way of working with a move only value.
* As always, one can not write no implicit copy the attribute without passing
the flag -enable-experimental-move-only so this is NFC.
rdar://83957088
Previsouly we were evaluating a tuple elt and then performing the relevant
sub-initialization. The problem is that a sub-initialization can invoke code
that could perform an early exit cleanup. So any later tuple-elements that may
need to be cleaned up along such path will not have had their cleanups
initialized, resulting in a leak along such paths.
With this commit, we instead evaluate all of the tuple elements and only them
perform the sub-initialization ensuring that any early exits clean up all of the
tuple elements.
rdar://83770295
Use APIs for creating terminator results that handle forwarding
ownership consistently.
Add ManagedValue::forForwardedRValue(SILValue) to handle cleanups
consistently based on ownership forwarding.
Add SILGenBuilder::createForwardedTermResult(SILType type) for
creating termator results with the correct ownership and cleanups.
Add SILGenBuilder::createTermResult(SILType type, ValueOwnershipKind
ownership) that handles cleanup based on terminator result ownership.
Add SILGenBuilder::createOptionalSomeResult(SwitchEnumInst) so a lot
of code doesn't need to deal with unwrapping Optional types,
terminator results, and ownership rules.
Replace the existing "phi" APIs with a single
SILGenBuilder::createPhi(SILType, ValueOwnershipKind) that handles
cleanup based on phi ownership.
Phis and terminator results are fundamentally different and need to be handled differently everywhere. Remove the confusion where terminator results were generated with a "phi argument" API.
This makes it easier to understand conceptually why a ValueOwnershipKind with
Any ownership is invalid and also allowed me to explicitly document the lattice
that relates ownership constraints/value ownership kinds.
The Original Bug
----------------
In ffbfcfa131, we fixed a bug around implicit
value initializers but did not cherry-pick it to 5.3. While investigating a bug
that turned out to be that same bug (no worries!), I noticed that there is
additional code that is "unsafely" correct in this area and that while
ffbfcfa131 is correct in the small, we can expand
on the fix to prevent future bugs.
The Larger Bug
--------------
Here we are still open coding using ManagedValue/Cleanup APIs /without/ a top
level function scope. The code is only correct since we never emit unconditional
cleanups and always manually forward conditional cleanups. If we did not do
either of these things, we would have another instance of this bug, namely a
cleanup that is never actually emitted. So the code on master today is correct,
albeit unsafe, and we already have coverage for this (namely the test case from
ffbfcfa131).
That being said, in general when working with ManagedValue APIs (especially in
utility functions) we assume that we have a scope already created for us by our
caller. So by fixing this issue we are standardizing to safer SILGen invariants.
Building on ffbfcfa131
----------------------------------------------------
This commit builds on the shoulders of ffbfcfa131
by adding the function level scope mentioned in the previous section so that we
are now "safely" correct.
While looking at this I also realized that just using a normal scope when open
coding here may be a bit bugprone for open coding situations like this since:
1. If one just creates a scope in open coding situations, the scope will fire at
end of the c++ function /after/ one has probably emitted a return.
2. Once one has emitted the return, the insertion point will no longer be set
implying =><=.
To avoid this, I created a move only composition type on top of Scope called
AssertingManualScope. This type just asserts in its destructor if the scope it
contains has not been popped yet.
While, one can pop it by ones self, I added an overload of createReturnInst on
SILGenBuilder that also takes an AssertingManualScope and pops it at the
appropriate time.
So now when performing simple open coding tasks, we have the ability to in code
tie together the function level scope to the actual creation of return inst,
simulating the hand-off of lifetimes/resources from caller/callee that often
happens in the epilog of functions.
<rdar://problem/63189210>
This means that it can only have a guaranteed object as an operandand that we
validate that all uses of the result address of open_existential_box occur only
within the lifetime of said object's borrow scope.
(BaseT, @inout @unowned(unsafe) T) -> @guaranteed T
The reason for the weird signature is that currently the Builtin infrastructure
does not handle results well. Also, note that we are not actually performing a
call here. We are SILGening directly so we can create a guaranteed result.
The intended semantics is that one passes in a base value that guarantees the
lifetime of the unowned(unsafe) value. The builtin then:
1. Borrows the base.
2. Loads the trivial unowned (unsafe), converts that value to a guaranteed ref
after unsafely unwrapping the optional.
3. Uses mark dependence to tie the lifetimes of the guaranteed base to the
guaranteed ref.
I also updated my small UnsafeValue.swift test to make sure we get the codegen
we expect.
For those who are unaware, a transformation terminator is a terminator like
switch_enum/checked_cast_br that always dominate their successor blocks. Since
they dominate their successor blocks by design and transform their input into
the args form, we can validate that they obey guaranteed ownership semantics
just like a forwarding instruction.
Beyond removing unnecessary code bloat, this also makes it significantly more
easier to optimize/work with transformation terminators when converting @owned
-> @guaranteed since we do not need to find end_borrow points when the owned
value is consumed.
<rdar://problem/59097063>
Otherwise, if we have a tuple whose first parameter is a .none ownership
non-trivial enum, we will leak the other parameters due to us not providing a
cleanup on the created tuple.
rdar://56806959
SIL type lowering erases DynamicSelfType, so we generate
incorrect code when casting to DynamicSelfType. Fixing this
requires a fair amount of plumbing, but most of the
changes are mechanical.
Note that the textual SIL syntax for casts has changed
slightly; the target type is now a formal type without a '$',
not a SIL type.
Also, the unconditional_checked_cast_value and
checked_cast_value_br instructions now take the _source_
formal type as well, just like the *_addr forms they are
intended to replace.
This will ensure that the optimizer never eliminates the strong_retain. This
operation is meant to be unmanaged, we should respect the user's choice here
even in optimized builds.
This is a large patch; I couldn't split it up further while still
keeping things working. There are four things being changed at
once here:
- Places that call SILType::isAddressOnly()/isLoadable() now call
the SILFunction overload and not the SILModule one.
- SILFunction's overloads of getTypeLowering() and getLoweredType()
now pass the function's resilience expansion down, instead of
hardcoding ResilienceExpansion::Minimal.
- Various other places with '// FIXME: Expansion' now use a better
resilience expansion.
- A few tests were updated to reflect SILGen's improved code
generation, and some new tests are added to cover more code paths
that previously were uncovered and only manifested themselves as
standard library build failures while I was working on this change.
This is necessary since our func may want to emit conditional code with an early
exit, emitting unused cleanups from the current scope via the function
emitBranchAndCleanups(). If we have not yet created those cleanups, we will
introduce a leak along that path.
rdar://49990484
