Skip to content

Commit 525ba8c

Browse files
gottesmmgottesmm
committed
[move-only] When passing a move only struct address, pass it as a true borrowed parameter.
This ensures that the address move checker does not have to perform any unsound access scope expansions. One note is that the current approach does not work for class member refs since in SILGenLValue, a class is considered a base of the SILGenLValue access which is immediately evaluated (causing a copy) rather than evaluated later at formal evaluation time. I have a few ways of working around this issue but am going to fix it in a subsequent commit when I fix this for classes and also read coroutines. rdar://99616492
1 parent ec29497 commit 525ba8c

File tree

3 files changed

+202
-5
lines changed

3 files changed

+202
-5
lines changed

‎lib/SILGen/ArgumentSource.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ class ArgumentSource {
223223
}
224224

225225
Expr *findStorageReferenceExprForBorrow() &&;
226+
Expr *findStorageReferenceExprForMoveOnlyBorrow() &&;
226227

227228
/// Given that this source is an expression, extract and clear
228229
/// that expression.

‎lib/SILGen/SILGenApply.cpp

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,27 @@
2525
#include "Varargs.h"
2626
#include "swift/AST/ASTContext.h"
2727
#include "swift/AST/DiagnosticsSIL.h"
28+
#include "swift/AST/DistributedDecl.h"
2829
#include "swift/AST/Effects.h"
30+
#include "swift/AST/Expr.h"
2931
#include "swift/AST/ForeignAsyncConvention.h"
3032
#include "swift/AST/ForeignErrorConvention.h"
3133
#include "swift/AST/GenericEnvironment.h"
32-
#include "swift/AST/DistributedDecl.h"
3334
#include "swift/AST/GenericSignature.h"
3435
#include "swift/AST/Module.h"
3536
#include "swift/AST/ModuleLoader.h"
3637
#include "swift/AST/ParameterList.h"
3738
#include "swift/AST/SubstitutionMap.h"
3839
#include "swift/Basic/ExternalUnion.h"
3940
#include "swift/Basic/Range.h"
40-
#include "swift/Basic/SourceManager.h"
4141
#include "swift/Basic/STLExtras.h"
42+
#include "swift/Basic/SourceManager.h"
4243
#include "swift/Basic/Unicode.h"
4344
#include "swift/SIL/PrettyStackTrace.h"
4445
#include "swift/SIL/SILArgument.h"
4546
#include "clang/AST/DeclCXX.h"
46-
#include "llvm/Support/Compiler.h"
4747
#include "clang/AST/DeclObjC.h"
48+
#include "llvm/Support/Compiler.h"
4849

4950
using namespace swift;
5051
using namespace Lowering;
@@ -2921,6 +2922,25 @@ static Expr *findStorageReferenceExprForBorrow(Expr *e) {
29212922
return nullptr;
29222923
}
29232924

2925+
Expr *ArgumentSource::findStorageReferenceExprForMoveOnlyBorrow() && {
2926+
if (!isExpr())
2927+
return nullptr;
2928+
2929+
auto argExpr = asKnownExpr();
2930+
auto *li = dyn_cast<LoadExpr>(argExpr);
2931+
if (!li)
2932+
return nullptr;
2933+
2934+
auto lvExpr = ::findStorageReferenceExprForBorrow(li->getSubExpr());
2935+
2936+
// Claim the value of this argument if we found a storage reference.
2937+
if (lvExpr) {
2938+
(void)std::move(*this).asKnownExpr();
2939+
}
2940+
2941+
return lvExpr;
2942+
}
2943+
29242944
Expr *ArgumentSource::findStorageReferenceExprForBorrow() && {
29252945
if (!isExpr()) return nullptr;
29262946

@@ -3057,6 +3077,13 @@ class ArgEmitter {
30573077
return;
30583078
}
30593079

3080+
if (loweredSubstArgType.isPureMoveOnly() && param.isGuaranteed()) {
3081+
if (tryEmitBorrowedMoveOnly(std::move(arg), loweredSubstArgType,
3082+
loweredSubstParamType, origParamType,
3083+
paramSlice))
3084+
return;
3085+
}
3086+
30603087
if (SGF.silConv.isSILIndirect(param)) {
30613088
emitIndirect(std::move(arg), loweredSubstArgType, origParamType, param);
30623089
return;
@@ -3226,6 +3253,23 @@ class ArgEmitter {
32263253
return true;
32273254
}
32283255

3256+
bool tryEmitBorrowedMoveOnly(ArgumentSource &&arg,
3257+
SILType loweredSubstArgType,
3258+
SILType loweredSubstParamType,
3259+
AbstractionPattern origParamType,
3260+
ClaimedParamsRef paramsSlice) {
3261+
assert(paramsSlice.size() == 1);
3262+
3263+
// Try to find an expression we can emit as an l-value.
3264+
auto lvExpr = std::move(arg).findStorageReferenceExprForMoveOnlyBorrow();
3265+
if (!lvExpr)
3266+
return false;
3267+
3268+
emitBorrowed(lvExpr, loweredSubstArgType, loweredSubstParamType,
3269+
origParamType, paramsSlice);
3270+
return true;
3271+
}
3272+
32293273
void emitBorrowed(Expr *arg, SILType loweredSubstArgType,
32303274
SILType loweredSubstParamType,
32313275
AbstractionPattern origParamType,

‎test/SILGen/moveonly.swift

Lines changed: 154 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,39 @@
44
// Declarations //
55
//////////////////
66

7+
@_moveOnly
8+
public class Klass2 {}
9+
710
@_moveOnly
811
public class Klass {
912
var intField: Int
10-
var klsField: Klass?
13+
var klsField: Klass2
1114

1215
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly5KlassCACycfc : $@convention(method) (@owned Klass) -> @owned Klass {
1316
// CHECK: bb0([[ARG:%.*]] : @owned $Klass):
1417
// CHECK: [[MARK_UNINIT:%.*]] = mark_uninitialized [rootself] [[ARG]]
1518
// CHECK: [[MARK_NO_IMP_COPY:%.*]] = mark_must_check [no_implicit_copy] [[MARK_UNINIT]]
1619
// CHECK: } // end sil function '$s8moveonly5KlassCACycfc'
1720
init() {
18-
klsField = Klass()
21+
klsField = Klass2()
1922
intField = 5
2023
}
2124
}
2225

2326
public func nonConsumingUseKlass(_ k: Klass) {}
27+
public func nonConsumingUseKlass2(_ k: Klass2) {}
28+
29+
@_moveOnly
30+
public struct NonTrivialStruct2 {
31+
var k = Klass()
32+
}
33+
34+
public func nonConsumingUseNonTrivialStruct2(_ s: NonTrivialStruct2) {}
2435

2536
@_moveOnly
2637
public struct NonTrivialStruct {
2738
var k = Klass()
39+
var k2 = NonTrivialStruct2()
2840
}
2941

3042
public func nonConsumingUseNonTrivialStruct(_ s: NonTrivialStruct) {}
@@ -243,3 +255,143 @@ func blackHoleVarInitialization2() {
243255
x = Klass()
244256
var _ = x
245257
}
258+
259+
////////////////////////////////
260+
// Borrow Function Call Tests //
261+
////////////////////////////////
262+
263+
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly24borrowObjectFunctionCallyyF : $@convention(thin) () -> () {
264+
// CHECK: [[CLS:%.*]] = mark_must_check [no_implicit_copy]
265+
// CHECK: [[BORROW:%.*]] = begin_borrow [[CLS]]
266+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly20nonConsumingUseKlassyyAA0E0CF
267+
// CHECK: apply [[FN]]([[BORROW]])
268+
// CHECK: end_borrow [[BORROW]]
269+
// CHECK: } // end sil function '$s8moveonly24borrowObjectFunctionCallyyF'
270+
func borrowObjectFunctionCall() {
271+
let k = Klass()
272+
nonConsumingUseKlass(k)
273+
}
274+
275+
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly25borrowAddressFunctionCallyyF : $@convention(thin) () -> () {
276+
// CHECK: [[BOX:%.*]] = mark_must_check [no_implicit_copy]
277+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[BOX]]
278+
// CHECK: [[BORROW:%.*]] = load_borrow [[ACCESS]]
279+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly20nonConsumingUseKlassyyAA0E0CF
280+
// CHECK: apply [[FN]]([[BORROW]])
281+
// CHECK: end_borrow [[BORROW]]
282+
// CHECK: end_access [[ACCESS]]
283+
// CHECK: } // end sil function '$s8moveonly25borrowAddressFunctionCallyyF'
284+
func borrowAddressFunctionCall() {
285+
var k = Klass()
286+
k = Klass()
287+
nonConsumingUseKlass(k)
288+
}
289+
290+
// We currently have the wrong behavior here since the class is treated by
291+
// LValue emission as a base. That being said, move only classes are not our
292+
// high order bit here, so I filed: ...;
293+
//
294+
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly31klassBorrowAddressFunctionCall2yyF : $@convention(thin) () -> () {
295+
// CHECK: [[MARK:%.*]] = mark_must_check [no_implicit_copy]
296+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARK]]
297+
// CHECK: } // end sil function '$s8moveonly31klassBorrowAddressFunctionCall2yyF'
298+
func klassBorrowAddressFunctionCall2() {
299+
var k = Klass()
300+
k = Klass()
301+
nonConsumingUseKlass2(k.klsField)
302+
}
303+
304+
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly31structBorrowObjectFunctionCall1yyF : $@convention(thin) () -> () {
305+
// CHECK: [[VALUE:%.*]] = mark_must_check [no_implicit_copy]
306+
// CHECK: [[BORROW:%.*]] = begin_borrow [[VALUE]]
307+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly31nonConsumingUseNonTrivialStructyyAA0efG0VF : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
308+
// CHECK: apply [[FN]]([[BORROW]])
309+
// CHECK: end_borrow [[BORROW]]
310+
// CHECK: [[BORROW:%.*]] = begin_borrow [[VALUE]]
311+
// CHECK: [[STRUCT_EXT:%.*]] = struct_extract [[BORROW]]
312+
// CHECK: [[STRUCT_EXT_COPY:%.*]] = copy_value [[STRUCT_EXT]]
313+
// TODO: Should we insert a mark_must_check_here?
314+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly20nonConsumingUseKlassyyAA0E0CF
315+
// CHECK: apply [[FN]]([[STRUCT_EXT_COPY]])
316+
// CHECK: destroy_value [[STRUCT_EXT_COPY]]
317+
// CHECK: end_borrow [[BORROW]]
318+
// CHECK: [[BORROW:%.*]] = begin_borrow [[VALUE]]
319+
// CHECK: [[STRUCT_EXT:%.*]] = struct_extract [[BORROW]]
320+
// CHECK: [[STRUCT_EXT_COPY:%.*]] = copy_value [[STRUCT_EXT]]
321+
// CHECK: [[STRUCT_EXT_COPY_BORROW:%.*]] = begin_borrow [[STRUCT_EXT_COPY]]
322+
// CHECK: [[METHOD:%.*]] = class_method [[STRUCT_EXT_COPY_BORROW]]
323+
// CHECK: [[KLS2:%.*]] = apply [[METHOD]]([[STRUCT_EXT_COPY_BORROW]])
324+
// CHECK: end_borrow [[STRUCT_EXT_COPY_BORROW]]
325+
// CHECK: [[BORROWED_KLS2:%.*]] = begin_borrow [[KLS2]]
326+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly21nonConsumingUseKlass2yyAA0E0CF
327+
// CHECK: apply [[FN]]([[BORROWED_KLS2]])
328+
// CHECK: destroy_value [[STRUCT_EXT_COPY]]
329+
// CHECK: destroy_value [[KLS2]]
330+
// CHECK: end_borrow [[BORROW]]
331+
// CHECK: destroy_value [[VALUE]]
332+
// CHECK: } // end sil function '$s8moveonly31structBorrowObjectFunctionCall1yyF'
333+
func structBorrowObjectFunctionCall1() {
334+
let k = NonTrivialStruct()
335+
nonConsumingUseNonTrivialStruct(k)
336+
nonConsumingUseKlass(k.k)
337+
nonConsumingUseKlass2(k.k.klsField)
338+
}
339+
340+
// CHECK-LABEL: sil hidden [ossa] @$s8moveonly31structBorrowObjectFunctionCall2yyF : $@convention(thin) () -> () {
341+
// CHECK: [[MARKED_ADDR:%.*]] = mark_must_check [no_implicit_copy]
342+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARKED_ADDR]]
343+
// CHECK: [[BORROW:%.*]] = load_borrow [[ACCESS]]
344+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly31nonConsumingUseNonTrivialStructyyAA0efG0VF :
345+
// CHECK: apply [[FN]]([[BORROW]])
346+
// CHECK: end_borrow [[BORROW]]
347+
// CHECK: end_access [[ACCESS]]
348+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARKED_ADDR]]
349+
// CHECK: [[STRUCT_EXT:%.*]] = struct_element_addr [[ACCESS]]
350+
// CHECK: [[BORROW:%.*]] = load_borrow [[STRUCT_EXT]]
351+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly20nonConsumingUseKlassyyAA0E0CF
352+
// CHECK: apply [[FN]]([[BORROW]])
353+
// CHECK: end_borrow [[BORROW]]
354+
// CHECK: end_access [[ACCESS]]
355+
//
356+
// This case we fail due to the class being a base element. We need to use the
357+
// scope expansion approach to handle this.
358+
//
359+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARKED_ADDR]]
360+
// CHECK: [[STRUCT_EXT:%.*]] = struct_element_addr [[ACCESS]]
361+
// CHECK: [[STRUCT_EXT_COPY:%.*]] = load [copy] [[STRUCT_EXT]]
362+
// CHECK: end_access [[ACCESS]]
363+
// CHECK: [[STRUCT_EXT_COPY_BORROW:%.*]] = begin_borrow [[STRUCT_EXT_COPY]]
364+
// CHECK: [[METHOD:%.*]] = class_method [[STRUCT_EXT_COPY_BORROW]]
365+
// CHECK: [[KLS:%.*]] = apply [[METHOD]]([[STRUCT_EXT_COPY_BORROW]])
366+
// CHECK: end_borrow [[STRUCT_EXT_COPY_BORROW]]
367+
// CHECK: [[BORROWED_KLS:%.*]] = begin_borrow [[KLS]]
368+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly21nonConsumingUseKlass2yyAA0E0CF
369+
// CHECK: apply [[FN]]([[BORROWED_KLS]])
370+
// CHECK: end_borrow [[BORROWED_KLS]]
371+
// CHECK: destroy_value [[STRUCT_EXT_COPY]]
372+
// CHECK: destroy_value [[KLS]]
373+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARKED_ADDR]]
374+
// CHECK: [[GEP:%.*]] = struct_element_addr [[ACCESS]] : $*NonTrivialStruct, #NonTrivialStruct.k2
375+
// CHECK: [[BORROW:%.*]] = load_borrow [[GEP]]
376+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly32nonConsumingUseNonTrivialStruct2yyAA0efG0VF : $@convention(thin) (@guaranteed NonTrivialStruct2) -> ()
377+
// CHECK: apply [[FN]]([[BORROW]])
378+
// CHECK: end_borrow [[BORROW]]
379+
// CHECK: end_access [[ACCESS]]
380+
// CHECK: [[ACCESS:%.*]] = begin_access [read] [unknown] [[MARKED_ADDR]]
381+
// CHECK: [[GEP1:%.*]] = struct_element_addr [[ACCESS]] : $*NonTrivialStruct, #NonTrivialStruct.k2
382+
// CHECK: [[GEP2:%.*]] = struct_element_addr [[GEP1]] : $*NonTrivialStruct2, #NonTrivialStruct2.k
383+
// CHECK: [[BORROW:%.*]] = load_borrow [[GEP2]]
384+
// CHECK: [[FN:%.*]] = function_ref @$s8moveonly20nonConsumingUseKlassyyAA0E0CF : $@convention(thin) (@guaranteed Klass) -> ()
385+
// CHECK: apply [[FN]]([[BORROW]])
386+
// CHECK: end_borrow [[BORROW]]
387+
// CHECK: end_access [[ACCESS]]
388+
// CHECK: } // end sil function '$s8moveonly31structBorrowObjectFunctionCall2yyF'
389+
func structBorrowObjectFunctionCall2() {
390+
var k = NonTrivialStruct()
391+
k = NonTrivialStruct()
392+
nonConsumingUseNonTrivialStruct(k)
393+
nonConsumingUseKlass(k.k)
394+
nonConsumingUseKlass2(k.k.klsField)
395+
nonConsumingUseNonTrivialStruct2(k.k2)
396+
nonConsumingUseKlass(k.k2.k)
397+
}

0 commit comments

Comments
 (0)