Skip to content

Commit 1d950a5

Browse files
committed
SILGen: Ease off +0 peepholes for load exprs.
Now that we more tightly close formal accesses on lvalues, having LoadExpr and friends try to return a +0 loaded value is unsafe without deeper analysis, since the access will be closed immediately after the load and potentially free temporary memory that might be the only thing keeping the borrowed object alive. Fixes rdar://problem/32730865. (cherry picked from commit 7705393)
1 parent c3ff4e3 commit 1d950a5

File tree

5 files changed

+47
-8
lines changed

5 files changed

+47
-8
lines changed

lib/SILGen/SILGenExpr.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,7 +1081,10 @@ RValue RValueEmitter::visitLoadExpr(LoadExpr *E, SGFContext C) {
10811081
// Any writebacks here are tightly scoped.
10821082
FormalEvaluationScope writeback(SGF);
10831083
LValue lv = SGF.emitLValue(E->getSubExpr(), AccessKind::Read);
1084-
return SGF.emitLoadOfLValue(E, std::move(lv), C);
1084+
// We can't load at immediate +0 from the lvalue without deeper analysis,
1085+
// since the access will be immediately ended and might invalidate the value
1086+
// we loaded.
1087+
return SGF.emitLoadOfLValue(E, std::move(lv), C.withFollowingSideEffects());
10851088
}
10861089

10871090
SILValue SILGenFunction::emitTemporaryAllocation(SILLocation loc,
@@ -2068,7 +2071,9 @@ RValue RValueEmitter::visitMemberRefExpr(MemberRefExpr *E, SGFContext C) {
20682071
FormalEvaluationScope scope(SGF);
20692072

20702073
LValue lv = SGF.emitLValue(E, AccessKind::Read);
2071-
return SGF.emitLoadOfLValue(E, std::move(lv), C);
2074+
// We can't load at +0 without further analysis, since the formal access into
2075+
// the lvalue will end immediately.
2076+
return SGF.emitLoadOfLValue(E, std::move(lv), C.withFollowingSideEffects());
20722077
}
20732078

20742079
RValue RValueEmitter::visitDynamicMemberRefExpr(DynamicMemberRefExpr *E,
@@ -2087,7 +2092,9 @@ RValue RValueEmitter::visitSubscriptExpr(SubscriptExpr *E, SGFContext C) {
20872092
FormalEvaluationScope scope(SGF);
20882093

20892094
LValue lv = SGF.emitLValue(E, AccessKind::Read);
2090-
return SGF.emitLoadOfLValue(E, std::move(lv), C);
2095+
// We can't load at +0 without further analysis, since the formal access into
2096+
// the lvalue will end immediately.
2097+
return SGF.emitLoadOfLValue(E, std::move(lv), C.withFollowingSideEffects());
20912098
}
20922099

20932100
RValue RValueEmitter::visitDynamicSubscriptExpr(

lib/SILGen/SILGenLValue.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3021,7 +3021,7 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
30213021
PathComponent &&component =
30223022
drillToLastComponent(*this, loc, std::move(src), addr, AccessKind::Read);
30233023

3024-
// If the last component is physical, just drill down and load from it.
3024+
// If the last component is physical, drill down and load from it.
30253025
if (component.isPhysical()) {
30263026
addr = std::move(component.asPhysical())
30273027
.offset(*this, loc, addr, AccessKind::Read);
@@ -3031,7 +3031,7 @@ RValue SILGenFunction::emitLoadOfLValue(SILLocation loc, LValue &&src,
30313031
isGuaranteedValid));
30323032
}
30333033

3034-
// If the last component is logical, just emit a get.
3034+
// If the last component is logical, emit a get.
30353035
return std::move(component.asLogical()).get(*this, loc, addr, C);
30363036
}
30373037

test/SILGen/dynamic_lookup.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func direct_to_static_method(_ obj: AnyObject) {
6262
// CHECK: store [[ARG_COPY]] to [init] [[PBOBJ]] : $*AnyObject
6363
// CHECK: end_borrow [[BORROWED_ARG]] from [[ARG]]
6464
// CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[PBOBJ]]
65-
// CHECK-NEXT: [[OBJCOPY:%[0-9]+]] = load_borrow [[READ]] : $*AnyObject
65+
// CHECK-NEXT: [[OBJCOPY:%[0-9]+]] = load [copy] [[READ]] : $*AnyObject
6666
// CHECK: end_access [[READ]]
6767
// CHECK-NEXT: [[OBJMETA:%[0-9]+]] = existential_metatype $@thick AnyObject.Type, [[OBJCOPY]] : $AnyObject
6868
// CHECK-NEXT: [[OPENMETA:%[0-9]+]] = open_existential_metatype [[OBJMETA]] : $@thick AnyObject.Type to $@thick (@opened([[UUID:".*"]]) AnyObject).Type
@@ -141,7 +141,7 @@ func opt_to_static_method(_ obj: AnyObject) {
141141
// CHECK: [[OPTBOX:%[0-9]+]] = alloc_box ${ var Optional<@callee_owned () -> ()> }
142142
// CHECK: [[PBO:%.*]] = project_box [[OPTBOX]]
143143
// CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[PBOBJ]]
144-
// CHECK: [[OBJCOPY:%[0-9]+]] = load_borrow [[READ]] : $*AnyObject
144+
// CHECK: [[OBJCOPY:%[0-9]+]] = load [copy] [[READ]] : $*AnyObject
145145
// CHECK: [[OBJMETA:%[0-9]+]] = existential_metatype $@thick AnyObject.Type, [[OBJCOPY]] : $AnyObject
146146
// CHECK: [[OPENMETA:%[0-9]+]] = open_existential_metatype [[OBJMETA]] : $@thick AnyObject.Type to $@thick (@opened
147147
// CHECK: [[OBJCMETA:%[0-9]+]] = thick_to_objc_metatype [[OPENMETA]]

test/SILGen/functions.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func calls(_ i:Int, j:Int, k:Int) {
176176
// -- Curry the Type onto static method argument lists.
177177

178178
// CHECK: [[READC:%.*]] = begin_access [read] [unknown] [[CADDR]]
179-
// CHECK: [[C:%[0-9]+]] = load_borrow [[READC]]
179+
// CHECK: [[C:%[0-9]+]] = load [copy] [[READC]]
180180
// CHECK: [[META:%.*]] = value_metatype $@thick SomeClass.Type, [[C]]
181181
// CHECK: [[METHOD:%[0-9]+]] = class_method [[META]] : {{.*}}, #SomeClass.static_method!1
182182
// CHECK: [[READI:%.*]] = begin_access [read] [unknown] [[IADDR]]
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// RUN: %target-swift-frontend -emit-silgen %s | %FileCheck %s
2+
3+
class A {
4+
lazy var b: B = B()
5+
}
6+
7+
final class B {
8+
var c: C? {
9+
get { return nil }
10+
set {}
11+
}
12+
}
13+
14+
struct C {
15+
let d: String
16+
}
17+
18+
// CHECK-LABEL: sil hidden @{{.*}}test
19+
func test(a: A) {
20+
let s: String?
21+
// CHECK: [[C_TEMP:%.*]] = alloc_stack $Optional<C>
22+
// CHECK: [[TAG:%.*]] = select_enum_addr [[C_TEMP]]
23+
// CHECK: cond_br [[TAG]], [[SOME:bb[0-9]+]], [[NONE:bb[0-9]+]]
24+
// CHECK: [[SOME]]:
25+
// CHECK: [[C_PAYLOAD:%.*]] = unchecked_take_enum_data_addr [[C_TEMP]]
26+
// -- This must be a copy, since we'll immediately destroy the value in the
27+
// temp buffer
28+
// CHECK: [[LOAD:%.*]] = load [copy] [[C_PAYLOAD]]
29+
// CHECK: destroy_addr [[C_TEMP]]
30+
s = a.b.c?.d
31+
print(s)
32+
}

0 commit comments

Comments
 (0)