[PATCH] D125802: Fix std::has_unique_object_representations for _BitInt types with padding bits

[Erich Keane via Phabricator via cfe-commits]

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