Skip to content

Commit 4f1cbbd

Browse files
eecksteineeckstein
committed
SIL Verifier: verify that inside a read-only access scope there are no stores to the memory location
This will e.g. catch violations like ``` %1 = begin_access [read] %0 store %2 to %0 end_access %1 ``` Also, fix all the sil tests which violate that.
1 parent 12b582a commit 4f1cbbd

File tree

11 files changed

+53
-28
lines changed

11 files changed

+53
-28
lines changed

‎SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,33 @@ extension LoadBorrowInst : VerifiableInstruction {
154154
}
155155
}
156156

157+
extension BeginAccessInst : VerifiableInstruction {
158+
func verify(_ context: VerifierContext) {
159+
if context.silStage == .raw {
160+
return
161+
}
162+
// Catch violations like
163+
// ```
164+
// %1 = begin_access [read] %0
165+
// store %2 to %0
166+
// end_access %1
167+
// ```
168+
if address.type.isMoveOnly && enforcement == .static {
169+
// This is a workaround for a bug in the move-only checker: rdar://151841926.
170+
// The move-only checker sometimes inserts destroy_addr within read-only static access scopes.
171+
// TODO: remove this once the bug is fixed.
172+
return
173+
}
174+
if accessKind == .read {
175+
var mutatingInstructions = MutatingUsesWalker(context)
176+
defer { mutatingInstructions.deinitialize() }
177+
178+
mutatingInstructions.findMutatingUses(of: self.address)
179+
mutatingInstructions.verifyNoMutatingUsesInLinearLiverange(of: self)
180+
}
181+
}
182+
}
183+
157184
extension VectorBaseAddrInst : VerifiableInstruction {
158185
func verify(_ context: VerifierContext) {
159186
require(vector.type.isBuiltinFixedArray,
@@ -210,9 +237,7 @@ private struct MutatingUsesWalker : AddressDefUseWalker {
210237
}
211238
}
212239

213-
private mutating func verifyNoMutatingUsesInLinearLiverange(of startInst: SingleValueInstruction) {
214-
assert(startInst is LoadBorrowInst || startInst is BorrowedFromInst)
215-
240+
mutating func verifyNoMutatingUsesInLinearLiverange(of startInst: SingleValueInstruction) {
216241
var instWorklist = InstructionWorklist(context)
217242
defer { instWorklist.deinitialize() }
218243

@@ -223,7 +248,7 @@ private struct MutatingUsesWalker : AddressDefUseWalker {
223248

224249
while let inst = instWorklist.pop() {
225250
require(!mutatingInstructions.contains(inst),
226-
"Load borrow invalidated by a local write", atInstruction: inst)
251+
"read-only scope invalidated by a local write", atInstruction: inst)
227252
instWorklist.pushPredecessors(of: inst, ignoring: startInst)
228253
}
229254
}

‎test/SIL/OwnershipVerifier/load_borrow_invalidation_partial_apply.sil‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ bb0(%0 : @owned $WrapperStruct):
5555
// we run sil-opt with -dont-abort-on-memory-lifetime-error.
5656

5757
// CHECK-LABEL: Begin Error in function caller2
58-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
58+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
5959
// CHECK-LABEL: End Error in function caller2
6060
sil [ossa] @caller2 : $@convention(thin) (@owned WrapperStruct) -> () {
6161
bb0(%0 : @owned $WrapperStruct):

‎test/SIL/OwnershipVerifier/load_borrow_verify_errors.sil‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ sil @use_guaranteed : $@convention(thin) (@guaranteed Klass) -> ()
88

99
// Write: store {{.*}} [assign] {{.*}}
1010
// CHECK: Begin Error in function test_write_reborrow
11-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
11+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
1212
// CHECK: End Error in function test_write_reborrow
1313
sil [ossa] @test_write_reborrow : $@convention(thin) (@owned Klass, @owned Klass) -> () {
1414
bb0(%0 : @owned $Klass, %1 : @owned $Klass):
@@ -30,7 +30,7 @@ bb2(%ldarg : @guaranteed $Klass):
3030
}
3131

3232
// CHECK: Begin Error in function test_multiple_loadborrows
33-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
33+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
3434
// CHECK: Verifying instruction:
3535
// CHECK: -> destroy_addr
3636
// CHECK: End Error in function test_multiple_loadborrows
@@ -84,7 +84,7 @@ struct MyStruct {
8484
}
8585

8686
// CHECK: Begin Error in function test_is_unique
87-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
87+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
8888
// CHECK: -> %4 = is_unique %1 : $*ArrayIntBuffer
8989
// CHECK: End Error in function test_is_unique
9090
sil [ossa] @test_is_unique : $@convention(thin) (@in MyArray<MyStruct>) -> () {

‎test/SIL/memory_lifetime.sil‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ bb0(%0 : @owned $T, %1 : @owned $Inner):
825825
%4 = alloc_stack $Inner
826826
store %1 to [init] %4
827827
%6 = mark_dependence %4 on %2
828-
%7 = begin_access [read] [dynamic] %6
828+
%7 = begin_access [modify] [dynamic] %6
829829
%8 = load [take] %7
830830
end_access %7
831831
dealloc_stack %4
@@ -841,7 +841,7 @@ bb0(%0 : @owned $T, %1 : @owned $Inner):
841841
%4 = alloc_stack $Inner
842842
store %1 to [init] %4
843843
mark_dependence_addr %4 on %2
844-
%7 = begin_access [read] [dynamic] %4
844+
%7 = begin_access [modify] [dynamic] %4
845845
%8 = load [take] %7
846846
end_access %7
847847
dealloc_stack %4

‎test/SILOptimizer/access_enforcement_opts.sil‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,12 +1673,12 @@ bb0:
16731673
// and the caller storage is an Argument at a different position.
16741674
//
16751675
// CHECK-LABEL: sil @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
1676-
// CHECK: begin_access [read] [dynamic] [no_nested_conflict]
1676+
// CHECK: begin_access [modify] [dynamic] [no_nested_conflict]
16771677
// CHECK-LABEL: } // end sil function 'testTransformArgumentCaller'
16781678
sil @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
16791679
bb0(%0 : $Int64, %1 : ${ var Int64 }, %2 : ${ var Int64 }):
16801680
%boxadr = project_box %1 : $ { var Int64 }, 0
1681-
%access = begin_access [read] [dynamic] [no_nested_conflict] %boxadr : $*Int64
1681+
%access = begin_access [modify] [dynamic] [no_nested_conflict] %boxadr : $*Int64
16821682
%f = function_ref @testTransformArgumentCallee : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
16831683
%call = apply %f(%2) : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
16841684
store %0 to %access : $*Int64

‎test/SILOptimizer/access_enforcement_opts_ossa.sil‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1757,12 +1757,12 @@ bb0:
17571757
// and the caller storage is an Argument at a different position.
17581758
//
17591759
// CHECK-LABEL: sil [ossa] @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
1760-
// CHECK: begin_access [read] [dynamic] [no_nested_conflict]
1760+
// CHECK: begin_access [modify] [dynamic] [no_nested_conflict]
17611761
// CHECK-LABEL: } // end sil function 'testTransformArgumentCaller'
17621762
sil [ossa] @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
17631763
bb0(%0 : $Int64, %1 : @guaranteed ${ var Int64 }, %2 : @guaranteed ${ var Int64 }):
17641764
%boxadr = project_box %1 : $ { var Int64 }, 0
1765-
%access = begin_access [read] [dynamic] [no_nested_conflict] %boxadr : $*Int64
1765+
%access = begin_access [modify] [dynamic] [no_nested_conflict] %boxadr : $*Int64
17661766
%f = function_ref @testTransformArgumentCallee : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
17671767
%call = apply %f(%2) : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
17681768
store %0 to [trivial] %access : $*Int64

‎test/SILOptimizer/access_storage_analysis.sil‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,12 @@ bb0(%0 : ${ var Int }, %1 : $Int):
103103
}
104104

105105
// CHECK-LABEL: @writeIdentifiedBox
106-
// CHECK: [read] Box %1 = alloc_box ${ var Int }
106+
// CHECK: [modify] Box %1 = alloc_box ${ var Int }
107107
sil @writeIdentifiedBox : $@convention(thin) (Int) -> () {
108108
bb0(%0 : $Int):
109109
%1 = alloc_box ${ var Int }
110110
%2 = project_box %1 : ${ var Int }, 0
111-
%3 = begin_access [read] [dynamic] %2 : $*Int
111+
%3 = begin_access [modify] [dynamic] %2 : $*Int
112112
store %0 to %3 : $*Int
113113
end_access %3 : $*Int
114114
%6 = tuple ()
@@ -398,7 +398,7 @@ bb0(%0 : $C):
398398
sil @writeIdentifiedNestedClass : $@convention(thin) (@guaranteed C, Int) -> () {
399399
bb0(%0 : $C, %1 : $Int):
400400
%2 = ref_element_addr %0 : $C, #C.property
401-
%3 = begin_access [read] [dynamic] %2 : $*Int
401+
%3 = begin_access [modify] [dynamic] %2 : $*Int
402402
%4 = begin_access [modify] [dynamic] %3 : $*Int
403403
store %1 to %4 : $*Int
404404
end_access %4 : $*Int

‎test/SILOptimizer/access_storage_analysis_ossa.sil‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,12 @@ bb0(%0 : @guaranteed ${ var Int }, %1 : $Int):
103103
}
104104

105105
// CHECK-LABEL: @writeIdentifiedBox
106-
// CHECK: [read] Box %1 = alloc_box ${ var Int }
106+
// CHECK: [modify] Box %1 = alloc_box ${ var Int }
107107
sil [ossa] @writeIdentifiedBox : $@convention(thin) (Int) -> () {
108108
bb0(%0 : $Int):
109109
%1 = alloc_box ${ var Int }
110110
%2 = project_box %1 : ${ var Int }, 0
111-
%3 = begin_access [read] [dynamic] %2 : $*Int
111+
%3 = begin_access [modify] [dynamic] %2 : $*Int
112112
store %0 to [trivial] %3 : $*Int
113113
end_access %3 : $*Int
114114
destroy_value %1 : ${ var Int }
@@ -409,7 +409,7 @@ sil [ossa] @writeIdentifiedNestedClass : $@convention(thin) (@guaranteed C, Int)
409409
bb0(%0 : @guaranteed $C, %1 : $Int):
410410
%borrow = begin_borrow %0 : $C
411411
%2 = ref_element_addr %borrow : $C, #C.property
412-
%3 = begin_access [read] [dynamic] %2 : $*Int
412+
%3 = begin_access [modify] [dynamic] %2 : $*Int
413413
%4 = begin_access [modify] [dynamic] %3 : $*Int
414414
store %1 to [trivial] %4 : $*Int
415415
end_access %4 : $*Int

‎test/SILOptimizer/copy_propagation.sil‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ bb0:
471471
// def
472472
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
473473
%copy = copy_value %def : $AnyObject
474-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
474+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
475475
// use
476476
store %def to [init] %adr : $*AnyObject
477477
%obj = load [copy] %access : $*AnyObject
@@ -519,7 +519,7 @@ bb0:
519519
// def
520520
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
521521
%copy = copy_value %def : $AnyObject
522-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
522+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
523523
// use
524524
store %def to [init] %adr : $*AnyObject
525525
br bb1
@@ -561,7 +561,7 @@ sil [ossa] @testFullOverlapInDefBlock : $@convention(thin) () -> () {
561561
bb0:
562562
%box = alloc_box ${ var AnyObject }, var, name "x"
563563
%adr = project_box %box : ${ var AnyObject }, 0
564-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
564+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
565565
%f = function_ref @getObject : $@convention(thin) () -> @owned AnyObject
566566
// def
567567
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
@@ -609,7 +609,7 @@ sil [ossa] @testFullOverlapBeforeDefBlock : $@convention(thin) () -> () {
609609
bb0:
610610
%box = alloc_box ${ var AnyObject }, var, name "x"
611611
%adr = project_box %box : ${ var AnyObject }, 0
612-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
612+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
613613
br bb1
614614

615615
bb1:
@@ -657,7 +657,7 @@ bb0:
657657
// def
658658
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
659659
%copy = copy_value %def : $AnyObject
660-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
660+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
661661
// use
662662
store %def to [init] %adr : $*AnyObject
663663
destroy_value %copy : $AnyObject
@@ -692,7 +692,7 @@ bb0:
692692
// def
693693
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
694694
%copy = copy_value %def : $AnyObject
695-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
695+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
696696
// use
697697
store %def to [init] %adr : $*AnyObject
698698
br bb1

‎test/SILOptimizer/temp_rvalue_opt_ossa.sil‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -988,7 +988,7 @@ bb0(%0 : $*Int):
988988
sil [ossa] @dontExtendAccessScopeOverBeginAccess : $@convention(thin) (@in Klass) -> () {
989989
bb0(%0 : $*Klass):
990990
%stack = alloc_stack $Klass
991-
%access = begin_access [read] [static] %0 : $*Klass
991+
%access = begin_access [modify] [static] %0 : $*Klass
992992
copy_addr [take] %access to [init] %stack : $*Klass
993993
end_access %access : $*Klass
994994
%f = function_ref @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> ()

0 commit comments

Comments
 (0)