From: Ted Kremenek Date: Thu, 11 Feb 2010 02:19:13 +0000 (+0000) Subject: Clean up ownership of 'AttributeList' objects in Parser. Apparently X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1e37765c9257ef1d051f54a674eaa964bdba9693;p=clang Clean up ownership of 'AttributeList' objects in Parser. Apparently we would just leak them all over the place, with no clear ownership of these objects at all. AttributeList objects would get leaked on both error and non-error paths. Note: I introduced the usage of llvm::OwningPtr to manage these objects, which is particularly useful for methods with multiple return sites. In at least one method I used them even when they weren't strictly necessary because it clarified the ownership semantics and made the code easier to read. Should the excessive 'take()' and 'reset()' calls become a performance issue we can always re-evaluate. Note+1: I believe I have not introduced any double-frees, but it would be nice for someone to review this. This fixes . git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@95847 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp index 7856e62f04..01156a1e96 100644 --- a/lib/Parse/ParseDecl.cpp +++ b/lib/Parse/ParseDecl.cpp @@ -1807,15 +1807,15 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc, SourceLocation RBraceLoc = MatchRHSPunctuation(tok::r_brace, LBraceLoc); - AttributeList *AttrList = 0; + llvm::OwningPtr AttrList; // If attributes exist after struct contents, parse them. if (Tok.is(tok::kw___attribute)) - AttrList = ParseGNUAttributes(); + AttrList.reset(ParseGNUAttributes()); Actions.ActOnFields(CurScope, RecordLoc, TagDecl, FieldDecls.data(), FieldDecls.size(), LBraceLoc, RBraceLoc, - AttrList); + AttrList.get()); StructScope.Exit(); Actions.ActOnTagFinishDefinition(CurScope, TagDecl, RBraceLoc); } @@ -1842,10 +1842,10 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS, ConsumeToken(); } - AttributeList *Attr = 0; + llvm::OwningPtr Attr; // If attributes exist after tag, parse them. if (Tok.is(tok::kw___attribute)) - Attr = ParseGNUAttributes(); + Attr.reset(ParseGNUAttributes()); CXXScopeSpec SS; if (getLang().CPlusPlus && ParseOptionalCXXScopeSpecifier(SS, 0, false)) { @@ -1895,7 +1895,8 @@ void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS, bool Owned = false; bool IsDependent = false; DeclPtrTy TagDecl = Actions.ActOnTag(CurScope, DeclSpec::TST_enum, TUK, - StartLoc, SS, Name, NameLoc, Attr, AS, + StartLoc, SS, Name, NameLoc, Attr.get(), + AS, Action::MultiTemplateParamsArg(Actions), Owned, IsDependent); assert(!IsDependent && "didn't expect dependent enum"); @@ -1975,14 +1976,14 @@ void Parser::ParseEnumBody(SourceLocation StartLoc, DeclPtrTy EnumDecl) { // Eat the }. SourceLocation RBraceLoc = MatchRHSPunctuation(tok::r_brace, LBraceLoc); - AttributeList *Attr = 0; + llvm::OwningPtr Attr; // If attributes exist after the identifier list, parse them. if (Tok.is(tok::kw___attribute)) - Attr = ParseGNUAttributes(); // FIXME: where do they do? + Attr.reset(ParseGNUAttributes()); // FIXME: where do they do? Actions.ActOnEnumBody(StartLoc, LBraceLoc, RBraceLoc, EnumDecl, EnumConstantDecls.data(), EnumConstantDecls.size(), - CurScope, Attr); + CurScope, Attr.get()); EnumScope.Exit(); Actions.ActOnTagFinishDefinition(CurScope, EnumDecl, RBraceLoc); @@ -2642,10 +2643,10 @@ void Parser::ParseParenDeclarator(Declarator &D) { // In either case, we need to eat any attributes to be able to determine what // sort of paren this is. // - AttributeList *AttrList = 0; + llvm::OwningPtr AttrList; bool RequiresArg = false; if (Tok.is(tok::kw___attribute)) { - AttrList = ParseGNUAttributes(); + AttrList.reset(ParseGNUAttributes()); // We require that the argument list (if this is a non-grouping paren) be // present even if the attribute list was empty. @@ -2655,7 +2656,7 @@ void Parser::ParseParenDeclarator(Declarator &D) { if (Tok.is(tok::kw___cdecl) || Tok.is(tok::kw___stdcall) || Tok.is(tok::kw___fastcall) || Tok.is(tok::kw___w64) || Tok.is(tok::kw___ptr64)) { - AttrList = ParseMicrosoftTypeAttributes(AttrList); + AttrList.reset(ParseMicrosoftTypeAttributes(AttrList.take())); } // If we haven't past the identifier yet (or where the identifier would be @@ -2686,7 +2687,7 @@ void Parser::ParseParenDeclarator(Declarator &D) { bool hadGroupingParens = D.hasGroupingParens(); D.setGroupingParens(true); if (AttrList) - D.AddAttributes(AttrList, SourceLocation()); + D.AddAttributes(AttrList.take(), SourceLocation()); ParseDeclaratorInternal(D, &Parser::ParseDirectDeclarator); // Match the ')'. @@ -2703,7 +2704,7 @@ void Parser::ParseParenDeclarator(Declarator &D) { // ParseFunctionDeclarator to handle of argument list. D.SetIdentifier(0, Tok.getLocation()); - ParseFunctionDeclarator(StartLoc, D, AttrList, RequiresArg); + ParseFunctionDeclarator(StartLoc, D, AttrList.take(), RequiresArg); } /// ParseFunctionDeclarator - We are after the identifier and have parsed the diff --git a/lib/Parse/ParseDeclCXX.cpp b/lib/Parse/ParseDeclCXX.cpp index e1c66e236d..51ee6a4434 100644 --- a/lib/Parse/ParseDeclCXX.cpp +++ b/lib/Parse/ParseDeclCXX.cpp @@ -64,12 +64,12 @@ Parser::DeclPtrTy Parser::ParseNamespace(unsigned Context, } // Read label attributes, if present. - AttributeList *AttrList = 0; + llvm::OwningPtr AttrList; if (Tok.is(tok::kw___attribute)) { attrTok = Tok; // FIXME: save these somewhere. - AttrList = ParseGNUAttributes(); + AttrList.reset(ParseGNUAttributes()); } if (Tok.is(tok::equal)) { @@ -91,7 +91,8 @@ Parser::DeclPtrTy Parser::ParseNamespace(unsigned Context, ParseScope NamespaceScope(this, Scope::DeclScope); DeclPtrTy NamespcDecl = - Actions.ActOnStartNamespaceDef(CurScope, IdentLoc, Ident, LBrace, AttrList); + Actions.ActOnStartNamespaceDef(CurScope, IdentLoc, Ident, LBrace, + AttrList.get()); PrettyStackTraceActionsDecl CrashInfo(NamespcDecl, NamespaceLoc, Actions, PP.getSourceManager(), @@ -327,8 +328,6 @@ Parser::DeclPtrTy Parser::ParseUsingDeclaration(unsigned Context, // Parse nested-name-specifier. ParseOptionalCXXScopeSpecifier(SS, /*ObjectType=*/0, false); - AttributeList *AttrList = 0; - // Check nested-name specifier. if (SS.isInvalid()) { SkipUntil(tok::semi); @@ -350,8 +349,9 @@ Parser::DeclPtrTy Parser::ParseUsingDeclaration(unsigned Context, } // Parse (optional) attributes (most likely GNU strong-using extension). + llvm::OwningPtr AttrList; if (Tok.is(tok::kw___attribute)) - AttrList = ParseGNUAttributes(); + AttrList.reset(ParseGNUAttributes()); // Eat ';'. DeclEnd = Tok.getLocation(); @@ -360,7 +360,7 @@ Parser::DeclPtrTy Parser::ParseUsingDeclaration(unsigned Context, tok::semi); return Actions.ActOnUsingDeclaration(CurScope, AS, true, UsingLoc, SS, Name, - AttrList, IsTypeName, TypenameLoc); + AttrList.get(), IsTypeName, TypenameLoc); } /// ParseStaticAssertDeclaration - Parse C++0x static_assert-declaratoion. @@ -1549,10 +1549,10 @@ void Parser::ParseCXXMemberSpecification(SourceLocation RecordLoc, SourceLocation RBraceLoc = MatchRHSPunctuation(tok::r_brace, LBraceLoc); - AttributeList *AttrList = 0; // If attributes exist after class contents, parse them. + llvm::OwningPtr AttrList; if (Tok.is(tok::kw___attribute)) - AttrList = ParseGNUAttributes(); // FIXME: where should I put them? + AttrList.reset(ParseGNUAttributes()); // FIXME: where should I put them? Actions.ActOnFinishCXXMemberSpecification(CurScope, RecordLoc, TagDecl, LBraceLoc, RBraceLoc); diff --git a/lib/Parse/ParseObjc.cpp b/lib/Parse/ParseObjc.cpp index ef65204e8c..3757ee2f6d 100644 --- a/lib/Parse/ParseObjc.cpp +++ b/lib/Parse/ParseObjc.cpp @@ -786,15 +786,15 @@ Parser::DeclPtrTy Parser::ParseObjCMethodDecl(SourceLocation mLoc, llvm::SmallVector CargNames; if (Tok.isNot(tok::colon)) { // If attributes exist after the method, parse them. - AttributeList *MethodAttrs = 0; + llvm::OwningPtr MethodAttrs; if (getLang().ObjC2 && Tok.is(tok::kw___attribute)) - MethodAttrs = ParseGNUAttributes(); + MethodAttrs.reset(ParseGNUAttributes()); Selector Sel = PP.getSelectorTable().getNullarySelector(SelIdent); DeclPtrTy Result = Actions.ActOnMethodDeclaration(mLoc, Tok.getLocation(), mType, IDecl, DSRet, ReturnType, Sel, - 0, CargNames, MethodAttrs, + 0, CargNames, MethodAttrs.get(), MethodImplKind); PD.complete(Result); return Result; @@ -862,9 +862,9 @@ Parser::DeclPtrTy Parser::ParseObjCMethodDecl(SourceLocation mLoc, // FIXME: Add support for optional parmameter list... // If attributes exist after the method, parse them. - AttributeList *MethodAttrs = 0; + llvm::OwningPtr MethodAttrs; if (getLang().ObjC2 && Tok.is(tok::kw___attribute)) - MethodAttrs = ParseGNUAttributes(); + MethodAttrs.reset(ParseGNUAttributes()); if (KeyIdents.size() == 0) return DeclPtrTy(); @@ -873,9 +873,16 @@ Parser::DeclPtrTy Parser::ParseObjCMethodDecl(SourceLocation mLoc, DeclPtrTy Result = Actions.ActOnMethodDeclaration(mLoc, Tok.getLocation(), mType, IDecl, DSRet, ReturnType, Sel, - &ArgInfos[0], CargNames, MethodAttrs, + &ArgInfos[0], CargNames, + MethodAttrs.get(), MethodImplKind, isVariadic); PD.complete(Result); + + // Delete referenced AttributeList objects. + for (llvm::SmallVectorImpl::iterator + I = ArgInfos.begin(), E = ArgInfos.end(); I != E; ++I) + delete I->ArgAttrs; + return Result; } diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp index 157e8b1a45..9fd145dc26 100644 --- a/lib/Parse/ParseStmt.cpp +++ b/lib/Parse/ParseStmt.cpp @@ -81,6 +81,7 @@ Parser::ParseStatementOrDeclaration(bool OnlyStatement) { CXX0XAttributeList Attr; if (getLang().CPlusPlus0x && isCXX0XAttributeSpecifier()) Attr = ParseCXX0XAttributes(); + llvm::OwningPtr AttrList(Attr.AttrList); // Cases in this switch statement should fall through if the parser expects // the token to end in a semicolon (in which case SemiError should be set), @@ -102,13 +103,14 @@ Parser::ParseStatementOrDeclaration(bool OnlyStatement) { case tok::identifier: if (NextToken().is(tok::colon)) { // C99 6.8.1: labeled-statement // identifier ':' statement - return ParseLabeledStatement(Attr.AttrList); + return ParseLabeledStatement(AttrList.take()); } // PASS THROUGH. default: { if ((getLang().CPlusPlus || !OnlyStatement) && isDeclarationStatement()) { SourceLocation DeclStart = Tok.getLocation(), DeclEnd; + AttrList.take(); //Passing 'Attr' to ParseDeclaration transfers ownership. DeclGroupPtrTy Decl = ParseDeclaration(Declarator::BlockContext, DeclEnd, Attr); return Actions.ActOnDeclStmt(Decl, DeclStart, DeclEnd); @@ -135,43 +137,43 @@ Parser::ParseStatementOrDeclaration(bool OnlyStatement) { } case tok::kw_case: // C99 6.8.1: labeled-statement - return ParseCaseStatement(Attr.AttrList); + return ParseCaseStatement(AttrList.take()); case tok::kw_default: // C99 6.8.1: labeled-statement - return ParseDefaultStatement(Attr.AttrList); + return ParseDefaultStatement(AttrList.take()); case tok::l_brace: // C99 6.8.2: compound-statement - return ParseCompoundStatement(Attr.AttrList); + return ParseCompoundStatement(AttrList.take()); case tok::semi: // C99 6.8.3p3: expression[opt] ';' return Actions.ActOnNullStmt(ConsumeToken()); case tok::kw_if: // C99 6.8.4.1: if-statement - return ParseIfStatement(Attr.AttrList); + return ParseIfStatement(AttrList.take()); case tok::kw_switch: // C99 6.8.4.2: switch-statement - return ParseSwitchStatement(Attr.AttrList); + return ParseSwitchStatement(AttrList.take()); case tok::kw_while: // C99 6.8.5.1: while-statement - return ParseWhileStatement(Attr.AttrList); + return ParseWhileStatement(AttrList.take()); case tok::kw_do: // C99 6.8.5.2: do-statement - Res = ParseDoStatement(Attr.AttrList); + Res = ParseDoStatement(AttrList.take()); SemiError = "do/while"; break; case tok::kw_for: // C99 6.8.5.3: for-statement - return ParseForStatement(Attr.AttrList); + return ParseForStatement(AttrList.take()); case tok::kw_goto: // C99 6.8.6.1: goto-statement - Res = ParseGotoStatement(Attr.AttrList); + Res = ParseGotoStatement(AttrList.take()); SemiError = "goto"; break; case tok::kw_continue: // C99 6.8.6.2: continue-statement - Res = ParseContinueStatement(Attr.AttrList); + Res = ParseContinueStatement(AttrList.take()); SemiError = "continue"; break; case tok::kw_break: // C99 6.8.6.3: break-statement - Res = ParseBreakStatement(Attr.AttrList); + Res = ParseBreakStatement(AttrList.take()); SemiError = "break"; break; case tok::kw_return: // C99 6.8.6.4: return-statement - Res = ParseReturnStatement(Attr.AttrList); + Res = ParseReturnStatement(AttrList.take()); SemiError = "return"; break; @@ -187,7 +189,7 @@ Parser::ParseStatementOrDeclaration(bool OnlyStatement) { } case tok::kw_try: // C++ 15: try-block - return ParseCXXTryBlock(Attr.AttrList); + return ParseCXXTryBlock(AttrList.take()); } // If we reached this code, the statement must end in a semicolon. @@ -215,6 +217,7 @@ Parser::OwningStmtResult Parser::ParseLabeledStatement(AttributeList *Attr) { assert(Tok.is(tok::identifier) && Tok.getIdentifierInfo() && "Not an identifier!"); + llvm::OwningPtr AttrList(Attr); Token IdentTok = Tok; // Save the whole token. ConsumeToken(); // eat the identifier. @@ -225,7 +228,7 @@ Parser::OwningStmtResult Parser::ParseLabeledStatement(AttributeList *Attr) { // Read label attributes, if present. if (Tok.is(tok::kw___attribute)) - Attr = addAttributeLists(Attr, ParseGNUAttributes()); + AttrList.reset(addAttributeLists(AttrList.take(), ParseGNUAttributes())); OwningStmtResult SubStmt(ParseStatement()); @@ -233,6 +236,7 @@ Parser::OwningStmtResult Parser::ParseLabeledStatement(AttributeList *Attr) { if (SubStmt.isInvalid()) SubStmt = Actions.ActOnNullStmt(ColonLoc); + // FIXME: use attributes? return Actions.ActOnLabelStmt(IdentTok.getLocation(), IdentTok.getIdentifierInfo(), ColonLoc, move(SubStmt)); @@ -246,6 +250,7 @@ Parser::OwningStmtResult Parser::ParseLabeledStatement(AttributeList *Attr) { Parser::OwningStmtResult Parser::ParseCaseStatement(AttributeList *Attr) { assert(Tok.is(tok::kw_case) && "Not a case stmt!"); // FIXME: Use attributes? + delete Attr; // It is very very common for code to contain many case statements recursively // nested, as in (but usually without indentation): @@ -371,6 +376,8 @@ Parser::OwningStmtResult Parser::ParseCaseStatement(AttributeList *Attr) { /// Parser::OwningStmtResult Parser::ParseDefaultStatement(AttributeList *Attr) { //FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_default) && "Not a default stmt!"); SourceLocation DefaultLoc = ConsumeToken(); // eat the 'default'. @@ -427,6 +434,8 @@ Parser::OwningStmtResult Parser::ParseDefaultStatement(AttributeList *Attr) { Parser::OwningStmtResult Parser::ParseCompoundStatement(AttributeList *Attr, bool isStmtExpr) { //FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::l_brace) && "Not a compount stmt!"); // Enter a scope to hold everything within the compound stmt. Compound @@ -562,6 +571,8 @@ bool Parser::ParseParenExprOrCondition(OwningExprResult &ExprResult, /// Parser::OwningStmtResult Parser::ParseIfStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_if) && "Not an if stmt!"); SourceLocation IfLoc = ConsumeToken(); // eat the 'if'. @@ -686,6 +697,8 @@ Parser::OwningStmtResult Parser::ParseIfStatement(AttributeList *Attr) { /// [C++] 'switch' '(' condition ')' statement Parser::OwningStmtResult Parser::ParseSwitchStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_switch) && "Not a switch stmt!"); SourceLocation SwitchLoc = ConsumeToken(); // eat the 'switch'. @@ -763,6 +776,8 @@ Parser::OwningStmtResult Parser::ParseSwitchStatement(AttributeList *Attr) { /// [C++] 'while' '(' condition ')' statement Parser::OwningStmtResult Parser::ParseWhileStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_while) && "Not a while stmt!"); SourceLocation WhileLoc = Tok.getLocation(); ConsumeToken(); // eat the 'while'. @@ -836,6 +851,8 @@ Parser::OwningStmtResult Parser::ParseWhileStatement(AttributeList *Attr) { /// Note: this lets the caller parse the end ';'. Parser::OwningStmtResult Parser::ParseDoStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_do) && "Not a do stmt!"); SourceLocation DoLoc = ConsumeToken(); // eat the 'do'. @@ -911,6 +928,8 @@ Parser::OwningStmtResult Parser::ParseDoStatement(AttributeList *Attr) { /// Parser::OwningStmtResult Parser::ParseForStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_for) && "Not a for stmt!"); SourceLocation ForLoc = ConsumeToken(); // eat the 'for'. @@ -1081,6 +1100,8 @@ Parser::OwningStmtResult Parser::ParseForStatement(AttributeList *Attr) { /// Parser::OwningStmtResult Parser::ParseGotoStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_goto) && "Not a goto stmt!"); SourceLocation GotoLoc = ConsumeToken(); // eat the 'goto'. @@ -1115,6 +1136,8 @@ Parser::OwningStmtResult Parser::ParseGotoStatement(AttributeList *Attr) { /// Parser::OwningStmtResult Parser::ParseContinueStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + SourceLocation ContinueLoc = ConsumeToken(); // eat the 'continue'. return Actions.ActOnContinueStmt(ContinueLoc, CurScope); } @@ -1127,6 +1150,8 @@ Parser::OwningStmtResult Parser::ParseContinueStatement(AttributeList *Attr) { /// Parser::OwningStmtResult Parser::ParseBreakStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + SourceLocation BreakLoc = ConsumeToken(); // eat the 'break'. return Actions.ActOnBreakStmt(BreakLoc, CurScope); } @@ -1136,6 +1161,8 @@ Parser::OwningStmtResult Parser::ParseBreakStatement(AttributeList *Attr) { /// 'return' expression[opt] ';' Parser::OwningStmtResult Parser::ParseReturnStatement(AttributeList *Attr) { // FIXME: Use attributes? + delete Attr; + assert(Tok.is(tok::kw_return) && "Not a return stmt!"); SourceLocation ReturnLoc = ConsumeToken(); // eat the 'return'. @@ -1447,6 +1474,8 @@ Parser::DeclPtrTy Parser::ParseFunctionTryBlock(DeclPtrTy Decl) { /// Parser::OwningStmtResult Parser::ParseCXXTryBlock(AttributeList* Attr) { // FIXME: Add attributes? + delete Attr; + assert(Tok.is(tok::kw_try) && "Expected 'try'"); SourceLocation TryLoc = ConsumeToken(); diff --git a/lib/Parse/Parser.cpp b/lib/Parse/Parser.cpp index e5bed3096d..30899c5ddd 100644 --- a/lib/Parse/Parser.cpp +++ b/lib/Parse/Parser.cpp @@ -742,11 +742,11 @@ void Parser::ParseKNRParamDeclarations(Declarator &D) { // Handle the full declarator list. while (1) { - Action::AttrTy *AttrList; // If attributes are present, parse them. + llvm::OwningPtr AttrList; if (Tok.is(tok::kw___attribute)) // FIXME: attach attributes too. - AttrList = ParseGNUAttributes(); + AttrList.reset(ParseGNUAttributes()); // Ask the actions module to compute the type for this declarator. Action::DeclPtrTy Param =