Skip to content

Commit 19329e3

Browse files
jckarterjckarter
committed
SILGen: Treat read-formal-accessed lvalues as borrows.
A read access asserts that the memory location is immutable for the duration of the access, so it can be treated as a borrow rather than a mutable lvalue. Doing this allows the borrow formal access scope fixes from #79084 to apply to situations where a loadable type undergoes an accessor-based access with indirect arguments (such as for public accessors when library evolution is enabled for the type). Fixes rdar://143334632.
1 parent 85faa52 commit 19329e3

File tree

8 files changed

+73
-23
lines changed

8 files changed

+73
-23
lines changed

‎include/swift/SIL/SILInstruction.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8895,7 +8895,9 @@ class CopyValueInst
88958895
friend class SILBuilder;
88968896

88978897
CopyValueInst(SILDebugLocation DebugLoc, SILValue operand)
8898-
: UnaryInstructionBase(DebugLoc, operand, operand->getType()) {}
8898+
: UnaryInstructionBase(DebugLoc, operand, operand->getType()) {
8899+
assert(operand->getType().isObject());
8900+
}
88998901
};
89008902

89018903
class ExplicitCopyValueInst

‎lib/SILGen/ManagedValue.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,16 @@
2323
using namespace swift;
2424
using namespace Lowering;
2525

26+
ManagedValue ManagedValue::forFormalAccessedAddress(SILValue address,
27+
SGFAccessKind accessKind) {
28+
if (isReadAccess(accessKind)) {
29+
return forBorrowedAddressRValue(address);
30+
} else {
31+
return forLValue(address);
32+
}
33+
}
34+
35+
2636
ManagedValue ManagedValue::forForwardedRValue(SILGenFunction &SGF,
2737
SILValue value) {
2838
if (!value)

‎lib/SILGen/ManagedValue.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ namespace Lowering {
3434

3535
class Initialization;
3636
class SILGenFunction;
37+
enum class SGFAccessKind : uint8_t;
3738

3839
/// ManagedValue - represents a singular SIL value and an optional cleanup.
3940
/// Ownership of the ManagedValue can be "forwarded" to disable its cleanup when
@@ -252,6 +253,13 @@ class ManagedValue {
252253
static ManagedValue forInContext() {
253254
return ManagedValue(SILValue(), true, CleanupHandle::invalid());
254255
}
256+
257+
/// Creates a managed value for an address that is undergoing a formal
258+
/// access. This will be `forLValue` if the `accessKind` is a mutating
259+
/// (exclusive) access or `forBorrowedRValueAddress` if the
260+
/// `accessKind` is borrowing (shared).
261+
static ManagedValue forFormalAccessedAddress(SILValue address,
262+
SGFAccessKind accessKind);
255263

256264
bool isValid() const {
257265
return valueAndFlag.getInt() || valueAndFlag.getPointer();

‎lib/SILGen/SILGenLValue.cpp

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -618,7 +618,8 @@ namespace {
618618
bool isFinal) override {
619619
loc.markAutoGenerated();
620620

621-
assert(base.isLValue());
621+
assert(base.isLValue() ||
622+
(base.isPlusZero() && base.getType().isAddress()));
622623
loc.markAutoGenerated();
623624
SGF.B.createEndAccess(loc, base.getValue(), /*abort*/ false);
624625
ExecutorHop.emit(SGF, loc);
@@ -655,7 +656,8 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
655656
/*fromBuiltin=*/false);
656657

657658
// Push a writeback to end it.
658-
auto accessedMV = ManagedValue::forLValue(addr);
659+
ManagedValue accessedMV
660+
= ManagedValue::forFormalAccessedAddress(addr, accessKind);
659661
std::unique_ptr<LogicalPathComponent> component(
660662
new EndAccessPseudoComponent(typeData, std::move(prevExecutor)));
661663
pushWriteback(SGF, loc, std::move(component), accessedMV,
@@ -673,7 +675,7 @@ static ManagedValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
673675
bool noNestedConflict = false) {
674676
auto access = enterAccessScope(SGF, loc, base, addr.getValue(), typeData,
675677
accessKind, enforcement, actorIso, noNestedConflict);
676-
return ManagedValue::forLValue(access);
678+
return ManagedValue::forFormalAccessedAddress(access, accessKind);
677679
}
678680

679681
// Find the base of the formal access at `address`. If the base requires an
@@ -1192,7 +1194,7 @@ namespace {
11921194
enterAccessScope(SGF, loc, base, addr, getTypeData(), getAccessKind(),
11931195
*Enforcement, takeActorIsolation());
11941196

1195-
return ManagedValue::forLValue(addr);
1197+
return ManagedValue::forFormalAccessedAddress(addr, getAccessKind());
11961198
}
11971199

11981200
bool isRValue() const override {
@@ -2292,7 +2294,20 @@ namespace {
22922294
static ManagedValue
22932295
makeBaseConsumableMaterializedRValue(SILGenFunction &SGF,
22942296
SILLocation loc, ManagedValue base) {
2295-
if (base.isLValue()) {
2297+
if (!SGF.useLoweredAddresses()
2298+
&& base.getType().isTrivial(SGF.F)
2299+
&& base.getType().isAddress()
2300+
&& !base.isLValue()) {
2301+
return SGF.emitLoad(loc, base.getValue(),
2302+
SGF.getTypeLowering(base.getType()), SGFContext(),
2303+
IsNotTake);
2304+
}
2305+
2306+
bool isBorrowed = base.isPlusZeroRValueOrTrivial()
2307+
&& !base.getType().isTrivial(SGF.F);
2308+
2309+
if (base.isLValue()
2310+
|| (isBorrowed && base.getType().isAddress())) {
22962311
if (SGF.useLoweredAddresses()) {
22972312
auto tmp = SGF.emitTemporaryAllocation(loc, base.getType());
22982313
SGF.B.createCopyAddr(loc, base.getValue(), tmp, IsNotTake,
@@ -2304,8 +2319,6 @@ makeBaseConsumableMaterializedRValue(SILGenFunction &SGF,
23042319
IsNotTake);
23052320
}
23062321

2307-
bool isBorrowed = base.isPlusZeroRValueOrTrivial()
2308-
&& !base.getType().isTrivial(SGF.F);
23092322
if (!base.getType().isAddress() || isBorrowed) {
23102323
if (SGF.useLoweredAddresses()) {
23112324
auto tmp = SGF.emitTemporaryAllocation(loc, base.getType());

‎test/Constraints/keypath_dynamic_member_lookup.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,15 @@ var lens = Lens(Rectangle(topLeft: topLeft,
4040
bottomRight: bottomRight))
4141

4242
// CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs15WritableKeyPathCyxqd__G_tcluig
43-
// CHECK-NEXT: apply %45<Rectangle, Point>({{.*}})
43+
// CHECK-NEXT: apply %{{[0-9]+}}<Rectangle, Point>({{.*}})
4444
// CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs7KeyPathCyxqd__G_tcluig
45-
// CHECK-NEXT: apply %{{.*}}<Point, Int>({{.*}})
45+
// CHECK-NEXT: apply %{{[0-9]+}}<Point, Int>({{.*}})
4646
_ = lens.topLeft.x
4747

4848
// CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs15WritableKeyPathCyxqd__G_tcluig
49-
// CHECK-NEXT: apply %68<Rectangle, Point>({{.*}})
49+
// CHECK-NEXT: apply %{{[0-9]+}}<Rectangle, Point>({{.*}})
5050
// CHECK: function_ref @$s29keypath_dynamic_member_lookup4LensV0B6MemberACyqd__Gs15WritableKeyPathCyxqd__G_tcluig
51-
// CHECK-NEXT: apply {{%[0-9]+}}<Point, Int>({{.*}})
51+
// CHECK-NEXT: apply %{{[0-9]+}}<Point, Int>({{.*}})
5252
_ = lens.topLeft.y
5353

5454
lens.topLeft = Lens(Point(x: 1, y: 2)) // Ok

‎test/SILGen/indirect_accessor_access.swift

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// rdar://143334632
2+
3+
// RUN: %target-swift-emit-silgen -enable-library-evolution %s | %FileCheck %s
4+
5+
public struct Foo {
6+
// CHECK-LABEL: sil {{.*}} @$s{{.*}}3FooV3foo
7+
mutating func foo() -> Int {
8+
// CHECK: [[BEGIN_READ:%.*]] = begin_access [read] [unknown] %0
9+
// CHECK: ([[RESULT:%.*]], [[TOKEN:%.*]]) = begin_apply %{{.*}}([[BEGIN_READ]])
10+
// CHECK: end_apply [[TOKEN]]
11+
// CHECK: end_access [[BEGIN_READ]]
12+
// CHECK: return [[RESULT]]
13+
return self[]
14+
}
15+
16+
var object : AnyObject
17+
18+
subscript() -> Int {
19+
_read {fatalError()}
20+
_modify {fatalError()}
21+
}
22+
}

‎test/SILOptimizer/init_accessor_raw_sil_lowering.swift

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -433,10 +433,8 @@ func test_handling_of_nonmutating_set() {
433433
// CHECK: [[INIT_VALUE:%.*]] = function_ref @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF4TestL_V5countSivpfi : $@convention(thin) () -> Int
434434
// CHECK-NEXT: [[VALUE:%.*]] = apply [[INIT_VALUE]]() : $@convention(thin) () -> Int
435435
// CHECK: assign_or_init [init] #<abstract function>Test.count, self [[SELF]] : $*Test, value [[VALUE]] : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set {{.*}} : $@noescape @callee_guaranteed (Int) -> ()
436-
// CHECK: [[SELF_REF:%.*]] = begin_access [read] [static] [[SELF]] : $*Test
437-
// CHECK: assign_or_init [set] #<abstract function>Test.count, self [[SELF_REF]] : $*Test, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set {{.*}} : $@noescape @callee_guaranteed (Int) -> ()
438-
// CHECK: [[SELF_REF:%.*]] = begin_access [read] [static] [[SELF]] : $*Test
439-
// CHECK: assign_or_init [set] #<abstract function>Test.count, self [[SELF_REF]] : $*Test, value [[ZERO:%.*]] : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set {{.*}} : $@noescape @callee_guaranteed (Int) -> ()
436+
// CHECK: assign_or_init [set] #<abstract function>Test.count, self [[SELF]] : $*Test, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set {{.*}} : $@noescape @callee_guaranteed (Int) -> ()
437+
// CHECK: assign_or_init [set] #<abstract function>Test.count, self [[SELF]] : $*Test, value [[ZERO:%.*]] : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set {{.*}} : $@noescape @callee_guaranteed (Int) -> ()
440438
init(count: Int) {
441439
self.count = count
442440
self.count = 0
@@ -456,29 +454,26 @@ func test_handling_of_nonmutating_set() {
456454

457455
// CHECK-LABEL: sil private [ossa] @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF14TestWithStoredL_V5countADSi_tcfC
458456
// CHECK: [[SELF_REF:%.*]] = mark_uninitialized [rootself] %2
459-
// CHECK: [[SELF_ACCESS:%.*]] = begin_access [read] [static] [[SELF_REF]] : $*TestWithStored
460457
// CHECK: [[SETTER_REF:%.*]] = function_ref @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF14TestWithStoredL_V5countSivs : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
461458
// CHECK: [[SELF:%.*]] = load [copy] {{.*}} : $*TestWithStored
462459
// CHECK-NEXT: [[SETTER_CLOSURE:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[SETTER_REF]]([[SELF]]) : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
463-
// CHECK-NEXT: assign_or_init [init] #<abstract function>TestWithStored.count, self [[SELF_ACCESS]] : $*TestWithStored, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
460+
// CHECK-NEXT: assign_or_init [init] #<abstract function>TestWithStored.count, self [[SELF_REF]] : $*TestWithStored, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
464461
init(count: Int) {
465462
self.count = count
466463
}
467464

468465
// CHECK-LABEL: sil private [ossa] @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF14TestWithStoredL_V5valueADSi_tcfC
469466
// CHECK: [[SELF:%.*]] = mark_uninitialized [rootself] %2
470467
//
471-
// CHECK: [[SELF_REF:%.*]] = begin_access [read] [static] [[SELF]] : $*TestWithStored
472468
// CHECK: [[SETTER_REF:%.*]] = function_ref @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF14TestWithStoredL_V5countSivs : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
473469
// CHECK: [[SELF_COPY:%.*]] = load [copy] {{.*}} : $*TestWithStored
474470
// CHECK-NEXT: [[SETTER_CLOSURE:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[SETTER_REF]]([[SELF_COPY]]) : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
475-
// CHECK-NEXT: assign_or_init [init] #<abstract function>TestWithStored.count, self [[SELF_REF]] : $*TestWithStored, value {{.*}} : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
471+
// CHECK-NEXT: assign_or_init [init] #<abstract function>TestWithStored.count, self [[SELF]] : $*TestWithStored, value {{.*}} : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
476472
//
477-
// CHECK: [[SELF_REF:%.*]] = begin_access [read] [static] [[SELF]] : $*TestWithStored
478473
// CHECK: [[SETTER_REF:%.*]] = function_ref @$s23assign_or_init_lowering32test_handling_of_nonmutating_setyyF14TestWithStoredL_V5countSivs : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
479474
// CHECK: [[SELF_COPY:%.*]] = load [copy] {{.*}} : $*TestWithStored
480475
// CHECK-NEXT: [[SETTER_CLOSURE:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[SETTER_REF]]([[SELF_COPY]]) : $@convention(method) (Int, @guaranteed TestWithStored) -> ()
481-
// CHECK-NEXT: assign_or_init [set] #<abstract function>TestWithStored.count, self [[SELF_REF]] : $*TestWithStored, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
476+
// CHECK-NEXT: assign_or_init [set] #<abstract function>TestWithStored.count, self [[SELF]] : $*TestWithStored, value %0 : $Int, init {{.*}} : $@noescape @callee_guaranteed (Int) -> @out Int, set [[SETTER_CLOSURE]] : $@noescape @callee_guaranteed (Int) -> ()
482477
init(value: Int) {
483478
self.count = 0
484479
self.count = value

‎test/SILOptimizer/issue-68875.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ struct Test {
3131
// CHECK-NEXT: [[INIT:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[INIT_REF]]([[METATYPE]]) : $@convention(thin) (@owned String, @thin Test.Type) -> @out String
3232
// CHECK: [[SETTER_REF:%.*]] = function_ref @$s4main4TestV4nameSSvs : $@convention(method) (@owned String, @in_guaranteed Test) -> ()
3333
// CHECK-NEXT: [[SETTER:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[SETTER_REF]]([[SELF_REF]]) : $@convention(method) (@owned String, @in_guaranteed Test) -> ()
34-
// CHECK-NEXT: assign_or_init #Test.name, self [[SELF_REF]] : $*Test, value {{.*}} : $String, init [[INIT]] : $@noescape @callee_guaranteed (@owned String) -> @out String, set [[SETTER]] : $@noescape @callee_guaranteed (@owned String) -> ()
34+
// CHECK-NEXT: assign_or_init #Test.name, self [[SELF]] : $*Test, value {{.*}} : $String, init [[INIT]] : $@noescape @callee_guaranteed (@owned String) -> @out String, set [[SETTER]] : $@noescape @callee_guaranteed (@owned String) -> ()
3535
init(id: ID, name: String) {
3636
self.id = id
3737
self.name = name

0 commit comments

Comments
 (0)