Skip to content

Commit 904ebc6

Browse files
committed
[send-non-sendable] Recurse to the full underlying value computation instead of just the object one when computing the underlying object of an address.
Otherwise, depending on the exact value that we perform the underlying look up at... we will get different underlying values. To see this consider the following SIL: ```sil %1 = alloc_stack $MyEnum<T> copy_addr %0 to [init] %1 %2 = unchecked_take_enum_data_addr %1, #MyEnum.some!enumelt %3 = load [take] %2 %4 = project_box %3, 0 %5 = load_borrow %4 %6 = copy_value %5 ``` If one were to perform an underlying object query on %4 or %3, one would get back an underlying object of %1. In contrast, if one performed the same operation on %5, then one would get back %3. The reason why this happens is that we first see we have an object but that it is from a load_borrow so we need to look through the load_borrow and perform the address underlying value computation. When we do that, we find project_box to be the value. project_box is special since it is the only address base we ever look through since from an underlying object perspective, we want to consider the box to be the underlying object rather than the projection. So thus we see that the result of the underlying address computation is that the underlying address is from a load [take]. Since we then pass in load [take] recursively into the underlying value object computation, we just return load [take]. In contrast, the correct behavior is to do the more general recurse that recognizes that we have a load [take] and that we need to look through it and perform the address computation. rdar://151598281
1 parent e805d93 commit 904ebc6

File tree

3 files changed

+62
-3
lines changed

3 files changed

+62
-3
lines changed

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -985,13 +985,12 @@ RegionAnalysisValueMap::getUnderlyingTrackedValueHelperAddress(
985985
// occur in the underlying DenseMap that backs getUnderlyingTrackedValue()
986986
// if we insert another entry into the DenseMap.
987987
if (!visitor.value)
988-
return UnderlyingTrackedValueInfo(
989-
getUnderlyingTrackedValueHelperObject(base));
988+
return UnderlyingTrackedValueInfo(getUnderlyingTrackedValueHelper(base));
990989

991990
// TODO: Should we us the base or value from
992991
// getUnderlyingTrackedValueHelperObject as our base?
993992
return UnderlyingTrackedValueInfo(
994-
visitor.value, getUnderlyingTrackedValueHelperObject(base).value);
993+
visitor.value, getUnderlyingTrackedValueHelper(base).value);
995994
}
996995

997996
// Otherwise, we return the actorIsolation that our visitor found.

test/Concurrency/regionanalysis_trackable_value.sil

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,12 @@ class NonSendableKlassWithState {
5656

5757
actor Custom {}
5858

59+
enum MyEnum<T> {
60+
case none
61+
indirect case some(NonSendableKlass)
62+
case some2(T)
63+
}
64+
5965
sil @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
6066
sil @useNonSendableKlass : $@convention(thin) (@guaranteed NonSendableKlass) -> ()
6167
sil @constructNonSendableKlass : $@convention(thin) () -> @owned NonSendableKlass
@@ -639,4 +645,40 @@ bb0(%0 : @owned $Struct2):
639645
dealloc_stack %1
640646
%9999 = tuple ()
641647
return %9999 : $()
648+
}
649+
650+
// CHECK-LABEL: begin running test 1 of 1 on indirect_enum_load_take: sil_regionanalysis_underlying_tracked_value with: @trace[0]
651+
// CHECK: TrackableValue. State: TrackableValueState[id: 0][is_no_alias: yes][is_sendable: no][region_value_kind: disconnected].
652+
// CHECK: Rep Value: %1 = alloc_stack $MyEnum<T>
653+
// CHECK: end running test 1 of 1 on indirect_enum_load_take: sil_regionanalysis_underlying_tracked_value with: @trace[0]
654+
sil [ossa] @indirect_enum_load_take : $@convention(thin) @async <T> (@in_guaranteed MyEnum<T>) -> () {
655+
bb0(%0 : $*MyEnum<T>):
656+
specify_test "sil_regionanalysis_underlying_tracked_value @trace[0]"
657+
%1 = alloc_stack $MyEnum<T>
658+
copy_addr %0 to [init] %1
659+
switch_enum_addr %1, case #MyEnum.some!enumelt: bb1, default bb2
660+
661+
bb1:
662+
%2 = unchecked_take_enum_data_addr %1, #MyEnum.some!enumelt
663+
%3 = load [take] %2
664+
%4 = project_box %3, 0
665+
%5 = load_borrow %4
666+
%6 = copy_value %5
667+
debug_value [trace] %5
668+
%7 = move_value [var_decl] %6
669+
debug_value %5, let, name "x"
670+
destroy_value %7
671+
end_borrow %5
672+
destroy_value %3
673+
dealloc_stack %1
674+
br bb3
675+
676+
bb2:
677+
destroy_addr %1
678+
dealloc_stack %1
679+
br bb3
680+
681+
bb3:
682+
%9999 = tuple ()
683+
return %9999 : $()
642684
}

test/Concurrency/transfernonsendable.swift

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ struct SendableGenericStruct : Sendable {
9393
var x = SendableKlass()
9494
}
9595

96+
enum MyEnum<T> {
97+
case none
98+
indirect case some(NonSendableKlass)
99+
case more(T)
100+
}
101+
96102
////////////////////////////
97103
// MARK: Actor Self Tests //
98104
////////////////////////////
@@ -2054,3 +2060,15 @@ func sendIsolatedValueToItsOwnIsolationDomain() {
20542060
}
20552061
}
20562062
}
2063+
2064+
// We used to crash on this since we were not looking finding underlying objects
2065+
// hard enough.
2066+
func indirectEnumTestCase<T>(_ e: MyEnum<T>) async -> Bool {
2067+
switch e {
2068+
case .some(let x):
2069+
_ = x
2070+
return true
2071+
default:
2072+
return false
2073+
}
2074+
}

0 commit comments

Comments
 (0)