Skip to content

Commit 0e985be

Browse files
committed
[silgen] Create a function level scope when open coding implicit value initializers.
The Original Bug ---------------- In ffbfcfa, we fixed a bug around implicit value initializers but did not cherry-pick it to 5.3. While investigating a bug that turned out to be that same bug (no worries!), I noticed that there is additional code that is "unsafely" correct in this area and that while ffbfcfa is correct in the small, we can expand on the fix to prevent future bugs. The Larger Bug -------------- Here we are still open coding using ManagedValue/Cleanup APIs /without/ a top level function scope. The code is only correct since we never emit unconditional cleanups and always manually forward conditional cleanups. If we did not do either of these things, we would have another instance of this bug, namely a cleanup that is never actually emitted. So the code on master today is correct, albeit unsafe, and we already have coverage for this (namely the test case from ffbfcfa). That being said, in general when working with ManagedValue APIs (especially in utility functions) we assume that we have a scope already created for us by our caller. So by fixing this issue we are standardizing to safer SILGen invariants. Building on ffbfcfa ---------------------------------------------------- This commit builds on the shoulders of ffbfcfa by adding the function level scope mentioned in the previous section so that we are now "safely" correct. While looking at this I also realized that just using a normal scope when open coding here may be a bit bugprone for open coding situations like this since: 1. If one just creates a scope in open coding situations, the scope will fire at end of the c++ function /after/ one has probably emitted a return. 2. Once one has emitted the return, the insertion point will no longer be set implying =><=. To avoid this, I created a move only composition type on top of Scope called AssertingManualScope. This type just asserts in its destructor if the scope it contains has not been popped yet. While, one can pop it by ones self, I added an overload of createReturnInst on SILGenBuilder that also takes an AssertingManualScope and pops it at the appropriate time. So now when performing simple open coding tasks, we have the ability to in code tie together the function level scope to the actual creation of return inst, simulating the hand-off of lifetimes/resources from caller/callee that often happens in the epilog of functions. <rdar://problem/63189210>
1 parent 21f5032 commit 0e985be

File tree

4 files changed

+59
-4
lines changed

4 files changed

+59
-4
lines changed

lib/SILGen/SILGenBuilder.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,13 @@ ReturnInst *SILGenBuilder::createReturn(SILLocation loc,
774774
return createReturn(loc, returnValue.forward(SGF));
775775
}
776776

777+
ReturnInst *
778+
SILGenBuilder::createReturn(SILLocation loc, SILValue returnValue,
779+
AssertingManualScope &&functionLevelScope) {
780+
std::move(functionLevelScope).pop();
781+
return createReturn(loc, returnValue);
782+
}
783+
777784
ManagedValue SILGenBuilder::createTuple(SILLocation loc, SILType type,
778785
ArrayRef<ManagedValue> elements) {
779786
// Handle the empty tuple case.

lib/SILGen/SILGenBuilder.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ namespace Lowering {
3434

3535
class SILGenFunction;
3636
class SGFContext;
37+
class AssertingManualScope;
3738

3839
/// A subclass of SILBuilder that wraps APIs to vend ManagedValues.
3940
/// APIs only vend ManagedValues.
@@ -362,6 +363,9 @@ class SILGenBuilder : public SILBuilder {
362363
using SILBuilder::createReturn;
363364
ReturnInst *createReturn(SILLocation Loc, ManagedValue ReturnValue);
364365

366+
ReturnInst *createReturn(SILLocation Loc, SILValue ReturnValue,
367+
AssertingManualScope &&functionLevelScope);
368+
365369
using SILBuilder::emitDestructureValueOperation;
366370
/// Perform either a tuple or struct destructure and then pass its components
367371
/// as managed value one by one with an index to the closure.

lib/SILGen/SILGenConstructor.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,10 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF,
152152
ConstructorDecl *ctor) {
153153
RegularLocation Loc(ctor);
154154
Loc.markAutoGenerated();
155+
156+
AssertingManualScope functionLevelScope(SGF.Cleanups,
157+
CleanupLocation::get(Loc));
158+
155159
// FIXME: Handle 'self' along with the other arguments.
156160
auto *paramList = ctor->getParameters();
157161
auto *selfDecl = ctor->getImplicitSelfDecl();
@@ -238,8 +242,9 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF,
238242
SGF.emitExprInto(field->getParentInitializer(), init.get());
239243
}
240244
}
245+
241246
SGF.B.createReturn(ImplicitReturnLocation::getImplicitReturnLoc(Loc),
242-
SGF.emitEmptyTuple(Loc));
247+
SGF.emitEmptyTuple(Loc), std::move(functionLevelScope));
243248
return;
244249
}
245250

@@ -287,7 +292,7 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF,
287292

288293
SILValue selfValue = SGF.B.createStruct(Loc, selfTy, eltValues);
289294
SGF.B.createReturn(ImplicitReturnLocation::getImplicitReturnLoc(Loc),
290-
selfValue);
295+
selfValue, std::move(functionLevelScope));
291296
return;
292297
}
293298

lib/SILGen/Scope.h

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,23 @@ class LLVM_LIBRARY_VISIBILITY Scope {
4747
Scope(const Scope &other) = delete;
4848
Scope &operator=(const Scope &other) = delete;
4949

50-
Scope(Scope &&other) = delete;
51-
Scope &operator=(Scope &&other) = delete; // implementable if needed
50+
Scope(Scope &&other)
51+
: cleanups(other.cleanups), depth(other.depth),
52+
savedInnermostScope(other.savedInnermostScope), loc(other.loc) {
53+
// Invalidate other.
54+
other.depth = CleanupsDepth::invalid();
55+
}
56+
57+
Scope &operator=(Scope &&other) {
58+
depth = other.depth;
59+
savedInnermostScope = other.savedInnermostScope;
60+
loc = other.loc;
61+
62+
// Invalidate other.
63+
other.depth = CleanupsDepth::invalid();
64+
65+
return *this;
66+
}
5267

5368
explicit Scope(SILGenFunction &SGF, SILLocation loc)
5469
: Scope(SGF.Cleanups, CleanupLocation::get(loc)) {}
@@ -83,6 +98,30 @@ class LLVM_LIBRARY_VISIBILITY Scope {
8398
void popImpl();
8499
};
85100

101+
/// A scope that must be manually popped by the using code. If not
102+
/// popped, the destructor asserts.
103+
class LLVM_LIBRARY_VISIBILITY AssertingManualScope {
104+
Scope scope;
105+
106+
public:
107+
explicit AssertingManualScope(CleanupManager &cleanups, CleanupLocation loc)
108+
: scope(cleanups, loc) {}
109+
110+
AssertingManualScope(AssertingManualScope &&other)
111+
: scope(std::move(other.scope)) {}
112+
113+
AssertingManualScope &operator=(AssertingManualScope &&other) {
114+
scope = std::move(other.scope);
115+
return *this;
116+
}
117+
118+
~AssertingManualScope() {
119+
assert(!scope.isValid() && "Unpopped manual scope?!");
120+
}
121+
122+
void pop() && { scope.pop(); }
123+
};
124+
86125
/// A FullExpr is a RAII object recording that a full-expression has
87126
/// been entered. A full-expression is essentially a very small scope
88127
/// for the temporaries in an expression, with the added complexity

0 commit comments

Comments
 (0)