On 17 November 2016 at 09:52, Malcolm Parsons via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Author: malcolm.parsons > Date: Thu Nov 17 11:52:58 2016 > New Revision: 287241 > > URL: http://llvm.org/viewvc/llvm-project?rev=287241&view=rev > Log: > Use unique_ptr for cached tokens for default arguments in C++. > > Summary: > This changes pointers to cached tokens for default arguments in C++ from > raw pointers to unique_ptrs. There was a fixme in the code where the > cached tokens are created about using a smart pointer. > > The change is straightforward, though I did have to track down and fix a > memory corruption caused by the change. memcpy was being used to copy > parameter information. This duplicated the unique_ptr, which led to the > cached token buffer being deleted prematurely. > > Patch by David Tarditi! > > Reviewers: malcolm.parsons > > Subscribers: arphaman, malcolm.parsons, cfe-commits > > Differential Revision: https://reviews.llvm.org/D26435 > > Modified: > cfe/trunk/include/clang/Parse/Parser.h > cfe/trunk/include/clang/Sema/DeclSpec.h > cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp > cfe/trunk/lib/Parse/ParseDecl.cpp > cfe/trunk/lib/Parse/ParseDeclCXX.cpp > cfe/trunk/lib/Sema/DeclSpec.cpp > cfe/trunk/lib/Sema/SemaDeclCXX.cpp > > Modified: cfe/trunk/include/clang/Parse/Parser.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Parse/Parser.h?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/Parse/Parser.h (original) > +++ cfe/trunk/include/clang/Parse/Parser.h Thu Nov 17 11:52:58 2016 > @@ -1017,8 +1017,8 @@ private: > /// (C++ [class.mem]p2). > struct LateParsedDefaultArgument { > explicit LateParsedDefaultArgument(Decl *P, > - CachedTokens *Toks = nullptr) > - : Param(P), Toks(Toks) { } > + std::unique_ptr<CachedTokens> Toks > = nullptr) > + : Param(P), Toks(std::move(Toks)) { } > > /// Param - The parameter declaration for this parameter. > Decl *Param; > @@ -1027,7 +1027,7 @@ private: > /// argument expression, not including the '=' or the terminating > /// ')' or ','. This will be NULL for parameters that have no > /// default argument. > - CachedTokens *Toks; > + std::unique_ptr<CachedTokens> Toks; > }; > > /// LateParsedMethodDeclaration - A method declaration inside a class > that > > Modified: cfe/trunk/include/clang/Sema/DeclSpec.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Sema/DeclSpec.h?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/Sema/DeclSpec.h (original) > +++ cfe/trunk/include/clang/Sema/DeclSpec.h Thu Nov 17 11:52:58 2016 > @@ -1188,14 +1188,14 @@ struct DeclaratorChunk { > /// declaration of a member function), it will be stored here as a > /// sequence of tokens to be parsed once the class definition is > /// complete. Non-NULL indicates that there is a default argument. > - CachedTokens *DefaultArgTokens; > + std::unique_ptr<CachedTokens> DefaultArgTokens; > > ParamInfo() = default; > ParamInfo(IdentifierInfo *ident, SourceLocation iloc, > Decl *param, > - CachedTokens *DefArgTokens = nullptr) > + std::unique_ptr<CachedTokens> DefArgTokens = nullptr) > : Ident(ident), IdentLoc(iloc), Param(param), > - DefaultArgTokens(DefArgTokens) {} > + DefaultArgTokens(std::move(DefArgTokens)) {} > }; > > struct TypeAndRange { > @@ -1310,10 +1310,8 @@ struct DeclaratorChunk { > /// > /// This is used in various places for error recovery. > void freeParams() { > - for (unsigned I = 0; I < NumParams; ++I) { > - delete Params[I].DefaultArgTokens; > - Params[I].DefaultArgTokens = nullptr; > - } > + for (unsigned I = 0; I < NumParams; ++I) > + Params[I].DefaultArgTokens.reset(); > if (DeleteParams) { > delete[] Params; > DeleteParams = false; > > Modified: cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ > ParseCXXInlineMethods.cpp?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp (original) > +++ cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp Thu Nov 17 11:52:58 2016 > @@ -319,7 +319,8 @@ void Parser::ParseLexedMethodDeclaration > // Introduce the parameter into scope. > bool HasUnparsed = Param->hasUnparsedDefaultArg(); > Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param); > - if (CachedTokens *Toks = LM.DefaultArgs[I].Toks) { > + std::unique_ptr<CachedTokens> Toks = std::move(LM.DefaultArgs[I]. > Toks); > + if (Toks) { > // Mark the end of the default argument so that we know when to > stop when > // we parse it later on. > Token LastDefaultArgToken = Toks->back(); > @@ -377,9 +378,6 @@ void Parser::ParseLexedMethodDeclaration > > if (Tok.is(tok::eof) && Tok.getEofData() == Param) > ConsumeAnyToken(); > - > - delete Toks; > - LM.DefaultArgs[I].Toks = nullptr; > } else if (HasUnparsed) { > assert(Param->hasInheritedDefaultArg()); > FunctionDecl *Old = cast<FunctionDecl>(LM.Method)- > >getPreviousDecl(); > > Modified: cfe/trunk/lib/Parse/ParseDecl.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ > ParseDecl.cpp?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Parse/ParseDecl.cpp (original) > +++ cfe/trunk/lib/Parse/ParseDecl.cpp Thu Nov 17 11:52:58 2016 > @@ -6022,7 +6022,7 @@ void Parser::ParseParameterDeclarationCl > > // DefArgToks is used when the parsing of default arguments needs > // to be delayed. > - CachedTokens *DefArgToks = nullptr; > + std::unique_ptr<CachedTokens> DefArgToks; > > // If no parameter was specified, verify that *something* was > specified, > // otherwise we have a missing type and identifier. > @@ -6058,13 +6058,11 @@ void Parser::ParseParameterDeclarationCl > // If we're inside a class definition, cache the tokens > // corresponding to the default argument. We'll actually parse > // them when we see the end of the class definition. > - // FIXME: Can we use a smart pointer for Toks? > - DefArgToks = new CachedTokens; > + DefArgToks.reset(new CachedTokens); > > SourceLocation ArgStartLoc = NextToken().getLocation(); > if (!ConsumeAndStoreInitializer(*DefArgToks, > CIK_DefaultArgument)) { > - delete DefArgToks; > - DefArgToks = nullptr; > + DefArgToks.reset(); > Actions.ActOnParamDefaultArgumentError(Param, EqualLoc); > } else { > Actions.ActOnParamUnparsedDefaultArgument(Param, EqualLoc, > @@ -6100,7 +6098,7 @@ void Parser::ParseParameterDeclarationCl > > ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII, > ParmDeclarator. > getIdentifierLoc(), > - Param, DefArgToks)); > + Param, std::move(DefArgToks))); > } > > if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) { > > Modified: cfe/trunk/lib/Parse/ParseDeclCXX.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ > ParseDeclCXX.cpp?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Parse/ParseDeclCXX.cpp (original) > +++ cfe/trunk/lib/Parse/ParseDeclCXX.cpp Thu Nov 17 11:52:58 2016 > @@ -2039,7 +2039,7 @@ void Parser::HandleMemberFunctionDeclDel > LateMethod->DefaultArgs.reserve(FTI.NumParams); > for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx) > LateMethod->DefaultArgs.push_back(LateParsedDefaultArgument( > - FTI.Params[ParamIdx].Param, FTI.Params[ParamIdx]. > DefaultArgTokens)); > + FTI.Params[ParamIdx].Param, std::move(FTI.Params[ParamIdx] > .DefaultArgTokens))); > This line is more than 80 columns long. > } > } > > > Modified: cfe/trunk/lib/Sema/DeclSpec.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > DeclSpec.cpp?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/DeclSpec.cpp (original) > +++ cfe/trunk/lib/Sema/DeclSpec.cpp Thu Nov 17 11:52:58 2016 > @@ -223,13 +223,19 @@ DeclaratorChunk DeclaratorChunk::getFunc > if (!TheDeclarator.InlineStorageUsed && > NumParams <= llvm::array_lengthof(TheDeclarator.InlineParams)) { > I.Fun.Params = TheDeclarator.InlineParams; > + // Zero the memory block so that unique pointers are initialized > + // properly. > + memset(I.Fun.Params, 0, sizeof(Params[0]) * NumParams); > This is not a reasonable way to set up the parameters. It's theoretically wrong, and practically a debug version of unique_ptr might have other members that need to be non-zero. Instead, you need to actually run the ParamInfo default constructor: new (I.Fun.Params) ParamInfo[NumParams]; or similar. > I.Fun.DeleteParams = false; > TheDeclarator.InlineStorageUsed = true; > } else { > - I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams]; > + // Call the version of new that zeroes memory so that unique > pointers > + // are initialized properly. > + I.Fun.Params = new DeclaratorChunk::ParamInfo[NumParams](); > You don't need the parens here. The new-expression will call the default constructor for the array element type. (There was an MSVC bug in this area but I don't *think* we support versions of MSVC with that bug any more.) I.Fun.DeleteParams = true; > } > - memcpy(I.Fun.Params, Params, sizeof(Params[0]) * NumParams); > + for (unsigned i = 0; i < NumParams; i++) > + I.Fun.Params[i] = std::move(Params[i]); > } > > // Check what exception specification information we should actually > store. > > Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaDeclCXX.cpp?rev=287241&r1=287240&r2=287241&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Thu Nov 17 11:52:58 2016 > @@ -395,7 +395,7 @@ void Sema::CheckExtraCXXDefaultArguments > ++argIdx) { > ParmVarDecl *Param = cast<ParmVarDecl>(chunk.Fun. > Params[argIdx].Param); > if (Param->hasUnparsedDefaultArg()) { > - CachedTokens *Toks = chunk.Fun.Params[argIdx].DefaultArgTokens; > + std::unique_ptr<CachedTokens> Toks = std::move(chunk.Fun.Params[ > argIdx].DefaultArgTokens); > This line is more than 80 columns long. > SourceRange SR; > if (Toks->size() > 1) > SR = SourceRange((*Toks)[1].getLocation(), > @@ -404,8 +404,6 @@ void Sema::CheckExtraCXXDefaultArguments > SR = UnparsedDefaultArgLocs[Param]; > Diag(Param->getLocation(), diag::err_param_default_ > argument_nonfunc) > << SR; > - delete Toks; > - chunk.Fun.Params[argIdx].DefaultArgTokens = nullptr; > } else if (Param->getDefaultArg()) { > Diag(Param->getLocation(), diag::err_param_default_ > argument_nonfunc) > << Param->getDefaultArg()->getSourceRange(); > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
