[Clang] Fix memory leak due to TemplateArgumentListInfo used in AST node.

[browneee]

Summary

It looks like the leak is rooted at the allocation here:
https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L3857

The VarTemplateSpecializationDecl is allocated using placement new which uses the AST structure for ownership: https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/lib/AST/DeclBase.cpp#L99

The problem is the TemplateArgumentListInfo inside https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/DeclTemplate.h#L2721
This object contains a vector which does not use placement new: https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L564

Apparently ASTTemplateArgumentListInfo should be used instead https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L575

https://reviews.llvm.org/D125802#3551305

[browneee]

More details on the bug: https://reviews.llvm.org/D125802#3551305

Alternative implementation for fix: https://reviews.llvm.org/D126937

I'm not yet sure if these changes are correct.

[aaron.ballman]

clang/include/clang/AST/DeclTemplate.h

2776

Can we make it a prerequisite that the argument has to be nonnull and thus we can pass in a const reference instead?

2778–2779

Do we want to assert that TemplateArgsInfo is nonnull and return a const reference instead?

Basically, I'm wondering if we can remove a bunch of the pointers and go with references as much as possible; it seems to me that a null pointer is a sign of an error, same as with TemplateArgs.

clang/lib/AST/ASTImporter.cpp

6021–6024

You should spell out the type name here instead of using auto (per coding standard).

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

5561

You should spell out the type name here as well.

5564–5566

More coding standard changes -- btw, it looks like you need to run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).

[browneee]

Remove auto used under clang/. Format.

[browneee]

Formatting.

clang/include/clang/AST/DeclTemplate.h

2776

I tried this initially. Tests broke, so I went with this more conservative change. Someone may be able to reduce use of nulls in follow up changes.

2778–2779

This may be why https://reviews.llvm.org/D126937 is failing tests.

My goals is to fix the memory leak, so the sanitizer buildbots are clean. I would agree there is probably more cleanup to do for this member variable after https://reviews.llvm.org/D126944 - but fixing the memory leak is the urgent thing for me.

[browneee]

Bump

[aaron.ballman]

LGTM; I think it's reasonable to not have test coverage specific to the changes here (the existing test coverage already uncovered this issue). It might be worth mentioning that the leak was fixed in a release note, however. Also, I did find one naming convention issue that should be fixed before landing.

Thanks for fixing this up!

clang/include/clang/AST/DeclTemplate.h

2778–2779

Okay, I'm fine not handling the extra safety measures at the moment.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

5564–5566

It looks like you may have missed the rename suggestion of switching arg to Arg per our usual coding conventions. You can fix that when landing though.

[browneee]

Added release notes and fixed variable naming.

[erichkeane]

LGTM! Thanks for this!

clang-tools-extra/clangd/AST.cpp

getTemplateSpecializationArgLocs(const NamedDecl &ND) {
   } else if (auto *Cls =
                  llvm::dyn_cast<ClassTemplatePartialSpecializationDecl>(&ND)) {
     if (auto *Args = Cls->getTemplateArgsAsWritten())
       return Args->arguments();
   } else if (auto *Var =
                  llvm::dyn_cast<VarTemplatePartialSpecializationDecl>(&ND)) {
     if (auto *Args = Var->getTemplateArgsAsWritten())
       return Args->arguments();
-  } else if (auto *Var = llvm::dyn_cast<VarTemplateSpecializationDecl>(&ND))
-    return Var->getTemplateArgsInfo().arguments();
+  } else if (auto *Var = llvm::dyn_cast<VarTemplateSpecializationDecl>(&ND)) {
+    if (auto *Args = Var->getTemplateArgsInfo())
+      return Args->arguments();
+  }
   // We return None for ClassTemplateSpecializationDecls because it does not
   // contain TemplateArgumentLoc information.
   return llvm::None;
 }
 
 template <class T>
 bool isTemplateSpecializationKind(const NamedDecl *D,
                                   TemplateSpecializationKind Kind) {

clang/docs/ReleaseNotes.rst

 - Unscoped and scoped enumeration types can no longer be initialized from a
   brace-init-list containing a single element of a different scoped enumeration
   type.
 - Allow use of an elaborated type specifier as a ``_Generic`` selection
   association in C++ mode. This fixes
   `Issue 55562 <https://github.com/llvm/llvm-project/issues/55562>`_.
 - Clang will allow calling a ``consteval`` function in a default argument. This
   fixes `Issue 48230 <https://github.com/llvm/llvm-project/issues/48230>`_.
+- Fixed memory leak due to ``VarTemplateSpecializationDecl`` using
+  ``TemplateArgumentListInfo`` instead of ``ASTTemplateArgumentListInfo``.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/include/clang/AST/DeclTemplate.h

class VarTemplateSpecializationDecl : public VarDecl,
   };
 
   /// Further info for explicit template specialization/instantiation.
   /// Does not apply to implicit specializations.
   ExplicitSpecializationInfo *ExplicitInfo = nullptr;
 
   /// The template arguments used to describe this specialization.
   const TemplateArgumentList *TemplateArgs;
-  TemplateArgumentListInfo TemplateArgsInfo;
+  const ASTTemplateArgumentListInfo *TemplateArgsInfo = nullptr;
 
   /// The point where this template was instantiated (if any).
   SourceLocation PointOfInstantiation;
 
   /// The kind of specialization this declaration refers to.
   /// Really a value of type TemplateSpecializationKind.
   unsigned SpecializationKind : 3;
 
public:
   VarTemplateDecl *getSpecializedTemplate() const;
 
   /// Retrieve the template arguments of the variable template
   /// specialization.
   const TemplateArgumentList &getTemplateArgs() const { return *TemplateArgs; }
 
   // TODO: Always set this when creating the new specialization?
   void setTemplateArgsInfo(const TemplateArgumentListInfo &ArgsInfo);
+  void setTemplateArgsInfo(const ASTTemplateArgumentListInfo *ArgsInfo);
 
-  const TemplateArgumentListInfo &getTemplateArgsInfo() const {
+  const ASTTemplateArgumentListInfo *getTemplateArgsInfo() const {
     return TemplateArgsInfo;
   }
 
   /// Determine the kind of specialization that this
   /// declaration represents.
   TemplateSpecializationKind getSpecializationKind() const {
     return static_cast<TemplateSpecializationKind>(SpecializationKind);
   }
 

clang/include/clang/AST/TemplateBase.h

struct ASTTemplateArgumentListInfo final
     : private llvm::TrailingObjects<ASTTemplateArgumentListInfo,
                                     TemplateArgumentLoc> {
 private:
   friend class ASTNodeImporter;
   friend TrailingObjects;
 
   ASTTemplateArgumentListInfo(const TemplateArgumentListInfo &List);
 
+  // FIXME: Is it ever necessary to copy to another context?
+  ASTTemplateArgumentListInfo(const ASTTemplateArgumentListInfo *List);
+
 public:
   /// The source location of the left angle bracket ('<').
   SourceLocation LAngleLoc;
 
   /// The source location of the right angle bracket ('>').
   SourceLocation RAngleLoc;
 
   /// The number of template arguments in TemplateArgs.
public:
   }
 
   const TemplateArgumentLoc &operator[](unsigned I) const {
     return getTemplateArgs()[I];
   }
 
   static const ASTTemplateArgumentListInfo *
   Create(const ASTContext &C, const TemplateArgumentListInfo &List);
+
+  // FIXME: Is it ever necessary to copy to another context?
+  static const ASTTemplateArgumentListInfo *
+  Create(const ASTContext &C, const ASTTemplateArgumentListInfo *List);
 };
 
 /// Represents an explicit template argument list in C++, e.g.,
 /// the "<int>" in "sort<int>".
 ///
 /// It is intended to be used as a trailing object on AST nodes, and
 /// as such, doesn't contain the array of TemplateArgumentLoc itself,
 /// but expects the containing object to also provide storage for

clang/lib/AST/ASTImporter.cpp

if (D2) {
     if (Error Err = importInto(T, D->getType()))
       return std::move(Err);
 
     auto TInfoOrErr = import(D->getTypeSourceInfo());
     if (!TInfoOrErr)
       return TInfoOrErr.takeError();
 
     TemplateArgumentListInfo ToTAInfo;
-    if (Error Err = ImportTemplateArgumentListInfo(
-        D->getTemplateArgsInfo(), ToTAInfo))
+    if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) {
+      if (Error Err = ImportTemplateArgumentListInfo(*Args, ToTAInfo))
         return std::move(Err);
+    }
 
     using PartVarSpecDecl = VarTemplatePartialSpecializationDecl;
     // Create a new specialization.
     if (auto *FromPartial = dyn_cast<PartVarSpecDecl>(D)) {
       // Import TemplateArgumentListInfo
       TemplateArgumentListInfo ArgInfos;
       const auto *FromTAArgsAsWritten = FromPartial->getTemplateArgsAsWritten();
       // NOTE: FromTAArgsAsWritten and template parameter list are non-null.

clang/lib/AST/DeclTemplate.cpp

VarTemplateDecl *VarTemplateSpecializationDecl::getSpecializedTemplate() const {
   if (const auto *PartialSpec =
           SpecializedTemplate.dyn_cast<SpecializedPartialSpecialization *>())
     return PartialSpec->PartialSpecialization->getSpecializedTemplate();
   return SpecializedTemplate.get<VarTemplateDecl *>();
 }
 
 void VarTemplateSpecializationDecl::setTemplateArgsInfo(
     const TemplateArgumentListInfo &ArgsInfo) {
-  TemplateArgsInfo.setLAngleLoc(ArgsInfo.getLAngleLoc());
-  TemplateArgsInfo.setRAngleLoc(ArgsInfo.getRAngleLoc());
-  for (const TemplateArgumentLoc &Loc : ArgsInfo.arguments())
-    TemplateArgsInfo.addArgument(Loc);
+  TemplateArgsInfo =
+      ASTTemplateArgumentListInfo::Create(getASTContext(), ArgsInfo);
+}
+
+void VarTemplateSpecializationDecl::setTemplateArgsInfo(
+    const ASTTemplateArgumentListInfo *ArgsInfo) {
+  TemplateArgsInfo =
+      ASTTemplateArgumentListInfo::Create(getASTContext(), ArgsInfo);
 }
 
 //===----------------------------------------------------------------------===//
 // VarTemplatePartialSpecializationDecl Implementation
 //===----------------------------------------------------------------------===//
 
 void VarTemplatePartialSpecializationDecl::anchor() {}
 

clang/lib/AST/TemplateBase.cpp

 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext &C,
                                     const TemplateArgumentListInfo &List) {
   std::size_t size = totalSizeToAlloc<TemplateArgumentLoc>(List.size());
   void *Mem = C.Allocate(size, alignof(ASTTemplateArgumentListInfo));
   return new (Mem) ASTTemplateArgumentListInfo(List);
 }
 
+const ASTTemplateArgumentListInfo *
+ASTTemplateArgumentListInfo::Create(const ASTContext &C,
+                                    const ASTTemplateArgumentListInfo *List) {
+  if (!List)
+    return nullptr;
+  std::size_t size =
+      totalSizeToAlloc<TemplateArgumentLoc>(List->getNumTemplateArgs());
+  void *Mem = C.Allocate(size, alignof(ASTTemplateArgumentListInfo));
+  return new (Mem) ASTTemplateArgumentListInfo(List);
+}
+
 ASTTemplateArgumentListInfo::ASTTemplateArgumentListInfo(
     const TemplateArgumentListInfo &Info) {
   LAngleLoc = Info.getLAngleLoc();
   RAngleLoc = Info.getRAngleLoc();
   NumTemplateArgs = Info.size();
 
   TemplateArgumentLoc *ArgBuffer = getTrailingObjects<TemplateArgumentLoc>();
   for (unsigned i = 0; i != NumTemplateArgs; ++i)
     new (&ArgBuffer[i]) TemplateArgumentLoc(Info[i]);
 }
 
+ASTTemplateArgumentListInfo::ASTTemplateArgumentListInfo(
+    const ASTTemplateArgumentListInfo *Info) {
+  LAngleLoc = Info->getLAngleLoc();
+  RAngleLoc = Info->getRAngleLoc();
+  NumTemplateArgs = Info->getNumTemplateArgs();
+
+  TemplateArgumentLoc *ArgBuffer = getTrailingObjects<TemplateArgumentLoc>();
+  for (unsigned i = 0; i != NumTemplateArgs; ++i)
+    new (&ArgBuffer[i]) TemplateArgumentLoc((*Info)[i]);
+}
+
 void ASTTemplateKWAndArgsInfo::initializeFrom(
     SourceLocation TemplateKWLoc, const TemplateArgumentListInfo &Info,
     TemplateArgumentLoc *OutArgArray) {
   this->TemplateKWLoc = TemplateKWLoc;
   LAngleLoc = Info.getLAngleLoc();
   RAngleLoc = Info.getRAngleLoc();
   NumTemplateArgs = Info.size();
 

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl(
 
   VarTemplateDecl *InstVarTemplate =
       cast_or_null<VarTemplateDecl>(SemaRef.FindInstantiatedDecl(
           D->getLocation(), VarTemplate, TemplateArgs));
   if (!InstVarTemplate)
     return nullptr;
 
   // Substitute the current template arguments.
-  const TemplateArgumentListInfo &TemplateArgsInfo = D->getTemplateArgsInfo();
-  VarTemplateArgsInfo.setLAngleLoc(TemplateArgsInfo.getLAngleLoc());
-  VarTemplateArgsInfo.setRAngleLoc(TemplateArgsInfo.getRAngleLoc());
+  if (const ASTTemplateArgumentListInfo *TemplateArgsInfo =
+          D->getTemplateArgsInfo()) {
+    VarTemplateArgsInfo.setLAngleLoc(TemplateArgsInfo->getLAngleLoc());
+    VarTemplateArgsInfo.setRAngleLoc(TemplateArgsInfo->getRAngleLoc());
 
-  if (SemaRef.SubstTemplateArguments(TemplateArgsInfo.arguments(), TemplateArgs,
-                                     VarTemplateArgsInfo))
+    if (SemaRef.SubstTemplateArguments(TemplateArgsInfo->arguments(),
+                                       TemplateArgs, VarTemplateArgsInfo))
       return nullptr;
+  }
 
   // Check that the template argument list is well-formed for this template.
   SmallVector<TemplateArgument, 4> Converted;
   if (SemaRef.CheckTemplateArgumentList(InstVarTemplate, D->getLocation(),
                                         VarTemplateArgsInfo, false, Converted,
                                         /*UpdateArgsWithConversions=*/true))
     return nullptr;
 
Var = cast_or_null<VarDecl>(SubstDecl(Def, Var->getDeclContext(),
                                           TemplateArgs));
   } else if (Var->isStaticDataMember() &&
              Var->getLexicalDeclContext()->isRecord()) {
     // We need to instantiate the definition of a static data member template,
     // and all we have is the in-class declaration of it. Instantiate a separate
     // declaration of the definition.
     TemplateDeclInstantiator Instantiator(*this, Var->getDeclContext(),
                                           TemplateArgs);
+
+    TemplateArgumentListInfo TemplateArgInfo;
+    if (const ASTTemplateArgumentListInfo *ArgInfo =
+            VarSpec->getTemplateArgsInfo()) {
+      TemplateArgInfo.setLAngleLoc(ArgInfo->getLAngleLoc());
+      TemplateArgInfo.setRAngleLoc(ArgInfo->getRAngleLoc());
+      for (const TemplateArgumentLoc &Arg : ArgInfo->arguments())
+        TemplateArgInfo.addArgument(Arg);
+    }
+
     Var = cast_or_null<VarDecl>(Instantiator.VisitVarTemplateSpecializationDecl(
-        VarSpec->getSpecializedTemplate(), Def, VarSpec->getTemplateArgsInfo(),
+        VarSpec->getSpecializedTemplate(), Def, TemplateArgInfo,
         VarSpec->getTemplateArgs().asArray(), VarSpec));
     if (Var) {
       llvm::PointerUnion<VarTemplateDecl *,
                          VarTemplatePartialSpecializationDecl *> PatternPtr =
           VarSpec->getSpecializedTemplateOrPartial();
       if (VarTemplatePartialSpecializationDecl *Partial =
           PatternPtr.dyn_cast<VarTemplatePartialSpecializationDecl *>())
         cast<VarTemplateSpecializationDecl>(Var)->setInstantiationOf(