Skip to content

Commit 196994f

Browse files
meg-guptameg-gupta
committed
Fix store_borrow generation in SILGen
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
1 parent 5bfef4f commit 196994f

34 files changed

+256
-250
lines changed

‎lib/SILGen/ManagedValue.cpp

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,6 @@ ManagedValue ManagedValue::materialize(SILGenFunction &SGF,
200200
auto temporary = SGF.emitTemporaryAllocation(loc, getType());
201201
bool hadCleanup = hasCleanup();
202202

203-
// The temporary memory is +0 if the value was.
204203
if (hadCleanup) {
205204
SGF.B.emitStoreValueOperation(loc, forward(SGF), temporary,
206205
StoreOwnershipQualifier::Init);
@@ -218,9 +217,37 @@ ManagedValue ManagedValue::materialize(SILGenFunction &SGF,
218217
return ManagedValue::forOwnedAddressRValue(
219218
temporary, SGF.enterDestroyCleanup(temporary));
220219
}
220+
// The temporary memory is +0 if the value was.
221221
auto object = SGF.emitManagedBeginBorrow(loc, getValue());
222-
SGF.emitManagedStoreBorrow(loc, object.getValue(), temporary);
223-
return ManagedValue::forBorrowedAddressRValue(temporary);
222+
auto borrowedAddr =
223+
SGF.emitManagedStoreBorrow(loc, object.getValue(), temporary);
224+
return ManagedValue::forBorrowedAddressRValue(borrowedAddr.getValue());
225+
}
226+
227+
ManagedValue ManagedValue::formallyMaterialize(SILGenFunction &SGF,
228+
SILLocation loc) const {
229+
auto temporary = SGF.emitTemporaryAllocation(loc, getType());
230+
bool hadCleanup = hasCleanup();
231+
auto &lowering = SGF.getTypeLowering(getType());
232+
233+
if (hadCleanup) {
234+
SGF.B.emitStoreValueOperation(loc, forward(SGF), temporary,
235+
StoreOwnershipQualifier::Init);
236+
237+
return ManagedValue::forOwnedAddressRValue(
238+
temporary, SGF.enterDestroyCleanup(temporary));
239+
}
240+
if (lowering.isAddressOnly()) {
241+
assert(!SGF.silConv.useLoweredAddresses());
242+
auto copy = SGF.B.createCopyValue(loc, getValue());
243+
SGF.B.emitStoreValueOperation(loc, copy, temporary,
244+
StoreOwnershipQualifier::Init);
245+
return ManagedValue::forOwnedAddressRValue(
246+
temporary, SGF.enterDestroyCleanup(temporary));
247+
}
248+
auto object = SGF.emitFormalEvaluationManagedBeginBorrow(loc, getValue());
249+
return SGF.emitFormalEvaluationManagedStoreBorrow(loc, object.getValue(),
250+
temporary);
224251
}
225252

226253
void ManagedValue::print(raw_ostream &os) const {

‎lib/SILGen/ManagedValue.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,8 @@ class ManagedValue {
320320
/// exact same level of cleanup it had before.
321321
ManagedValue materialize(SILGenFunction &SGF, SILLocation loc) const;
322322

323+
ManagedValue formallyMaterialize(SILGenFunction &SGF, SILLocation loc) const;
324+
323325
/// Disable the cleanup for this value.
324326
void forwardCleanup(SILGenFunction &SGF) const;
325327

‎lib/SILGen/SILGenApply.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5722,7 +5722,11 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() {
57225722
assert(!selfParam.isIndirectMutating() &&
57235723
"passing unmaterialized r-value as inout argument");
57245724

5725-
base = base.materialize(SGF, loc);
5725+
base = base.formallyMaterialize(SGF, loc);
5726+
auto shouldTake = IsTake_t(base.hasCleanup());
5727+
base = SGF.emitFormalAccessLoad(loc, base.forward(SGF),
5728+
SGF.getTypeLowering(baseLoweredType),
5729+
SGFContext(), shouldTake);
57265730
}
57275731

57285732
return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base));

‎lib/SILGen/SILGenBridging.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,8 @@ emitBridgeNativeToObjectiveC(SILGenFunction &SGF,
154154
if (witnessConv.isSILIndirect(witnessConv.getParameters()[0])
155155
&& !swiftValue.getType().isAddress()) {
156156
auto tmp = SGF.emitTemporaryAllocation(loc, swiftValue.getType());
157-
SGF.B.createStoreBorrowOrTrivial(loc, swiftValue.borrow(SGF, loc), tmp);
158-
swiftValue = ManagedValue::forUnmanaged(tmp);
157+
swiftValue = SGF.emitManagedStoreBorrow(
158+
loc, swiftValue.borrow(SGF, loc).getValue(), tmp);
159159
}
160160

161161
// Call the witness.

‎lib/SILGen/SILGenBuilder.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -721,21 +721,23 @@ createValueMetatype(SILLocation loc, SILType metatype,
721721
return ManagedValue::forUnmanaged(v);
722722
}
723723

724-
void SILGenBuilder::createStoreBorrow(SILLocation loc, ManagedValue value,
725-
SILValue address) {
724+
ManagedValue SILGenBuilder::createStoreBorrow(SILLocation loc,
725+
ManagedValue value,
726+
SILValue address) {
726727
assert(value.getOwnershipKind() == OwnershipKind::Guaranteed);
727-
createStoreBorrow(loc, value.getValue(), address);
728+
auto *sbi = createStoreBorrow(loc, value.getValue(), address);
729+
return ManagedValue(sbi, CleanupHandle::invalid());
728730
}
729731

730-
void SILGenBuilder::createStoreBorrowOrTrivial(SILLocation loc,
731-
ManagedValue value,
732-
SILValue address) {
732+
ManagedValue SILGenBuilder::createStoreBorrowOrTrivial(SILLocation loc,
733+
ManagedValue value,
734+
SILValue address) {
733735
if (value.getOwnershipKind() == OwnershipKind::None) {
734736
createStore(loc, value, address, StoreOwnershipQualifier::Trivial);
735-
return;
737+
return ManagedValue(address, CleanupHandle::invalid());
736738
}
737739

738-
createStoreBorrow(loc, value, address);
740+
return createStoreBorrow(loc, value, address);
739741
}
740742

741743
ManagedValue SILGenBuilder::createBridgeObjectToRef(SILLocation loc,

‎lib/SILGen/SILGenBuilder.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,13 @@ class SILGenBuilder : public SILBuilder {
190190
ManagedValue createFormalAccessLoadBorrow(SILLocation loc, ManagedValue base);
191191

192192
using SILBuilder::createStoreBorrow;
193-
void createStoreBorrow(SILLocation loc, ManagedValue value, SILValue address);
193+
ManagedValue createStoreBorrow(SILLocation loc, ManagedValue value,
194+
SILValue address);
194195

195196
/// Create a store_borrow if we have a non-trivial value and a store [trivial]
196197
/// otherwise.
197-
void createStoreBorrowOrTrivial(SILLocation loc, ManagedValue value,
198-
SILValue address);
198+
ManagedValue createStoreBorrowOrTrivial(SILLocation loc, ManagedValue value,
199+
SILValue address);
199200

200201
/// Prepares a buffer to receive the result of an expression, either using the
201202
/// 'emit into' initialization buffer if available, or allocating a temporary

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -129,12 +129,13 @@ ManagedValue SILGenFunction::emitManagedStoreBorrow(
129129
assert(lowering.getLoweredType().getObjectType() == v->getType());
130130
if (lowering.isTrivial() || v->getOwnershipKind() == OwnershipKind::None) {
131131
lowering.emitStore(B, loc, v, addr, StoreOwnershipQualifier::Trivial);
132-
return ManagedValue::forUnmanaged(v);
132+
return ManagedValue::forTrivialAddressRValue(addr);
133133
}
134134
assert((!lowering.isAddressOnly() || !silConv.useLoweredAddresses()) &&
135135
"cannot retain an unloadable type");
136136
auto *sbi = B.createStoreBorrow(loc, v, addr);
137-
return emitManagedBorrowedRValueWithCleanup(sbi->getSrc(), sbi, lowering);
137+
Cleanups.pushCleanup<EndBorrowCleanup>(sbi);
138+
return ManagedValue(sbi, CleanupHandle::invalid());
138139
}
139140

140141
ManagedValue SILGenFunction::emitManagedBeginBorrow(SILLocation loc,
@@ -243,6 +244,18 @@ ManagedValue SILGenFunction::emitFormalEvaluationManagedBeginBorrow(
243244
lowering);
244245
}
245246

247+
ManagedValue SILGenFunction::emitFormalEvaluationManagedStoreBorrow(
248+
SILLocation loc, SILValue v, SILValue addr) {
249+
auto &lowering = getTypeLowering(v->getType());
250+
if (lowering.isTrivial() || v->getOwnershipKind() == OwnershipKind::None) {
251+
lowering.emitStore(B, loc, v, addr, StoreOwnershipQualifier::Trivial);
252+
return ManagedValue::forTrivialAddressRValue(addr);
253+
}
254+
auto *sbi = B.createStoreBorrow(loc, v, addr);
255+
return emitFormalEvaluationManagedBorrowedRValueWithCleanup(loc, v, sbi,
256+
lowering);
257+
}
258+
246259
ManagedValue
247260
SILGenFunction::emitFormalEvaluationManagedBorrowedRValueWithCleanup(
248261
SILLocation loc, SILValue original, SILValue borrowed) {
@@ -260,10 +273,6 @@ SILGenFunction::emitFormalEvaluationManagedBorrowedRValueWithCleanup(
260273
if (lowering.isTrivial())
261274
return ManagedValue::forUnmanaged(borrowed);
262275

263-
if (!borrowed->getType().isObject()) {
264-
return ManagedValue(borrowed, CleanupHandle::invalid());
265-
}
266-
267276
assert(isInFormalEvaluationScope() && "Must be in formal evaluation scope");
268277
auto &cleanup = Cleanups.pushCleanup<FormalEvaluationEndBorrowCleanup>();
269278
CleanupHandle handle = Cleanups.getTopCleanup();
@@ -329,10 +338,7 @@ ManagedValue SILGenFunction::emitManagedBorrowedRValueWithCleanup(
329338
original->getOwnershipKind() == OwnershipKind::None)
330339
return ManagedValue::forUnmanaged(borrowed);
331340

332-
if (borrowed->getType().isObject()) {
333-
Cleanups.pushCleanup<EndBorrowCleanup>(borrowed);
334-
}
335-
341+
Cleanups.pushCleanup<EndBorrowCleanup>(borrowed);
336342
return ManagedValue(borrowed, CleanupHandle::invalid());
337343
}
338344

‎lib/SILGen/SILGenFunction.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1544,6 +1544,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
15441544
emitFormalEvaluationManagedBeginBorrow(SILLocation loc, SILValue v,
15451545
const TypeLowering &lowering);
15461546

1547+
ManagedValue emitFormalEvaluationManagedStoreBorrow(SILLocation loc,
1548+
SILValue v,
1549+
SILValue addr);
1550+
15471551
ManagedValue emitManagedRValueWithCleanup(SILValue v);
15481552
ManagedValue emitManagedRValueWithCleanup(SILValue v,
15491553
const TypeLowering &lowering);

‎test/SIL/OwnershipVerifier/false_positive_leaks.sil

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ sil [ossa] @leak_loop_test : $@convention(thin) (@owned Builtin.NativeObject) ->
2626
bb0(%0 : @owned $Builtin.NativeObject):
2727
%1 = alloc_stack $Builtin.NativeObject
2828
%2 = begin_borrow %0 : $Builtin.NativeObject
29-
store_borrow %2 to %1 : $*Builtin.NativeObject
29+
%sbi = store_borrow %2 to %1 : $*Builtin.NativeObject
3030
%3 = function_ref @in_guaranteed_user : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
31-
apply %3(%1) : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
31+
apply %3(%sbi) : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
32+
end_borrow %sbi : $*Builtin.NativeObject
3233
end_borrow %2 : $Builtin.NativeObject
3334
dealloc_stack %1 : $*Builtin.NativeObject
3435
br bb1

‎test/SIL/OwnershipVerifier/interior_pointer.sil

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,7 @@ bb0(%0 : @owned $Box<T>, %1 : $*Int):
119119
// CHECK-NEXT: Found outside of lifetime use?!
120120
// CHECK-NEXT: Value: %1 = begin_borrow %0 : $Builtin.NativeObject // users: %4, %3
121121
// CHECK-NEXT: Consuming User: end_borrow %1 : $Builtin.NativeObject // id: %4
122-
// CHECK-NEXT: Non Consuming User: %7 = apply %6(%3) : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
123-
// CHECK-NEXT: Block: bb0
122+
// CHECK-NEXT: Non Consuming User: end_borrow %3 : $*Builtin.NativeObject
124123
sil [ossa] @store_borrow_result_used_outside_of_borrow_lifetime : $@convention(thin) (@owned Builtin.NativeObject) -> () {
125124
bb0(%0 : @owned $Builtin.NativeObject):
126125
%0a = begin_borrow %0 : $Builtin.NativeObject
@@ -130,6 +129,7 @@ bb0(%0 : @owned $Builtin.NativeObject):
130129
destroy_value %0 : $Builtin.NativeObject
131130
%func = function_ref @use_builtinnativeobject_inguaranteed : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
132131
apply %func(%result) : $@convention(thin) (@in_guaranteed Builtin.NativeObject) -> ()
132+
end_borrow %result : $*Builtin.NativeObject
133133
dealloc_stack %1 : $*Builtin.NativeObject
134134
%9999 = tuple()
135135
return %9999 : $()

0 commit comments

Comments
 (0)