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.
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
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.
It is illegal to borrow an unowned value. Unowned values are only
allowed to have specific instantaneous uses.
In this case, an unchecked conversion is valid because there is an
assumption that within a destructor, self is guaranteed.
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>
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 prevents ManagedValue APIs on SILGenBuilder from by mistake accesing
non-conformance tracking APIs on SILBuilder.
NOTE: Eventually, we should make SILGenBuilder compose with SILGenSILBuilder and
then rename SILGenBuilder to ManagedValueBuilder. I did not do that in this PR
since it becomes very disruptive since there is still a lot of code in SILGen
that uses SILValue APIs and I have not found a concise way to write such
code. But this patch at least defines away this error which has bitten us
before.
The problem here is that we were taking advantage of swift not-trashing the
original memory location after performing a load [take] to use load [take] as a
+0 load. This breaks the ownership verifier since the load [take] is never
actually destroyed. Instead this commit changes this code to use a load_borrow.
I found this when trying to enable ownership verification on
test/SILGen/indirect_enum.swift using some out of tree work that should have
fixed all of the ownership issues with SILGenPattern. Turns out I can just
enable ownership verification with just this change... so I did that as well in
this PR.
rdar://29791263
ConvertFunction and reabstraction thunks need this attribute. Otherwise,
there is no way to identify that withoutActuallyEscaping was used
to explicitly perform a conversion.
The destination of a [without_actually_escaping] conversion always has
an escaping function type. The source may have either an escaping or
@noescape function type. The conversion itself may be a nop, and there
is nothing distinctive about it. The thing that is special about these
conversions is that the source function type may have unboxed
captures. i.e. they have @inout_aliasable parameters. Exclusivity
requires that the compiler enforce a SIL data flow invariant that
nonescaping closures with unboxed captures can never be stored or
passed as an @escaping function argument. Adding this attribute allows
the compiler to enforce the invariant in general with an escape hatch
for withoutActuallyEscaping.
For now, the accessors have been underscored as `_read` and `_modify`.
I'll prepare an evolution proposal for this feature which should allow
us to remove the underscores or, y'know, rename them to `purple` and
`lettuce`.
`_read` accessors do not make any effort yet to avoid copying the
value being yielded. I'll work on it in follow-up patches.
Opaque accesses to properties and subscripts defined with `_modify`
accessors will use an inefficient `materializeForSet` pattern that
materializes the value to a temporary instead of accessing it in-place.
That will be fixed by migrating to `modify` over `materializeForSet`,
which is next up after the `read` optimizations.
SIL ownership verification doesn't pass yet for the test cases here
because of a general fault in SILGen where borrows can outlive their
borrowed value due to being cleaned up on the general cleanup stack
when the borrowed value is cleaned up on the formal-access stack.
Michael, Andy, and I discussed various ways to fix this, but it seems
clear to me that it's not in any way specific to coroutine accesses.
rdar://35399664
closure lifetimes.
SILGen will now unconditionally emit
%cvt = convert_escape_to_noescape [guaranteed] %op
instructions. The mandatory ClosureLifetimeFixup pass ensures that %op's
lifetime spans %cvt's uses.
The code in DefiniteInitialization that handled a subset of cases is
removed.
To mark when a user of it is known to escape the value. This happens
with materializeForSet arguments which are captured and used in the
write-back. This means we need to keep the context alive until after
the write-back.
Follow-up patches to fully replace the PostponedCleanup hack in SILGen
by a mandatory SIL transformation pass to guarantee the proper lifetime
will use this flag to be more conservative when extending the lifetime.
The problem:
%pa = partial_apply %f(%some_context)
%cvt = convert_escape_to_noescape [not_guaranteed] [escaped] %pa
%ptr = %materialize_for_set(..., %cvt)
... write_back
... // <-- %pa needs to be alive until after write_back
This method was not distinguishing in between in_guaranteed and in
parameters. This would cause the curry thunk where this is used to not copy
in_guaranteed parameters before passing in the parameter to the
partial_apply. This can not affect +1 code since the curry thunk will always
have self at +1.
I also refactored code in:
1. SILGenPoly.
2. SILGenProlog.
3. SILGenConstructor.
to use this function instead of their own reimplementations of the same thing.
This should be NFC for +1 code and is tested by test updates when +0 is enabled.
rdar://34222540
Previously SwitchEnumBuilder was in SILGenBuilder.{h,cpp} and
SwitchEnumCaseFullExpr was in its own file. This really made no sense since:
1. The two classes are inherently related to each other so really should be
together in the source base.
2. SwitchEnumBuilder uses a SILGenBuilder, but is really a separate independent
concept/entity, so there really is no reason to keep it in SILGenBuilder.cpp.
Just a quick fix to eliminate something that was bugging me.
NFC.
This eliminates a case where we were not moving a cleanup onto a forwarding case
and instead just jammed the original cleanup and the new value into 1 managed
value.
NFC, but helps the ownership verifier.
rdar://34222540
I was thinking about this last night. I think the reason that this difference
came up is that people saw the SILGenBuilder SILValue based conformance APIs and
copied their calling of SILBuilder SILValue APIs. In the general case this is
not safe, since in some cases we need to add conformances.
Noticed by inspection.
This commit does two different things:
1. Changes some code paths in SILGenConvert to use SILGenBuilder APIs.
2. It eliminates a few bugs in this path exposed by running the SILGen tests
with +0 enabled.
The bug specifically is that we are assuming in this code that the input value
is always at +1, so we unconditionally forward the input value and then create
destroys on the output. This is not correct in general (since we have guaranteed
values) and occurs all the time with +0 arguments enabled.
This is tested by running the SILGen tests with +0 arguments enabled.
rdar://34222540
This is a refactor that we want in general and allows me to use
ensurePlusOne(...) to ensure we retain all values we put into the partial
apply.
rdar://34222540
at AnyObject?.
Also, ensure that @convention(block) functions can be erased to
AnyObject, and teach SILGen how to do this with unchecked_ref_cast.
Fixes a source compatibility suite regression.
This takes a series of ManagedValues and combines them together into 1 tuple. It
assumes that all non-trivial ManagedValues are all at +1 or all at +0. It leaves
the verification to the ownership verifier since this would immediately trigger
the ownership verifier when the instruction is created by the SILBuilder.
rdar://34222540
