[clang] Ensure that statements, expressions and types are trivially destructible

[riccibruno]

Summary

Since statements, expressions and types are allocated with the BumpPtrAllocator from ASTContext their destructor is not executed. Two classes are currently exempted from the check : InitListExpr due to its ASTVector and ConstantArrayType due to its APInt.

No functional changes.

[lebedev.ri]

SGTM, but i wonder if this should be done one level up, in BumpPtrAllocator itself?

[riccibruno]

In D66646#1642708, @lebedev.ri wrote:

SGTM, but i wonder if this should be done one level up, in BumpPtrAllocator itself?

I don't understand. BumpPtrAllocator is only used to allocate raw memory and doesn't know anything about how it is going to be used.

[lebedev.ri]

In D66646#1642730, @riccibruno wrote:

In D66646#1642708, @lebedev.ri wrote:

SGTM, but i wonder if this should be done one level up, in BumpPtrAllocator itself?

I don't understand. BumpPtrAllocator is only used to allocate raw memory and doesn't know anything about how it is going to be used.

I was looking at SpecificBumpPtrAllocator, which knows it's type.
But if i look down the indirection, i think AllocatorBase::Allocate<T>()/AllocatorBase::Deallocate<T>() is the place.

[riccibruno]

I was looking at SpecificBumpPtrAllocator, which knows it's type.
But if i look down the indirection, i think AllocatorBase::Allocate<T>()/AllocatorBase::Deallocate<T>() is the place.

I did look at that, but I think it won't work for two reasons:

1. AST nodes are very often allocated with some trailing objects and so the typed version of Allocate is not used.
2. It would be difficult to add exceptions as done in this patch. The two exceptions I added are (I think!) latent bugs, but they seem to be relatively harmless (for ConstantArrayType you would need to have an array with a size which do not fit inline in the APInt, and then it would only cause a leak).

In time it would be nice to just remove the two exceptions, but then point 1 still apply.

[lebedev.ri]

SGTM but please wait a bit in case @aaron.ballman / @rsmith wants to comment.
Thanks.

cfe/trunk/lib/AST/Stmt.cpp

 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstring>
 #include <string>
 #include <utility>
+#include <type_traits>
 
 using namespace clang;
 
 static struct StmtClassNameTable {
   const char *Name;
   unsigned Counter;
   unsigned Size;
 } StmtClassInfo[Stmt::lastStmtConstant+1];

 // Check that no statement / expression class is polymorphic. LLVM style RTTI
 // should be used instead. If absolutely needed an exception can still be added
 // here by defining the appropriate macro (but please don't do this).
 #define STMT(CLASS, PARENT) \
   static_assert(!std::is_polymorphic<CLASS>::value, \
                 #CLASS " should not be polymorphic!");
 #include "clang/AST/StmtNodes.inc"
 
+// Check that no statement / expression class has a non-trival destructor.
+// Statements and expressions are allocated with the BumpPtrAllocator from
+// ASTContext and therefore their destructor is not executed.
+#define STMT(CLASS, PARENT)                                                    \
+  static_assert(std::is_trivially_destructible<CLASS>::value,                  \
+                #CLASS " should be trivially destructible!");
+// FIXME: InitListExpr is not trivially destructible due to its ASTVector.
+#define INITLISTEXPR(CLASS, PARENT)
+#include "clang/AST/StmtNodes.inc"
+
 void Stmt::PrintStats() {
   // Ensure the table is primed.
   getStmtInfoTableEntry(Stmt::NullStmtClass);
 
   unsigned sum = 0;
   llvm::errs() << "\n*** Stmt/Expr Stats:\n";
   for (int i = 0; i != Stmt::lastStmtConstant+1; i++) {
     if (StmtClassInfo[i].Name == nullptr) continue;

cfe/trunk/lib/AST/Type.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <cstring>
+#include <type_traits>
 
 using namespace clang;
 
 bool Qualifiers::isStrictSupersetOf(Qualifiers Other) const {
   return (*this != Other) &&
     // CVR qualifiers superset
     (((Mask & CVRMask) | (Other.Mask & CVRMask)) == (Mask & CVRMask)) &&
     // ObjC GC qualifiers superset

 // Check that no type class is polymorphic. LLVM style RTTI should be used
 // instead. If absolutely needed an exception can still be added here by
 // defining the appropriate macro (but please don't do this).
 #define TYPE(CLASS, BASE) \
   static_assert(!std::is_polymorphic<CLASS##Type>::value, \
                 #CLASS "Type should not be polymorphic!");
 #include "clang/AST/TypeNodes.def"
 
+// Check that no type class has a non-trival destructor. Types are
+// allocated with the BumpPtrAllocator from ASTContext and therefore
+// their destructor is not executed.
+//
+// FIXME: ConstantArrayType is not trivially destructible because of its
+// APInt member. It should be replaced in favor of ASTContext allocation.
+#define TYPE(CLASS, BASE)                                                      \
+  static_assert(std::is_trivially_destructible<CLASS##Type>::value ||          \
+                    std::is_same<CLASS##Type, ConstantArrayType>::value,       \
+                #CLASS "Type should be trivially destructible!");
+#include "clang/AST/TypeNodes.def"
+
 QualType Type::getLocallyUnqualifiedSingleStepDesugaredType() const {
   switch (getTypeClass()) {
 #define ABSTRACT_TYPE(Class, Parent)
 #define TYPE(Class, Parent) \
   case Type::Class: { \
     const auto *ty = cast<Class##Type>(this); \
     if (!ty->isSugared()) return QualType(ty, 0); \
     return ty->desugar(); \