Skip to content

Commit c52ae08

Browse files
jckarterjckarter
committed
SILGen: Don't reuse the Initialization across branches of an if or switch expression.
`Initialization` is stateful and not meant to be emitted into multiple times across different contexts. If emitting into an initialization causes it to be split or aborted, that will carry over into further uses of the initialization. This was happening during `if` and `switch` expression emission, leading to miscompiles or compiler crashes. Fix this by saving only the buffer when we prepare emission for a statement expression, and creating the initialization in the scope where the expression for a branch actually gets emitted. Fixes rdar://112213253.
1 parent ddeab73 commit c52ae08

File tree

8 files changed

+92
-45
lines changed

8 files changed

+92
-45
lines changed

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2200,17 +2200,9 @@ RValue RValueEmitter::visitSingleValueStmtExpr(SingleValueStmtExpr *E,
22002200
auto &lowering = SGF.getTypeLowering(E->getType());
22012201
auto resultAddr = SGF.emitTemporaryAllocation(E, lowering.getLoweredType());
22022202

2203-
// This won't give us a useful diagnostic if the result doesn't end up
2204-
// initialized ("variable '<unknown>' used before being initialized"), but it
2205-
// will at least catch a potential miscompile when the SIL verifier is
2206-
// disabled.
2207-
resultAddr = SGF.B.createMarkUninitialized(
2208-
E, resultAddr, MarkUninitializedInst::Kind::Var);
2209-
KnownAddressInitialization init(resultAddr);
2210-
22112203
// Collect the target exprs that will be used for initialization.
22122204
SmallVector<Expr *, 4> scratch;
2213-
SILGenFunction::SingleValueStmtInitialization initInfo(&init);
2205+
SILGenFunction::SingleValueStmtInitialization initInfo(resultAddr);
22142206
for (auto *E : E->getSingleExprBranches(scratch))
22152207
initInfo.Exprs.insert(E);
22162208

‎lib/SILGen/SILGenFunction.cpp

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1650,7 +1650,7 @@ void SILGenFunction::emitGeneratorFunction(
16501650
mergeCleanupBlocks();
16511651
}
16521652

1653-
Initialization *SILGenFunction::getSingleValueStmtInit(Expr *E) {
1653+
std::unique_ptr<Initialization> SILGenFunction::getSingleValueStmtInit(Expr *E) {
16541654
if (SingleValueStmtInitStack.empty())
16551655
return nullptr;
16561656

@@ -1659,7 +1659,15 @@ Initialization *SILGenFunction::getSingleValueStmtInit(Expr *E) {
16591659
if (!SingleValueStmtInitStack.back().Exprs.contains(E))
16601660
return nullptr;
16611661

1662-
return SingleValueStmtInitStack.back().Init;
1662+
// This won't give us a useful diagnostic if the result doesn't end up
1663+
// initialized ("variable '<unknown>' used before being initialized"), but it
1664+
// will at least catch a potential miscompile when the SIL verifier is
1665+
// disabled.
1666+
auto resultAddr = SingleValueStmtInitStack.back().InitializationBuffer;
1667+
resultAddr = B.createMarkUninitialized(
1668+
E, resultAddr, MarkUninitializedInst::Kind::Var);
1669+
1670+
return std::make_unique<KnownAddressInitialization>(resultAddr);
16631671
}
16641672

16651673
void SILGenFunction::emitProfilerIncrement(ASTNode Node) {

‎lib/SILGen/SILGenFunction.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,9 +353,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
353353
struct SingleValueStmtInitialization {
354354
/// The target expressions to be used for initialization.
355355
SmallPtrSet<Expr *, 4> Exprs;
356-
Initialization *Init;
356+
SILValue InitializationBuffer;
357357

358-
SingleValueStmtInitialization(Initialization *init) : Init(init) {}
358+
SingleValueStmtInitialization(SILValue buffer)
359+
: InitializationBuffer(buffer) {}
359360
};
360361

361362
/// A stack of active SingleValueStmtExpr initializations that may be
@@ -741,7 +742,7 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
741742
/// Check to see if an initalization for a SingleValueStmtExpr is active, and
742743
/// if the provided expression is for one of its branches. If so, returns the
743744
/// initialization to use for the expression. Otherwise returns \c nullptr.
744-
Initialization *getSingleValueStmtInit(Expr *E);
745+
std::unique_ptr<Initialization> getSingleValueStmtInit(Expr *E);
745746

746747
//===--------------------------------------------------------------------===//
747748
// Entry points for codegen

‎lib/SILGen/SILGenLValue.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2981,6 +2981,7 @@ LValue SILGenLValue::visitRec(Expr *e, SGFAccessKind accessKind,
29812981
// a `borrow x` operator, the operator is used on the base here), we want to
29822982
// apply the lvalue within a formal access to the original value instead of
29832983
// an actual loaded copy.
2984+
29842985
if (e->getType()->isPureMoveOnly()) {
29852986
if (auto load = dyn_cast<LoadExpr>(e)) {
29862987
LValue lv = visitRec(load->getSubExpr(), SGFAccessKind::BorrowedAddressRead,
@@ -2992,7 +2993,7 @@ LValue SILGenLValue::visitRec(Expr *e, SGFAccessKind accessKind,
29922993
return lv;
29932994
}
29942995
}
2995-
2996+
29962997
// Otherwise we have a non-lvalue type (references, values, metatypes,
29972998
// etc). These act as the root of a logical lvalue. Compute the root value,
29982999
// wrap it in a ValueComponent, and return it for our caller.

‎lib/SILGen/SILGenStmt.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -423,9 +423,9 @@ void StmtEmitter::visitBraceStmt(BraceStmt *S) {
423423
// active, and if so, use it for this expression branch. If the expression
424424
// is uninhabited, we can skip this, and let unreachability checking
425425
// handle it.
426-
auto *init = SGF.getSingleValueStmtInit(E);
426+
auto init = SGF.getSingleValueStmtInit(E);
427427
if (init && !E->getType()->isStructurallyUninhabited()) {
428-
SGF.emitExprInto(E, init);
428+
SGF.emitExprInto(E, init.get());
429429
} else {
430430
SGF.emitIgnoredExpr(E);
431431
}

‎test/SILGen/if_expr.swift

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,23 +7,24 @@ func foo() -> Int {
77

88
// CHECK-LABEL: sil hidden [ossa] @$s7if_expr3fooSiyF : $@convention(thin) () -> Int
99
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $Int
10-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
1110
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
1211
//
1312
// CHECK: [[TRUEBB]]:
13+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
1414
// CHECK: [[ONE_BUILTIN:%[0-9]+]] = integer_literal $Builtin.IntLiteral, 1
1515
// CHECK: [[ONE:%[0-9]+]] = apply {{%[0-9]+}}([[ONE_BUILTIN]], {{%[0-9]+}}) : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int
1616
// CHECK: store [[ONE]] to [trivial] [[RESULT]] : $*Int
1717
// CHECK: br [[EXITBB:bb[0-9]+]]
1818
//
1919
// CHECK: [[FALSEBB]]:
20+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
2021
// CHECK: [[TWO_BUILTIN:%[0-9]+]] = integer_literal $Builtin.IntLiteral, 2
2122
// CHECK: [[TWO:%[0-9]+]] = apply {{%[0-9]+}}([[TWO_BUILTIN]], {{%[0-9]+}}) : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int
2223
// CHECK: store [[TWO]] to [trivial] [[RESULT]] : $*Int
2324
// CHECK: br [[EXITBB]]
2425
//
2526
// CHECK: [[EXITBB]]:
26-
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT]] : $*Int
27+
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT_STORAGE]] : $*Int
2728
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*Int
2829
// CHECK: return [[VAL]] : $Int
2930

@@ -36,22 +37,23 @@ func bar(_ x: C) -> C {
3637
// CHECK-LABEL: sil hidden [ossa] @$s7if_expr3baryAA1CCADF : $@convention(thin) (@guaranteed C) -> @owned C
3738
// CHECK: bb0([[CPARAM:%[0-9]+]] : @guaranteed $C):
3839
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $C
39-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*C
4040
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
4141
//
4242
// CHECK: [[TRUEBB]]:
43+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*C
4344
// CHECK: [[C:%[0-9]+]] = copy_value [[CPARAM]] : $C
4445
// CHECK: store [[C]] to [init] [[RESULT]] : $*C
4546
// CHECK: br [[EXITBB:bb[0-9]+]]
4647
//
4748
// CHECK: [[FALSEBB]]:
49+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*C
4850
// CHECK: [[CTOR:%[0-9]+]] = function_ref @$s7if_expr1CCACycfC : $@convention(method) (@thick C.Type) -> @owned C
4951
// CHECK: [[C:%[0-9]+]] = apply [[CTOR]]({{%[0-9]+}}) : $@convention(method) (@thick C.Type) -> @owned C
5052
// CHECK: store [[C]] to [init] [[RESULT]] : $*C
5153
// CHECK: br [[EXITBB]]
5254
//
5355
// CHECK: [[EXITBB]]:
54-
// CHECK: [[VAL:%[0-9]+]] = load [take] [[RESULT]] : $*C
56+
// CHECK: [[VAL:%[0-9]+]] = load [take] [[RESULT_STORAGE]] : $*C
5557
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*C
5658
// CHECK: return [[VAL]] : $C
5759

@@ -69,7 +71,6 @@ func baz() throws -> Int {
6971

7072
// CHECK-LABEL: sil hidden [ossa] @$s7if_expr3bazSiyKF : $@convention(thin) () -> (Int, @error any Error)
7173
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $Int
72-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
7374
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
7475
//
7576
// CHECK: [[FALSEBB]]:
@@ -82,7 +83,7 @@ func baz() throws -> Int {
8283
// CHECK: br [[EXITBB:bb[0-9]+]]
8384
//
8485
// CHECK: [[EXITBB]]:
85-
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT]] : $*Int
86+
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT_STORAGE]] : $*Int
8687
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*Int
8788
// CHECK: return [[VAL]] : $Int
8889

@@ -92,18 +93,18 @@ func qux() throws -> Int {
9293

9394
// CHECK-LABEL: sil hidden [ossa] @$s7if_expr3quxSiyKF : $@convention(thin) () -> (Int, @error any Error)
9495
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $Int
95-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
9696
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
9797
//
9898
// CHECK: [[FALSEBB]]:
99+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
99100
// CHECK: try_apply {{%[0-9]+}}() : $@convention(thin) () -> (Int, @error any Error), normal [[NORMALBB:bb[0-9]+]], error [[ERRORBB:bb[0-9]+]]
100101
//
101102
// CHECK: [[NORMALBB]]([[BAZVAL:%[0-9]+]] : $Int):
102103
// CHECK: store [[BAZVAL]] to [trivial] [[RESULT]] : $*Int
103104
// CHECK: br [[EXITBB:bb[0-9]+]]
104105
//
105106
// CHECK: [[EXITBB]]:
106-
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT]] : $*Int
107+
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT_STORAGE]] : $*Int
107108
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*Int
108109
// CHECK: return [[VAL]] : $Int
109110
//
@@ -141,18 +142,18 @@ func testClosure() throws -> Int {
141142

142143
// CHECK-LABEL: sil private [ossa] @$s7if_expr11testClosureSiyKFSiyKcfU_ : $@convention(thin) () -> (Int, @error any Error)
143144
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $Int
144-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
145145
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
146146
//
147147
// CHECK: [[FALSEBB]]:
148+
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
148149
// CHECK: try_apply {{%[0-9]+}}() : $@convention(thin) () -> (Int, @error any Error), normal [[NORMALBB:bb[0-9]+]], error [[ERRORBB:bb[0-9]+]]
149150
//
150151
// CHECK: [[NORMALBB]]([[BAZVAL:%[0-9]+]] : $Int):
151152
// CHECK: store [[BAZVAL]] to [trivial] [[RESULT]] : $*Int
152153
// CHECK: br [[EXITBB:bb[0-9]+]]
153154
//
154155
// CHECK: [[EXITBB]]:
155-
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT]] : $*Int
156+
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT_STORAGE]] : $*Int
156157
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*Int
157158
// CHECK: return [[VAL]] : $Int
158159
//
@@ -174,7 +175,6 @@ func testNested() throws -> Int {
174175

175176
// CHECK-LABEL: sil hidden [ossa] @$s7if_expr10testNestedSiyKF : $@convention(thin) () -> (Int, @error any Error)
176177
// CHECK: [[RESULT_STORAGE:%[0-9]+]] = alloc_stack $Int
177-
// CHECK: [[RESULT:%[0-9]+]] = mark_uninitialized [var] [[RESULT_STORAGE]] : $*Int
178178
// CHECK: cond_br {{%[0-9]+}}, [[TRUEBB:bb[0-9]+]], [[FALSEBB:bb[0-9]+]]
179179
//
180180
// CHECK: [[FALSEBB]]:
@@ -187,7 +187,7 @@ func testNested() throws -> Int {
187187
// CHECK: br [[EXITBB:bb[0-9]+]]
188188
//
189189
// CHECK: [[EXITBB]]:
190-
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT]] : $*Int
190+
// CHECK: [[VAL:%[0-9]+]] = load [trivial] [[RESULT_STORAGE]] : $*Int
191191
// CHECK: dealloc_stack [[RESULT_STORAGE]] : $*Int
192192
// CHECK: return [[VAL]] : $Int
193193

0 commit comments

Comments
 (0)