Skip to content

Commit aa16864

Browse files
eecksteineeckstein
committed
SIL: verify store_borrow
Verify that * the destination address is an alloc_stack * the stack location is not modified beside a store_borrow * the stack location has been initialized when used
1 parent e064ff3 commit aa16864

File tree

6 files changed

+190
-12
lines changed

6 files changed

+190
-12
lines changed

‎lib/SIL/Verifier/MemoryLifetime.cpp

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,7 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
347347
break;
348348
case SILInstructionKind::LoadInst:
349349
case SILInstructionKind::StoreInst:
350+
case SILInstructionKind::StoreBorrowInst:
350351
case SILInstructionKind::EndAccessInst:
351352
case SILInstructionKind::LoadBorrowInst:
352353
case SILInstructionKind::DestroyAddrInst:
@@ -608,6 +609,9 @@ class MemoryLifetimeVerifier {
608609
SILFunction *function;
609610
MemoryLocations locations;
610611

612+
/// alloc_stack memory locations which are used for store_borrow.
613+
Bits storeBorrowLocations;
614+
611615
/// Returns true if the enum location \p locIdx can be proven to hold a
612616
/// hold a trivial value (e non-payload case) at \p atInst.
613617
bool isEnumTrivialAt(int locIdx, SILInstruction *atInst);
@@ -640,6 +644,18 @@ class MemoryLifetimeVerifier {
640644
/// \p addr, are set in \p bits.
641645
void requireBitsSet(const Bits &bits, SILValue addr, SILInstruction *where);
642646

647+
bool isStoreBorrowLocation(SILValue addr) {
648+
auto *loc = locations.getLocation(addr);
649+
return loc && storeBorrowLocations.anyCommon(loc->subLocations);
650+
}
651+
652+
/// Require that the location of addr is not an alloc_stack used for a
653+
/// store_borrow.
654+
void requireNoStoreBorrowLocation(SILValue addr, SILInstruction *where);
655+
656+
/// Register the destination address of a store_borrow as borrowed location.
657+
void registerStoreBorrowLocation(SILValue addr);
658+
643659
/// Handles locations of the predecessor's terminator, which are only valid
644660
/// in \p block.
645661
/// Example: @out results of try_apply. They are only valid in the
@@ -800,6 +816,21 @@ void MemoryLifetimeVerifier::requireBitsSet(const Bits &bits, SILValue addr,
800816
}
801817
}
802818

819+
void MemoryLifetimeVerifier::requireNoStoreBorrowLocation(SILValue addr,
820+
SILInstruction *where) {
821+
if (isStoreBorrowLocation(addr)) {
822+
reportError("store-borrow location cannot be written",
823+
locations.getLocation(addr)->selfAndParents.find_first(), where);
824+
}
825+
}
826+
827+
void MemoryLifetimeVerifier::registerStoreBorrowLocation(SILValue addr) {
828+
if (auto *loc = locations.getLocation(addr)) {
829+
storeBorrowLocations.resize(locations.getNumLocations());
830+
storeBorrowLocations |= loc->subLocations;
831+
}
832+
}
833+
803834
void MemoryLifetimeVerifier::initDataflow(MemoryDataflow &dataFlow) {
804835
// Initialize the entry and exit sets to all-bits-set. Except for the function
805836
// entry.
@@ -849,6 +880,12 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
849880
case SILInstructionKind::StoreInst:
850881
state.genBits(cast<StoreInst>(&I)->getDest(), locations);
851882
break;
883+
case SILInstructionKind::StoreBorrowInst: {
884+
SILValue destAddr = cast<StoreBorrowInst>(&I)->getDest();
885+
state.genBits(destAddr, locations);
886+
registerStoreBorrowLocation(destAddr);
887+
break;
888+
}
852889
case SILInstructionKind::CopyAddrInst: {
853890
auto *CAI = cast<CopyAddrInst>(&I);
854891
if (CAI->isTakeOfSrc())
@@ -1019,6 +1056,7 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
10191056
switch (LI->getOwnershipQualifier()) {
10201057
case LoadOwnershipQualifier::Take:
10211058
locations.clearBits(bits, LI->getOperand());
1059+
requireNoStoreBorrowLocation(LI->getOperand(), &I);
10221060
break;
10231061
case LoadOwnershipQualifier::Copy:
10241062
case LoadOwnershipQualifier::Trivial:
@@ -1044,19 +1082,29 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
10441082
case StoreOwnershipQualifier::Unqualified:
10451083
llvm_unreachable("unqualified store shouldn't be in ownership SIL");
10461084
}
1085+
requireNoStoreBorrowLocation(SI->getDest(), &I);
1086+
break;
1087+
}
1088+
case SILInstructionKind::StoreBorrowInst: {
1089+
SILValue destAddr = cast<StoreBorrowInst>(&I)->getDest();
1090+
locations.setBits(bits, destAddr);
1091+
registerStoreBorrowLocation(destAddr);
10471092
break;
10481093
}
10491094
case SILInstructionKind::CopyAddrInst: {
10501095
auto *CAI = cast<CopyAddrInst>(&I);
10511096
requireBitsSet(bits, CAI->getSrc(), &I);
1052-
if (CAI->isTakeOfSrc())
1097+
if (CAI->isTakeOfSrc()) {
10531098
locations.clearBits(bits, CAI->getSrc());
1099+
requireNoStoreBorrowLocation(CAI->getSrc(), &I);
1100+
}
10541101
if (CAI->isInitializationOfDest()) {
10551102
requireBitsClear(bits & nonTrivialLocations, CAI->getDest(), &I);
10561103
} else {
10571104
requireBitsSet(bits | ~nonTrivialLocations, CAI->getDest(), &I);
10581105
}
10591106
locations.setBits(bits, CAI->getDest());
1107+
requireNoStoreBorrowLocation(CAI->getDest(), &I);
10601108
break;
10611109
}
10621110
case SILInstructionKind::InjectEnumAddrInst: {
@@ -1068,25 +1116,31 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
10681116
requireBitsClear(bits & nonTrivialLocations, IEAI->getOperand(), &I);
10691117
locations.setBits(bits, IEAI->getOperand());
10701118
}
1119+
requireNoStoreBorrowLocation(IEAI->getOperand(), &I);
10711120
break;
10721121
}
1073-
case SILInstructionKind::InitEnumDataAddrInst:
1074-
requireBitsClear(bits, cast<InitEnumDataAddrInst>(&I)->getOperand(), &I);
1122+
case SILInstructionKind::InitEnumDataAddrInst: {
1123+
SILValue enumAddr = cast<InitEnumDataAddrInst>(&I)->getOperand();
1124+
requireBitsClear(bits, enumAddr, &I);
1125+
requireNoStoreBorrowLocation(enumAddr, &I);
10751126
break;
1127+
}
10761128
case SILInstructionKind::UncheckedTakeEnumDataAddrInst: {
10771129
// Note that despite the name, unchecked_take_enum_data_addr does _not_
10781130
// "take" the payload of the Swift.Optional enum. This is a terrible
10791131
// hack in SIL.
1080-
auto *UTEDAI = cast<UncheckedTakeEnumDataAddrInst>(&I);
1081-
int enumIdx = locations.getLocationIdx(UTEDAI->getOperand());
1132+
SILValue enumAddr = cast<UncheckedTakeEnumDataAddrInst>(&I)->getOperand();
1133+
int enumIdx = locations.getLocationIdx(enumAddr);
10821134
if (enumIdx >= 0)
1083-
requireBitsSet(bits, UTEDAI->getOperand(), &I);
1135+
requireBitsSet(bits, enumAddr, &I);
1136+
requireNoStoreBorrowLocation(enumAddr, &I);
10841137
break;
10851138
}
10861139
case SILInstructionKind::DestroyAddrInst: {
10871140
SILValue opVal = cast<DestroyAddrInst>(&I)->getOperand();
10881141
requireBitsSet(bits | ~nonTrivialLocations, opVal, &I);
10891142
locations.clearBits(bits, opVal);
1143+
requireNoStoreBorrowLocation(opVal, &I);
10901144
break;
10911145
}
10921146
case SILInstructionKind::EndBorrowInst: {
@@ -1117,7 +1171,11 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
11171171
break;
11181172
case SILInstructionKind::DeallocStackInst: {
11191173
SILValue opVal = cast<DeallocStackInst>(&I)->getOperand();
1120-
requireBitsClear(bits & nonTrivialLocations, opVal, &I);
1174+
if (isStoreBorrowLocation(opVal)) {
1175+
requireBitsSet(bits, opVal, &I);
1176+
} else {
1177+
requireBitsClear(bits & nonTrivialLocations, opVal, &I);
1178+
}
11211179
// Needed to clear any bits of trivial locations (which are not required
11221180
// to be zero).
11231181
locations.clearBits(bits, opVal);
@@ -1132,6 +1190,9 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) {
11321190
void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
11331191
SILArgumentConvention argumentConvention,
11341192
SILInstruction *applyInst) {
1193+
if (argumentConvention != SILArgumentConvention::Indirect_In_Guaranteed)
1194+
requireNoStoreBorrowLocation(argumentOp.get(), applyInst);
1195+
11351196
switch (argumentConvention) {
11361197
case SILArgumentConvention::Indirect_In:
11371198
case SILArgumentConvention::Indirect_In_Constant:
@@ -1169,6 +1230,7 @@ void MemoryLifetimeVerifier::verify() {
11691230
}
11701231
// Second step: handle single-block locations.
11711232
locations.handleSingleBlockLocations([this](SILBasicBlock *block) {
1233+
storeBorrowLocations.clear();
11721234
Bits bits(locations.getNumLocations());
11731235
checkBlock(block, bits);
11741236
});

‎lib/SIL/Verifier/SILVerifier.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2193,6 +2193,23 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
21932193
}
21942194
}
21952195

2196+
void checkStoreBorrowInst(StoreBorrowInst *SI) {
2197+
require(SI->getSrc()->getType().isObject(),
2198+
"Can't store from an address source");
2199+
require(!fnConv.useLoweredAddresses()
2200+
|| SI->getSrc()->getType().isLoadable(*SI->getFunction()),
2201+
"Can't store a non loadable type");
2202+
require(SI->getDest()->getType().isAddress(),
2203+
"Must store to an address dest");
2204+
requireSameType(SI->getDest()->getType().getObjectType(),
2205+
SI->getSrc()->getType(),
2206+
"Store operand type and dest type mismatch");
2207+
2208+
// Note: This is the current implementation and the design is not final.
2209+
require(isa<AllocStackInst>(SI->getDest()),
2210+
"store_borrow destination can only be an alloc_stack");
2211+
}
2212+
21962213
void checkAssignInst(AssignInst *AI) {
21972214
SILValue Src = AI->getSrc(), Dest = AI->getDest();
21982215
require(AI->getModule().getStage() == SILStage::Raw,

‎lib/SILOptimizer/Transforms/DestroyHoisting.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ void DestroyHoisting::getUsedLocationsOfInst(Bits &bits, SILInstruction *I) {
350350
break;
351351
case SILInstructionKind::LoadInst:
352352
case SILInstructionKind::StoreInst:
353+
case SILInstructionKind::StoreBorrowInst:
353354
case SILInstructionKind::CopyAddrInst:
354355
case SILInstructionKind::InjectEnumAddrInst:
355356
case SILInstructionKind::PartialApplyInst:

‎test/SIL/memory_lifetime.sil

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,3 +484,12 @@ bb0(%0 : $*T):
484484
return %res : $()
485485
}
486486

487+
sil [ossa] @test_store_borrow : $@convention(thin) (@guaranteed T) -> () {
488+
bb0(%0 : @guaranteed $T):
489+
%s = alloc_stack $T
490+
store_borrow %0 to %s : $*T
491+
dealloc_stack %s : $*T
492+
%res = tuple ()
493+
return %res : $()
494+
}
495+

‎test/SIL/memory_lifetime_failures.sil

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,3 +260,93 @@ bb0(%0 : $*T):
260260
return %res : $()
261261
}
262262

263+
// CHECK: SIL memory lifetime failure in @test_store_borrow_destroy: memory is not initialized, but should
264+
sil [ossa] @test_store_borrow_destroy : $@convention(thin) (@guaranteed T) -> () {
265+
bb0(%0 : @guaranteed $T):
266+
%s = alloc_stack $T
267+
store_borrow %0 to %s : $*T
268+
destroy_addr %s : $*T
269+
dealloc_stack %s : $*T
270+
%res = tuple ()
271+
return %res : $()
272+
}
273+
274+
sil [ossa] @func_with_inout_param : $@convention(thin) (@inout T) -> ()
275+
276+
// CHECK: SIL memory lifetime failure in @test_store_borrow_inout: store-borrow location cannot be written
277+
sil [ossa] @test_store_borrow_inout : $@convention(thin) (@guaranteed T) -> () {
278+
bb0(%0 : @guaranteed $T):
279+
%s = alloc_stack $T
280+
store_borrow %0 to %s : $*T
281+
%f = function_ref @func_with_inout_param : $@convention(thin) (@inout T) -> ()
282+
%a = apply %f(%s) : $@convention(thin) (@inout T) -> ()
283+
dealloc_stack %s : $*T
284+
%res = tuple ()
285+
return %res : $()
286+
}
287+
288+
// CHECK: SIL memory lifetime failure in @test_store_borrow_store: store-borrow location cannot be written
289+
sil [ossa] @test_store_borrow_store : $@convention(thin) (@guaranteed T, @owned T) -> () {
290+
bb0(%0 : @guaranteed $T, %1 : @owned $T):
291+
%s = alloc_stack $T
292+
store_borrow %0 to %s : $*T
293+
store %1 to [assign] %s : $*T
294+
dealloc_stack %s : $*T
295+
%res = tuple ()
296+
return %res : $()
297+
}
298+
299+
// CHECK: SIL memory lifetime failure in @test_store_borrow_load: store-borrow location cannot be written
300+
sil [ossa] @test_store_borrow_load : $@convention(thin) (@guaranteed T) -> @owned T {
301+
bb0(%0 : @guaranteed $T):
302+
%s = alloc_stack $T
303+
store_borrow %0 to %s : $*T
304+
%res = load [take] %s : $*T
305+
dealloc_stack %s : $*T
306+
return %res : $T
307+
}
308+
309+
// CHECK: SIL memory lifetime failure in @test_store_borrow_copy_src: store-borrow location cannot be written
310+
sil [ossa] @test_store_borrow_copy_src : $@convention(thin) (@guaranteed T) -> @out T {
311+
bb0(%0 : $*T, %1 : @guaranteed $T):
312+
%s = alloc_stack $T
313+
store_borrow %1 to %s : $*T
314+
copy_addr [take] %s to [initialization] %0 : $*T
315+
dealloc_stack %s : $*T
316+
%res = tuple ()
317+
return %res : $()
318+
}
319+
320+
// CHECK: SIL memory lifetime failure in @test_store_borrow_copy_dst: store-borrow location cannot be written
321+
sil [ossa] @test_store_borrow_copy_dst : $@convention(thin) (@in_guaranteed T, @guaranteed T) -> () {
322+
bb0(%0 : $*T, %1 : @guaranteed $T):
323+
%s = alloc_stack $T
324+
store_borrow %1 to %s : $*T
325+
copy_addr %0 to %s : $*T
326+
dealloc_stack %s : $*T
327+
%res = tuple ()
328+
return %res : $()
329+
}
330+
331+
// CHECK: SIL memory lifetime failure in @test_store_borrow_init_enum: store-borrow location cannot be written
332+
sil [ossa] @test_store_borrow_init_enum : $@convention(thin) (@guaranteed Optional<T>) -> () {
333+
bb0(%0 : @guaranteed $Optional<T>):
334+
%s = alloc_stack $Optional<T>
335+
store_borrow %0 to %s : $*Optional<T>
336+
%ie = init_enum_data_addr %s : $*Optional<T>, #Optional.some!enumelt
337+
dealloc_stack %s : $*Optional<T>
338+
%res = tuple ()
339+
return %res : $()
340+
}
341+
342+
// CHECK: SIL memory lifetime failure in @test_store_borrow_take_enum: store-borrow location cannot be written
343+
sil [ossa] @test_store_borrow_take_enum : $@convention(thin) (@guaranteed Optional<T>) -> () {
344+
bb0(%0 : @guaranteed $Optional<T>):
345+
%s = alloc_stack $Optional<T>
346+
store_borrow %0 to %s : $*Optional<T>
347+
%ue = unchecked_take_enum_data_addr %s : $*Optional<T>, #Optional.some!enumelt
348+
dealloc_stack %s : $*Optional<T>
349+
%res = tuple ()
350+
return %res : $()
351+
}
352+

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,19 +125,18 @@ bb0(%0 : @owned $Builtin.NativeObject):
125125

126126
// CHECK-LABEL: Error#: 0. Begin Error in Function: 'recursive_interior_pointer_error'
127127
// CHECK-NEXT: Found outside of lifetime use?!
128-
// CHECK-NEXT: Value: %2 = begin_borrow %0 : $KlassUser // users: %5, %3
129-
// CHECK-NEXT: Consuming User: end_borrow %2 : $KlassUser // id: %5
130-
// CHECK-NEXT: Non Consuming User: %7 = load [copy] %4 : $*Klass // user: %8
128+
// CHECK-NEXT: Value: %2 = begin_borrow %0 : $KlassUser
129+
// CHECK-NEXT: Consuming User: end_borrow %2 : $KlassUser
130+
// CHECK-NEXT: Non Consuming User: %6 = load [copy] %3 : $*Klass
131131
// CHECK-NEXT: Block: bb0
132132
// CHECK: Error#: 0. End Error in Function: 'recursive_interior_pointer_error'
133133
sil [ossa] @recursive_interior_pointer_error : $@convention(thin) (@owned KlassUser, @guaranteed Klass) -> @owned Klass {
134134
bb0(%0 : @owned $KlassUser, %0a : @guaranteed $Klass):
135135
%1 = begin_borrow %0 : $KlassUser
136136
%2 = ref_tail_addr %1 : $KlassUser, $Klass
137-
%result = store_borrow %0a to %2 : $*Klass
138137
end_borrow %1 : $KlassUser
139138
destroy_value %0 : $KlassUser
140-
%3 = load [copy] %result : $*Klass
139+
%3 = load [copy] %2 : $*Klass
141140
return %3 : $Klass
142141
}
143142

0 commit comments

Comments
 (0)