Skip to content

Commit f57a6cf

Browse files
committed
[silgen] Teach CleanupCloner how to handle OwnedValueWritebackCleanups correctly.
Previously we would just forward the cleanup and create a normal "destroy" cleanup resulting in early deallocation and use after frees along error paths. As part of this I also had to tweak how DI recognizes self init writebacks to not use SILLocations. This is approach is more robust (since we aren't relying on SourceLocs/have verifiers to make sure we don't violate SIL) and also avoids issues from the write back store not necessarily have the same SILLocation. <rdar://problem/59830255>
1 parent bcdebc6 commit f57a6cf

9 files changed

+839
-11
lines changed

lib/SILGen/Cleanup.cpp

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,21 @@ void CleanupManager::setCleanupState(CleanupsDepth depth, CleanupState state) {
220220
popTopDeadCleanups();
221221
}
222222

223+
std::tuple<Cleanup::Flags, Optional<SILValue>>
224+
CleanupManager::getFlagsAndWritebackBuffer(CleanupHandle depth) {
225+
auto iter = stack.find(depth);
226+
assert(iter != stack.end() && "can't change end of cleanups stack");
227+
assert(iter->getState() != CleanupState::Dead &&
228+
"Trying to get writeback buffer of a dead cleanup?!");
229+
230+
auto resultFlags = iter->getFlags();
231+
Optional<SILValue> result;
232+
bool foundValue = iter->getWritebackBuffer([&](SILValue v) { result = v; });
233+
(void)foundValue;
234+
assert(result.hasValue() == foundValue);
235+
return std::make_tuple(resultFlags, result);
236+
}
237+
223238
void CleanupManager::forwardCleanup(CleanupsDepth handle) {
224239
auto iter = stack.find(handle);
225240
assert(iter != stack.end() && "can't change end of cleanups stack");
@@ -344,7 +359,16 @@ void CleanupStateRestorationScope::pop() && { popImpl(); }
344359
//===----------------------------------------------------------------------===//
345360

346361
CleanupCloner::CleanupCloner(SILGenFunction &SGF, const ManagedValue &mv)
347-
: SGF(SGF), hasCleanup(mv.hasCleanup()), isLValue(mv.isLValue()) {}
362+
: SGF(SGF), hasCleanup(mv.hasCleanup()), isLValue(mv.isLValue()),
363+
writebackBuffer(None) {
364+
if (hasCleanup) {
365+
auto handle = mv.getCleanup();
366+
auto state = SGF.Cleanups.getFlagsAndWritebackBuffer(handle);
367+
if (SILValue value = std::get<1>(state).getValueOr(SILValue())) {
368+
writebackBuffer = value;
369+
}
370+
}
371+
}
348372

349373
CleanupCloner::CleanupCloner(SILGenBuilder &builder, const ManagedValue &mv)
350374
: CleanupCloner(builder.getSILGenFunction(), mv) {}
@@ -372,6 +396,14 @@ ManagedValue CleanupCloner::clone(SILValue value) const {
372396
return ManagedValue::forUnmanaged(value);
373397
}
374398

399+
if (writebackBuffer.hasValue()) {
400+
auto loc = RegularLocation::getAutoGeneratedLocation();
401+
auto cleanup =
402+
SGF.enterOwnedValueWritebackCleanup(loc, *writebackBuffer, value);
403+
return ManagedValue::forExclusivelyBorrowedOwnedObjectRValue(value,
404+
cleanup);
405+
}
406+
375407
if (value->getType().isAddress()) {
376408
return SGF.emitManagedBufferWithCleanup(value);
377409
}

lib/SILGen/Cleanup.h

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include "swift/Basic/DiverseStack.h"
2121
#include "swift/SIL/SILLocation.h"
22+
#include "swift/SIL/SILValue.h"
2223
#include "llvm/ADT/SmallVector.h"
2324

2425
namespace swift {
@@ -75,10 +76,23 @@ llvm::raw_ostream &operator<<(raw_ostream &os, CleanupState state);
7576

7677
class LLVM_LIBRARY_VISIBILITY Cleanup {
7778
friend class CleanupManager;
79+
friend class CleanupCloner;
7880

79-
unsigned allocatedSize;
81+
protected:
82+
// A set of flags that categorize the type of cleanup such that it can be
83+
// recreated via SILGenFunction methods based on the type of argument input.
84+
//
85+
// Example: Distinguishing in between @owned cleanups with a writeback buffer
86+
// (ExclusiveBorrowCleanup) or ones that involve formal access cleanups.
87+
enum class Flags : uint8_t {
88+
None = 0,
89+
ExclusiveBorrowCleanup = 1,
90+
};
8091

92+
private:
8193
CleanupState state;
94+
unsigned allocatedSize : 24;
95+
Flags flags : 8;
8296

8397
protected:
8498
Cleanup() {}
@@ -99,6 +113,16 @@ class LLVM_LIBRARY_VISIBILITY Cleanup {
99113
virtual void emit(SILGenFunction &SGF, CleanupLocation loc,
100114
ForUnwind_t forUnwind) = 0;
101115
virtual void dump(SILGenFunction &SGF) const = 0;
116+
117+
protected:
118+
Flags getFlags() const { return flags; }
119+
120+
/// Call func passing in the SILValue address that this cleanup will write
121+
/// back to if supported and any flags associated with the cleanup. Returns
122+
/// false otherwise.
123+
virtual bool getWritebackBuffer(function_ref<void(SILValue)> func) {
124+
return false;
125+
}
102126
};
103127

104128
/// A cleanup depth is generally used to denote the set of cleanups
@@ -117,6 +141,7 @@ typedef DiverseStackImpl<Cleanup>::stable_iterator CleanupHandle;
117141

118142
class LLVM_LIBRARY_VISIBILITY CleanupManager {
119143
friend class Scope;
144+
friend class CleanupCloner;
120145

121146
SILGenFunction &SGF;
122147

@@ -229,7 +254,7 @@ class LLVM_LIBRARY_VISIBILITY CleanupManager {
229254
/// Set the state of the cleanup at the given depth.
230255
/// The transition must be non-trivial and legal.
231256
void setCleanupState(CleanupHandle depth, CleanupState state);
232-
257+
233258
/// True if there are any active cleanups in the scope between the two
234259
/// cleanup handles.
235260
bool hasAnyActiveCleanups(CleanupsDepth from, CleanupsDepth to);
@@ -246,6 +271,12 @@ class LLVM_LIBRARY_VISIBILITY CleanupManager {
246271

247272
/// Verify that the given cleanup handle is valid.
248273
void checkIterator(CleanupHandle handle) const;
274+
275+
private:
276+
// Look up the flags and optionally the writeback address associated with the
277+
// cleanup at \p depth. If
278+
std::tuple<Cleanup::Flags, Optional<SILValue>>
279+
getFlagsAndWritebackBuffer(CleanupHandle depth);
249280
};
250281

251282
/// An RAII object that allows the state of a cleanup to be
@@ -274,10 +305,14 @@ class CleanupStateRestorationScope {
274305
void popImpl();
275306
};
276307

308+
/// Extract enough information from a managed value to reliably clone its
309+
/// cleanup (if it has any) on a newly computed type. This includes modeling
310+
/// writeback buffers.
277311
class CleanupCloner {
278312
SILGenFunction &SGF;
279313
bool hasCleanup;
280314
bool isLValue;
315+
Optional<SILValue> writebackBuffer;
281316

282317
public:
283318
CleanupCloner(SILGenFunction &SGF, const ManagedValue &mv);

lib/SILGen/SILGenExpr.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,7 @@ namespace {
722722
/// cleanup to take ownership of the value and thus prevent it form being
723723
/// written back.
724724
struct OwnedValueWritebackCleanup final : Cleanup {
725+
using Flags = Cleanup::Flags;
725726

726727
/// We store our own loc so that we can ensure that DI ignores our writeback.
727728
SILLocation loc;
@@ -733,6 +734,11 @@ struct OwnedValueWritebackCleanup final : Cleanup {
733734
SILValue value)
734735
: loc(loc), lvalueAddress(lvalueAddress), value(value) {}
735736

737+
bool getWritebackBuffer(function_ref<void(SILValue)> func) override {
738+
func(lvalueAddress);
739+
return true;
740+
}
741+
736742
void emit(SILGenFunction &SGF, CleanupLocation l, ForUnwind_t forUnwind) override {
737743
SILValue valueToStore = value;
738744
SILType lvalueObjTy = lvalueAddress->getType().getObjectType();

lib/SILGen/SILGenFunction.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,6 +1188,8 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
11881188
CleanupHandle enterDeallocateUninitializedArrayCleanup(SILValue array);
11891189
void emitUninitializedArrayDeallocation(SILLocation loc, SILValue array);
11901190

1191+
/// Emit a cleanup for an owned value that should be written back at end of
1192+
/// scope if the value is not forwarded.
11911193
CleanupHandle enterOwnedValueWritebackCleanup(SILLocation loc,
11921194
SILValue address,
11931195
SILValue newValue);

lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1639,6 +1639,7 @@ void ClassInitElementUseCollector::collectClassInitSelfUses() {
16391639
}
16401640

16411641
// A store of a load from the box is ignored.
1642+
//
16421643
// SILGen emits these if delegation to another initializer was
16431644
// interrupted before the initializer was called.
16441645
SILValue src = SI->getSrc();
@@ -1778,14 +1779,30 @@ void ClassInitElementUseCollector::collectClassInitSelfLoadUses(
17781779
}
17791780
}
17801781

1781-
// If this load's value is being stored back into the delegating
1782-
// mark_uninitialized buffer and it is a self init use, skip the
1783-
// use. This is to handle situations where due to usage of a metatype to
1784-
// allocate, we do not actually consume self.
1782+
// If this load's value is being stored immediately back into the delegating
1783+
// mark_uninitialized buffer, skip the use.
1784+
//
1785+
// This is to handle situations where we do not actually consume self as a
1786+
// result of situations such as:
1787+
//
1788+
// 1. The usage of a metatype to allocate the object.
1789+
//
1790+
// 2. If our self init call has a throwing function as an argument that
1791+
// actually throws.
17851792
if (auto *SI = dyn_cast<StoreInst>(User)) {
1786-
if (SI->getDest() == MUI &&
1787-
(isSelfInitUse(User) || isSuperInitUse(User))) {
1788-
continue;
1793+
if (SI->getDest() == MUI) {
1794+
SILValue src = SI->getSrc();
1795+
1796+
// Look through conversions.
1797+
while (auto *conversion = dyn_cast<ConversionInst>(src)) {
1798+
src = conversion->getConverted();
1799+
}
1800+
1801+
if (auto *li = dyn_cast<LoadInst>(src)) {
1802+
if (li->getOperand() == MUI) {
1803+
continue;
1804+
}
1805+
}
17891806
}
17901807
}
17911808

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,7 @@ void LifetimeChecker::doIt() {
785785
break;
786786
case DIUseKind::LoadForTypeOfSelf:
787787
handleLoadForTypeOfSelfUse(Use);
788+
break;
788789
}
789790
}
790791

0 commit comments

Comments
 (0)