Skip to content

Commit 1db06d1

Browse files
gottesmmgottesmm
committed
[sil-ownership] Allow for switch_enum to take a guaranteed parameter.
This is needed to ensure that SILGenPattern propagates values at +0 instead of +1. I also improved our handling of terminators in general when it comes to having "borrowed" arguments. The basic verification requirement is that the end_borrow_argument is treated as a use of the parent borrow. This ensures that the borrowed argument can not extend the lifetime of the borrowed value without the verifier triggering. rdar://31880847
1 parent 7b1eea6 commit 1db06d1

File tree

3 files changed

+271
-14
lines changed

3 files changed

+271
-14
lines changed

‎lib/SIL/SILOwnershipVerifier.cpp

Lines changed: 121 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,15 @@ static bool compatibleOwnershipKinds(ValueOwnershipKind K1,
174174
return K1.merge(K2).hasValue();
175175
}
176176

177+
/// Returns true if \p Kind is trivial or if \p Kind is compatible with \p
178+
/// ComparisonKind.
179+
static bool
180+
trivialOrCompatibleOwnershipKinds(ValueOwnershipKind Kind,
181+
ValueOwnershipKind ComparisonKind) {
182+
return compatibleOwnershipKinds(Kind, ValueOwnershipKind::Trivial) ||
183+
compatibleOwnershipKinds(Kind, ComparisonKind);
184+
}
185+
177186
static bool isValueAddressOrTrivial(SILValue V, SILModule &M) {
178187
return V->getType().isAddress() ||
179188
V.getOwnershipKind() == ValueOwnershipKind::Trivial ||
@@ -198,6 +207,7 @@ static bool isOwnershipForwardingValueKind(ValueKind K) {
198207
case ValueKind::UncheckedEnumDataInst:
199208
case ValueKind::MarkUninitializedInst:
200209
case ValueKind::SelectEnumInst:
210+
case ValueKind::SwitchEnumInst:
201211
return true;
202212
default:
203213
return false;
@@ -420,7 +430,6 @@ NO_OPERAND_INST(KeyPath)
420430
return {compatibleWithOwnership(ValueOwnershipKind::OWNERSHIP), \
421431
SHOULD_CHECK_FOR_DATAFLOW_VIOLATIONS}; \
422432
}
423-
CONSTANT_OWNERSHIP_INST(Guaranteed, true, EndBorrowArgument)
424433
CONSTANT_OWNERSHIP_INST(Guaranteed, false, RefElementAddr)
425434
CONSTANT_OWNERSHIP_INST(Owned, true, AutoreleaseValue)
426435
CONSTANT_OWNERSHIP_INST(Owned, true, DeallocBox)
@@ -513,7 +522,6 @@ CONSTANT_OWNERSHIP_INST(Trivial, false, DeallocValueBuffer)
513522
}
514523
CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Owned, true, CheckedCastBranch)
515524
CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Owned, true, CheckedCastValueBranch)
516-
CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Owned, true, SwitchEnum)
517525
CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Owned, true, InitExistentialOpaque)
518526
CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Owned, true, DeinitExistentialOpaque)
519527
#undef CONSTANT_OR_TRIVIAL_OWNERSHIP_INST
@@ -655,6 +663,17 @@ FORWARD_CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Guaranteed, false, TupleExtract)
655663
FORWARD_CONSTANT_OR_TRIVIAL_OWNERSHIP_INST(Guaranteed, false, StructExtract)
656664
#undef CONSTANT_OR_TRIVIAL_OWNERSHIP_INST
657665

666+
OwnershipUseCheckerResult
667+
OwnershipCompatibilityUseChecker::visitEndBorrowArgumentInst(EndBorrowArgumentInst *I) {
668+
// If we are currently checking an end_borrow_argument as a subobject, then we
669+
// treat this as just a use.
670+
if (isCheckingSubObject())
671+
return {true, false};
672+
673+
// Otherwise, we must be checking an actual argument. Make sure it is guaranteed!
674+
return {true, compatibleWithOwnership(ValueOwnershipKind::Guaranteed)};
675+
}
676+
658677
OwnershipUseCheckerResult
659678
OwnershipCompatibilityUseChecker::visitSelectEnumInst(SelectEnumInst *I) {
660679
if (getValue() == I->getEnumOperand()) {
@@ -724,6 +743,50 @@ OwnershipCompatibilityUseChecker::visitCondBranchInst(CondBranchInst *CBI) {
724743
getOperandIndex() - FalseOffset);
725744
}
726745

746+
OwnershipUseCheckerResult
747+
OwnershipCompatibilityUseChecker::visitSwitchEnumInst(SwitchEnumInst *SEI) {
748+
// If our operand was trivial, return early.
749+
if (compatibleWithOwnership(ValueOwnershipKind::Trivial))
750+
return {true, false};
751+
752+
// Then we need to go through all of our destinations and make sure that if
753+
// they have a payload, the payload's convention matches our
754+
// convention.
755+
//
756+
// *NOTE* since we are dealing with enums, we ignore trivial and no payload
757+
// enums.
758+
// *NOTE* we assume that all of our types line up and are checked by the
759+
// normal verifier.
760+
for (auto *Succ : SEI->getParent()->getSuccessorBlocks()) {
761+
// This must be a no-payload case... continue.
762+
if (Succ->args_size() == 0)
763+
continue;
764+
765+
// If we have a trivial value or a value with ownership kind that matches
766+
// the switch_enum, then continue.
767+
auto OwnershipKind = Succ->getArgument(0)->getOwnershipKind();
768+
if (OwnershipKind == ValueOwnershipKind::Trivial ||
769+
compatibleWithOwnership(OwnershipKind))
770+
continue;
771+
772+
// Otherwise, emit an error.
773+
handleError([&]() {
774+
llvm::errs()
775+
<< "Function: '" << Succ->getParent()->getName() << "'\n"
776+
<< "Error! Argument ownership kind does not match switch_enum!\n"
777+
<< "SwitchEnum: " << *SEI << "Argument: " << *Succ->getArgument(0)
778+
<< "Expected convention: " << SEI->getOperand().getOwnershipKind()
779+
<< ".\n"
780+
<< "Actual convention: " << OwnershipKind << '\n'
781+
<< '\n';
782+
});
783+
}
784+
785+
// Finally, if everything lines up, emit that we match and are a lifetime
786+
// ending point if we are owned.
787+
return {true, compatibleWithOwnership(ValueOwnershipKind::Owned)};
788+
}
789+
727790
OwnershipUseCheckerResult
728791
OwnershipCompatibilityUseChecker::visitReturnInst(ReturnInst *RI) {
729792
SILModule &M = RI->getModule();
@@ -1454,8 +1517,8 @@ void SILValueOwnershipChecker::gatherUsers(
14541517
// we need to look through subobject uses for more uses. Otherwise, if we are
14551518
// forwarding, we do not create any lifetime ending users/non lifetime ending
14561519
// users since we verify against our base.
1457-
bool IsGuaranteed =
1458-
Value.getOwnershipKind() == ValueOwnershipKind::Guaranteed;
1520+
auto OwnershipKind = Value.getOwnershipKind();
1521+
bool IsGuaranteed = OwnershipKind == ValueOwnershipKind::Guaranteed;
14591522

14601523
if (IsGuaranteed && isOwnershipForwardingValue(Value))
14611524
return;
@@ -1512,19 +1575,63 @@ void SILValueOwnershipChecker::gatherUsers(
15121575

15131576
// At this point, we know that we must have a forwarded subobject. Since the
15141577
// base type is guaranteed, we know that the subobject is either guaranteed
1515-
// or trivial. The trivial case is not interesting for ARC verification, so
1516-
// if the user has a trivial ownership kind, continue.
1517-
if (SILValue(User).getOwnershipKind() == ValueOwnershipKind::Trivial) {
1578+
// or trivial. We now split into two cases, if the user is a terminator or
1579+
// not. If we do not have a terminator, then just add User->getUses() to the
1580+
// worklist.
1581+
auto *TI = dyn_cast<TermInst>(User);
1582+
if (!TI) {
1583+
if (SILValue(User).getOwnershipKind() == ValueOwnershipKind::Trivial) {
1584+
continue;
1585+
}
1586+
1587+
// Now, we /must/ have a guaranteed subobject, so lets assert that the
1588+
// user
1589+
// is actually guaranteed and add the subobject's users to our worklist.
1590+
assert(SILValue(User).getOwnershipKind() ==
1591+
ValueOwnershipKind::Guaranteed &&
1592+
"Our value is guaranteed and this is a forwarding instruction. "
1593+
"Should have guaranteed ownership as well.");
1594+
std::copy(User->use_begin(), User->use_end(), std::back_inserter(Users));
15181595
continue;
15191596
}
15201597

1521-
// Now, we /must/ have a guaranteed subobject, so lets assert that the user
1522-
// is actually guaranteed and add the subobject's users to our worklist.
1523-
assert(SILValue(User).getOwnershipKind() ==
1524-
ValueOwnershipKind::Guaranteed &&
1525-
"Our value is guaranteed and this is a forwarding instruction. "
1526-
"Should have guaranteed ownership as well.");
1527-
std::copy(User->use_begin(), User->use_end(), std::back_inserter(Users));
1598+
// Otherwise if we have a terminator, add any as uses any
1599+
// end_borrow_argument to ensure that the subscope is completely enclsed
1600+
// within the super scope. all of the arguments to the work list. We require
1601+
// all of our arguments to be either trivial or guaranteed.
1602+
for (auto &Succ : TI->getSuccessors()) {
1603+
auto *BB = Succ.getBB();
1604+
1605+
// If we do not have any arguments, then continue.
1606+
if (BB->args_empty())
1607+
continue;
1608+
1609+
// Otherwise, make sure that all arguments are trivial or guaranteed. If
1610+
// we fail, emit an error.
1611+
//
1612+
// TODO: We could ignore this error and emit a more specific error on the
1613+
// actual terminator.
1614+
for (auto *BBArg : BB->getArguments()) {
1615+
// *NOTE* We do not emit an error here since we want to allow for more
1616+
// specific errors to be found during use_verification.
1617+
//
1618+
// TODO: Add a flag that associates the terminator instruction with
1619+
// needing to be verified. If it isn't verified appropriately, assert
1620+
// when the verifier is destroyed.
1621+
if (!trivialOrCompatibleOwnershipKinds(BBArg->getOwnershipKind(),
1622+
OwnershipKind)) {
1623+
// This is where the error would go.
1624+
continue;
1625+
}
1626+
1627+
// If we have a trivial value, just continue.
1628+
if (BBArg->getOwnershipKind() == ValueOwnershipKind::Trivial)
1629+
continue;
1630+
1631+
// Otherwise,
1632+
std::copy(BBArg->use_begin(), BBArg->use_end(), std::back_inserter(Users));
1633+
}
1634+
}
15281635
}
15291636
}
15301637

‎test/SIL/ownership-verifier/over_consume.sil

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,3 +172,74 @@ bb0(%0 : @owned $Optional<Builtin.NativeObject>):
172172
%9999 = tuple()
173173
return %9999 : $()
174174
}
175+
176+
// CHECK-LABEL: Function: 'switch_enum_mismatching_argument_guaranteed_to_owned'
177+
// CHECK: Error! Argument ownership kind does not match switch_enum!
178+
// CHECK: SwitchEnum: switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
179+
// CHECK: Argument: %2 = argument of bb1 : $Builtin.NativeObject
180+
// CHECK: Expected convention: guaranteed.
181+
// CHECK: Actual convention: owned
182+
sil @switch_enum_mismatching_argument_guaranteed_to_owned : $@convention(thin) (@guaranteed Optional<Builtin.NativeObject>) -> () {
183+
bb0(%0 : @guaranteed $Optional<Builtin.NativeObject>):
184+
switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
185+
186+
bb1(%1 : @owned $Builtin.NativeObject):
187+
destroy_value %1 : $Builtin.NativeObject
188+
br bb3
189+
190+
bb2:
191+
br bb3
192+
193+
bb3:
194+
%9999 = tuple()
195+
return %9999 : $()
196+
}
197+
198+
// CHECK-LABEL: Function: 'switch_enum_mismatching_argument_owned_to_guaranteed'
199+
// CHECK: Error! Argument ownership kind does not match switch_enum!
200+
// CHECK: SwitchEnum: switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
201+
// CHECK: Argument: %2 = argument of bb1 : $Builtin.NativeObject
202+
// CHECK: Expected convention: owned.
203+
// CHECK: Actual convention: guaranteed
204+
sil @switch_enum_mismatching_argument_owned_to_guaranteed : $@convention(thin) (@owned Optional<Builtin.NativeObject>) -> () {
205+
bb0(%0 : @owned $Optional<Builtin.NativeObject>):
206+
switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
207+
208+
bb1(%1 : @guaranteed $Builtin.NativeObject):
209+
end_borrow_argument %1 : $Builtin.NativeObject
210+
br bb3
211+
212+
bb2:
213+
br bb3
214+
215+
bb3:
216+
%9999 = tuple()
217+
return %9999 : $()
218+
}
219+
220+
221+
// CHECK-LABEL: Function: 'switch_enum_guaranteed_arg_outlives_original_value'
222+
// CHECK: Found use after free?!
223+
// CHECK: Value: %1 = begin_borrow %0 : $Optional<Builtin.NativeObject>
224+
// CHECK: Consuming User: end_borrow %1 from %0 : $Optional<Builtin.NativeObject>, $Optional<Builtin.NativeObject>
225+
// CHECK: Non Consuming User: end_borrow_argument %3 : $Builtin.NativeObject
226+
// CHECK: Block: bb1
227+
sil @switch_enum_guaranteed_arg_outlives_original_value : $@convention(thin) (@owned Optional<Builtin.NativeObject>) -> () {
228+
bb0(%0 : @owned $Optional<Builtin.NativeObject>):
229+
%1 = begin_borrow %0 : $Optional<Builtin.NativeObject>
230+
switch_enum %1 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
231+
232+
bb1(%2 : @guaranteed $Builtin.NativeObject):
233+
end_borrow %1 from %0 : $Optional<Builtin.NativeObject>, $Optional<Builtin.NativeObject>
234+
end_borrow_argument %2 : $Builtin.NativeObject
235+
br bb3
236+
237+
bb2:
238+
br bb3
239+
240+
bb3:
241+
destroy_value %0 : $Optional<Builtin.NativeObject>
242+
%9999 = tuple()
243+
return %9999 : $()
244+
}
245+

‎test/SIL/ownership-verifier/use_verifier.sil

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,85 @@ bb3:
385385
return %9999 : $()
386386
}
387387

388+
// Make sure that we can properly handle a guaranteed switch_enum.
389+
sil @switch_enum_guaranteed_arg_test : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
390+
bb0(%0 : @guaranteed $Builtin.NativeObject):
391+
%1 = enum $Optional<Builtin.NativeObject>, #Optional.some!enumelt.1, %0 : $Builtin.NativeObject
392+
switch_enum %1 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
393+
394+
bb1(%2 : @guaranteed $Builtin.NativeObject):
395+
end_borrow_argument %2 : $Builtin.NativeObject
396+
br bb3
397+
398+
bb2:
399+
br bb3
400+
401+
bb3:
402+
%9999 = tuple()
403+
return %9999 : $()
404+
}
405+
406+
sil @switch_enum_guaranteed_beginborrow_test_1a : $@convention(thin) (@owned Builtin.NativeObject) -> () {
407+
bb0(%0 : @owned $Builtin.NativeObject):
408+
%1 = begin_borrow %0 : $Builtin.NativeObject
409+
%2 = enum $Optional<Builtin.NativeObject>, #Optional.some!enumelt.1, %1 : $Builtin.NativeObject
410+
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
411+
412+
bb1(%3 : @guaranteed $Builtin.NativeObject):
413+
end_borrow_argument %3 : $Builtin.NativeObject
414+
end_borrow %1 from %0 : $Builtin.NativeObject, $Builtin.NativeObject
415+
br bb3
416+
417+
bb2:
418+
end_borrow %1 from %0 : $Builtin.NativeObject, $Builtin.NativeObject
419+
br bb3
420+
421+
bb3:
422+
destroy_value %0 : $Builtin.NativeObject
423+
%9999 = tuple()
424+
return %9999 : $()
425+
}
426+
427+
sil @switch_enum_guaranteed_beginborrow_test_1b : $@convention(thin) (@owned Builtin.NativeObject) -> () {
428+
bb0(%0 : @owned $Builtin.NativeObject):
429+
%1 = begin_borrow %0 : $Builtin.NativeObject
430+
%2 = enum $Optional<Builtin.NativeObject>, #Optional.some!enumelt.1, %1 : $Builtin.NativeObject
431+
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
432+
433+
bb1(%3 : @guaranteed $Builtin.NativeObject):
434+
end_borrow_argument %3 : $Builtin.NativeObject
435+
br bb3
436+
437+
bb2:
438+
br bb3
439+
440+
bb3:
441+
end_borrow %1 from %0 : $Builtin.NativeObject, $Builtin.NativeObject
442+
destroy_value %0 : $Builtin.NativeObject
443+
%9999 = tuple()
444+
return %9999 : $()
445+
}
446+
447+
sil @switch_enum_guaranteed_beginborrow_test_2 : $@convention(thin) (@owned Builtin.NativeObject) -> () {
448+
bb0(%0 : @owned $Builtin.NativeObject):
449+
%1 = enum $Optional<Builtin.NativeObject>, #Optional.some!enumelt.1, %0 : $Builtin.NativeObject
450+
%2 = begin_borrow %1 : $Optional<Builtin.NativeObject>
451+
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
452+
453+
bb1(%3 : @guaranteed $Builtin.NativeObject):
454+
end_borrow_argument %3 : $Builtin.NativeObject
455+
br bb3
456+
457+
bb2:
458+
br bb3
459+
460+
bb3:
461+
end_borrow %2 from %1 : $Optional<Builtin.NativeObject>, $Optional<Builtin.NativeObject>
462+
destroy_value %1 : $Optional<Builtin.NativeObject>
463+
%9999 = tuple()
464+
return %9999 : $()
465+
}
466+
388467
// Make sure that we can properly handle a switch enum case where the input
389468
// argument is an enum, but the final argument is trivial.
390469
sil @switch_enum_no_payload_test : $@convention(thin) () -> () {

0 commit comments

Comments
 (0)