Skip to content

Commit 59fdfad

Browse files
gottesmmgottesmm
committed
[move-only] Spill noncopyable types that are objects using store_borrow if we call a resilient function that takes it in_guaranteed.
This ensures that given a class that contains a noncopyable type that contains another noncopyable type: ``` @_moveOnly struct S2 {} @_moveOnly struct S { var s2: S2 } class C { var s: S } ``` if we call a resilient function that takes C.S.S2: ``` borrowVal(c.s.s2) ``` we properly spill s2 onto the stack using a store_borrow. Why Do This? ------------ Currently SILGenLValue treats ref_element_addr as a base that it needs to load from for both copyable and non-copyable types. We keep a separation of concerns and require emission of resilient functions to handle these loaded values. For copyable types this means copying the value and storing it into a temporary stack allocation. For noncopyable types, we never actually implemented this so we would hit an error in SILGenApply telling us that our resilient function expected an address argument, but we are passing an object. To work around this, I updated how we emit borrowed lvalue arguments to in this case to spill the value into a temporary allocation using a store_borrow. I also included a test that validates that we properly have a read exclusivity scope around the original loaded from memory for the entire call site so even though we are performing a load_borrow and then spilling it, we still have read exclusivity to the original memory for the entire region meaning that we still preserve the semantics. rdar://109171001
1 parent 7d7c929 commit 59fdfad

5 files changed

+200
-3
lines changed

‎lib/SILGen/SILGenApply.cpp

+11
Original file line numberDiff line numberDiff line change
@@ -4261,7 +4261,18 @@ static void emitBorrowedLValueRecursive(SILGenFunction &SGF,
42614261
}
42624262
}
42634263

4264+
// TODO: This does not take into account resilience, we should probably use
4265+
// getArgumentType()... but we do not have the SILFunctionType here...
42644266
assert(param.getInterfaceType() == value.getType().getASTType());
4267+
4268+
// If we have an indirect_guaranteed argument, move this using store_borrow
4269+
// into an alloc_stack.
4270+
if (param.isIndirectInGuaranteed() && value.getType().isObject()) {
4271+
SILValue alloca = SGF.emitTemporaryAllocation(loc, value.getType());
4272+
value = SGF.emitFormalEvaluationManagedStoreBorrow(loc, value.getValue(),
4273+
alloca);
4274+
}
4275+
42654276
args[argIndex++] = value;
42664277
}
42674278

‎lib/SILGen/SILGenBuilder.cpp

+10
Original file line numberDiff line numberDiff line change
@@ -778,9 +778,19 @@ ManagedValue SILGenBuilder::createStoreBorrow(SILLocation loc,
778778
SILValue address) {
779779
assert(value.getOwnershipKind() == OwnershipKind::Guaranteed);
780780
auto *sbi = createStoreBorrow(loc, value.getValue(), address);
781+
SGF.Cleanups.pushCleanup<EndBorrowCleanup>(sbi);
781782
return ManagedValue(sbi, CleanupHandle::invalid());
782783
}
783784

785+
ManagedValue SILGenBuilder::createFormalAccessStoreBorrow(SILLocation loc,
786+
ManagedValue value,
787+
SILValue address) {
788+
assert(value.getOwnershipKind() == OwnershipKind::Guaranteed);
789+
auto *sbi = createStoreBorrow(loc, value.getValue(), address);
790+
return SGF.emitFormalEvaluationManagedBorrowedRValueWithCleanup(
791+
loc, value.getValue(), sbi);
792+
}
793+
784794
ManagedValue SILGenBuilder::createStoreBorrowOrTrivial(SILLocation loc,
785795
ManagedValue value,
786796
SILValue address) {

‎lib/SILGen/SILGenBuilder.h

+2
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,8 @@ class SILGenBuilder : public SILBuilder {
202202
using SILBuilder::createStoreBorrow;
203203
ManagedValue createStoreBorrow(SILLocation loc, ManagedValue value,
204204
SILValue address);
205+
ManagedValue createFormalAccessStoreBorrow(SILLocation loc, ManagedValue value,
206+
SILValue address);
205207

206208
/// Create a store_borrow if we have a non-trivial value and a store [trivial]
207209
/// otherwise.

‎test/SILGen/moveonly_library_evolution.swift

+37
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,21 @@
11
// RUN: %target-swift-emit-silgen -enable-experimental-feature NoImplicitCopy -enable-library-evolution %s | %FileCheck %s
22

3+
////////////////////////
4+
// MARK: Declarations //
5+
////////////////////////
6+
7+
public struct EmptyStruct : ~Copyable {}
8+
public struct NonEmptyStruct : ~Copyable {
9+
var e = EmptyStruct()
10+
}
11+
public class CopyableKlass {
12+
var s = NonEmptyStruct()
13+
14+
let letStruct = NonEmptyStruct()
15+
}
16+
17+
public func borrowVal(_ x: borrowing EmptyStruct) {}
18+
319
//////////////////////
420
// MARK: DeinitTest //
521
//////////////////////
@@ -12,3 +28,24 @@ public struct DeinitTest : ~Copyable {
1228
deinit {
1329
}
1430
}
31+
32+
//////////////////////////////////////////
33+
// MARK: Caller Argument Spilling Tests //
34+
//////////////////////////////////////////
35+
36+
// CHECK-LABEL: sil [ossa] @$s26moveonly_library_evolution29callerArgumentSpillingTestArgyyAA13CopyableKlassCF : $@convention(thin) (@guaranteed CopyableKlass) -> () {
37+
// CHECK: bb0([[ARG:%.*]] : @guaranteed $CopyableKlass):
38+
// CHECK: [[ADDR:%.*]] = ref_element_addr [[ARG]]
39+
// CHECK: [[MARKED_ADDR:%.*]] = mark_must_check [no_consume_or_assign] [[ADDR]]
40+
// CHECK: [[LOADED_VALUE:%.*]] = load [copy] [[MARKED_ADDR]]
41+
// CHECK: [[BORROWED_LOADED_VALUE:%.*]] = begin_borrow [[LOADED_VALUE]]
42+
// CHECK: [[EXT:%.*]] = struct_extract [[BORROWED_LOADED_VALUE]]
43+
// CHECK: [[SPILL:%.*]] = alloc_stack $EmptyStruct
44+
// CHECK: [[STORE_BORROW:%.*]] = store_borrow [[EXT]] to [[SPILL]]
45+
// CHECK: apply {{%.*}}([[STORE_BORROW]]) : $@convention(thin) (@in_guaranteed EmptyStruct) -> ()
46+
// CHECK: end_borrow [[STORE_BORROW]]
47+
// CHECK: end_borrow [[BORROWED_LOADED_VALUE]]
48+
// CHECK: } // end sil function '$s26moveonly_library_evolution29callerArgumentSpillingTestArgyyAA13CopyableKlassCF'
49+
public func callerArgumentSpillingTestArg(_ x: CopyableKlass) {
50+
borrowVal(x.letStruct.e)
51+
}

‎test/SILOptimizer/moveonly_addresschecker_diagnostics_library_evolution.swift

+140-3
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,29 @@
1-
// RUN: %target-swift-emit-sil -sil-verify-all -verify -enable-library-evolution %s
1+
// RUN: %target-swift-emit-sil -enable-experimental-feature NoImplicitCopy -sil-verify-all -verify -enable-library-evolution %s
22

33
// This test is used to validate that we properly handle library evolution code
44
// until we can get all of the normal moveonly_addresschecker_diagnostics test
55
// case to pass.
66

77
////////////////////////
8-
// MARK: Deinit Tests //
8+
// MARK: Declarations //
99
////////////////////////
1010

1111
@_moveOnly public struct EmptyStruct {}
1212
@_moveOnly public struct NonEmptyStruct {
1313
var e = EmptyStruct()
1414
}
1515
public class CopyableKlass {
16-
var s = NonEmptyStruct()
16+
var varS = NonEmptyStruct()
17+
var letS = NonEmptyStruct()
1718
}
1819

1920
public func borrowVal(_ x: borrowing NonEmptyStruct) {}
2021
public func borrowVal(_ x: borrowing EmptyStruct) {}
22+
public func consumeVal(_ x: consuming NonEmptyStruct) {}
23+
public func consumeVal(_ x: consuming EmptyStruct) {}
24+
25+
let copyableKlassLetGlobal = CopyableKlass()
26+
var copyableKlassVarGlobal = CopyableKlass()
2127

2228
/////////////////
2329
// MARK: Tests //
@@ -36,3 +42,134 @@ public struct GenericDeinitTest<T : P> {
3642
deinit {}
3743
}
3844

45+
//////////////////////////////////////////
46+
// MARK: Caller Argument Let Spill Test //
47+
//////////////////////////////////////////
48+
49+
public func callerBorrowClassLetFieldForArgumentSpillingTestLet() {
50+
let x = CopyableKlass()
51+
borrowVal(x.letS.e)
52+
}
53+
54+
public func callerBorrowClassLetFieldForArgumentSpillingTestVar() {
55+
var x = CopyableKlass()
56+
x = CopyableKlass()
57+
borrowVal(x.letS.e)
58+
}
59+
60+
public func callerBorrowClassLetFieldForArgumentSpillingTestArg(_ x: CopyableKlass) {
61+
borrowVal(x.letS.e)
62+
}
63+
64+
public func callerBorrowClassLetFieldForArgumentSpillingTestInOutArg(_ x: inout CopyableKlass) {
65+
borrowVal(x.letS.e)
66+
}
67+
68+
public func callerBorrowClassLetFieldForArgumentSpillingTestConsumingArg(_ x: consuming CopyableKlass) {
69+
borrowVal(x.letS.e)
70+
}
71+
72+
public func callerBorrowClassLetFieldForArgumentSpillingTestLetGlobal() {
73+
borrowVal(copyableKlassLetGlobal.letS.e)
74+
}
75+
76+
public func callerBorrowClassLetFieldForArgumentSpillingTestVarGlobal() {
77+
borrowVal(copyableKlassVarGlobal.letS.e)
78+
}
79+
80+
public func callerConsumeClassLetFieldForArgumentSpillingTestLet() {
81+
let x = CopyableKlass()
82+
consumeVal(x.letS.e)
83+
}
84+
85+
public func callerConsumeClassLetFieldForArgumentSpillingTestVar() {
86+
var x = CopyableKlass()
87+
x = CopyableKlass()
88+
consumeVal(x.letS.e)
89+
}
90+
91+
public func callerConsumeClassLetFieldForArgumentSpillingTestArg(_ x: CopyableKlass) {
92+
consumeVal(x.letS.e)
93+
}
94+
95+
public func callerConsumeClassLetFieldForArgumentSpillingTestInOutArg(_ x: inout CopyableKlass) {
96+
consumeVal(x.letS.e)
97+
}
98+
99+
public func callerConsumeClassLetFieldForArgumentSpillingTestConsumingArg(_ x: consuming CopyableKlass) {
100+
consumeVal(x.letS.e)
101+
}
102+
103+
public func callerConsumeClassLetFieldForArgumentSpillingTestLetGlobal() {
104+
consumeVal(copyableKlassLetGlobal.letS.e)
105+
}
106+
107+
public func callerConsumeClassLetFieldForArgumentSpillingTestVarGlobal() {
108+
consumeVal(copyableKlassVarGlobal.letS.e)
109+
}
110+
111+
////////////////////
112+
// MARK: Var Test //
113+
////////////////////
114+
115+
public func callerBorrowClassVarFieldForArgumentSpillingTestLet() {
116+
let x = CopyableKlass()
117+
borrowVal(x.varS.e)
118+
}
119+
120+
public func callerBorrowClassVarFieldForArgumentSpillingTestVar() {
121+
var x = CopyableKlass()
122+
x = CopyableKlass()
123+
borrowVal(x.varS.e)
124+
}
125+
126+
public func callerBorrowClassVarFieldForArgumentSpillingTestArg(_ x: CopyableKlass) {
127+
borrowVal(x.varS.e)
128+
}
129+
130+
public func callerBorrowClassVarFieldForArgumentSpillingTestInOutArg(_ x: inout CopyableKlass) {
131+
borrowVal(x.varS.e)
132+
}
133+
134+
public func callerBorrowClassVarFieldForArgumentSpillingTestConsumingArg(_ x: consuming CopyableKlass) {
135+
borrowVal(x.varS.e)
136+
}
137+
138+
public func callerBorrowClassVarFieldForArgumentSpillingTestLetGlobal() {
139+
borrowVal(copyableKlassLetGlobal.varS.e)
140+
}
141+
142+
public func callerBorrowClassVarFieldForArgumentSpillingTestVarGlobal() {
143+
borrowVal(copyableKlassVarGlobal.varS.e)
144+
}
145+
146+
public func callerConsumeClassVarFieldForArgumentSpillingTestLet() {
147+
let x = CopyableKlass()
148+
consumeVal(x.varS.e)
149+
}
150+
151+
public func callerConsumeClassVarFieldForArgumentSpillingTestVar() {
152+
var x = CopyableKlass()
153+
x = CopyableKlass()
154+
consumeVal(x.varS.e)
155+
}
156+
157+
public func callerConsumeClassVarFieldForArgumentSpillingTestArg(_ x: CopyableKlass) {
158+
consumeVal(x.varS.e)
159+
}
160+
161+
public func callerConsumeClassVarFieldForArgumentSpillingTestInOutArg(_ x: inout CopyableKlass) {
162+
consumeVal(x.varS.e)
163+
}
164+
165+
public func callerConsumeClassVarFieldForArgumentSpillingTestConsumingArg(_ x: consuming CopyableKlass) {
166+
consumeVal(x.varS.e)
167+
}
168+
169+
public func callerConsumeClassVarFieldForArgumentSpillingTestLetGlobal() {
170+
consumeVal(copyableKlassLetGlobal.varS.e)
171+
}
172+
173+
public func callerConsumeClassVarFieldForArgumentSpillingTestVarGlobal() {
174+
consumeVal(copyableKlassVarGlobal.varS.e)
175+
}

0 commit comments

Comments
 (0)