Use unique_ptr for cached tokens for default arguments in C++.

[dtarditi]

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.

[malcolm.parsons]

include/clang/Sema/DeclSpec.h

1314 ↗

(On Diff #77289)

Did you mean reset()?

lib/Parse/ParseDecl.cpp

6064 ↗

(On Diff #77289)

Did you mean reset()?

[arphaman]

include/clang/Sema/DeclSpec.h

1313 ↗

(On Diff #77289)

You can elide the curly braces here now that the loop has just one statement.

[dtarditi]

Thanks for the code review feedback - I've addressed it. Yes, we should use reset() instead of release(). I also deleted the unnecessary braces. I don't have commit access, so if this looks good, could someone commit this on my behalf?

[malcolm.parsons]

Your email went in my spam folder.
LGTM.
Will commit later.

[malcolm.parsons]

Expected Passes    : 4977
Expected Failures  : 18
Unsupported Tests  : 25
Unexpected Failures: 5037

Needs more work?

[dtarditi]

I sync'ed to the head of the tree. I tried to reproduce the failures that you saw and couldn't. I'm building on Windows 10 x64 using Visual Studio. What platform/OS did you build on? I built and test both Debug and RelWithDebugInfo.

Here's what I saw for RelWithDebugInfo

Running the Clang regression tests
-- Testing: 10026 tests, 24 threads --

Testing Time: 604.17s
  Expected Passes    : 9922
  Expected Failures  : 21
  Unsupported Tests  : 83

[malcolm.parsons]

In D26435#595282, @dtarditi wrote:

What platform/OS did you build on?

I build release with asserts on 64 bit Linux. My host compiler is gcc.

[dtarditi]

The parameter array needed to be initialized so that assignments involving unique pointers work properly. The memory could be uninitialized according to C++ semantics.. Visual C++ was zeroing the memory and GCC was not. This is why the tests passed on Windows and failed on Linux. I updated Sema/DeclSpec.cpp to zero the parameter array before it is used.

Here are the test results from an x64 Linux Ubuntu box:

Testing Time: 465.12s

Expected Passes    : 10017
Expected Failures  : 18
Unsupported Tests  : 27

[malcolm.parsons]

LGTM.

cfe/trunk/include/clang/Parse/Parser.h

private:
   };
 
   /// LateParsedDefaultArgument - Keeps track of a parameter that may
   /// have a default argument that cannot be parsed yet because it
   /// occurs within a member function declaration inside the class
   /// (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;
 
     /// Toks - The sequence of tokens that comprises the default
     /// 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
   /// contains at least one entity whose parsing needs to be delayed
   /// until the class itself is completely-defined, such as a default
   /// argument (C++ [class.mem]p2).
   struct LateParsedMethodDeclaration : public LateParsedDeclaration {
     explicit LateParsedMethodDeclaration(Parser *P, Decl *M)

cfe/trunk/include/clang/Sema/DeclSpec.h

struct ParamInfo {
     SourceLocation IdentLoc;
     Decl *Param;
 
     /// DefaultArgTokens - When the parameter's default argument
     /// cannot be parsed immediately (because it occurs within the
     /// 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 {
     ParsedType Ty;
     SourceRange Range;
   };
 
   struct FunctionTypeInfo : TypeInfoCommon {
struct FunctionTypeInfo : TypeInfoCommon {
     /// \brief If HasTrailingReturnType is true, this is the trailing return
     /// type specified.
     UnionParsedType TrailingReturnType;
 
     /// \brief Reset the parameter list to having zero parameters.
     ///
     /// 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;
       }
       NumParams = 0;
     }
 
     void destroy() {

cfe/trunk/lib/Parse/ParseCXXInlineMethods.cpp

void Parser::ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM) {
   // arguments.
   ParseScope PrototypeScope(this, Scope::FunctionPrototypeScope |
                             Scope::FunctionDeclarationScope | Scope::DeclScope);
   for (unsigned I = 0, N = LM.DefaultArgs.size(); I != N; ++I) {
     auto Param = cast<ParmVarDecl>(LM.DefaultArgs[I].Param);
     // 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();
       Token DefArgEnd;
       DefArgEnd.startToken();
       DefArgEnd.setKind(tok::eof);
       DefArgEnd.setLocation(LastDefaultArgToken.getEndLoc());
       DefArgEnd.setEofData(Param);
if (Toks) {
 
       // There could be leftover tokens (e.g. because of an error).
       // Skip through until we reach the 'end of default argument' token.
       while (Tok.isNot(tok::eof))
         ConsumeAnyToken();
 
       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();
       ParmVarDecl *OldParam = Old->getParamDecl(I);
       assert (!OldParam->hasUnparsedDefaultArg());
       if (OldParam->hasUninstantiatedDefaultArg())
         Param->setUninstantiatedDefaultArg(
             OldParam->getUninstantiatedDefaultArg());

cfe/trunk/lib/Parse/ParseDecl.cpp

do {
     // Parse GNU attributes, if present.
     MaybeParseGNUAttributes(ParmDeclarator);
 
     // Remember this parsed parameter in ParamInfo.
     IdentifierInfo *ParmII = ParmDeclarator.getIdentifier();
 
     // 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.
     if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
         ParmDeclarator.getNumTypeObjects() == 0) {
       // Completely missing, emit error.
       Diag(DSStart, diag::err_missing_param);
     } else {
if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
       if (Tok.is(tok::equal)) {
         SourceLocation EqualLoc = Tok.getLocation();
 
         // Parse the default argument
         if (D.getContext() == Declarator::MemberContext) {
           // 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,
                                                       ArgStartLoc);
           }
         } else {
           // Consume the '='.
           ConsumeToken();
if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
             Actions.ActOnParamDefaultArgument(Param, EqualLoc,
                                               DefArgResult.get());
           }
         }
       }
 
       ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
                                           ParmDeclarator.getIdentifierLoc(), 
-                                          Param, DefArgToks));
+                                          Param, std::move(DefArgToks)));
     }
 
     if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
       if (!getLangOpts().CPlusPlus) {
         // We have ellipsis without a preceding ',', which is ill-formed
         // in C. Complain and provide the fix.
         Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis)
             << FixItHint::CreateInsertion(EllipsisLoc, ", ");

cfe/trunk/lib/Parse/ParseDeclCXX.cpp

if (NeedLateParse) {
     LateMethod->ExceptionSpecTokens = FTI.ExceptionSpecTokens;
     FTI.ExceptionSpecTokens = nullptr;
 
     // Push tokens for each parameter.  Those that do not have
     // defaults will be NULL.
     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)));
   }
 }
 
 /// isCXX11VirtSpecifier - Determine whether the given token is a C++11
 /// virt-specifier.
 ///
 ///       virt-specifier:
 ///         override

cfe/trunk/lib/Sema/DeclSpec.cpp

DeclaratorChunk DeclaratorChunk::getFunction(bool hasProto,
   if (NumParams) {
     // If the 'InlineParams' in Declarator is unused and big enough, put our
     // parameter list there (in an effort to avoid new/delete traffic).  If it
     // is already used (consider a function returning a function pointer) or too
     // small (function with too many parameters), go to the heap.
     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);
       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]();
       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.
   switch (ESpecType) {
   default: break; // By default, save nothing.
   case EST_Dynamic:
     // new[] an exception array if needed
     if (NumExceptions) {

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

if (chunk.Kind == DeclaratorChunk::Function) {
         // arguments.
         MightBeFunction = false;
         continue;
       }
       for (unsigned argIdx = 0, e = chunk.Fun.NumParams; argIdx != e;
            ++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);
           SourceRange SR;
           if (Toks->size() > 1)
             SR = SourceRange((*Toks)[1].getLocation(),
                              Toks->back().getLocation());
           else
             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();
           Param->setDefaultArg(nullptr);
         }
       }
     } else if (chunk.Kind != DeclaratorChunk::Paren) {
       MightBeFunction = false;