Skip to content

[Exclusivity] Improve static enforcement diagnostics #8952

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 1 commit into from
Apr 23, 2017
Merged

[Exclusivity] Improve static enforcement diagnostics #8952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions include/swift/AST/DiagnosticsSIL.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,17 @@ NOTE(previous_inout_alias,none,
// This is temporarily a warning during staging to make it easier to evaluate.
// The intent is to change it to an error before turning it on by default.
WARNING(exclusivity_access_required,none,
"simultaneous accesses to %0 %1; "
"%select{initialization|read|modification|deinitialization}2 requires "
"exclusive access", (DescriptiveDeclKind, Identifier, unsigned))

WARNING(exclusivity_access_required_unknown_decl,none,
"simultaneous accesses; "
"%select{initialization|read|modification|deinitialization}0 requires "
"%select{exclusive|shared}1 access", (unsigned, unsigned))
"exclusive access", (unsigned))

NOTE(exclusivity_conflicting_access,none,
"conflicting %select{initialization|read|modification|deinitialization}0 "
"requires %select{exclusive|shared}1 access", (unsigned, unsigned))
"conflicting access is here", ())

ERROR(unsupported_c_function_pointer_conversion,none,
"C function pointer signature %0 is not compatible with expected type %1",
Expand Down
72 changes: 59 additions & 13 deletions lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "swift/AST/Decl.h"
#include "swift/AST/Expr.h"
#include "swift/AST/Stmt.h"
#include "swift/Basic/SourceLoc.h"
#include "swift/SIL/CFG.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILInstruction.h"
Expand Down Expand Up @@ -135,6 +136,29 @@ class AccessedStorage {
assert(Kind == AccessedStorageKind::ClassProperty);
return ObjProj;
}

/// Returns the ValueDecl for the underlying storage, if it can be
/// determined. Otherwise returns null. For diagonostic purposes.
const ValueDecl *getStorageDecl() const {
switch(Kind) {
case AccessedStorageKind::GlobalVar:
return getGlobal()->getDecl();
case AccessedStorageKind::Value:
if (auto *Box = dyn_cast<AllocBoxInst>(getValue())) {
return Box->getLoc().getAsASTNode<VarDecl>();
}
if (auto *Arg = dyn_cast<SILFunctionArgument>(getValue())) {
return Arg->getDecl();
}
break;
case AccessedStorageKind::ClassProperty: {
const ObjectProjection &OP = getObjectProjection();
const Projection &P = OP.getProjection();
return P.getVarDecl(OP.getObject()->getType());
}
}
return nullptr;
}
};

/// Models the in-progress accesses for a single storage location.
Expand Down Expand Up @@ -218,6 +242,7 @@ using StorageMap = llvm::SmallDenseMap<AccessedStorage, AccessInfo, 4>;

/// A pair of 'begin_access' instructions that conflict.
struct ConflictingAccess {
AccessedStorage Storage;
const BeginAccessInst *FirstAccess;
const BeginAccessInst *SecondAccess;
};
Expand Down Expand Up @@ -334,25 +359,44 @@ isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
/// Emits a diagnostic if beginning an access with the given in-progress
/// accesses violates the law of exclusivity. Returns true when a
/// diagnostic was emitted.
static void diagnoseExclusivityViolation(const BeginAccessInst *PriorAccess,
static void diagnoseExclusivityViolation(const AccessedStorage &Storage,
const BeginAccessInst *PriorAccess,
const BeginAccessInst *NewAccess,
ASTContext &Ctx) {

// Can't have a conflict if both accesses are reads.
assert(!(PriorAccess->getAccessKind() == SILAccessKind::Read &&
NewAccess->getAccessKind() == SILAccessKind::Read));

ExclusiveOrShared_t NewRequires = getRequiredAccess(NewAccess);
ExclusiveOrShared_t PriorRequires = getRequiredAccess(PriorAccess);

diagnose(Ctx, NewAccess->getLoc().getSourceLoc(),
diag::exclusivity_access_required,
static_cast<unsigned>(NewAccess->getAccessKind()),
static_cast<unsigned>(NewRequires));
diagnose(Ctx, PriorAccess->getLoc().getSourceLoc(),
diag::exclusivity_conflicting_access,
static_cast<unsigned>(PriorAccess->getAccessKind()),
static_cast<unsigned>(PriorRequires));
// Diagnose on the first access that requires exclusivity.
const BeginAccessInst *AccessForMainDiagnostic = PriorAccess;
const BeginAccessInst *AccessForNote = NewAccess;
if (PriorRequires != ExclusiveOrShared_t::ExclusiveAccess) {
AccessForMainDiagnostic = NewAccess;
AccessForNote = PriorAccess;
}

SourceRange rangeForMain =
AccessForMainDiagnostic->getLoc().getSourceRange();

if (const ValueDecl *VD = Storage.getStorageDecl()) {
diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
diag::exclusivity_access_required,
VD->getDescriptiveKind(),
VD->getName(),
static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
.highlight(rangeForMain);
} else {
diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
diag::exclusivity_access_required_unknown_decl,
static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
.highlight(rangeForMain);
}
diagnose(Ctx, AccessForNote->getLoc().getSourceLoc(),
diag::exclusivity_conflicting_access)
.highlight(AccessForNote->getLoc().getSourceRange());
}

/// Make a best effort to find the underlying object for the purpose
Expand Down Expand Up @@ -508,11 +552,12 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
// Ending onr decrements the count.
if (auto *BAI = dyn_cast<BeginAccessInst>(&I)) {
SILAccessKind Kind = BAI->getAccessKind();
AccessInfo &Info = Accesses[findAccessedStorage(BAI->getSource())];
const AccessedStorage &Storage = findAccessedStorage(BAI->getSource());
AccessInfo &Info = Accesses[Storage];
if (Info.conflictsWithAccess(Kind) && !Info.alreadyHadConflict()) {
const BeginAccessInst *Conflict = Info.getFirstAccess();
assert(Conflict && "Must already have had access to conflict!");
ConflictingAccesses.push_back({ Conflict, BAI });
ConflictingAccesses.push_back({ Storage, Conflict, BAI });
}

Info.beginAccess(BAI);
Expand Down Expand Up @@ -554,7 +599,8 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
CallsToSuppress))
continue;

diagnoseExclusivityViolation(PriorAccess, NewAccess, Fn.getASTContext());
diagnoseExclusivityViolation(Violation.Storage, PriorAccess, NewAccess,
Fn.getASTContext());
}
}

Expand Down
52 changes: 26 additions & 26 deletions test/SILOptimizer/exclusivity_static_diagnostics.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ bb0(%0 : $Int):
%3 = project_box %2 : ${ var Int }, 0
store %0 to [trivial] %3 : $*Int
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
%7 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
end_access %6 : $*Int
end_access %5: $*Int
Expand All @@ -55,9 +55,9 @@ bb0(%0 : $Int):
%3 = project_box %2 : ${ var Int }, 0
store %0 to [trivial] %3 : $*Int
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %5 : $*Int
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
end_access %7 : $*Int
end_access %6 : $*Int
Expand Down Expand Up @@ -147,8 +147,8 @@ bb0(%0 : $Int, %1 : $Builtin.Int1):
br bb1
bb1:
// Make sure we don't diagnose twice.
%4 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%4 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
end_access %5: $*Int
end_access %4: $*Int
cond_br %1, bb1, bb2
Expand Down Expand Up @@ -198,8 +198,8 @@ bb0(%0 : $Int):
%1 = alloc_box ${ var Int }
%2 = project_box %1 : ${ var Int }, 0
store %0 to [trivial] %2 : $*Int
%4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting read requires shared access}}
%5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{modification requires exclusive access}}
%4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
%5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
end_access %5 : $*Int
end_access %4: $*Int
destroy_value %1 : ${ var Int }
Expand All @@ -213,8 +213,8 @@ bb0(%0 : $Int):
%1 = alloc_box ${ var Int }
%2 = project_box %1 : ${ var Int }, 0
store %0 to [trivial] %2 : $*Int
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%5 = begin_access [read] [unknown] %2 : $*Int // expected-warning {{read requires shared access}}
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%5 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
end_access %5 : $*Int
end_access %4: $*Int
destroy_value %1 : ${ var Int }
Expand All @@ -232,11 +232,11 @@ sil hidden @classStoredProperty : $@convention(thin) (ClassWithStoredProperty) -
bb0(%0 : $ClassWithStoredProperty):
%1 = ref_element_addr %0 : $ClassWithStoredProperty, #ClassWithStoredProperty.f

// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+1{{simultaneous accesses to var 'f'; modification requires exclusive access}}
%2 = begin_access [modify] [dynamic] %1 : $*Int
%3 = ref_element_addr %0 : $ClassWithStoredProperty, #ClassWithStoredProperty.f

// expected-warning@+1{{modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
%4 = begin_access [modify] [dynamic] %3 : $*Int
end_access %4 : $*Int
end_access %2 : $*Int
Expand All @@ -252,11 +252,11 @@ bb0(%0 : $ClassWithStoredProperty):
%2 = begin_borrow %0 : $ClassWithStoredProperty
%3 = ref_element_addr %1 : $ClassWithStoredProperty, #ClassWithStoredProperty.f

// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+1{{simultaneous accesses to var 'f'; modification requires exclusive access}}
%4 = begin_access [modify] [dynamic] %3 : $*Int
%5 = ref_element_addr %2 : $ClassWithStoredProperty, #ClassWithStoredProperty.f

// expected-warning@+1{{modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
%6 = begin_access [modify] [dynamic] %5 : $*Int
end_access %6 : $*Int
end_access %4 : $*Int
Expand All @@ -279,8 +279,8 @@ bb0(%0 : $Int):
store %0 to [trivial] %3 : $*Int
%4 = copy_value %2 : ${ var Int }
%5 = project_box %4 : ${ var Int }, 0
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%7 = begin_access [modify] [unknown] %5 : $*Int // expected-warning {{modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%7 = begin_access [modify] [unknown] %5 : $*Int // expected-note {{conflicting access is here}}
end_access %7 : $*Int
end_access %6: $*Int
destroy_value %2 : ${ var Int }
Expand All @@ -297,8 +297,8 @@ bb0(%0 : $Int):
%3 = project_box %2 : ${ var Int }, 0
store %0 to [trivial] %3 : $*Int
%4 = project_box %2 : ${ var Int }, 0
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %4 : $*Int // expected-warning {{modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %4 : $*Int // expected-note {{conflicting access is here}}
end_access %6 : $*Int
end_access %5: $*Int
destroy_value %2 : ${ var Int }
Expand All @@ -315,8 +315,8 @@ sil hidden @modifySameGlobal : $@convention(thin) (Int) -> () {
bb0(%0 : $Int):
%1 = global_addr @global1 :$*Int
%2 = global_addr @global1 :$*Int
%3 = begin_access [modify] [unknown] %1 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{modification requires exclusive access}}
%3 = begin_access [modify] [unknown] %1 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
end_access %4 : $*Int
end_access %3: $*Int
%5 = tuple ()
Expand Down Expand Up @@ -347,8 +347,8 @@ bb0(%0 : $Int):
%3 = project_box %2 : ${ var Int }, 0
store %0 to [trivial] %3 : $*Int
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
%5 = begin_access [read] [unknown] %3 : $*Int // expected-note {{conflicting read requires shared access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%5 = begin_access [read] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%7 = begin_access [read] [unknown] %3 : $*Int // no-error
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
end_access %7 : $*Int
Expand All @@ -368,8 +368,8 @@ bb0(%0 : $Int):
%3 = project_box %2 : ${ var Int }, 0
store %0 to [trivial] %3 : $*Int
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
%7 = begin_access [modify] [unknown] %3 : $*Int // no-error
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
end_access %7 : $*Int
Expand All @@ -392,8 +392,8 @@ bb0(%0 : $Int):
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
%5 = begin_access [modify] [unknown] %3 : $*Int // no-note
end_access %5 : $*Int
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
end_access %7 : $*Int
end_access %6: $*Int
Expand Down
41 changes: 23 additions & 18 deletions test/SILOptimizer/exclusivity_static_diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,22 @@ func takesTwoInouts<T>(_ p1: inout T, _ p2: inout T) { }
func simpleInoutDiagnostic() {
var i = 7

// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to var 'i'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
takesTwoInouts(&i, &i)
}

func inoutOnInoutParameter(p: inout Int) {
// expected-warning@+2{{simultaneous accesses to parameter 'p'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
takesTwoInouts(&p, &p)
}

func swapNoSuppression(_ i: Int, _ j: Int) {
var a: [Int] = [1, 2, 3]

// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to var 'a'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
swap(&a[i], &a[j]) // no-warning
}

Expand All @@ -29,23 +34,23 @@ struct StructWithMutatingMethodThatTakesSelfInout {
mutating func mutate(_ other: inout SomeClass) { }

mutating func callMutatingMethodThatTakesSelfInout() {
// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
mutate(&self)
}

mutating func callMutatingMethodThatTakesSelfStoredPropInout() {
// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
mutate(&self.f)
}
}

var global1 = StructWithMutatingMethodThatTakesSelfInout()
var globalStruct1 = StructWithMutatingMethodThatTakesSelfInout()
func callMutatingMethodThatTakesGlobalStoredPropInout() {
// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
global1.mutate(&global1.f)
// expected-warning@+2{{simultaneous accesses to var 'globalStruct1'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
globalStruct1.mutate(&globalStruct1.f)
}

class ClassWithFinalStoredProp {
Expand All @@ -55,21 +60,21 @@ class ClassWithFinalStoredProp {
func callMutatingMethodThatTakesClassStoredPropInout() {
s1.mutate(&s2.f) // no-warning

// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to var 's1'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
s1.mutate(&s1.f)

let local1 = self

// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to var 's1'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
local1.s1.mutate(&local1.s1.f)
}
}

func violationWithGenericType<T>(_ p: T) {
var local = p
// expected-warning@+2{{modification requires exclusive access}}
// expected-note@+1{{conflicting modification requires exclusive access}}
// expected-warning@+2{{simultaneous accesses to var 'local'; modification requires exclusive access}}
// expected-note@+1{{conflicting access is here}}
takesTwoInouts(&local, &local)
}
Loading