From 0e985be3f4814c6a6d1e4f7cb3e5447ee03a01db Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Fri, 29 May 2020 13:28:24 -0700 Subject: [PATCH] [silgen] Create a function level scope when open coding implicit value initializers. The Original Bug ---------------- In ffbfcfa131465daca2018fbdd34015899a9c0d00, 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 ffbfcfa131465daca2018fbdd34015899a9c0d00 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 ffbfcfa131465daca2018fbdd34015899a9c0d00). 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 ffbfcfa131465daca2018fbdd34015899a9c0d00 ---------------------------------------------------- This commit builds on the shoulders of ffbfcfa131465daca2018fbdd34015899a9c0d00 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. --- lib/SILGen/SILGenBuilder.cpp | 7 ++++++ lib/SILGen/SILGenBuilder.h | 4 +++ lib/SILGen/SILGenConstructor.cpp | 9 +++++-- lib/SILGen/Scope.h | 43 ++++++++++++++++++++++++++++++-- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/lib/SILGen/SILGenBuilder.cpp b/lib/SILGen/SILGenBuilder.cpp index 21fcd0dc9cc9f..bf0e64a90cc79 100644 --- a/lib/SILGen/SILGenBuilder.cpp +++ b/lib/SILGen/SILGenBuilder.cpp @@ -774,6 +774,13 @@ ReturnInst *SILGenBuilder::createReturn(SILLocation loc, return createReturn(loc, returnValue.forward(SGF)); } +ReturnInst * +SILGenBuilder::createReturn(SILLocation loc, SILValue returnValue, + AssertingManualScope &&functionLevelScope) { + std::move(functionLevelScope).pop(); + return createReturn(loc, returnValue); +} + ManagedValue SILGenBuilder::createTuple(SILLocation loc, SILType type, ArrayRef elements) { // Handle the empty tuple case. diff --git a/lib/SILGen/SILGenBuilder.h b/lib/SILGen/SILGenBuilder.h index cce1589276537..b5b6c3567d213 100644 --- a/lib/SILGen/SILGenBuilder.h +++ b/lib/SILGen/SILGenBuilder.h @@ -34,6 +34,7 @@ namespace Lowering { class SILGenFunction; class SGFContext; +class AssertingManualScope; /// A subclass of SILBuilder that wraps APIs to vend ManagedValues. /// APIs only vend ManagedValues. @@ -362,6 +363,9 @@ class SILGenBuilder : public SILBuilder { using SILBuilder::createReturn; ReturnInst *createReturn(SILLocation Loc, ManagedValue ReturnValue); + ReturnInst *createReturn(SILLocation Loc, SILValue ReturnValue, + AssertingManualScope &&functionLevelScope); + using SILBuilder::emitDestructureValueOperation; /// Perform either a tuple or struct destructure and then pass its components /// as managed value one by one with an index to the closure. diff --git a/lib/SILGen/SILGenConstructor.cpp b/lib/SILGen/SILGenConstructor.cpp index 6e1a94e9a2eff..83ad4870154aa 100644 --- a/lib/SILGen/SILGenConstructor.cpp +++ b/lib/SILGen/SILGenConstructor.cpp @@ -152,6 +152,10 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, ConstructorDecl *ctor) { RegularLocation Loc(ctor); Loc.markAutoGenerated(); + + AssertingManualScope functionLevelScope(SGF.Cleanups, + CleanupLocation::get(Loc)); + // FIXME: Handle 'self' along with the other arguments. auto *paramList = ctor->getParameters(); auto *selfDecl = ctor->getImplicitSelfDecl(); @@ -238,8 +242,9 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, SGF.emitExprInto(field->getParentInitializer(), init.get()); } } + SGF.B.createReturn(ImplicitReturnLocation::getImplicitReturnLoc(Loc), - SGF.emitEmptyTuple(Loc)); + SGF.emitEmptyTuple(Loc), std::move(functionLevelScope)); return; } @@ -287,7 +292,7 @@ static void emitImplicitValueConstructor(SILGenFunction &SGF, SILValue selfValue = SGF.B.createStruct(Loc, selfTy, eltValues); SGF.B.createReturn(ImplicitReturnLocation::getImplicitReturnLoc(Loc), - selfValue); + selfValue, std::move(functionLevelScope)); return; } diff --git a/lib/SILGen/Scope.h b/lib/SILGen/Scope.h index 28f880cbe4aef..4eef14b3b9a41 100644 --- a/lib/SILGen/Scope.h +++ b/lib/SILGen/Scope.h @@ -47,8 +47,23 @@ class LLVM_LIBRARY_VISIBILITY Scope { Scope(const Scope &other) = delete; Scope &operator=(const Scope &other) = delete; - Scope(Scope &&other) = delete; - Scope &operator=(Scope &&other) = delete; // implementable if needed + Scope(Scope &&other) + : cleanups(other.cleanups), depth(other.depth), + savedInnermostScope(other.savedInnermostScope), loc(other.loc) { + // Invalidate other. + other.depth = CleanupsDepth::invalid(); + } + + Scope &operator=(Scope &&other) { + depth = other.depth; + savedInnermostScope = other.savedInnermostScope; + loc = other.loc; + + // Invalidate other. + other.depth = CleanupsDepth::invalid(); + + return *this; + } explicit Scope(SILGenFunction &SGF, SILLocation loc) : Scope(SGF.Cleanups, CleanupLocation::get(loc)) {} @@ -83,6 +98,30 @@ class LLVM_LIBRARY_VISIBILITY Scope { void popImpl(); }; +/// A scope that must be manually popped by the using code. If not +/// popped, the destructor asserts. +class LLVM_LIBRARY_VISIBILITY AssertingManualScope { + Scope scope; + +public: + explicit AssertingManualScope(CleanupManager &cleanups, CleanupLocation loc) + : scope(cleanups, loc) {} + + AssertingManualScope(AssertingManualScope &&other) + : scope(std::move(other.scope)) {} + + AssertingManualScope &operator=(AssertingManualScope &&other) { + scope = std::move(other.scope); + return *this; + } + + ~AssertingManualScope() { + assert(!scope.isValid() && "Unpopped manual scope?!"); + } + + void pop() && { scope.pop(); } +}; + /// A FullExpr is a RAII object recording that a full-expression has /// been entered. A full-expression is essentially a very small scope /// for the temporaries in an expression, with the added complexity