Skip to content

Commit 5acb6c9

Browse files
committed
[move-only] Perform an exclusive borrow when passing a var to a consuming var.
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
1 parent a4fc911 commit 5acb6c9

File tree

12 files changed

+402
-46
lines changed

12 files changed

+402
-46
lines changed

include/swift/AST/StorageImpl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ enum class AccessKind : uint8_t {
8181
Write,
8282

8383
/// The access may require either reading or writing the current value.
84-
ReadWrite
84+
ReadWrite,
8585
};
8686

8787
/// Produce the aggregate access kind of the combination of two accesses.

include/swift/AST/Types.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3951,6 +3951,10 @@ class SILParameterInfo {
39513951
return getConvention() == ParameterConvention::Indirect_In_Guaranteed;
39523952
}
39533953

3954+
bool isIndirectIn() const {
3955+
return getConvention() == ParameterConvention::Indirect_In;
3956+
}
3957+
39543958
bool isIndirectInOut() const {
39553959
return getConvention() == ParameterConvention::Indirect_Inout;
39563960
}

include/swift/SIL/SILBuilder.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1777,6 +1777,24 @@ class SILBuilder {
17771777
void emitDestructureAddressOperation(SILLocation loc, SILValue operand,
17781778
SmallVectorImpl<SILValue> &result);
17791779

1780+
void emitDestructureAddressOperation(
1781+
SILLocation loc, SILValue operand,
1782+
function_ref<void(unsigned, SILValue)> result);
1783+
1784+
void emitDestructureOperation(SILLocation loc, SILValue operand,
1785+
SmallVectorImpl<SILValue> &result) {
1786+
if (operand->getType().isAddress())
1787+
return emitDestructureAddressOperation(loc, operand, result);
1788+
return emitDestructureValueOperation(loc, operand, result);
1789+
}
1790+
1791+
void emitDestructureOperation(SILLocation loc, SILValue operand,
1792+
function_ref<void(unsigned, SILValue)> result) {
1793+
if (operand->getType().isAddress())
1794+
return emitDestructureAddressOperation(loc, operand, result);
1795+
return emitDestructureValueOperation(loc, operand, result);
1796+
}
1797+
17801798
ClassMethodInst *createClassMethod(SILLocation Loc, SILValue Operand,
17811799
SILDeclRef Member, SILType MethodTy) {
17821800
return insert(new (getModule()) ClassMethodInst(

lib/SIL/IR/SILBuilder.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,25 @@ void SILBuilder::emitDestructureAddressOperation(
570570
});
571571
}
572572

573+
void SILBuilder::emitDestructureAddressOperation(
574+
SILLocation loc, SILValue v,
575+
function_ref<void(unsigned, SILValue)> results) {
576+
577+
// If we do not have a tuple or a struct, add to our results list.
578+
SILType type = v->getType();
579+
if (!(type.is<TupleType>() || type.getStructOrBoundGenericStruct())) {
580+
return;
581+
}
582+
583+
SmallVector<Projection, 16> projections;
584+
Projection::getFirstLevelProjections(v->getType(), getModule(),
585+
getTypeExpansionContext(), projections);
586+
for (auto pair : llvm::enumerate(projections)) {
587+
results(pair.index(),
588+
pair.value().createAddressProjection(*this, loc, v).get());
589+
}
590+
}
591+
573592
void SILBuilder::emitDestructureValueOperation(
574593
SILLocation loc, SILValue operand,
575594
function_ref<void(unsigned, SILValue)> func) {

lib/SIL/Verifier/SILVerifier.cpp

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2341,14 +2341,26 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
23412341

23422342
switch (BAI->getAccessKind()) {
23432343
case SILAccessKind::Init:
2344-
case SILAccessKind::Deinit:
23452344
// A signed access preserves the access marker until IRGen
2346-
require(
2347-
BAI->getEnforcement() == SILAccessEnforcement::Static ||
2348-
BAI->getEnforcement() == SILAccessEnforcement::Signed,
2349-
"init/deinit accesses cannot use non-static/non-signed enforcement");
2345+
require(BAI->getEnforcement() == SILAccessEnforcement::Static ||
2346+
BAI->getEnforcement() == SILAccessEnforcement::Signed,
2347+
"init accesses cannot use non-static/non-signed enforcement");
23502348
break;
23512349

2350+
case SILAccessKind::Deinit:
2351+
// A signed access preserves the access marker until IRGen.
2352+
//
2353+
// NOTE: We allow for deinit to be non-static before Lowered SIL to allow
2354+
// for it to be used to represent deinit accesses for move only types
2355+
// stored in classes, globals, and escaping mutable captures. This is ok
2356+
// to do since even though we allow for them to be represented there, the
2357+
// move only checker passes will always emit an error for them implying
2358+
// that we will never get to LoweredSIL and codegen.
2359+
require(BAI->getEnforcement() == SILAccessEnforcement::Static ||
2360+
BAI->getEnforcement() == SILAccessEnforcement::Signed ||
2361+
BAI->getModule().getStage() != SILStage::Lowered,
2362+
"init accesses cannot use non-static/non-signed enforcement");
2363+
break;
23522364
case SILAccessKind::Read:
23532365
case SILAccessKind::Modify:
23542366
break;
@@ -2407,10 +2419,22 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
24072419

24082420
switch (BUAI->getAccessKind()) {
24092421
case SILAccessKind::Init:
2410-
case SILAccessKind::Deinit:
24112422
require(BUAI->getEnforcement() == SILAccessEnforcement::Static,
2412-
"init/deinit accesses cannot use non-static enforcement");
2423+
"init accesses cannot use non-static enforcement");
24132424
break;
2425+
case SILAccessKind::Deinit:
2426+
// NOTE: We allow for deinit to be non-static before Lowered SIL to allow
2427+
// for it to be used to represent deinit accesses for move only types
2428+
// stored in classes, globals, and escaping mutable captures. This is ok
2429+
// to do since even though we allow for them to be represented there, the
2430+
// move only checker passes will always emit an error for them implying
2431+
// that we will never get to LoweredSIL and codegen.
2432+
require(
2433+
BUAI->getEnforcement() == SILAccessEnforcement::Static ||
2434+
BUAI->getModule().getStage() != SILStage::Lowered,
2435+
"deinit accesses cannot use non-static enforcement in Lowered SIL");
2436+
break;
2437+
24142438
case SILAccessKind::Read:
24152439
case SILAccessKind::Modify:
24162440
break;

lib/SILGen/SILGenApply.cpp

Lines changed: 154 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2439,17 +2439,21 @@ class DelayedArgument {
24392439

24402440
/// A string r-value needs to be converted to a pointer type.
24412441
RValueStringToPointer,
2442-
2442+
24432443
/// A function conversion needs to occur.
24442444
FunctionConversion,
2445-
2445+
24462446
LastRVKind = FunctionConversion,
2447-
2447+
24482448
/// This is an immutable borrow from an l-value.
24492449
BorrowedLValue,
24502450

24512451
/// A default argument that needs to be evaluated.
24522452
DefaultArgument,
2453+
2454+
/// This is a consume of an l-value. It acts like a BorrowedLValue, but we
2455+
/// use a deinit access scope.
2456+
ConsumedLValue,
24532457
};
24542458

24552459
private:
@@ -2495,10 +2499,16 @@ class DelayedArgument {
24952499
ClaimedParamsRef ParamsToEmit;
24962500
};
24972501

2502+
struct ConsumedLValueStorage {
2503+
LValue LV;
2504+
SILLocation Loc;
2505+
AbstractionPattern OrigParamType;
2506+
ClaimedParamsRef ParamsToEmit;
2507+
};
2508+
24982509
using ValueMembers =
2499-
ExternalUnionMembers<RValueStorage, LValueStorage,
2500-
DefaultArgumentStorage,
2501-
BorrowedLValueStorage>;
2510+
ExternalUnionMembers<RValueStorage, LValueStorage, DefaultArgumentStorage,
2511+
BorrowedLValueStorage, ConsumedLValueStorage>;
25022512
static ValueMembers::Index getValueMemberIndexForKind(KindTy kind) {
25032513
switch (kind) {
25042514
case InOut:
@@ -2513,6 +2523,8 @@ class DelayedArgument {
25132523
return ValueMembers::indexOf<DefaultArgumentStorage>();
25142524
case BorrowedLValue:
25152525
return ValueMembers::indexOf<BorrowedLValueStorage>();
2526+
case ConsumedLValue:
2527+
return ValueMembers::indexOf<ConsumedLValueStorage>();
25162528
}
25172529
llvm_unreachable("bad kind");
25182530
}
@@ -2586,13 +2598,18 @@ class DelayedArgument {
25862598
}
25872599

25882600
DelayedArgument(LValue &&lv, SILLocation loc,
2589-
AbstractionPattern origResultType,
2590-
ClaimedParamsRef params)
2591-
: Kind(BorrowedLValue) {
2592-
Value.emplaceAggregate<BorrowedLValueStorage>(Kind, std::move(lv), loc,
2593-
origResultType, params);
2601+
AbstractionPattern origResultType, ClaimedParamsRef params,
2602+
bool isBorrowed = true)
2603+
: Kind(isBorrowed ? BorrowedLValue : ConsumedLValue) {
2604+
if (isBorrowed) {
2605+
Value.emplaceAggregate<BorrowedLValueStorage>(Kind, std::move(lv), loc,
2606+
origResultType, params);
2607+
} else {
2608+
Value.emplaceAggregate<ConsumedLValueStorage>(Kind, std::move(lv), loc,
2609+
origResultType, params);
2610+
}
25942611
}
2595-
2612+
25962613
DelayedArgument(SILLocation loc,
25972614
ConcreteDeclRef defaultArgsOwner,
25982615
unsigned destIndex,
@@ -2654,6 +2671,10 @@ class DelayedArgument {
26542671
emitBorrowedLValue(SGF, Value.get<BorrowedLValueStorage>(Kind),
26552672
args, argIndex);
26562673
return;
2674+
case ConsumedLValue:
2675+
emitConsumedLValue(SGF, Value.get<ConsumedLValueStorage>(Kind), args,
2676+
argIndex);
2677+
return;
26572678
}
26582679
llvm_unreachable("bad kind");
26592680
}
@@ -2700,6 +2721,10 @@ class DelayedArgument {
27002721
SmallVectorImpl<ManagedValue> &args,
27012722
size_t &argIndex);
27022723

2724+
void emitConsumedLValue(SILGenFunction &SGF, ConsumedLValueStorage &info,
2725+
SmallVectorImpl<ManagedValue> &args,
2726+
size_t &argIndex);
2727+
27032728
// (value, owner)
27042729
std::pair<ManagedValue, ManagedValue>
27052730
finishOriginalExpr(SILGenFunction &SGF, Expr *expr) {
@@ -2741,6 +2766,7 @@ class DelayedArgument {
27412766
switch (Kind) {
27422767
case InOut:
27432768
case BorrowedLValue:
2769+
case ConsumedLValue:
27442770
case DefaultArgument:
27452771
llvm_unreachable("no original expr to finish in these cases");
27462772

@@ -3184,6 +3210,13 @@ class ArgEmitter {
31843210
return;
31853211
}
31863212

3213+
if (param.isConsumed()) {
3214+
if (tryEmitConsumedMoveOnly(std::move(arg), loweredSubstArgType,
3215+
loweredSubstParamType, origParamType,
3216+
paramSlice))
3217+
return;
3218+
}
3219+
31873220
if (SGF.silConv.isSILIndirect(param)) {
31883221
emitIndirect(std::move(arg), loweredSubstArgType, origParamType, param);
31893222
return;
@@ -3426,6 +3459,55 @@ class ArgEmitter {
34263459
origParamType, claimedParams);
34273460
}
34283461

3462+
bool tryEmitConsumedMoveOnly(ArgumentSource &&arg,
3463+
SILType loweredSubstArgType,
3464+
SILType loweredSubstParamType,
3465+
AbstractionPattern origParamType,
3466+
ClaimedParamsRef paramsSlice) {
3467+
assert(paramsSlice.size() == 1);
3468+
3469+
// Try to find an expression we can emit as a borrowed l-value.
3470+
auto lvExpr = std::move(arg).findStorageReferenceExprForMoveOnlyBorrow(SGF);
3471+
if (!lvExpr)
3472+
return false;
3473+
3474+
emitConsumed(lvExpr, loweredSubstArgType, loweredSubstParamType,
3475+
origParamType, paramsSlice);
3476+
3477+
return true;
3478+
}
3479+
3480+
void emitConsumed(Expr *arg, SILType loweredSubstArgType,
3481+
SILType loweredSubstParamType,
3482+
AbstractionPattern origParamType,
3483+
ClaimedParamsRef claimedParams) {
3484+
auto emissionKind = SGFAccessKind::OwnedAddressConsume;
3485+
3486+
LValue argLV = SGF.emitLValue(arg, emissionKind);
3487+
3488+
if (loweredSubstParamType.hasAbstractionDifference(Rep,
3489+
loweredSubstArgType)) {
3490+
argLV.addSubstToOrigComponent(origParamType, loweredSubstParamType);
3491+
}
3492+
3493+
DelayedArguments.emplace_back(std::move(argLV), arg, origParamType,
3494+
claimedParams, false /*is borrowed*/);
3495+
Args.push_back(ManagedValue());
3496+
}
3497+
3498+
void emitExpandedConsumed(Expr *arg, AbstractionPattern origParamType) {
3499+
CanType substArgType = arg->getType()->getCanonicalType();
3500+
auto count = getFlattenedValueCount(origParamType, substArgType);
3501+
auto claimedParams = claimNextParameters(count);
3502+
3503+
SILType loweredSubstArgType = SGF.getLoweredType(substArgType);
3504+
SILType loweredSubstParamType =
3505+
SGF.getLoweredType(origParamType, substArgType);
3506+
3507+
return emitConsumed(arg, loweredSubstArgType, loweredSubstParamType,
3508+
origParamType, claimedParams);
3509+
}
3510+
34293511
void emitDirect(ArgumentSource &&arg, SILType loweredSubstArgType,
34303512
AbstractionPattern origParamType,
34313513
SILParameterInfo param) {
@@ -3806,6 +3888,66 @@ void DelayedArgument::emitBorrowedLValue(SILGenFunction &SGF,
38063888
// That should drain all the parameters.
38073889
assert(params.empty());
38083890
}
3891+
3892+
static void emitConsumedLValueRecursive(SILGenFunction &SGF, SILLocation loc,
3893+
ManagedValue value,
3894+
AbstractionPattern origParamType,
3895+
ClaimedParamsRef &params,
3896+
MutableArrayRef<ManagedValue> args,
3897+
size_t &argIndex) {
3898+
// Recurse into tuples.
3899+
if (origParamType.isTuple()) {
3900+
SGF.B.emitDestructureOperation(
3901+
loc, value, [&](unsigned eltIndex, ManagedValue eltValue) {
3902+
auto origEltType = origParamType.getTupleElementType(eltIndex);
3903+
// Recurse.
3904+
emitConsumedLValueRecursive(SGF, loc, eltValue, origEltType, params,
3905+
args, argIndex);
3906+
});
3907+
return;
3908+
}
3909+
3910+
// Claim the next parameter.
3911+
auto param = params.front();
3912+
params = params.slice(1);
3913+
3914+
// Load if necessary.
3915+
assert(param.isConsumed() && "Should have a consumed parameter?");
3916+
if (value.getType().isAddress()) {
3917+
if (!param.isIndirectIn() || !SGF.silConv.useLoweredAddresses()) {
3918+
value = SGF.B.createFormalAccessLoadTake(loc, value);
3919+
}
3920+
}
3921+
3922+
assert(param.getInterfaceType() == value.getType().getASTType());
3923+
args[argIndex++] = value;
3924+
}
3925+
3926+
void DelayedArgument::emitConsumedLValue(SILGenFunction &SGF,
3927+
ConsumedLValueStorage &info,
3928+
SmallVectorImpl<ManagedValue> &args,
3929+
size_t &argIndex) {
3930+
// Begin the access.
3931+
auto value = SGF.emitConsumedLValue(info.Loc, std::move(info.LV));
3932+
ClaimedParamsRef params = info.ParamsToEmit;
3933+
3934+
// We inserted exactly one space in the argument array, so fix that up
3935+
// to have the right number of spaces.
3936+
if (params.size() == 0) {
3937+
args.erase(args.begin() + argIndex);
3938+
return;
3939+
} else if (params.size() > 1) {
3940+
args.insert(args.begin() + argIndex + 1, params.size() - 1, ManagedValue());
3941+
}
3942+
3943+
// Recursively expand.
3944+
emitConsumedLValueRecursive(SGF, info.Loc, value, info.OrigParamType, params,
3945+
args, argIndex);
3946+
3947+
// That should drain all the parameters.
3948+
assert(params.empty());
3949+
}
3950+
38093951
} // end anonymous namespace
38103952

38113953
namespace {

0 commit comments

Comments
 (0)