Skip to content

Commit c52011a

Browse files
gottesmmgottesmm
committed
[sil] When expanding aggregate instructions, do so consistently based off the code-size hueristic.
We were expanding retain_value, release_value properly, but not copy_addr/destroy_addr. This caused ARC invariants to break resulting in miscompiles. rdar://36509461
1 parent 4973417 commit c52011a

File tree

9 files changed

+279
-16
lines changed

9 files changed

+279
-16
lines changed

‎include/swift/SIL/TypeLowering.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -259,14 +259,6 @@ class TypeLowering {
259259

260260
enum class LoweringStyle { Shallow, Deep };
261261

262-
/// Given the result of the expansion heuristic,
263-
/// return appropriate lowering style.
264-
static LoweringStyle getLoweringStyle(bool shouldExpand) {
265-
if (shouldExpand)
266-
return TypeLowering::LoweringStyle::Deep;
267-
return TypeLowering::LoweringStyle::Shallow;
268-
}
269-
270262
//===--------------------------------------------------------------------===//
271263
// DestroyValue
272264
//===--------------------------------------------------------------------===//

‎lib/SILOptimizer/Transforms/SILLowerAggregateInstrs.cpp

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,17 +112,23 @@ static bool expandCopyAddr(CopyAddrInst *CA) {
112112
// retain_value %new : $*T
113113
IsTake_t IsTake = CA->isTakeOfSrc();
114114
if (IsTake_t::IsNotTake == IsTake) {
115-
TL.emitLoweredCopyValue(Builder, CA->getLoc(), New,
116-
TypeLowering::getLoweringStyle(expand));
115+
if (expand) {
116+
TL.emitLoweredCopyValueDeep(Builder, CA->getLoc(), New);
117+
} else {
118+
TL.emitCopyValue(Builder, CA->getLoc(), New);
119+
}
117120
}
118121

119122
// If we are not initializing:
120123
// strong_release %old : $*T
121124
// *or*
122125
// release_value %old : $*T
123126
if (Old) {
124-
TL.emitLoweredDestroyValue(Builder, CA->getLoc(), Old,
125-
TypeLowering::getLoweringStyle(expand));
127+
if (expand) {
128+
TL.emitLoweredDestroyValueDeep(Builder, CA->getLoc(), Old);
129+
} else {
130+
TL.emitDestroyValue(Builder, CA->getLoc(), Old);
131+
}
126132
}
127133
}
128134

@@ -155,8 +161,11 @@ static bool expandDestroyAddr(DestroyAddrInst *DA) {
155161
LoadInst *LI = Builder.createLoad(DA->getLoc(), Addr,
156162
LoadOwnershipQualifier::Unqualified);
157163
auto &TL = Module.getTypeLowering(Type);
158-
TL.emitLoweredDestroyValue(Builder, DA->getLoc(), LI,
159-
TypeLowering::getLoweringStyle(expand));
164+
if (expand) {
165+
TL.emitLoweredDestroyValueDeep(Builder, DA->getLoc(), LI);
166+
} else {
167+
TL.emitDestroyValue(Builder, DA->getLoc(), LI);
168+
}
160169
}
161170

162171
++NumExpand;

‎lib/SILOptimizer/Transforms/SILMem2Reg.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,12 @@ static void replaceDestroy(DestroyAddrInst *DAI, SILValue NewValue) {
350350

351351
bool expand = shouldExpand(DAI->getModule(),
352352
DAI->getOperand()->getType().getObjectType());
353-
TL.emitLoweredDestroyValue(Builder, DAI->getLoc(), NewValue,
354-
Lowering::TypeLowering::getLoweringStyle(expand));
353+
if (expand) {
354+
TL.emitLoweredDestroyValueDeep(Builder, DAI->getLoc(), NewValue);
355+
} else {
356+
TL.emitDestroyValue(Builder, DAI->getLoc(), NewValue);
357+
}
358+
355359
DAI->eraseFromParent();
356360
}
357361

‎test/Executable/Inputs/arc_36509461.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
2+
#ifndef ARC36509461_H
3+
#define ARC36509461_H
4+
5+
#include <stdbool.h>
6+
7+
typedef bool (^fake_apply_t)(const char *key, id value);
8+
9+
bool fake_apply(id obj, fake_apply_t applier);
10+
11+
#endif

‎test/Executable/Inputs/arc_36509461.m

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
2+
#import "arc_36509461.h"
3+
4+
bool fake_apply(id obj, fake_apply_t applier) {
5+
return false;
6+
}

‎test/Executable/arc_36509461.swift

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// RUN: %empty-directory(%T)
2+
// RUN: %target-clang -x objective-c -c %S/Inputs/arc_36509461.m -o %T/arc_36509461.m.o
3+
// RUN: %target-swift-frontend -c -O -import-objc-header %S/Inputs/arc_36509461.h -sanitize=address %s -o %T/arc_36509461.swift.o
4+
// RUN: %target-build-swift %T/arc_36509461.m.o %T/arc_36509461.swift.o -sanitize=address -o %t
5+
// RUN: %t
6+
7+
// REQUIRES: executable_test
8+
// REQUIRES: asan_runtime
9+
// REQUIRES: objc_interop
10+
11+
import Foundation
12+
13+
struct FakeUUID {
14+
var bigTuple: (UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8, UInt8)
15+
16+
init() {
17+
bigTuple = (0, 0, 0, 0, 0, 0, 0, 0)
18+
}
19+
}
20+
21+
struct Record {
22+
let name: String
23+
let uuid: FakeUUID
24+
let storage: NSObject
25+
26+
init(storage: NSObject, name: String, uuid: FakeUUID) {
27+
self.name = name
28+
self.uuid = uuid
29+
self.storage = storage
30+
}
31+
32+
func copy() -> Record {
33+
let copiedNSObject = NSObject()
34+
35+
fake_apply(self.storage) { (key, value) -> Bool in
36+
let x = copiedNSObject
37+
return true
38+
}
39+
40+
var record = Record(storage: copiedNSObject, name: self.name, uuid: self.uuid)
41+
return record
42+
}
43+
}
44+
45+
@inline(never)
46+
func foo(record: Record) -> Record {
47+
return record.copy()
48+
}
49+
50+
func main() {
51+
let record = Record(storage: NSObject(), name: "", uuid: FakeUUID())
52+
_ = foo(record: record)
53+
}
54+
55+
main()

‎test/SILOptimizer/loweraggregateinstrs.sil

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enable-sil-verify-all -enable-expand-all %s -lower-aggregate-instrs | %FileCheck %s
22

3+
// This file makes sure that the mechanics of expanding aggregate instructions
4+
// work. With that in mind, we expand all structs here ignoring code-size
5+
// trade-offs.
6+
37
sil_stage canonical
48

59
import Swift

‎test/SILOptimizer/loweraggregateinstrs_codesize.sil

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enable-sil-verify-all %s -lower-aggregate-instrs | %FileCheck %s
2+
3+
// This file makes sure that given the current code-size metric we properly
4+
// expand operations for small structs and not for large structs in a consistent
5+
// way for all operations we expand.
6+
7+
sil_stage canonical
8+
9+
import Swift
10+
import Builtin
11+
12+
class C1 {
13+
var data : Builtin.Int64
14+
init()
15+
}
16+
17+
class C2 {
18+
var data : Builtin.FPIEEE32
19+
init()
20+
}
21+
22+
class C3 {
23+
var data : Builtin.FPIEEE64
24+
init()
25+
}
26+
27+
struct S2 {
28+
var cls1 : C1
29+
var cls2 : C2
30+
var trivial : Builtin.FPIEEE32
31+
}
32+
33+
struct S {
34+
var trivial : Builtin.Int64
35+
var cls : C3
36+
var inner_struct : S2
37+
}
38+
39+
enum E {
40+
case NoElement()
41+
case TrivialElement(Builtin.Int64)
42+
case ReferenceElement(C1)
43+
case StructNonTrivialElt(S)
44+
case TupleNonTrivialElt((Builtin.Int64, S, C3))
45+
}
46+
47+
// This struct is larger than our current code-size limit (> 6 leaf nodes).
48+
struct LargeStruct {
49+
var trivial1 : Builtin.Int64
50+
var cls : S
51+
var trivial2 : Builtin.Int64
52+
var trivial3 : Builtin.Int64
53+
}
54+
55+
///////////
56+
// Tests //
57+
///////////
58+
59+
// This test makes sure that we /do not/ expand retain_value, release_value and
60+
// promote copy_addr/destroy_value to non-expanded retain_value, release_value.
61+
// CHECK-LABEL: sil @large_struct_test : $@convention(thin) (@owned LargeStruct, @in LargeStruct) -> () {
62+
// CHECK: bb0([[ARG0:%.*]] : $LargeStruct, [[ARG1:%.*]] : $*LargeStruct):
63+
// CHECK: retain_value [[ARG0]]
64+
// CHECK: release_value [[ARG0]]
65+
// CHECK: [[ALLOC_STACK:%.*]] = alloc_stack $LargeStruct
66+
// CHECK: [[LOADED_ARG1:%.*]] = load [[ARG1]]
67+
// CHECK: [[LOADED_OLD_VAL:%.*]] = load [[ALLOC_STACK]]
68+
// CHECK: retain_value [[LOADED_ARG1]]
69+
// CHECK: release_value [[LOADED_OLD_VAL]]
70+
// CHECK: store [[LOADED_ARG1]] to [[ALLOC_STACK]]
71+
// CHECK: [[LOADED_ARG1:%.*]] = load [[ARG1]]
72+
// CHECK: release_value [[LOADED_ARG1]]
73+
// CHECK: dealloc_stack [[ALLOC_STACK]]
74+
// CHECK: } // end sil function 'large_struct_test'
75+
sil @large_struct_test : $@convention(thin) (@owned LargeStruct, @in LargeStruct) -> () {
76+
bb0(%0 : $LargeStruct, %1 : $*LargeStruct):
77+
retain_value %0 : $LargeStruct
78+
release_value %0 : $LargeStruct
79+
%2 = alloc_stack $LargeStruct
80+
copy_addr %1 to %2 : $*LargeStruct
81+
destroy_addr %1 : $*LargeStruct
82+
dealloc_stack %2 : $*LargeStruct
83+
%9999 = tuple()
84+
return %9999 : $()
85+
}
86+
87+
// CHECK-LABEL: sil @small_struct_test : $@convention(thin) (@owned S2, @in S2) -> () {
88+
// CHECK: bb0([[ARG0:%.*]] : $S2, [[ARG1:%.*]] : $*S2):
89+
// CHECK: [[ARG0cls1:%.*]] = struct_extract [[ARG0]] : $S2, #S2.cls1
90+
// CHECK: strong_retain [[ARG0cls1]] : $C1
91+
// CHECK: [[ARG0cls2:%.*]] = struct_extract [[ARG0]] : $S2, #S2.cls2
92+
// CHECK: strong_retain [[ARG0cls2]] : $C2
93+
// CHECK: [[ARG0cls1:%.*]] = struct_extract [[ARG0]] : $S2, #S2.cls1
94+
// CHECK: strong_release [[ARG0cls1]] : $C1
95+
// CHECK: [[ARG0cls2:%.*]] = struct_extract [[ARG0]] : $S2, #S2.cls2
96+
// CHECK: strong_release [[ARG0cls2]] : $C2
97+
//
98+
// CHECK: [[ALLOC_STACK:%.*]] = alloc_stack $S2
99+
// CHECK: [[LOADED_ARG1:%.*]] = load [[ARG1]]
100+
// CHECK: [[LOADED_OLDVALUE:%.*]] = load [[ALLOC_STACK]]
101+
// CHECK: [[LOADED_ARG1cls1:%.*]] = struct_extract [[LOADED_ARG1]] : $S2, #S2.cls1
102+
// CHECK: strong_retain [[LOADED_ARG1cls1]] : $C1
103+
// CHECK: [[LOADED_ARG1cls2:%.*]] = struct_extract [[LOADED_ARG1]] : $S2, #S2.cls2
104+
// CHECK: strong_retain [[LOADED_ARG1cls2]] : $C2
105+
// CHECK: [[LOADED_OLDVALUEcls1:%.*]] = struct_extract [[LOADED_OLDVALUE]] : $S2, #S2.cls1
106+
// CHECK: strong_release [[LOADED_OLDVALUEcls1]] : $C1
107+
// CHECK: [[LOADED_OLDVALUEcls2:%.*]] = struct_extract [[LOADED_OLDVALUE]] : $S2, #S2.cls2
108+
// CHECK: strong_release [[LOADED_OLDVALUEcls2]] : $C2
109+
//
110+
// CHECK: [[LOADED_ARG1:%.*]] = load [[ARG1]]
111+
// CHECK: [[LOADED_ARG1cls1:%.*]] = struct_extract [[LOADED_ARG1]] : $S2, #S2.cls1
112+
// CHECK: strong_release [[LOADED_ARG1cls1]] : $C1
113+
// CHECK: [[LOADED_ARG1cls2:%.*]] = struct_extract [[LOADED_ARG1]] : $S2, #S2.cls2
114+
// CHECK: strong_release [[LOADED_ARG1cls2]] : $C2
115+
// CHECK: } // end sil function 'small_struct_test'
116+
sil @small_struct_test : $@convention(thin) (@owned S2, @in S2) -> () {
117+
bb0(%0 : $S2, %1 : $*S2):
118+
retain_value %0 : $S2
119+
release_value %0 : $S2
120+
%2 = alloc_stack $S2
121+
copy_addr %1 to %2 : $*S2
122+
destroy_addr %1 : $*S2
123+
dealloc_stack %2 : $*S2
124+
%9999 = tuple()
125+
return %9999 : $()
126+
}

‎test/SILOptimizer/mem2reg.sil

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,29 @@
33
import Builtin
44
import Swift
55

6+
//////////////////
7+
// Declarations //
8+
//////////////////
9+
10+
class Klass {}
11+
12+
struct SmallCodesizeStruct {
13+
var cls1 : Klass
14+
var cls2 : Klass
15+
}
16+
17+
struct LargeCodesizeStruct {
18+
var s1: SmallCodesizeStruct
19+
var s2: SmallCodesizeStruct
20+
var s3: SmallCodesizeStruct
21+
var s4: SmallCodesizeStruct
22+
var s5: SmallCodesizeStruct
23+
}
24+
25+
///////////
26+
// Tests //
27+
///////////
28+
629
// CHECK-LABEL: sil @store_only_allocas
730
// CHECK-NOT: alloc_stack
831
// CHECK: return
@@ -356,3 +379,36 @@ bb0(%0 : $*Int64):
356379
%4 = tuple ()
357380
return %4 : $()
358381
}
382+
383+
// Make sure that we do expand destroy_addr appropriately for code-size
384+
// trade-offs.
385+
// CHECK-LABEL: sil @large_struct_test : $@convention(thin) (@owned LargeCodesizeStruct) -> () {
386+
// CHECK: bb0([[ARG0:%.*]] : $LargeCodesizeStruct):
387+
// CHECK: release_value [[ARG0]]
388+
// CHECK: } // end sil function 'large_struct_test'
389+
sil @large_struct_test : $@convention(thin) (@owned LargeCodesizeStruct) -> () {
390+
bb0(%0 : $LargeCodesizeStruct):
391+
%1 = alloc_stack $LargeCodesizeStruct
392+
store %0 to %1 : $*LargeCodesizeStruct
393+
destroy_addr %1 : $*LargeCodesizeStruct
394+
dealloc_stack %1 : $*LargeCodesizeStruct
395+
%7 = tuple ()
396+
return %7 : $()
397+
}
398+
399+
// CHECK-LABEL: sil @small_struct_test : $@convention(thin) (@owned SmallCodesizeStruct) -> () {
400+
// CHECK: bb0([[ARG0:%.*]] : $SmallCodesizeStruct):
401+
// CHECK: [[ARG0cls1:%.*]] = struct_extract [[ARG0]]
402+
// CHECK: strong_release [[ARG0cls1]]
403+
// CHECK: [[ARG0cls2:%.*]] = struct_extract [[ARG0]]
404+
// CHECK: strong_release [[ARG0cls2]]
405+
// CHECK: } // end sil function 'small_struct_test'
406+
sil @small_struct_test : $@convention(thin) (@owned SmallCodesizeStruct) -> () {
407+
bb0(%0 : $SmallCodesizeStruct):
408+
%1 = alloc_stack $SmallCodesizeStruct
409+
store %0 to %1 : $*SmallCodesizeStruct
410+
destroy_addr %1 : $*SmallCodesizeStruct
411+
dealloc_stack %1 : $*SmallCodesizeStruct
412+
%7 = tuple ()
413+
return %7 : $()
414+
}

0 commit comments

Comments
 (0)