Skip to content

Commit 05f03b0

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 a07b476 commit 05f03b0

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 ||
@@ -3275,8 +3294,6 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32753294
using MemberStorageAccessEmitter::MemberStorageAccessEmitter;
32763295

32773296
void emitUsingStorage(LValueTypeData typeData) {
3278-
assert(!ActorIso);
3279-
32803297
// For static variables, emit a reference to the global variable backing
32813298
// them.
32823299
// FIXME: This has to be dynamically looked up for classes, and
@@ -3290,7 +3307,7 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32903307
typeData.getAccessKind(),
32913308
AccessStrategy::getStorage(),
32923309
FormalRValueType,
3293-
/*actorIsolation=*/None);
3310+
ActorIso);
32943311
return;
32953312
}
32963313

@@ -3299,10 +3316,12 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
32993316
SGF.getTypeExpansionContext(), Storage, FormalRValueType);
33003317

33013318
if (BaseFormalType->mayHaveSuperclass()) {
3302-
LV.add<RefElementComponent>(Storage, Options, varStorageType, typeData);
3319+
LV.add<RefElementComponent>(Storage, Options, varStorageType,
3320+
typeData, ActorIso);
33033321
} else {
33043322
assert(BaseFormalType->getStructOrBoundGenericStruct());
3305-
LV.add<StructElementComponent>(Storage, varStorageType, typeData);
3323+
LV.add<StructElementComponent>(Storage, varStorageType, typeData,
3324+
ActorIso);
33063325
}
33073326

33083327
// If the member has weak or unowned storage, convert it away.
@@ -4175,18 +4194,17 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
41754194

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

@@ -4199,7 +4217,6 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
41994217

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

‎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)