Skip to content

Commit c6d9fdd

Browse files
authored
Merge pull request #8952 from devincoughlin/exclusivity-diagnostics
2 parents 304b6c9 + 9cdff5f commit c6d9fdd

File tree

5 files changed

+123
-66
lines changed

5 files changed

+123
-66
lines changed

include/swift/AST/DiagnosticsSIL.def

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,17 @@ NOTE(previous_inout_alias,none,
8787
// This is temporarily a warning during staging to make it easier to evaluate.
8888
// The intent is to change it to an error before turning it on by default.
8989
WARNING(exclusivity_access_required,none,
90+
"simultaneous accesses to %0 %1; "
91+
"%select{initialization|read|modification|deinitialization}2 requires "
92+
"exclusive access", (DescriptiveDeclKind, Identifier, unsigned))
93+
94+
WARNING(exclusivity_access_required_unknown_decl,none,
95+
"simultaneous accesses; "
9096
"%select{initialization|read|modification|deinitialization}0 requires "
91-
"%select{exclusive|shared}1 access", (unsigned, unsigned))
97+
"exclusive access", (unsigned))
98+
9299
NOTE(exclusivity_conflicting_access,none,
93-
"conflicting %select{initialization|read|modification|deinitialization}0 "
94-
"requires %select{exclusive|shared}1 access", (unsigned, unsigned))
100+
"conflicting access is here", ())
95101

96102
ERROR(unsupported_c_function_pointer_conversion,none,
97103
"C function pointer signature %0 is not compatible with expected type %1",

lib/SILOptimizer/Mandatory/DiagnoseStaticExclusivity.cpp

Lines changed: 59 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "swift/AST/Decl.h"
3030
#include "swift/AST/Expr.h"
3131
#include "swift/AST/Stmt.h"
32+
#include "swift/Basic/SourceLoc.h"
3233
#include "swift/SIL/CFG.h"
3334
#include "swift/SIL/SILArgument.h"
3435
#include "swift/SIL/SILInstruction.h"
@@ -135,6 +136,29 @@ class AccessedStorage {
135136
assert(Kind == AccessedStorageKind::ClassProperty);
136137
return ObjProj;
137138
}
139+
140+
/// Returns the ValueDecl for the underlying storage, if it can be
141+
/// determined. Otherwise returns null. For diagonostic purposes.
142+
const ValueDecl *getStorageDecl() const {
143+
switch(Kind) {
144+
case AccessedStorageKind::GlobalVar:
145+
return getGlobal()->getDecl();
146+
case AccessedStorageKind::Value:
147+
if (auto *Box = dyn_cast<AllocBoxInst>(getValue())) {
148+
return Box->getLoc().getAsASTNode<VarDecl>();
149+
}
150+
if (auto *Arg = dyn_cast<SILFunctionArgument>(getValue())) {
151+
return Arg->getDecl();
152+
}
153+
break;
154+
case AccessedStorageKind::ClassProperty: {
155+
const ObjectProjection &OP = getObjectProjection();
156+
const Projection &P = OP.getProjection();
157+
return P.getVarDecl(OP.getObject()->getType());
158+
}
159+
}
160+
return nullptr;
161+
}
138162
};
139163

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

219243
/// A pair of 'begin_access' instructions that conflict.
220244
struct ConflictingAccess {
245+
AccessedStorage Storage;
221246
const BeginAccessInst *FirstAccess;
222247
const BeginAccessInst *SecondAccess;
223248
};
@@ -334,25 +359,44 @@ isConflictOnInoutArgumentsToSuppressed(const BeginAccessInst *Access1,
334359
/// Emits a diagnostic if beginning an access with the given in-progress
335360
/// accesses violates the law of exclusivity. Returns true when a
336361
/// diagnostic was emitted.
337-
static void diagnoseExclusivityViolation(const BeginAccessInst *PriorAccess,
362+
static void diagnoseExclusivityViolation(const AccessedStorage &Storage,
363+
const BeginAccessInst *PriorAccess,
338364
const BeginAccessInst *NewAccess,
339365
ASTContext &Ctx) {
340366

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

345-
ExclusiveOrShared_t NewRequires = getRequiredAccess(NewAccess);
346371
ExclusiveOrShared_t PriorRequires = getRequiredAccess(PriorAccess);
347372

348-
diagnose(Ctx, NewAccess->getLoc().getSourceLoc(),
349-
diag::exclusivity_access_required,
350-
static_cast<unsigned>(NewAccess->getAccessKind()),
351-
static_cast<unsigned>(NewRequires));
352-
diagnose(Ctx, PriorAccess->getLoc().getSourceLoc(),
353-
diag::exclusivity_conflicting_access,
354-
static_cast<unsigned>(PriorAccess->getAccessKind()),
355-
static_cast<unsigned>(PriorRequires));
373+
// Diagnose on the first access that requires exclusivity.
374+
const BeginAccessInst *AccessForMainDiagnostic = PriorAccess;
375+
const BeginAccessInst *AccessForNote = NewAccess;
376+
if (PriorRequires != ExclusiveOrShared_t::ExclusiveAccess) {
377+
AccessForMainDiagnostic = NewAccess;
378+
AccessForNote = PriorAccess;
379+
}
380+
381+
SourceRange rangeForMain =
382+
AccessForMainDiagnostic->getLoc().getSourceRange();
383+
384+
if (const ValueDecl *VD = Storage.getStorageDecl()) {
385+
diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
386+
diag::exclusivity_access_required,
387+
VD->getDescriptiveKind(),
388+
VD->getName(),
389+
static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
390+
.highlight(rangeForMain);
391+
} else {
392+
diagnose(Ctx, AccessForMainDiagnostic->getLoc().getSourceLoc(),
393+
diag::exclusivity_access_required_unknown_decl,
394+
static_cast<unsigned>(AccessForMainDiagnostic->getAccessKind()))
395+
.highlight(rangeForMain);
396+
}
397+
diagnose(Ctx, AccessForNote->getLoc().getSourceLoc(),
398+
diag::exclusivity_conflicting_access)
399+
.highlight(AccessForNote->getLoc().getSourceRange());
356400
}
357401

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

518563
Info.beginAccess(BAI);
@@ -554,7 +599,8 @@ static void checkStaticExclusivity(SILFunction &Fn, PostOrderFunctionInfo *PO) {
554599
CallsToSuppress))
555600
continue;
556601

557-
diagnoseExclusivityViolation(PriorAccess, NewAccess, Fn.getASTContext());
602+
diagnoseExclusivityViolation(Violation.Storage, PriorAccess, NewAccess,
603+
Fn.getASTContext());
558604
}
559605
}
560606

test/SILOptimizer/exclusivity_static_diagnostics.sil

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ bb0(%0 : $Int):
3838
%3 = project_box %2 : ${ var Int }, 0
3939
store %0 to [trivial] %3 : $*Int
4040
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
41-
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
42-
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
41+
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
42+
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
4343
%7 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
4444
end_access %6 : $*Int
4545
end_access %5: $*Int
@@ -55,9 +55,9 @@ bb0(%0 : $Int):
5555
%3 = project_box %2 : ${ var Int }, 0
5656
store %0 to [trivial] %3 : $*Int
5757
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
58-
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
58+
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
5959
%6 = begin_access [modify] [unknown] %5 : $*Int
60-
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
60+
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
6161
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
6262
end_access %7 : $*Int
6363
end_access %6 : $*Int
@@ -147,8 +147,8 @@ bb0(%0 : $Int, %1 : $Builtin.Int1):
147147
br bb1
148148
bb1:
149149
// Make sure we don't diagnose twice.
150-
%4 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
151-
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
150+
%4 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
151+
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
152152
end_access %5: $*Int
153153
end_access %4: $*Int
154154
cond_br %1, bb1, bb2
@@ -198,8 +198,8 @@ bb0(%0 : $Int):
198198
%1 = alloc_box ${ var Int }
199199
%2 = project_box %1 : ${ var Int }, 0
200200
store %0 to [trivial] %2 : $*Int
201-
%4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting read requires shared access}}
202-
%5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{modification requires exclusive access}}
201+
%4 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
202+
%5 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
203203
end_access %5 : $*Int
204204
end_access %4: $*Int
205205
destroy_value %1 : ${ var Int }
@@ -213,8 +213,8 @@ bb0(%0 : $Int):
213213
%1 = alloc_box ${ var Int }
214214
%2 = project_box %1 : ${ var Int }, 0
215215
store %0 to [trivial] %2 : $*Int
216-
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-note {{conflicting modification requires exclusive access}}
217-
%5 = begin_access [read] [unknown] %2 : $*Int // expected-warning {{read requires shared access}}
216+
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
217+
%5 = begin_access [read] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
218218
end_access %5 : $*Int
219219
end_access %4: $*Int
220220
destroy_value %1 : ${ var Int }
@@ -232,11 +232,11 @@ sil hidden @classStoredProperty : $@convention(thin) (ClassWithStoredProperty) -
232232
bb0(%0 : $ClassWithStoredProperty):
233233
%1 = ref_element_addr %0 : $ClassWithStoredProperty, #ClassWithStoredProperty.f
234234

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

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

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

259-
// expected-warning@+1{{modification requires exclusive access}}
259+
// expected-note@+1{{conflicting access is here}}
260260
%6 = begin_access [modify] [dynamic] %5 : $*Int
261261
end_access %6 : $*Int
262262
end_access %4 : $*Int
@@ -279,8 +279,8 @@ bb0(%0 : $Int):
279279
store %0 to [trivial] %3 : $*Int
280280
%4 = copy_value %2 : ${ var Int }
281281
%5 = project_box %4 : ${ var Int }, 0
282-
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
283-
%7 = begin_access [modify] [unknown] %5 : $*Int // expected-warning {{modification requires exclusive access}}
282+
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
283+
%7 = begin_access [modify] [unknown] %5 : $*Int // expected-note {{conflicting access is here}}
284284
end_access %7 : $*Int
285285
end_access %6: $*Int
286286
destroy_value %2 : ${ var Int }
@@ -297,8 +297,8 @@ bb0(%0 : $Int):
297297
%3 = project_box %2 : ${ var Int }, 0
298298
store %0 to [trivial] %3 : $*Int
299299
%4 = project_box %2 : ${ var Int }, 0
300-
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
301-
%6 = begin_access [modify] [unknown] %4 : $*Int // expected-warning {{modification requires exclusive access}}
300+
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
301+
%6 = begin_access [modify] [unknown] %4 : $*Int // expected-note {{conflicting access is here}}
302302
end_access %6 : $*Int
303303
end_access %5: $*Int
304304
destroy_value %2 : ${ var Int }
@@ -315,8 +315,8 @@ sil hidden @modifySameGlobal : $@convention(thin) (Int) -> () {
315315
bb0(%0 : $Int):
316316
%1 = global_addr @global1 :$*Int
317317
%2 = global_addr @global1 :$*Int
318-
%3 = begin_access [modify] [unknown] %1 : $*Int // expected-note {{conflicting modification requires exclusive access}}
319-
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-warning {{modification requires exclusive access}}
318+
%3 = begin_access [modify] [unknown] %1 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
319+
%4 = begin_access [modify] [unknown] %2 : $*Int // expected-note {{conflicting access is here}}
320320
end_access %4 : $*Int
321321
end_access %3: $*Int
322322
%5 = tuple ()
@@ -347,8 +347,8 @@ bb0(%0 : $Int):
347347
%3 = project_box %2 : ${ var Int }, 0
348348
store %0 to [trivial] %3 : $*Int
349349
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
350-
%5 = begin_access [read] [unknown] %3 : $*Int // expected-note {{conflicting read requires shared access}}
351-
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
350+
%5 = begin_access [read] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
351+
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
352352
%7 = begin_access [read] [unknown] %3 : $*Int // no-error
353353
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
354354
end_access %7 : $*Int
@@ -368,8 +368,8 @@ bb0(%0 : $Int):
368368
%3 = project_box %2 : ${ var Int }, 0
369369
store %0 to [trivial] %3 : $*Int
370370
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
371-
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
372-
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
371+
%5 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
372+
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
373373
%7 = begin_access [modify] [unknown] %3 : $*Int // no-error
374374
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
375375
end_access %7 : $*Int
@@ -392,8 +392,8 @@ bb0(%0 : $Int):
392392
%4 = function_ref @takesTwoInouts : $@convention(thin) (@inout Int, @inout Int) -> ()
393393
%5 = begin_access [modify] [unknown] %3 : $*Int // no-note
394394
end_access %5 : $*Int
395-
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting modification requires exclusive access}}
396-
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{modification requires exclusive access}}
395+
%6 = begin_access [modify] [unknown] %3 : $*Int // expected-warning {{simultaneous accesses; modification requires exclusive access}}
396+
%7 = begin_access [modify] [unknown] %3 : $*Int // expected-note {{conflicting access is here}}
397397
%8 = apply %4(%5, %6) : $@convention(thin) (@inout Int, @inout Int) -> ()
398398
end_access %7 : $*Int
399399
end_access %6: $*Int

test/SILOptimizer/exclusivity_static_diagnostics.swift

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,22 @@ func takesTwoInouts<T>(_ p1: inout T, _ p2: inout T) { }
77
func simpleInoutDiagnostic() {
88
var i = 7
99

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

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

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

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

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

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

3742
mutating func callMutatingMethodThatTakesSelfStoredPropInout() {
38-
// expected-warning@+2{{modification requires exclusive access}}
39-
// expected-note@+1{{conflicting modification requires exclusive access}}
43+
// expected-warning@+2{{simultaneous accesses to parameter 'self'; modification requires exclusive access}}
44+
// expected-note@+1{{conflicting access is here}}
4045
mutate(&self.f)
4146
}
4247
}
4348

44-
var global1 = StructWithMutatingMethodThatTakesSelfInout()
49+
var globalStruct1 = StructWithMutatingMethodThatTakesSelfInout()
4550
func callMutatingMethodThatTakesGlobalStoredPropInout() {
46-
// expected-warning@+2{{modification requires exclusive access}}
47-
// expected-note@+1{{conflicting modification requires exclusive access}}
48-
global1.mutate(&global1.f)
51+
// expected-warning@+2{{simultaneous accesses to var 'globalStruct1'; modification requires exclusive access}}
52+
// expected-note@+1{{conflicting access is here}}
53+
globalStruct1.mutate(&globalStruct1.f)
4954
}
5055

5156
class ClassWithFinalStoredProp {
@@ -55,21 +60,21 @@ class ClassWithFinalStoredProp {
5560
func callMutatingMethodThatTakesClassStoredPropInout() {
5661
s1.mutate(&s2.f) // no-warning
5762

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

6267
let local1 = self
6368

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

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

0 commit comments

Comments
 (0)