Skip to content

[5.5] Fix SILGen of access to isolated static properties #38190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 2, 2021
Merged

[5.5] Fix SILGen of access to isolated static properties #38190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83cdf45
Fix SILGen of access to isolated static properties
kavon Jun 25, 2021
b0ec79c
increase strictness of hop_to_executor test
kavon Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions lib/SILGen/LValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,18 +207,26 @@ class PathComponent {
/// The only operation on this component is `project`.
class PhysicalPathComponent : public PathComponent {
virtual void _anchor() override;
Optional<ActorIsolation> ActorIso;

protected:
Optional<ActorIsolation> ActorIso;
PhysicalPathComponent(LValueTypeData typeData, KindTy Kind,
Optional<ActorIsolation> actorIso = None)
: PathComponent(typeData, Kind), ActorIso(actorIso) {
assert(isPhysical() && "PhysicalPathComponent Kind isn't physical");
}

public:
// Obtains the actor-isolation required for any loads of this component.
Optional<ActorIsolation> getActorIsolation() const { return ActorIso; }
/// Obtains and consumes the actor-isolation required for any loads of
/// this component.
Optional<ActorIsolation> takeActorIsolation() {
Optional<ActorIsolation> current = ActorIso;
ActorIso = None;
return current;
}

/// Determines whether this component has any actor-isolation.
bool hasActorIsolation() const { return ActorIso.hasValue(); }
};

inline PhysicalPathComponent &PathComponent::asPhysical() {
Expand Down
91 changes: 54 additions & 37 deletions lib/SILGen/SILGenLValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,9 +537,13 @@ namespace {
};

class EndAccessPseudoComponent : public WritebackPseudoComponent {
private:
ExecutorBreadcrumb ExecutorHop;
public:
EndAccessPseudoComponent(const LValueTypeData &typeData)
: WritebackPseudoComponent(typeData) {}
EndAccessPseudoComponent(const LValueTypeData &typeData,
ExecutorBreadcrumb &&executorHop)
: WritebackPseudoComponent(typeData),
ExecutorHop(std::move(executorHop)) {}

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

assert(base.isLValue());
SGF.B.createEndAccess(loc, base.getValue(), /*abort*/ false);
ExecutorHop.emit(SGF, loc);
}

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

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

// Only expecting global-actor isolation here, since there's no base / self.
assert(!actorIso || actorIso->isGlobalActor());
ExecutorBreadcrumb prevExecutor =
SGF.emitHopToTargetActor(loc, actorIso, /*maybeSelf=*/None);

// Enter the access.
addr = SGF.B.createBeginAccess(loc, addr, silAccessKind, enforcement,
/*hasNoNestedConflict=*/false,
Expand All @@ -576,7 +587,8 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
// Push a writeback to end it.
auto accessedMV = ManagedValue::forLValue(addr);
std::unique_ptr<LogicalPathComponent>
component(new EndAccessPseudoComponent(typeData));
component(new EndAccessPseudoComponent(typeData,
std::move(prevExecutor)));
pushWriteback(SGF, loc, std::move(component), accessedMV,
MaterializedLValue());

Expand All @@ -586,10 +598,11 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
static ManagedValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
ManagedValue addr, LValueTypeData typeData,
SGFAccessKind accessKind,
SILAccessEnforcement enforcement) {
SILAccessEnforcement enforcement,
Optional<ActorIsolation> actorIso) {
return ManagedValue::forLValue(
enterAccessScope(SGF, loc, addr.getLValueAddress(), typeData,
accessKind, enforcement));
accessKind, enforcement, actorIso));
}

// Find the base of the formal access at `address`. If the base requires an
Expand Down Expand Up @@ -717,8 +730,9 @@ namespace {
bool IsNonAccessing;
public:
RefElementComponent(VarDecl *field, LValueOptions options,
SILType substFieldType, LValueTypeData typeData)
: PhysicalPathComponent(typeData, RefElementKind),
SILType substFieldType, LValueTypeData typeData,
Optional<ActorIsolation> actorIso)
: PhysicalPathComponent(typeData, RefElementKind, actorIso),
Field(field), SubstFieldType(substFieldType),
IsNonAccessing(options.IsNonAccessing) {}

Expand All @@ -745,7 +759,8 @@ namespace {
if (!IsNonAccessing && !Field->isLet()) {
if (auto enforcement = SGF.getDynamicEnforcement(Field)) {
result = enterAccessScope(SGF, loc, result, getTypeData(),
getAccessKind(), *enforcement);
getAccessKind(), *enforcement,
takeActorIsolation());
}
}

Expand All @@ -761,7 +776,8 @@ namespace {
unsigned ElementIndex;
public:
TupleElementComponent(unsigned elementIndex, LValueTypeData typeData)
: PhysicalPathComponent(typeData, TupleElementKind),
: PhysicalPathComponent(typeData, TupleElementKind,
/*actorIsolation=*/None),
ElementIndex(elementIndex) {}

virtual bool isLoadingPure() const override { return true; }
Expand Down Expand Up @@ -790,8 +806,9 @@ namespace {
SILType SubstFieldType;
public:
StructElementComponent(VarDecl *field, SILType substFieldType,
LValueTypeData typeData)
: PhysicalPathComponent(typeData, StructElementKind),
LValueTypeData typeData,
Optional<ActorIsolation> actorIso)
: PhysicalPathComponent(typeData, StructElementKind, actorIso),
Field(field), SubstFieldType(substFieldType) {}

virtual bool isLoadingPure() const override { return true; }
Expand Down Expand Up @@ -819,9 +836,9 @@ namespace {
class ForceOptionalObjectComponent : public PhysicalPathComponent {
bool isImplicitUnwrap;
public:
ForceOptionalObjectComponent(LValueTypeData typeData,
bool isImplicitUnwrap)
: PhysicalPathComponent(typeData, OptionalObjectKind),
ForceOptionalObjectComponent(LValueTypeData typeData, bool isImplicitUnwrap)
: PhysicalPathComponent(typeData, OptionalObjectKind,
/*actorIsolation=*/None),
isImplicitUnwrap(isImplicitUnwrap) {}

ManagedValue project(SILGenFunction &SGF, SILLocation loc,
Expand All @@ -842,7 +859,8 @@ namespace {
public:
OpenOpaqueExistentialComponent(CanArchetypeType openedArchetype,
LValueTypeData typeData)
: PhysicalPathComponent(typeData, OpenOpaqueExistentialKind) {
: PhysicalPathComponent(typeData, OpenOpaqueExistentialKind,
/*actorIsolation=*/None) {
assert(getSubstFormalType() == openedArchetype);
}

Expand Down Expand Up @@ -1017,7 +1035,8 @@ namespace {

SILValue addr = Value.getLValueAddress();
addr = enterAccessScope(SGF, loc, addr, getTypeData(),
getAccessKind(), *Enforcement);
getAccessKind(), *Enforcement,
takeActorIsolation());

return ManagedValue::forLValue(addr);
}
Expand All @@ -1034,10 +1053,7 @@ namespace {
} else {
OS << "unenforced";
}
if (ActorIso) {
OS << "requires actor-hop because ";
simple_display(OS, *ActorIso);
}
if (hasActorIsolation()) OS << " requires actor-hop";
OS << "):\n";
Value.dump(OS, indent + 2);
}
Expand Down Expand Up @@ -1451,11 +1467,12 @@ namespace {
backingVar, AccessKind::Write);
} else if (BaseFormalType->mayHaveSuperclass()) {
RefElementComponent REC(backingVar, LValueOptions(), varStorageType,
typeData);
typeData, /*actorIsolation=*/None);
proj = std::move(REC).project(SGF, loc, base);
} else {
assert(BaseFormalType->getStructOrBoundGenericStruct());
StructElementComponent SEC(backingVar, varStorageType, typeData);
StructElementComponent SEC(backingVar, varStorageType,
typeData, /*actorIsolation=*/None);
proj = std::move(SEC).project(SGF, loc, base);
}

Expand Down Expand Up @@ -1782,7 +1799,8 @@ namespace {

// Enter an unsafe access scope for the access.
addr = enterAccessScope(SGF, loc, addr, getTypeData(), getAccessKind(),
SILAccessEnforcement::Unsafe);
SILAccessEnforcement::Unsafe,
ActorIso);

return addr;
}
Expand Down Expand Up @@ -2087,7 +2105,8 @@ namespace {
PhysicalKeyPathApplicationComponent(LValueTypeData typeData,
KeyPathTypeKind typeKind,
ManagedValue keyPath)
: PhysicalPathComponent(typeData, PhysicalKeyPathApplicationKind),
: PhysicalPathComponent(typeData, PhysicalKeyPathApplicationKind,
/*actorIsolation=*/None),
TypeKind(typeKind), KeyPath(keyPath) {
assert(typeKind == KPTK_KeyPath ||
typeKind == KPTK_WritableKeyPath ||
Expand Down Expand Up @@ -3274,8 +3293,6 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
using MemberStorageAccessEmitter::MemberStorageAccessEmitter;

void emitUsingStorage(LValueTypeData typeData) {
assert(!ActorIso);

// For static variables, emit a reference to the global variable backing
// them.
// FIXME: This has to be dynamically looked up for classes, and
Expand All @@ -3289,7 +3306,7 @@ void LValue::addMemberVarComponent(SILGenFunction &SGF, SILLocation loc,
typeData.getAccessKind(),
AccessStrategy::getStorage(),
FormalRValueType,
/*actorIsolation=*/None);
ActorIso);
return;
}

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

if (BaseFormalType->mayHaveSuperclass()) {
LV.add<RefElementComponent>(Storage, Options, varStorageType, typeData);
LV.add<RefElementComponent>(Storage, Options, varStorageType,
typeData, ActorIso);
} else {
assert(BaseFormalType->getStructOrBoundGenericStruct());
LV.add<StructElementComponent>(Storage, varStorageType, typeData);
LV.add<StructElementComponent>(Storage, varStorageType, typeData,
ActorIso);
}

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

// If the last component is physical, drill down and load from it.
if (component.isPhysical()) {
auto actorIso = component.asPhysical().getActorIsolation();

// If the load must happen in the context of an actor, do a hop first.
prevExecutor = emitHopToTargetActor(loc, actorIso, /*actorSelf=*/None);

auto projection = std::move(component).project(*this, loc, addr);
if (projection.getType().isAddress()) {
auto actorIso = component.asPhysical().takeActorIsolation();

// If the load must happen in the context of an actor, do a hop first.
prevExecutor = emitHopToTargetActor(loc, actorIso, /*actorSelf=*/None);
projection =
emitLoad(loc, projection.getValue(), origFormalType,
substFormalType, rvalueTL, C, IsNotTake, isBaseGuaranteed);
} else if (isReadAccessResultOwned(src.getAccessKind()) &&
!projection.isPlusOne(*this)) {
!projection.isPlusOne(*this)) {
projection = projection.copy(*this, loc);
}

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

// If we hopped to the target's executor, then we need to hop back.
prevExecutor.emit(*this, loc);

return result;
}

Expand Down
26 changes: 26 additions & 0 deletions test/Concurrency/Runtime/async_properties_actor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,21 @@
// UNSUPPORTED: use_os_stdlib
// UNSUPPORTED: back_deployment_runtime

struct Container {
@MainActor static var counter: Int = 10
@MainActor static var this: Container?

var noniso: Int = 20

static func getCount() async -> Int {
return await counter
}

static func getValue() async -> Int? {
return await this?.noniso
}
}

@propertyWrapper
struct SuccessTracker {
private var _stored: Bool
Expand Down Expand Up @@ -142,6 +157,13 @@ actor Tester {
print("done property wrapper test")
return true
}

func doContainerTest() async -> Bool {
var total: Int = 0
total += await Container.getCount()
total += await Container.getValue() ?? 0
return total == 10
}
}

@globalActor
Expand Down Expand Up @@ -171,6 +193,10 @@ var expectedResult : Bool {
fatalError("fail property wrapper test")
}

guard await test.doContainerTest() == success else {
fatalError("fail container test")
}

// CHECK: done all testing
print("done all testing")
}
Expand Down
Loading