Skip to content

Commit 05fcf5f

Browse files
committed
Fix OSSA RAUW utility for move_value that changes ownership.
CSE "looks through" ownership operations, which can lead to problematic substitutions. This fix cleans up owned operands even when the newly substituted value has no ownership. For example: %0 = enum $Optional<Interface>, #Optional.none!enumelt %1 = move_value [lexical] %0 %2 = enum $Optional<Interface>, #Optional.none!enumelt %3 = struct $EndpointCommon (%2) %4 = struct $EndpointCommon (%1) CSE combines the two .none enums: %0 = enum $Optional<Interface>, #Optional.none!enumelt %2 = enum $Optional<Interface>, #Optional.none!enumelt Then combines the two structs: %3 = struct $EndpointCommon (%2) %4 = struct $EndpointCommon (%1) Leaving a dead move_value: %1 = move_value [lexical] %0 which is invalid OSSA. Now, when replacing the owned struct, we add destroys for its operands. Fixes rdar://156431548 Error! Found a leaked owned value that was never consumed.
1 parent 663ec93 commit 05fcf5f

File tree

3 files changed

+43
-4
lines changed

3 files changed

+43
-4
lines changed

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,13 +1241,14 @@ SILValue OwnershipRAUWPrepare::prepareReplacement(SILValue newValue) {
12411241
OwnershipRAUWHelper::hasValidRAUWOwnership(oldValue, newValue,
12421242
ctx.guaranteedUsePoints) &&
12431243
"Should have checked if can perform this operation before calling it?!");
1244-
// If our new value is just none, we can pass anything to it so just RAUW
1244+
// If our new value is just none, we can pass it to anything so just RAUW
12451245
// and return.
12461246
//
12471247
// NOTE: This handles RAUWing with undef.
1248-
if (newValue->getOwnershipKind() == OwnershipKind::None)
1248+
if (newValue->getOwnershipKind() == OwnershipKind::None) {
1249+
cleanupOperandsBeforeDeletion(getConsumingPoint(), ctx.callbacks);
12491250
return newValue;
1250-
1251+
}
12511252
assert(oldValue->getOwnershipKind() != OwnershipKind::None);
12521253

12531254
switch (oldValue->getOwnershipKind()) {

test/SILOptimizer/cse_ossa.sil

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1062,7 +1062,7 @@ bb0(%0 : $*Builtin.Int64, %1 : @guaranteed $Builtin.NativeObject):
10621062
return %6 : $(Builtin.Int64, Builtin.Int64)
10631063
}
10641064

1065-
protocol Proto : class {
1065+
protocol Proto : AnyObject {
10661066
func doThis()
10671067
func doThat()
10681068
}

test/SILOptimizer/cse_ossa_nontrivial.sil

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,3 +1126,41 @@ bb0:
11261126
%res = tuple ()
11271127
return %res : $()
11281128
}
1129+
1130+
// testMoveEnumChangesOwnership requires HasOptC and HasOptC to declare
1131+
// their properties with @_hasStorage.
1132+
public struct HasOptKlass {
1133+
@_hasStorage let c: Klass? { get }
1134+
}
1135+
1136+
public struct HasOptKlassHolder {
1137+
@_hasStorage public var hasOptKlass: HasOptKlass { get set }
1138+
}
1139+
1140+
// CHECK-LABEL: sil [ossa] @testMoveEnumChangesOwnership : $@convention(thin) () -> () {
1141+
// CHECK: [[E1:%[0-9]+]] = enum $Optional<Klass>, #Optional.none!enumelt
1142+
// CHECK: [[MV1:%[0-9]+]] = move_value [lexical] [[E1]] : $Optional<Klass>
1143+
// CHECK: [[S:%[0-9]+]] = struct $HasOptKlass (%0 : $Optional<Klass>)
1144+
// CHECK: store [[S]] to [init]
1145+
// CHECK: destroy_value [[MV1]] : $Optional<Klass>
1146+
// CHECK: [[MV2:%[0-9]+]] = move_value [lexical] [[S]] : $HasOptKlass
1147+
// CHECK: store [[MV2]] to [assign]
1148+
// CHECK: } // end sil function 'testMoveEnumChangesOwnership'
1149+
sil [ossa] @testMoveEnumChangesOwnership : $@convention(thin) () -> () {
1150+
bb0:
1151+
%0 = enum $Optional<Klass>, #Optional.none!enumelt
1152+
%1 = move_value [lexical] %0
1153+
%2 = alloc_stack [lexical] [var_decl] $HasOptKlassHolder
1154+
%3 = enum $Optional<Klass>, #Optional.none!enumelt
1155+
%4 = struct $HasOptKlass (%3)
1156+
%5 = struct_element_addr %2, #HasOptKlassHolder.hasOptKlass
1157+
store %4 to [init] %5
1158+
%7 = struct $HasOptKlass (%1)
1159+
%8 = move_value [lexical] %7
1160+
store %8 to [assign] %5
1161+
%10 = load [take] %2
1162+
dealloc_stack %2
1163+
destroy_value %10
1164+
%13 = tuple ()
1165+
return %13
1166+
}

0 commit comments

Comments
 (0)