From c7689fd552cdf2b37e804a59322bd0661ccdd3c5 Mon Sep 17 00:00:00 2001 From: Andrew Browne Date: Thu, 2 Jun 2022 17:42:54 -0700 Subject: [PATCH] [Clang] Fix memory leak due to TemplateArgumentListInfo used in AST node. 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 Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D126944 --- clang-tools-extra/clangd/AST.cpp | 6 +++-- clang/docs/ReleaseNotes.rst | 2 ++ clang/include/clang/AST/DeclTemplate.h | 5 ++-- clang/include/clang/AST/TemplateBase.h | 7 +++++ clang/lib/AST/ASTImporter.cpp | 7 ++--- clang/lib/AST/DeclTemplate.cpp | 12 ++++++--- clang/lib/AST/TemplateBase.cpp | 22 ++++++++++++++++ .../lib/Sema/SemaTemplateInstantiateDecl.cpp | 26 ++++++++++++++----- 8 files changed, 69 insertions(+), 18 deletions(-) diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp index 70d98d0e0bb4..ca838618badd 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -53,8 +53,10 @@ getTemplateSpecializationArgLocs(const NamedDecl &ND) { llvm::dyn_cast(&ND)) { if (auto *Args = Var->getTemplateArgsAsWritten()) return Args->arguments(); - } else if (auto *Var = llvm::dyn_cast(&ND)) - return Var->getTemplateArgsInfo().arguments(); + } else if (auto *Var = llvm::dyn_cast(&ND)) { + if (auto *Args = Var->getTemplateArgsInfo()) + return Args->arguments(); + } // We return None for ClassTemplateSpecializationDecls because it does not // contain TemplateArgumentLoc information. return llvm::None; diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 1e95d3cef51c..1b1649ac09c8 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -168,6 +168,8 @@ Bug Fixes `Issue 55562 `_. - Clang will allow calling a ``consteval`` function in a default argument. This fixes `Issue 48230 `_. +- Fixed memory leak due to ``VarTemplateSpecializationDecl`` using + ``TemplateArgumentListInfo`` instead of ``ASTTemplateArgumentListInfo``. Improvements to Clang's diagnostics ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h index 66e0bb2c02a1..a00917913e41 100644 --- a/clang/include/clang/AST/DeclTemplate.h +++ b/clang/include/clang/AST/DeclTemplate.h @@ -2718,7 +2718,7 @@ class VarTemplateSpecializationDecl : public VarDecl, /// 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; @@ -2773,8 +2773,9 @@ public: // 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; } diff --git a/clang/include/clang/AST/TemplateBase.h b/clang/include/clang/AST/TemplateBase.h index e8064121d279..3ac755ef74a1 100644 --- a/clang/include/clang/AST/TemplateBase.h +++ b/clang/include/clang/AST/TemplateBase.h @@ -618,6 +618,9 @@ private: 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; @@ -647,6 +650,10 @@ public: 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., diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 5b35b70f0960..21b9f21b8e6e 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -6018,9 +6018,10 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateSpecializationDecl( return TInfoOrErr.takeError(); TemplateArgumentListInfo ToTAInfo; - if (Error Err = ImportTemplateArgumentListInfo( - D->getTemplateArgsInfo(), ToTAInfo)) - return std::move(Err); + if (const ASTTemplateArgumentListInfo *Args = D->getTemplateArgsInfo()) { + if (Error Err = ImportTemplateArgumentListInfo(*Args, ToTAInfo)) + return std::move(Err); + } using PartVarSpecDecl = VarTemplatePartialSpecializationDecl; // Create a new specialization. diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp index 3b4ed3107e5a..e7e5f355809b 100644 --- a/clang/lib/AST/DeclTemplate.cpp +++ b/clang/lib/AST/DeclTemplate.cpp @@ -1335,10 +1335,14 @@ VarTemplateDecl *VarTemplateSpecializationDecl::getSpecializedTemplate() const { 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); } //===----------------------------------------------------------------------===// diff --git a/clang/lib/AST/TemplateBase.cpp b/clang/lib/AST/TemplateBase.cpp index 229a8db21ab6..e0f5916a9a0b 100644 --- a/clang/lib/AST/TemplateBase.cpp +++ b/clang/lib/AST/TemplateBase.cpp @@ -617,6 +617,17 @@ ASTTemplateArgumentListInfo::Create(const ASTContext &C, return new (Mem) ASTTemplateArgumentListInfo(List); } +const ASTTemplateArgumentListInfo * +ASTTemplateArgumentListInfo::Create(const ASTContext &C, + const ASTTemplateArgumentListInfo *List) { + if (!List) + return nullptr; + std::size_t size = + totalSizeToAlloc(List->getNumTemplateArgs()); + void *Mem = C.Allocate(size, alignof(ASTTemplateArgumentListInfo)); + return new (Mem) ASTTemplateArgumentListInfo(List); +} + ASTTemplateArgumentListInfo::ASTTemplateArgumentListInfo( const TemplateArgumentListInfo &Info) { LAngleLoc = Info.getLAngleLoc(); @@ -628,6 +639,17 @@ ASTTemplateArgumentListInfo::ASTTemplateArgumentListInfo( new (&ArgBuffer[i]) TemplateArgumentLoc(Info[i]); } +ASTTemplateArgumentListInfo::ASTTemplateArgumentListInfo( + const ASTTemplateArgumentListInfo *Info) { + LAngleLoc = Info->getLAngleLoc(); + RAngleLoc = Info->getRAngleLoc(); + NumTemplateArgs = Info->getNumTemplateArgs(); + + TemplateArgumentLoc *ArgBuffer = getTrailingObjects(); + for (unsigned i = 0; i != NumTemplateArgs; ++i) + new (&ArgBuffer[i]) TemplateArgumentLoc((*Info)[i]); +} + void ASTTemplateKWAndArgsInfo::initializeFrom( SourceLocation TemplateKWLoc, const TemplateArgumentListInfo &Info, TemplateArgumentLoc *OutArgArray) { diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp index 77c5677b65c8..c89dc8c3513d 100644 --- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -3801,13 +3801,15 @@ Decl *TemplateDeclInstantiator::VisitVarTemplateSpecializationDecl( 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)) - return nullptr; + if (SemaRef.SubstTemplateArguments(TemplateArgsInfo->arguments(), + TemplateArgs, VarTemplateArgsInfo)) + return nullptr; + } // Check that the template argument list is well-formed for this template. SmallVector Converted; @@ -5554,8 +5556,18 @@ void Sema::InstantiateVariableDefinition(SourceLocation PointOfInstantiation, // 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(Instantiator.VisitVarTemplateSpecializationDecl( - VarSpec->getSpecializedTemplate(), Def, VarSpec->getTemplateArgsInfo(), + VarSpec->getSpecializedTemplate(), Def, TemplateArgInfo, VarSpec->getTemplateArgs().asArray(), VarSpec)); if (Var) { llvm::PointerUnion