Skip to content

Commit c893c74

Browse files
committed
[SemanticARCOpts] Fix miscompile at dead-ends.
With incomplete lifetimes, a value may have an "incomplete lifetime"--it may not be destroyed on paths into dead-end regions. When a value lacks a destroy on a path into a dead-end region, it is effectively destroyed at its availability boundary (e.g. an `unreachable` instruction). Indeed, lifetime completion amounts to making such implicit destroys explicit (with correct nesting). `SemanticARCOpts`' `performGuaranteedCopyValueOptimization` attempts to eliminate a `copy_value` of a guaranteed value. To do so, it computes the live range (`OwnershipLiveRange`) of the copy_value. Among other things, it checks that all destroys of the copy_value are within the scope of all guaranteed bases of the copied value. If any destroys are _not_ within any of those scopes, the copy cannot be eliminated (because the copy would have been originally live beyond the range which it would be live in after copy elimination). A value with an incomplete lifetime is implicitly destroyed at its availability boundary. So those implicit destroys along the availability boundary must _also_ be within the scopes of those guaranteed bases. Previously, implicit destroys along availability boundaries were not considered. This resulted in invalid shortening of lifetimes--before the transformation a value would be unconsumed up to its availability boundary; after the transformation it would be consumed somewhere before that. For example: ``` sil [ossa] @f : $@convention(thin) (@owned Outer) -> () { entry(%o : $*Outer): %o_borrow = begin_borrow %o : $Outer %i = struct_extract %o_borrow : $Outer, #Outer.inner %i_copy = copy_value %i : $Inner end_borrow %o_borrow : $Outer destroy_value %o : $Outer apply @g(%i_copy) : $@convention(thin) (@guaranteed Inner) -> () unreachable // %i_copy implicitly destroyed } ``` Here, `%i_copy` is implicitly destroyed at `unreachable` but `%i` is consumed at the scope end of `%o_borrow`. That means that an attempt to RAUW `%i_copy` with `%i` would be invalid because uses of `%i_copy` may be outside the live range of `%i`. (Note that this happens not to occur in the above example because there's a bailout in the case that there are no destroys, but perturbing the example to include a cond_br on one branch of which the value is destroyed results in the miscompile described above.) Here, this is fixed by adding the instructions on the availability boundary at which the value is implicitly destroyed to the live range. Do this by adding the instructions on the availability boundary of each def from which the live range is generated to the live range. One wrinkle is that the OwnershipLiveRange utility is used in one place by a utility when SIL is in an invalid state (phi uses have been replaced with undef so values leaks on those paths). Account for this by manually fixing up the liveness instance passed to the lifetime completion utility. rdar://157291161
1 parent 3a6799b commit c893c74

File tree

5 files changed

+49
-5
lines changed

5 files changed

+49
-5
lines changed

lib/SILOptimizer/SemanticARC/OwnedToGuaranteedPhiOpt.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,11 @@ bool swift::semanticarc::tryConvertOwnedPhisToGuaranteedPhis(Context &ctx) {
212212
SmallVector<std::pair<SILValue, unsigned>, 8> incomingValueUpdates;
213213
for (auto introducer : ownedValueIntroducers) {
214214
SILValue v = introducer.value;
215-
OwnershipLiveRange lr(v);
215+
SmallVector<std::pair<Operand *, SILValue>, 4> extraConsumes;
216+
for (auto op : incomingValueOperandList) {
217+
extraConsumes.push_back({op.getOperand(), op.getOriginal()});
218+
}
219+
OwnershipLiveRange lr(v, extraConsumes);
216220

217221
// For now, we only handle copy_value for simplicity.
218222
//

lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@
1515

1616
#include "swift/Basic/Assertions.h"
1717
#include "swift/SIL/BasicBlockUtils.h"
18+
#include "swift/SIL/OSSALifetimeCompletion.h"
1819
#include "swift/SIL/OwnershipUtils.h"
1920

2021
using namespace swift;
2122
using namespace swift::semanticarc;
2223

23-
OwnershipLiveRange::OwnershipLiveRange(SILValue value)
24+
OwnershipLiveRange::OwnershipLiveRange(
25+
SILValue value, ArrayRef<std::pair<Operand *, SILValue>> extraUses)
2426
: introducer(OwnedValueIntroducer::get(value)), destroyingUses(),
2527
ownershipForwardingUses(), unknownConsumingUses() {
2628
assert(introducer);
@@ -30,6 +32,9 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
3032
SmallVector<UsePoint, 32> tmpForwardingConsumingUses;
3133
SmallVector<UsePoint, 32> tmpUnknownConsumingUses;
3234

35+
SmallVector<SILValue, 4> defs;
36+
defs.push_back(value);
37+
3338
// We know that our silvalue produces an @owned value. Look through all of our
3439
// uses and classify them as either consuming or not.
3540
SmallVector<Operand *, 32> worklist(introducer.value->getUses());
@@ -111,6 +116,7 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
111116
continue;
112117
}
113118
llvm::copy(v->getUses(), std::back_inserter(worklist));
119+
defs.push_back(v);
114120
}
115121
continue;
116122
}
@@ -138,8 +144,32 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
138144
// Otherwise add all users of this BBArg to the worklist to visit
139145
// recursively.
140146
llvm::copy(succArg->getUses(), std::back_inserter(worklist));
147+
defs.push_back(succArg);
148+
}
149+
}
150+
}
151+
152+
for (auto def : defs) {
153+
if (def->use_begin() == def->use_end()) {
154+
continue;
155+
}
156+
SmallVector<SILBasicBlock *, 32> blocks;
157+
SSAPrunedLiveness liveness(def->getFunction(), &blocks);
158+
liveness.initializeDef(def);
159+
liveness.computeSimple();
160+
for (auto use : extraUses) {
161+
if (use.second != def) {
162+
continue;
141163
}
164+
liveness.updateForUse(use.first->getUser(), /*lifetimeEnding=*/true);
142165
}
166+
OSSALifetimeCompletion::visitAvailabilityBoundary(
167+
def, liveness, [&tmpDestroyingUses](auto *inst, auto end) {
168+
if (end != OSSALifetimeCompletion::LifetimeEnd::Boundary) {
169+
return;
170+
}
171+
tmpDestroyingUses.push_back(inst);
172+
});
143173
}
144174

145175
// The order in which we append these to consumingUses matters since we assume
@@ -264,6 +294,8 @@ void OwnershipLiveRange::convertToGuaranteedAndRAUW(
264294
auto point = destroyingUses.back();
265295
auto *destroy = point.getInstruction();
266296
destroyingUses = destroyingUses.drop_back();
297+
if (isa<TermInst>(destroy))
298+
continue;
267299
callbacks.deleteInst(destroy);
268300
}
269301

@@ -323,6 +355,8 @@ void OwnershipLiveRange::convertJoinedLiveRangePhiToGuaranteed(
323355
auto point = destroyingUses.back();
324356
auto *destroy = point.getInstruction();
325357
destroyingUses = destroyingUses.drop_back();
358+
if (isa<TermInst>(destroy))
359+
continue;
326360
callbacks.deleteInst(destroy);
327361
}
328362

lib/SILOptimizer/SemanticARC/OwnershipLiveRange.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ class LLVM_LIBRARY_VISIBILITY OwnershipLiveRange {
9494
ArrayRef<UsePoint> unknownConsumingUses;
9595

9696
public:
97-
OwnershipLiveRange(SILValue value);
97+
OwnershipLiveRange(SILValue value,
98+
ArrayRef<std::pair<Operand *, SILValue>> extraUses = {});
9899
OwnershipLiveRange(const OwnershipLiveRange &) = delete;
99100
OwnershipLiveRange &operator=(const OwnershipLiveRange &) = delete;
100101

lib/SILOptimizer/SemanticARC/OwnershipPhiOperand.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ class LLVM_LIBRARY_VISIBILITY OwnershipPhiOperand {
3636

3737
private:
3838
Operand *op;
39+
SILValue original;
3940

4041
OwnershipPhiOperand(Operand *op) : op(op) {}
4142

@@ -70,10 +71,14 @@ class LLVM_LIBRARY_VISIBILITY OwnershipPhiOperand {
7071
Operand *getOperand() const { return op; }
7172
SILValue getValue() const { return op->get(); }
7273
SILType getType() const { return op->get()->getType(); }
74+
SILValue getOriginal() const { return original; }
7375

7476
unsigned getOperandNumber() const { return op->getOperandNumber(); }
7577

76-
void markUndef() & { op->set(SILUndef::get(getValue())); }
78+
void markUndef() & {
79+
original = op->get();
80+
op->set(SILUndef::get(getValue()));
81+
}
7782

7883
SILInstruction *getInst() const { return op->getUser(); }
7984

test/SILOptimizer/semantic-arc-opts-unit.sil

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: not --crash %target-sil-opt -test-runner %s -o /dev/null 2>&1 | %FileCheck %s
1+
// RUN: %target-sil-opt -test-runner %s -o /dev/null 2>&1 | %FileCheck %s
22

33
sil_stage canonical
44

0 commit comments

Comments
 (0)