From 9f9269e810bfe9aea0a57b09250be215808fc1a2 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Fri, 19 Feb 2010 00:42:33 +0000 Subject: [PATCH] Change InitListExpr to allocate the array for holding references to initializer expressions in an array allocated using ASTContext. This plugs a memory leak when ASTContext uses a BumpPtrAllocator to allocate memory for AST nodes. In my mind this isn't an ideal solution; it would be nice to have a general "vector"-like class that allocates memory using ASTContext, but whose guts could be separated from the methods of InitListExpr itself. I haven't gone and taken this approach yet because it isn't clear yet if we'll eventually want an alternate solution for recylcing memory using by InitListExprs as we are constructing the ASTs. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@96642 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 35 ++++++++------ lib/AST/Expr.cpp | 85 +++++++++++++++++++++++----------- lib/Frontend/PCHReaderStmt.cpp | 4 +- lib/Frontend/RewriteObjC.cpp | 16 ++++--- lib/Sema/SemaExpr.cpp | 10 ++-- lib/Sema/SemaInit.cpp | 15 +++--- 6 files changed, 105 insertions(+), 60 deletions(-) diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index 23076b93e1..ecb821eb67 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -2419,8 +2419,15 @@ public: /// return NULL, indicating that the current initializer list also /// serves as its syntactic form. class InitListExpr : public Expr { - // FIXME: Eliminate this vector in favor of ASTContext allocation - std::vector InitExprs; + /// The initializers. + // FIXME: Instead of directly maintaining the initializer array, + // we may wish to use a specialized vector class that uses ASTContext + // to allocate memory. This will keep the implementation simpler, and + // we can possibly use ASTContext to recylce memory even when using + // a BumpPtrAllocator. + Stmt **InitExprs; + unsigned NumInits, Capacity; + SourceLocation LBraceLoc, RBraceLoc; /// Contains the initializer list that describes the syntactic form @@ -2436,13 +2443,15 @@ class InitListExpr : public Expr { bool HadArrayRangeDesignator; public: - InitListExpr(SourceLocation lbraceloc, Expr **initexprs, unsigned numinits, - SourceLocation rbraceloc); + InitListExpr(ASTContext &C, SourceLocation lbraceloc, Expr **initexprs, + unsigned numinits, SourceLocation rbraceloc); + + virtual void DoDestroy(ASTContext &C); /// \brief Build an empty initializer list. explicit InitListExpr(EmptyShell Empty) : Expr(InitListExprClass, Empty) { } - unsigned getNumInits() const { return InitExprs.size(); } + unsigned getNumInits() const { return NumInits; } const Expr* getInit(unsigned Init) const { assert(Init < getNumInits() && "Initializer access out of range!"); @@ -2460,7 +2469,7 @@ public: } /// \brief Reserve space for some number of initializers. - void reserveInits(unsigned NumInits); + void reserveInits(ASTContext &Context, unsigned NumInits); /// @brief Specify the number of initializers /// @@ -2477,7 +2486,7 @@ public: /// When @p Init is out of range for this initializer list, the /// initializer list will be extended with NULL expressions to /// accomodate the new entry. - Expr *updateInit(unsigned Init, Expr *expr); + Expr *updateInit(ASTContext &Context, unsigned Init, Expr *expr); /// \brief If this initializes a union, specifies which field in the /// union to initialize. @@ -2523,13 +2532,13 @@ public: virtual child_iterator child_begin(); virtual child_iterator child_end(); - typedef std::vector::iterator iterator; - typedef std::vector::reverse_iterator reverse_iterator; + typedef Stmt** iterator; + typedef std::reverse_iterator reverse_iterator; - iterator begin() { return InitExprs.begin(); } - iterator end() { return InitExprs.end(); } - reverse_iterator rbegin() { return InitExprs.rbegin(); } - reverse_iterator rend() { return InitExprs.rend(); } + iterator begin() { return InitExprs ? InitExprs : 0; } + iterator end() { return InitExprs ? InitExprs + NumInits : 0; } + reverse_iterator rbegin() { return reverse_iterator(end()); } + reverse_iterator rend() { return reverse_iterator(begin()); } }; /// @brief Represents a C99 designated initializer expression. diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index fac65064c0..58776f47d9 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -713,40 +713,75 @@ OverloadedOperatorKind BinaryOperator::getOverloadedOperator(Opcode Opc) { return OverOps[Opc]; } -InitListExpr::InitListExpr(SourceLocation lbraceloc, +InitListExpr::InitListExpr(ASTContext &C, SourceLocation lbraceloc, Expr **initExprs, unsigned numInits, SourceLocation rbraceloc) : Expr(InitListExprClass, QualType(), false, false), + InitExprs(0), NumInits(numInits), Capacity(numInits), LBraceLoc(lbraceloc), RBraceLoc(rbraceloc), SyntacticForm(0), - UnionFieldInit(0), HadArrayRangeDesignator(false) -{ - for (unsigned I = 0; I != numInits; ++I) { - if (initExprs[I]->isTypeDependent()) + UnionFieldInit(0), HadArrayRangeDesignator(false) +{ + if (NumInits == 0) + return; + + InitExprs = new (C) Stmt*[Capacity]; + + for (unsigned I = 0; I != NumInits; ++I) { + Expr *Ex = initExprs[I]; + if (Ex->isTypeDependent()) TypeDependent = true; - if (initExprs[I]->isValueDependent()) + if (Ex->isValueDependent()) ValueDependent = true; + InitExprs[I] = Ex; } - - InitExprs.insert(InitExprs.end(), initExprs, initExprs+numInits); } -void InitListExpr::reserveInits(unsigned NumInits) { - if (NumInits > InitExprs.size()) - InitExprs.reserve(NumInits); +void InitListExpr::DoDestroy(ASTContext &C) { + DestroyChildren(C); + if (InitExprs) + C.Deallocate(InitExprs); + this->~InitListExpr(); + C.Deallocate((void*) this); } -void InitListExpr::resizeInits(ASTContext &Context, unsigned NumInits) { - for (unsigned Idx = NumInits, LastIdx = InitExprs.size(); - Idx < LastIdx; ++Idx) - InitExprs[Idx]->Destroy(Context); - InitExprs.resize(NumInits, 0); +void InitListExpr::reserveInits(ASTContext &C, unsigned newCapacity) { + if (newCapacity > Capacity) { + if (!Capacity) + Capacity = newCapacity; + else if ((Capacity *= 2) < newCapacity) + Capacity = newCapacity; + + Stmt **newInits = new (C) Stmt*[Capacity]; + if (InitExprs) { + memcpy(newInits, InitExprs, NumInits * sizeof(*InitExprs)); + C.Deallocate(InitExprs); + } + InitExprs = newInits; + } } -Expr *InitListExpr::updateInit(unsigned Init, Expr *expr) { - if (Init >= InitExprs.size()) { - InitExprs.insert(InitExprs.end(), Init - InitExprs.size() + 1, 0); - InitExprs.back() = expr; - return 0; +void InitListExpr::resizeInits(ASTContext &C, unsigned N) { + // If the new number of expressions is less than the old one, destroy + // the expressions that are beyond the new size. + for (unsigned i = N, LastIdx = NumInits; i < LastIdx; ++i) + InitExprs[i]->Destroy(C); + + // If we are expanding the number of expressions, reserve space. + reserveInits(C, N); + + // If we are expanding the number of expressions, zero out beyond our + // current capacity. + for (unsigned i = NumInits; i < N; ++i) + InitExprs[i] = 0; + + NumInits = N; +} + +Expr *InitListExpr::updateInit(ASTContext &C, unsigned Init, Expr *expr) { + if (Init >= NumInits) { + // Resize the number of initializers. This will adjust the amount + // of memory allocated as well as zero-pad the initializers. + resizeInits(C, Init+1); } Expr *Result = cast_or_null(InitExprs[Init]); @@ -2586,12 +2621,8 @@ Stmt::child_iterator VAArgExpr::child_begin() { return &Val; } Stmt::child_iterator VAArgExpr::child_end() { return &Val+1; } // InitListExpr -Stmt::child_iterator InitListExpr::child_begin() { - return InitExprs.size() ? &InitExprs[0] : 0; -} -Stmt::child_iterator InitListExpr::child_end() { - return InitExprs.size() ? &InitExprs[0] + InitExprs.size() : 0; -} +Stmt::child_iterator InitListExpr::child_begin() { return begin(); } +Stmt::child_iterator InitListExpr::child_end() { return end(); } // DesignatedInitExpr Stmt::child_iterator DesignatedInitExpr::child_begin() { diff --git a/lib/Frontend/PCHReaderStmt.cpp b/lib/Frontend/PCHReaderStmt.cpp index d123694d69..c69a5dc9dc 100644 --- a/lib/Frontend/PCHReaderStmt.cpp +++ b/lib/Frontend/PCHReaderStmt.cpp @@ -554,9 +554,9 @@ unsigned PCHStmtReader::VisitExtVectorElementExpr(ExtVectorElementExpr *E) { unsigned PCHStmtReader::VisitInitListExpr(InitListExpr *E) { VisitExpr(E); unsigned NumInits = Record[Idx++]; - E->reserveInits(NumInits); + E->reserveInits(*Reader.getContext(), NumInits); for (unsigned I = 0; I != NumInits; ++I) - E->updateInit(I, + E->updateInit(*Reader.getContext(), I, cast(StmtStack[StmtStack.size() - NumInits - 1 + I])); E->setSyntacticForm(cast_or_null(StmtStack.back())); E->setLBraceLoc(SourceLocation::getFromRawEncoding(Record[Idx++])); diff --git a/lib/Frontend/RewriteObjC.cpp b/lib/Frontend/RewriteObjC.cpp index fff8b54ba6..40d444d94e 100644 --- a/lib/Frontend/RewriteObjC.cpp +++ b/lib/Frontend/RewriteObjC.cpp @@ -2613,9 +2613,11 @@ Stmt *RewriteObjC::SynthMessageExpr(ObjCMessageExpr *Exp) { CastExpr::CK_Unknown, SuperRep); } else { // (struct objc_super) { } - InitListExpr *ILE = new (Context) InitListExpr(SourceLocation(), - &InitExprs[0], InitExprs.size(), - SourceLocation()); + InitListExpr *ILE = new (Context) InitListExpr(*Context, + SourceLocation(), + &InitExprs[0], + InitExprs.size(), + SourceLocation()); TypeSourceInfo *superTInfo = Context->getTrivialTypeSourceInfo(superType); SuperRep = new (Context) CompoundLiteralExpr(SourceLocation(), superTInfo, @@ -2698,9 +2700,11 @@ Stmt *RewriteObjC::SynthMessageExpr(ObjCMessageExpr *Exp) { CastExpr::CK_Unknown, SuperRep); } else { // (struct objc_super) { } - InitListExpr *ILE = new (Context) InitListExpr(SourceLocation(), - &InitExprs[0], InitExprs.size(), - SourceLocation()); + InitListExpr *ILE = new (Context) InitListExpr(*Context, + SourceLocation(), + &InitExprs[0], + InitExprs.size(), + SourceLocation()); TypeSourceInfo *superTInfo = Context->getTrivialTypeSourceInfo(superType); SuperRep = new (Context) CompoundLiteralExpr(SourceLocation(), superTInfo, diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index e950be0485..0d54a16412 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -3763,8 +3763,8 @@ Sema::ActOnInitList(SourceLocation LBraceLoc, MultiExprArg initlist, // Semantic analysis for initializers is done by ActOnDeclarator() and // CheckInitializer() - it requires knowledge of the object being intialized. - InitListExpr *E = new (Context) InitListExpr(LBraceLoc, InitList, NumInit, - RBraceLoc); + InitListExpr *E = new (Context) InitListExpr(Context, LBraceLoc, InitList, + NumInit, RBraceLoc); E->setType(Context.VoidTy); // FIXME: just a place holder for now. return Owned(E); } @@ -4038,7 +4038,8 @@ Sema::ActOnCastOfParenListExpr(Scope *S, SourceLocation LParenLoc, // FIXME: This means that pretty-printing the final AST will produce curly // braces instead of the original commas. Op.release(); - InitListExpr *E = new (Context) InitListExpr(LParenLoc, &initExprs[0], + InitListExpr *E = new (Context) InitListExpr(Context, LParenLoc, + &initExprs[0], initExprs.size(), RParenLoc); E->setType(Ty); return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, Owned(E)); @@ -4752,8 +4753,7 @@ static void ConstructTransparentUnion(ASTContext &C, Expr *&E, QualType UnionType, FieldDecl *Field) { // Build an initializer list that designates the appropriate member // of the transparent union. - InitListExpr *Initializer = new (C) InitListExpr(SourceLocation(), - &E, 1, + InitListExpr *Initializer = new (C) InitListExpr(C, SourceLocation(), &E, 1, SourceLocation()); Initializer->setType(UnionType); Initializer->setInitializedFieldInUnion(Field); diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp index 7b4a41777b..1c4e2c837a 100644 --- a/lib/Sema/SemaInit.cpp +++ b/lib/Sema/SemaInit.cpp @@ -280,7 +280,7 @@ void InitListChecker::FillInValueInitForField(unsigned Init, FieldDecl *Field, // extend the initializer list to include the constructor // call and make a note that we'll need to take another pass // through the initializer list. - ILE->updateInit(Init, MemberInit.takeAs()); + ILE->updateInit(SemaRef.Context, Init, MemberInit.takeAs()); RequiresSecondPass = true; } } else if (InitListExpr *InnerILE @@ -390,7 +390,7 @@ InitListChecker::FillInValueInitializations(const InitializedEntity &Entity, // extend the initializer list to include the constructor // call and make a note that we'll need to take another pass // through the initializer list. - ILE->updateInit(Init, ElementInit.takeAs()); + ILE->updateInit(SemaRef.Context, Init, ElementInit.takeAs()); RequiresSecondPass = true; } } else if (InitListExpr *InnerILE @@ -1669,8 +1669,8 @@ InitListChecker::getStructuredSubobjectInit(InitListExpr *IList, unsigned Index, } InitListExpr *Result - = new (SemaRef.Context) InitListExpr(InitRange.getBegin(), 0, 0, - InitRange.getEnd()); + = new (SemaRef.Context) InitListExpr(SemaRef.Context, InitRange.getBegin(), + 0, 0, InitRange.getEnd()); Result->setType(CurrentObjectType.getNonReferenceType()); @@ -1707,12 +1707,12 @@ InitListChecker::getStructuredSubobjectInit(InitListExpr *IList, unsigned Index, if (NumElements < NumInits) NumElements = IList->getNumInits(); - Result->reserveInits(NumElements); + Result->reserveInits(SemaRef.Context, NumElements); // Link this new initializer list into the structured initializer // lists. if (StructuredList) - StructuredList->updateInit(StructuredIndex, Result); + StructuredList->updateInit(SemaRef.Context, StructuredIndex, Result); else { Result->setSyntacticForm(IList); SyntacticToSemantic[IList] = Result; @@ -1730,7 +1730,8 @@ void InitListChecker::UpdateStructuredListElement(InitListExpr *StructuredList, if (!StructuredList) return; - if (Expr *PrevInit = StructuredList->updateInit(StructuredIndex, expr)) { + if (Expr *PrevInit = StructuredList->updateInit(SemaRef.Context, + StructuredIndex, expr)) { // This initializer overwrites a previous initializer. Warn. SemaRef.Diag(expr->getSourceRange().getBegin(), diag::warn_initializer_overrides) -- 2.40.0