Skip to content

Commit 83cdf45

Browse files
kavonkavon
committed
Fix SILGen of access to isolated static properties
When accessing a static property isolated to a global-actor, such as MainActor, a `hop_to_executor` was not being emitted. This was caught by an assertion I had left to catch such cases, but of course in release builds this will lead to incorrect SIL, etc. This issue revealed some other problems with how the implementation was done for other kinds of accesses starting from a static property, e.g., emitting a redundant hop for the same access. This was fixed by modelling the actor-isolation placed into a component as only being accessible by consuming it from the component. This prevents the emission of the hops more than once. Thus, the regression test was updated to catch unexpected hop_to_executor instructions. Resolves rdar://78292384
1 parent fd4d619 commit 83cdf45

File tree

4 files changed

+260
-42
lines changed

4 files changed

+260
-42
lines changed

‎lib/SILGen/LValue.h

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -207,18 +207,26 @@ class PathComponent {
207207
/// The only operation on this component is `project`.
208208
class PhysicalPathComponent : public PathComponent {
209209
virtual void _anchor() override;
210+
Optional<ActorIsolation> ActorIso;
210211

211212
protected:
212-
Optional<ActorIsolation> ActorIso;
213213
PhysicalPathComponent(LValueTypeData typeData, KindTy Kind,
214214
Optional<ActorIsolation> actorIso = None)
215215
: PathComponent(typeData, Kind), ActorIso(actorIso) {
216216
assert(isPhysical() && "PhysicalPathComponent Kind isn't physical");
217217
}
218218

219219
public:
220-
// Obtains the actor-isolation required for any loads of this component.
221-
Optional<ActorIsolation> getActorIsolation() const { return ActorIso; }
220+
/// Obtains and consumes the actor-isolation required for any loads of
221+
/// this component.
222+
Optional<ActorIsolation> takeActorIsolation() {
223+
Optional<ActorIsolation> current = ActorIso;
224+
ActorIso = None;
225+
return current;
226+
}
227+
228+
/// Determines whether this component has any actor-isolation.
229+
bool hasActorIsolation() const { return ActorIso.hasValue(); }
222230
};
223231

224232
inline PhysicalPathComponent &PathComponent::asPhysical() {

‎lib/SILGen/SILGenLValue.cpp

Lines changed: 54 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -537,9 +537,13 @@ namespace {
537537
};
538538

539539
class EndAccessPseudoComponent : public WritebackPseudoComponent {
540+
private:
541+
ExecutorBreadcrumb ExecutorHop;
540542
public:
541-
EndAccessPseudoComponent(const LValueTypeData &typeData)
542-
: WritebackPseudoComponent(typeData) {}
543+
EndAccessPseudoComponent(const LValueTypeData &typeData,
544+
ExecutorBreadcrumb &&executorHop)
545+
: WritebackPseudoComponent(typeData),
546+
ExecutorHop(std::move(executorHop)) {}
543547

544548
private:
545549
void writeback(SILGenFunction &SGF, SILLocation loc,
@@ -550,6 +554,7 @@ namespace {
550554

551555
assert(base.isLValue());
552556
SGF.B.createEndAccess(loc, base.getValue(), /*abort*/ false);
557+
ExecutorHop.emit(SGF, loc);
553558
}
554559

555560
void dump(raw_ostream &OS, unsigned indent) const override {
@@ -561,13 +566,19 @@ namespace {
561566
static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
562567
SILValue addr, LValueTypeData typeData,
563568
SGFAccessKind accessKind,
564-
SILAccessEnforcement enforcement) {
569+
SILAccessEnforcement enforcement,
570+
Optional<ActorIsolation> actorIso) {
565571
auto silAccessKind = isReadAccess(accessKind) ? SILAccessKind::Read
566572
: SILAccessKind::Modify;
567573

568574
assert(SGF.isInFormalEvaluationScope() &&
569575
"tried to enter access scope without a writeback scope!");
570576

577+
// Only expecting global-actor isolation here, since there's no base / self.
578+
assert(!actorIso || actorIso->isGlobalActor());
579+
ExecutorBreadcrumb prevExecutor =
580+
SGF.emitHopToTargetActor(loc, actorIso, /*maybeSelf=*/None);
581+
571582
// Enter the access.
572583
addr = SGF.B.createBeginAccess(loc, addr, silAccessKind, enforcement,
573584
/*hasNoNestedConflict=*/false,
@@ -576,7 +587,8 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
576587
// Push a writeback to end it.
577588
auto accessedMV = ManagedValue::forLValue(addr);
578589
std::unique_ptr<LogicalPathComponent>
579-
component(new EndAccessPseudoComponent(typeData));
590+
component(new EndAccessPseudoComponent(typeData,
591+
std::move(prevExecutor)));
580592
pushWriteback(SGF, loc, std::move(component), accessedMV,
581593
MaterializedLValue());
582594

@@ -586,10 +598,11 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
586598
static ManagedValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
587599
ManagedValue addr, LValueTypeData typeData,
588600
SGFAccessKind accessKind,
589-
SILAccessEnforcement enforcement) {
601+
SILAccessEnforcement enforcement,
602+
Optional<ActorIsolation> actorIso) {
590603
return ManagedValue::forLValue(
591604
enterAccessScope(SGF, loc, addr.getLValueAddress(), typeData,
592-
accessKind, enforcement));
605+
accessKind, enforcement, actorIso));
593606
}
594607

595608
// Find the base of the formal access at `address`. If the base requires an
@@ -717,8 +730,9 @@ namespace {
717730
bool IsNonAccessing;
718731
public:
719732
RefElementComponent(VarDecl *field, LValueOptions options,
720-
SILType substFieldType, LValueTypeData typeData)
721-
: PhysicalPathComponent(typeData, RefElementKind),
733+
SILType substFieldType, LValueTypeData typeData,
734+
Optional<ActorIsolation> actorIso)
735+
: PhysicalPathComponent(typeData, RefElementKind, actorIso),
722736
Field(field), SubstFieldType(substFieldType),
723737
IsNonAccessing(options.IsNonAccessing) {}
724738

@@ -745,7 +759,8 @@ namespace {
745759
if (!IsNonAccessing && !Field->isLet()) {
746760
if (auto enforcement = SGF.getDynamicEnforcement(Field)) {
747761
result = enterAccessScope(SGF, loc, result, getTypeData(),
748-
getAccessKind(), *enforcement);
762+
getAccessKind(), *enforcement,
763+
takeActorIsolation());
749764
}
750765
}
751766

@@ -761,7 +776,8 @@ namespace {
761776
unsigned ElementIndex;
762777
public:
763778
TupleElementComponent(unsigned elementIndex, LValueTypeData typeData)
764-
: PhysicalPathComponent(typeData, TupleElementKind),
779+
: PhysicalPathComponent(typeData, TupleElementKind,
780+
/*actorIsolation=*/None),
765781
ElementIndex(elementIndex) {}
766782

767783
virtual bool isLoadingPure() const override { return true; }
@@ -790,8 +806,9 @@ namespace {
790806
SILType SubstFieldType;
791807
public:
792808
StructElementComponent(VarDecl *field, SILType substFieldType,
793-
LValueTypeData typeData)
794-
: PhysicalPathComponent(typeData, StructElementKind),
809+
LValueTypeData typeData,
810+
Optional<ActorIsolation> actorIso)
811+
: PhysicalPathComponent(typeData, StructElementKind, actorIso),
795812
Field(field), SubstFieldType(substFieldType) {}
796813

797814
virtual bool isLoadingPure() const override { return true; }
@@ -819,9 +836,9 @@ namespace {
819836
class ForceOptionalObjectComponent : public PhysicalPathComponent {
820837
bool isImplicitUnwrap;
821838
public:
822-
ForceOptionalObjectComponent(LValueTypeData typeData,
823-
bool isImplicitUnwrap)
824-
: PhysicalPathComponent(typeData, OptionalObjectKind),
839+
ForceOptionalObjectComponent(LValueTypeData typeData, bool isImplicitUnwrap)
840+
: PhysicalPathComponent(typeData, OptionalObjectKind,
841+
/*actorIsolation=*/None),
825842
isImplicitUnwrap(isImplicitUnwrap) {}
826843

827844
ManagedValue project(SILGenFunction &SGF, SILLocation loc,
@@ -842,7 +859,8 @@ namespace {
842859
public:
843860
OpenOpaqueExistentialComponent(CanArchetypeType openedArchetype,
844861
LValueTypeData typeData)
845-
: PhysicalPathComponent(typeData, OpenOpaqueExistentialKind) {
862+
: PhysicalPathComponent(typeData, OpenOpaqueExistentialKind,
863+
/*actorIsolation=*/None) {
846864
assert(getSubstFormalType() == openedArchetype);
847865
}
848866

@@ -1017,7 +1035,8 @@ namespace {
10171035

10181036
SILValue addr = Value.getLValueAddress();
10191037
addr = enterAccessScope(SGF, loc, addr, getTypeData(),
1020-
getAccessKind(), *Enforcement);
1038+
getAccessKind(), *Enforcement,
1039+
takeActorIsolation());
10211040

10221041
return ManagedValue::forLValue(addr);
10231042
}
@@ -1034,10 +1053,7 @@ namespace {
10341053
} else {
10351054
OS << "unenforced";
10361055
}
1037-
if (ActorIso) {
1038-
OS << "requires actor-hop because ";
1039-
simple_display(OS, *ActorIso);
1040-
}
1056+
if (hasActorIsolation()) OS << " requires actor-hop";
10411057
OS << "):\n";
10421058
Value.dump(OS, indent + 2);
10431059
}
@@ -1451,11 +1467,12 @@ namespace {
14511467
backingVar, AccessKind::Write);
14521468
} else if (BaseFormalType->mayHaveSuperclass()) {
14531469
RefElementComponent REC(backingVar, LValueOptions(), varStorageType,
1454-
typeData);
1470+
typeData, /*actorIsolation=*/None);
14551471
proj = std::move(REC).project(SGF, loc, base);
14561472
} else {
14571473
assert(BaseFormalType->getStructOrBoundGenericStruct());
1458-
StructElementComponent SEC(backingVar, varStorageType, typeData);
1474+
StructElementComponent SEC(backingVar, varStorageType,
1475+
typeData, /*actorIsolation=*/None);
14591476
proj = std::move(SEC).project(SGF, loc, base);
14601477
}
14611478

@@ -1782,7 +1799,8 @@ namespace {
17821799

17831800
// Enter an unsafe access scope for the access.
17841801
addr = enterAccessScope(SGF, loc, addr, getTypeData(), getAccessKind(),
1785-
SILAccessEnforcement::Unsafe);
1802+
SILAccessEnforcement::Unsafe,
1803+
ActorIso);
17861804

17871805
return addr;
17881806
}
@@ -2087,7 +2105,8 @@ namespace {
20872105
PhysicalKeyPathApplicationComponent(LValueTypeData typeData,
20882106
KeyPathTypeKind typeKind,
20892107
ManagedValue keyPath)
2090-
: PhysicalPathComponent(typeData, PhysicalKeyPathApplicationKind),
2108+
: PhysicalPathComponent(typeData, PhysicalKeyPathApplicationKind,
2109+
/*actorIsolation=*/None),
20912110
TypeKind(typeKind), KeyPath(keyPath) {
20922111
assert(typeKind == KPTK_KeyPath ||
20932112
typeKind == KPTK_WritableKeyPath ||
@@ -3274,8 +3293,6 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32743293
using MemberStorageAccessEmitter::MemberStorageAccessEmitter;
32753294

32763295
void emitUsingStorage(LValueTypeData typeData) {
3277-
assert(!ActorIso);
3278-
32793296
// For static variables, emit a reference to the global variable backing
32803297
// them.
32813298
// FIXME: This has to be dynamically looked up for classes, and
@@ -3289,7 +3306,7 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32893306
typeData.getAccessKind(),
32903307
AccessStrategy::getStorage(),
32913308
FormalRValueType,
3292-
/*actorIsolation=*/None);
3309+
ActorIso);
32933310
return;
32943311
}
32953312

@@ -3298,10 +3315,12 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32983315
SGF.getTypeExpansionContext(), Storage, FormalRValueType);
32993316

33003317
if (BaseFormalType->mayHaveSuperclass()) {
3301-
LV.add<RefElementComponent>(Storage, Options, varStorageType, typeData);
3318+
LV.add<RefElementComponent>(Storage, Options, varStorageType,
3319+
typeData, ActorIso);
33023320
} else {
33033321
assert(BaseFormalType->getStructOrBoundGenericStruct());
3304-
LV.add<StructElementComponent>(Storage, varStorageType, typeData);
3322+
LV.add<StructElementComponent>(Storage, varStorageType, typeData,
3323+
ActorIso);
33053324
}
33063325

33073326
// If the member has weak or unowned storage, convert it away.
@@ -4174,18 +4193,17 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
41744193

41754194
// If the last component is physical, drill down and load from it.
41764195
if (component.isPhysical()) {
4177-
auto actorIso = component.asPhysical().getActorIsolation();
4178-
4179-
// If the load must happen in the context of an actor, do a hop first.
4180-
prevExecutor = emitHopToTargetActor(loc, actorIso, /*actorSelf=*/None);
4181-
41824196
auto projection = std::move(component).project(*this, loc, addr);
41834197
if (projection.getType().isAddress()) {
4198+
auto actorIso = component.asPhysical().takeActorIsolation();
4199+
4200+
// If the load must happen in the context of an actor, do a hop first.
4201+
prevExecutor = emitHopToTargetActor(loc, actorIso, /*actorSelf=*/None);
41844202
projection =
41854203
emitLoad(loc, projection.getValue(), origFormalType,
41864204
substFormalType, rvalueTL, C, IsNotTake, isBaseGuaranteed);
41874205
} else if (isReadAccessResultOwned(src.getAccessKind()) &&
4188-
!projection.isPlusOne(*this)) {
4206+
!projection.isPlusOne(*this)) {
41894207
projection = projection.copy(*this, loc);
41904208
}
41914209

@@ -4198,7 +4216,6 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
41984216

41994217
// If we hopped to the target's executor, then we need to hop back.
42004218
prevExecutor.emit(*this, loc);
4201-
42024219
return result;
42034220
}
42044221

‎test/Concurrency/Runtime/async_properties_actor.swift

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,21 @@
88
// UNSUPPORTED: use_os_stdlib
99
// UNSUPPORTED: back_deployment_runtime
1010

11+
struct Container {
12+
@MainActor static var counter: Int = 10
13+
@MainActor static var this: Container?
14+
15+
var noniso: Int = 20
16+
17+
static func getCount() async -> Int {
18+
return await counter
19+
}
20+
21+
static func getValue() async -> Int? {
22+
return await this?.noniso
23+
}
24+
}
25+
1126
@propertyWrapper
1227
struct SuccessTracker {
1328
private var _stored: Bool
@@ -142,6 +157,13 @@ actor Tester {
142157
print("done property wrapper test")
143158
return true
144159
}
160+
161+
func doContainerTest() async -> Bool {
162+
var total: Int = 0
163+
total += await Container.getCount()
164+
total += await Container.getValue() ?? 0
165+
return total == 10
166+
}
145167
}
146168

147169
@globalActor
@@ -171,6 +193,10 @@ var expectedResult : Bool {
171193
fatalError("fail property wrapper test")
172194
}
173195

196+
guard await test.doContainerTest() == success else {
197+
fatalError("fail container test")
198+
}
199+
174200
// CHECK: done all testing
175201
print("done all testing")
176202
}

0 commit comments

Comments
 (0)