r287241 - Use unique_ptr for cached tokens for default arguments in C++.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 17 11:20:43 PST 2016
On 17 November 2016 at 09:52, Malcolm Parsons via cfe-commits <
cfe-commits at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161117/339b550e/attachment-0001.html>
More information about the cfe-commits
mailing list