[clang][Interp] Make sure we free() allocated InitMaps

[tbaeder]

Summary

They get allocated when calling initialize() on a primitive array. And they get free'd when the array is fully initialized. However, when that never happens, they get leaked. Fix that by calling the destructor of global variables.

No test attached here, since the problem only exists when asan is used. However, the next patch I upload will test that InitMaps work.

[aaron.ballman]

The changes look correct to me, but you may want to wait a little bit before landing to give the other reviewers a chance to look this over given that there's no tests possible for it.

[shafik]

clang/lib/AST/Interp/Descriptor.cpp

46

I believe Ptr is not longer valid b/c of free(IM) b/c what Ptr points to has not been free'ed

I am looking at the wording now but I am curious what @aaron.ballman thinks.

[aaron.ballman]

clang/lib/AST/Interp/Descriptor.cpp

46

How I see it is that it's converting Ptr to an IntMap **, dereferencing that back to IntMap * and then freeing *that* pointer. So it doesn't free Ptr itself, but what Ptr points to.

[shafik]

clang/lib/AST/Interp/Descriptor.cpp

46

Right but the address is no longer valid even if the type we free'ed as is different. So the pointer is not longer pointing to a valid location. At least that is how I have seen it explained in other contexts but I have to dig up a reference.

[antondaubert]

clang/lib/AST/Interp/Program.h

54–56

Seems like this change generates a

use-of-uninitialized-value lib/AST/Interp/Descriptor.cpp:150:22 in dtorRecord(clang::interp::Block*, char*, clang::interp::Descriptor*)

when executing the test test/AST/Interp/arrays.cpp with a memory sanitizer.

[tbaeder]

clang/lib/AST/Interp/Program.h

54–56

Should be fixed by https://github.com/llvm/llvm-project/commit/da4b929da79f4ce886f5612e3c84d29c1d2632f7

clang/lib/AST/Interp/Descriptor.cpp

 static void ctorArrayTy(Block *, char *Ptr, bool, bool, bool, Descriptor *D) {
   for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
     new (&reinterpret_cast<T *>(Ptr)[I]) T();
   }
 }
 
 template <typename T>
 static void dtorArrayTy(Block *, char *Ptr, Descriptor *D) {
+  InitMap *IM = *reinterpret_cast<InitMap **>(Ptr);
+  if (IM != (InitMap *)-1)
+    free(IM);
+
+  Ptr += sizeof(InitMap *);
   for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
     reinterpret_cast<T *>(Ptr)[I].~T();
   }
 }
 
 template <typename T>
 static void moveArrayTy(Block *, char *Src, char *Dst, Descriptor *D) {
   for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
static BlockMoveFn getMovePrim(PrimType Type) {
   COMPOSITE_TYPE_SWITCH(Type, return moveTy<T>, return nullptr);
 }
 
 static BlockCtorFn getCtorArrayPrim(PrimType Type) {
   COMPOSITE_TYPE_SWITCH(Type, return ctorArrayTy<T>, return nullptr);
 }
 
 static BlockDtorFn getDtorArrayPrim(PrimType Type) {
-  COMPOSITE_TYPE_SWITCH(Type, return dtorArrayTy<T>, return nullptr);
+  TYPE_SWITCH(Type, return dtorArrayTy<T>);
 }
 
 static BlockMoveFn getMoveArrayPrim(PrimType Type) {
   COMPOSITE_TYPE_SWITCH(Type, return moveArrayTy<T>, return nullptr);
 }
 
 Descriptor::Descriptor(const DeclTy &D, PrimType Type, bool IsConst,
                        bool IsTemporary, bool IsMutable)

clang/lib/AST/Interp/InterpBlock.h

public:
   /// Invokes the constructor.
   void invokeCtor() {
     std::memset(data(), 0, getSize());
     if (Desc->CtorFn)
       Desc->CtorFn(this, data(), Desc->IsConst, Desc->IsMutable,
                    /*isActive=*/true, Desc);
   }
 
+  // Invokes the Destructor.
+  void invokeDtor() {
+    if (Desc->DtorFn)
+      Desc->DtorFn(this, data(), Desc);
+  }
+
 protected:
   friend class Pointer;
   friend class DeadBlock;
   friend class InterpState;
 
   Block(Descriptor *Desc, bool IsExtern, bool IsStatic, bool IsDead)
     : IsStatic(IsStatic), IsExtern(IsExtern), IsDead(true), Desc(Desc) {}
 

clang/lib/AST/Interp/Pointer.cpp

APValue Pointer::toAPValue() const {
   return APValue(Base, Offset, Path, IsOnePastEnd, IsNullPtr);
 }
 
 bool Pointer::isInitialized() const {
   assert(Pointee && "Cannot check if null pointer was initialized");
   Descriptor *Desc = getFieldDesc();
   assert(Desc);
   if (Desc->isPrimitiveArray()) {
-    if (Pointee->IsStatic)
+    if (isStatic() && Base == 0)
       return true;
     // Primitive array field are stored in a bitset.
     InitMap *Map = getInitMap();
     if (!Map)
       return false;
     if (Map == (InitMap *)-1)
       return true;
     return Map->isInitialized(getIndex());
   } else {
     // Field has its bit in an inline descriptor.
     return Base == 0 || getInlineDesc()->IsInitialized;
   }
 }
 
 void Pointer::initialize() const {
   assert(Pointee && "Cannot initialize null pointer");
   Descriptor *Desc = getFieldDesc();
 
   assert(Desc);
   if (Desc->isArray()) {
-    if (Desc->isPrimitiveArray() && !Pointee->IsStatic) {
+    if (Desc->isPrimitiveArray()) {
+      // Primitive global arrays don't have an initmap.
+      if (isStatic() && Base == 0)
+        return;
+
       // Primitive array initializer.
       InitMap *&Map = getInitMap();
       if (Map == (InitMap *)-1)
         return;
       if (Map == nullptr)
         Map = InitMap::allocate(Desc->getNumElems());
       if (Map->initialize(getIndex())) {
         free(Map);

clang/lib/AST/Interp/Program.h

public:
   Program(Context &Ctx) : Ctx(Ctx) {}
 
   ~Program() {
     // Records might actually allocate memory themselves, but they
     // are allocated using a BumpPtrAllocator. Call their desctructors
     // here manually so they are properly freeing their resources.
     for (auto RecordPair : Records)
       RecordPair.second->~Record();
+
+    // Manually destroy all the blocks. They are almost all harmless,
+    // but primitive arrays might have an InitMap* heap allocated and
+    // that needs to be freed.
+    for (Global *G : Globals) {
+      G->block()->invokeDtor();
+    }
   }
 
   /// Marshals a native pointer to an ID for embedding in bytecode.
   unsigned getOrCreateNativePointer(const void *Ptr);
 
   /// Returns the value of a marshalled native pointer.
   const void *getNativePointer(unsigned Idx);
 

clang/lib/AST/Interp/Program.cpp

Descriptor *Desc = allocateDescriptor(S, CharType, S->getLength() + 1,
                                         /*isMutable=*/false);
 
   // Allocate storage for the string.
   // The byte length does not include the null terminator.
   unsigned I = Globals.size();
   unsigned Sz = Desc->getAllocSize();
   auto *G = new (Allocator, Sz) Global(Desc, /*isStatic=*/true,
                                        /*isExtern=*/false);
+  G->block()->invokeCtor();
   Globals.push_back(G);
 
   // Construct the string in storage.
   const Pointer Ptr(G->block());
   for (unsigned I = 0, N = S->getLength(); I <= N; ++I) {
     Pointer Field = Ptr.atIndex(I).narrow();
     const uint32_t CodePoint = I == N ? 0 : S->getCodeUnit(I);
     switch (CharType) {