erichkeane added a comment. In D125802#3553015 <https://reviews.llvm.org/D125802#3553015>, @aaron.ballman wrote:
> In D125802#3552047 <https://reviews.llvm.org/D125802#3552047>, @browneee > wrote: > >> 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 >> <https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L612> >> should be used instead >> https://github.com/llvm/llvm-project/blob/1a155ee7de3b62a2fabee86fb470a1554fadc54d/clang/include/clang/AST/TemplateBase.h#L575 > > Wow, thank you for the fantastic sleuthing! It seems that declaration is nine > years old, so I'm surprised the leak is only being discovered now and as part > of this particular test case. > > I don't have a particularly easy way to test this locally at the moment -- do > you know if switching `VarTemplateSpecializationDecl::TemplateArgsInfo` to be > a `ASTTemplateArgumentListInfo` solves the issue for you? Ah, ouch! This is definitely going to be the problem. `TemplateArgumentList` is generally just an 'observing' collection, stuff stored in the AST seems to need to use `ASTTEmplateArgumentListInfo`. The reason you might not notice it, is much of the time the former just references a bunch of template arguments stored elsewhere in the AST, so unless you hold it juuust right and find one that gets deleted before the rest of the AST, you won't have this problem. I'm not sure what is causing it in this test though. Either way, I very much suggest we should make this change. Note there are a few places where this might be used that a conversion between the two will have to be made, but that is expected. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125802/new/ https://reviews.llvm.org/D125802 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
