[clang] 3671a69 - [clang][Interp] Call destructors of local variables

Timm Bäder via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 23 21:27:27 PDT 2023


Author: Timm Bäder
Date: 2023-10-24T06:27:10+02:00
New Revision: 3671a69470f3fa708926a9e5ce579751c6b51dac

URL: https://github.com/llvm/llvm-project/commit/3671a69470f3fa708926a9e5ce579751c6b51dac
DIFF: https://github.com/llvm/llvm-project/commit/3671a69470f3fa708926a9e5ce579751c6b51dac.diff

LOG: [clang][Interp] Call destructors of local variables

This prevents us from leaking memory if the interpretation of byte code
fails midway-throug.

Differential Revision: https://reviews.llvm.org/D154581

Added: 
    

Modified: 
    clang/lib/AST/Interp/Descriptor.cpp
    clang/lib/AST/Interp/Descriptor.h
    clang/lib/AST/Interp/EvalEmitter.cpp
    clang/lib/AST/Interp/EvalEmitter.h
    clang/lib/AST/Interp/InterpBlock.h
    clang/lib/AST/Interp/InterpFrame.cpp
    clang/lib/AST/Interp/InterpState.cpp
    clang/lib/AST/Interp/Pointer.cpp
    clang/lib/AST/Interp/Pointer.h
    clang/test/AST/Interp/arrays.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/Interp/Descriptor.cpp b/clang/lib/AST/Interp/Descriptor.cpp
index 3990282686fe3d6..abd75a8d67bbd3f 100644
--- a/clang/lib/AST/Interp/Descriptor.cpp
+++ b/clang/lib/AST/Interp/Descriptor.cpp
@@ -40,6 +40,9 @@ static void moveTy(Block *, const std::byte *Src, std::byte *Dst,
 template <typename T>
 static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool,
                         const Descriptor *D) {
+  new (Ptr) InitMapPtr(std::nullopt);
+
+  Ptr += sizeof(InitMapPtr);
   for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
     new (&reinterpret_cast<T *>(Ptr)[I]) T();
   }
@@ -47,11 +50,11 @@ static void ctorArrayTy(Block *, std::byte *Ptr, bool, bool, bool,
 
 template <typename T>
 static void dtorArrayTy(Block *, std::byte *Ptr, const Descriptor *D) {
-  InitMap *IM = *reinterpret_cast<InitMap **>(Ptr);
-  if (IM != (InitMap *)-1)
-    free(IM);
+  InitMapPtr &IMP = *reinterpret_cast<InitMapPtr *>(Ptr);
 
-  Ptr += sizeof(InitMap *);
+  if (IMP)
+    IMP = std::nullopt;
+  Ptr += sizeof(InitMapPtr);
   for (unsigned I = 0, NE = D->getNumElems(); I < NE; ++I) {
     reinterpret_cast<T *>(Ptr)[I].~T();
   }
@@ -209,7 +212,8 @@ static BlockMoveFn getMovePrim(PrimType Type) {
 }
 
 static BlockCtorFn getCtorArrayPrim(PrimType Type) {
-  COMPOSITE_TYPE_SWITCH(Type, return ctorArrayTy<T>, return nullptr);
+  TYPE_SWITCH(Type, return ctorArrayTy<T>);
+  llvm_unreachable("unknown Expr");
 }
 
 static BlockDtorFn getDtorArrayPrim(PrimType Type) {
@@ -218,7 +222,8 @@ static BlockDtorFn getDtorArrayPrim(PrimType Type) {
 }
 
 static BlockMoveFn getMoveArrayPrim(PrimType Type) {
-  COMPOSITE_TYPE_SWITCH(Type, return moveArrayTy<T>, return nullptr);
+  TYPE_SWITCH(Type, return moveArrayTy<T>);
+  llvm_unreachable("unknown Expr");
 }
 
 /// Primitives.
@@ -238,7 +243,7 @@ Descriptor::Descriptor(const DeclTy &D, PrimType Type, MetadataSize MD,
                        bool IsMutable)
     : Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
       MDSize(MD.value_or(0)),
-      AllocSize(align(Size) + sizeof(InitMap *) + MDSize), IsConst(IsConst),
+      AllocSize(align(Size) + sizeof(InitMapPtr) + MDSize), IsConst(IsConst),
       IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
       CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
       MoveFn(getMoveArrayPrim(Type)) {
@@ -249,9 +254,10 @@ Descriptor::Descriptor(const DeclTy &D, PrimType Type, MetadataSize MD,
 Descriptor::Descriptor(const DeclTy &D, PrimType Type, bool IsTemporary,
                        UnknownSize)
     : Source(D), ElemSize(primSize(Type)), Size(UnknownSizeMark), MDSize(0),
-      AllocSize(alignof(void *)), IsConst(true), IsMutable(false),
-      IsTemporary(IsTemporary), IsArray(true), CtorFn(getCtorArrayPrim(Type)),
-      DtorFn(getDtorArrayPrim(Type)), MoveFn(getMoveArrayPrim(Type)) {
+      AllocSize(alignof(void *) + sizeof(InitMapPtr)), IsConst(true),
+      IsMutable(false), IsTemporary(IsTemporary), IsArray(true),
+      CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
+      MoveFn(getMoveArrayPrim(Type)) {
   assert(Source && "Missing source");
 }
 
@@ -272,10 +278,10 @@ Descriptor::Descriptor(const DeclTy &D, Descriptor *Elem, MetadataSize MD,
 Descriptor::Descriptor(const DeclTy &D, Descriptor *Elem, bool IsTemporary,
                        UnknownSize)
     : Source(D), ElemSize(Elem->getAllocSize() + sizeof(InlineDescriptor)),
-      Size(UnknownSizeMark), MDSize(0), AllocSize(alignof(void *)),
-      ElemDesc(Elem), IsConst(true), IsMutable(false), IsTemporary(IsTemporary),
-      IsArray(true), CtorFn(ctorArrayDesc), DtorFn(dtorArrayDesc),
-      MoveFn(moveArrayDesc) {
+      Size(UnknownSizeMark), MDSize(0),
+      AllocSize(alignof(void *) + sizeof(InitMapPtr)), ElemDesc(Elem),
+      IsConst(true), IsMutable(false), IsTemporary(IsTemporary), IsArray(true),
+      CtorFn(ctorArrayDesc), DtorFn(dtorArrayDesc), MoveFn(moveArrayDesc) {
   assert(Source && "Missing source");
 }
 
@@ -314,21 +320,12 @@ SourceLocation Descriptor::getLocation() const {
   llvm_unreachable("Invalid descriptor type");
 }
 
-InitMap::InitMap(unsigned N) : UninitFields(N) {
-  std::fill_n(data(), (N + PER_FIELD - 1) / PER_FIELD, 0);
+InitMap::InitMap(unsigned N)
+    : UninitFields(N), Data(std::make_unique<T[]>(numFields(N))) {
+  std::fill_n(data(), numFields(N), 0);
 }
 
-InitMap::T *InitMap::data() {
-  auto *Start = reinterpret_cast<char *>(this) + align(sizeof(InitMap));
-  return reinterpret_cast<T *>(Start);
-}
-
-const InitMap::T *InitMap::data() const {
-  auto *Start = reinterpret_cast<const char *>(this) + align(sizeof(InitMap));
-  return reinterpret_cast<const T *>(Start);
-}
-
-bool InitMap::initialize(unsigned I) {
+bool InitMap::initializeElement(unsigned I) {
   unsigned Bucket = I / PER_FIELD;
   T Mask = T(1) << (I % PER_FIELD);
   if (!(data()[Bucket] & Mask)) {
@@ -338,13 +335,7 @@ bool InitMap::initialize(unsigned I) {
   return UninitFields == 0;
 }
 
-bool InitMap::isInitialized(unsigned I) const {
+bool InitMap::isElementInitialized(unsigned I) const {
   unsigned Bucket = I / PER_FIELD;
   return data()[Bucket] & (T(1) << (I % PER_FIELD));
 }
-
-InitMap *InitMap::allocate(unsigned N) {
-  const size_t NumFields = ((N + PER_FIELD - 1) / PER_FIELD);
-  const size_t Size = align(sizeof(InitMap)) + NumFields * PER_FIELD;
-  return new (malloc(Size)) InitMap(N);
-}

diff  --git a/clang/lib/AST/Interp/Descriptor.h b/clang/lib/AST/Interp/Descriptor.h
index 55a754c3505cce7..be9a380138a7b11 100644
--- a/clang/lib/AST/Interp/Descriptor.h
+++ b/clang/lib/AST/Interp/Descriptor.h
@@ -20,10 +20,12 @@ namespace clang {
 namespace interp {
 class Block;
 class Record;
+struct InitMap;
 struct Descriptor;
 enum PrimType : unsigned;
 
 using DeclTy = llvm::PointerUnion<const Decl *, const Expr *>;
+using InitMapPtr = std::optional<std::pair<bool, std::shared_ptr<InitMap>>>;
 
 /// Invoked whenever a block is created. The constructor method fills in the
 /// inline descriptors of all fields and array elements. It also initializes
@@ -193,9 +195,6 @@ struct Descriptor final {
 };
 
 /// Bitfield tracking the initialisation status of elements of primitive arrays.
-/// A pointer to this is embedded at the end of all primitive arrays.
-/// If the map was not yet created and nothing was initialized, the pointer to
-/// this structure is 0. If the object was fully initialized, the pointer is -1.
 struct InitMap final {
 private:
   /// Type packing bits.
@@ -203,26 +202,29 @@ struct InitMap final {
   /// Bits stored in a single field.
   static constexpr uint64_t PER_FIELD = sizeof(T) * CHAR_BIT;
 
+public:
   /// Initializes the map with no fields set.
-  InitMap(unsigned N);
+  explicit InitMap(unsigned N);
+
+private:
+  friend class Pointer;
 
   /// Returns a pointer to storage.
-  T *data();
-  const T *data() const;
+  T *data() { return Data.get(); }
+  const T *data() const { return Data.get(); }
 
-public:
   /// Initializes an element. Returns true when object if fully initialized.
-  bool initialize(unsigned I);
+  bool initializeElement(unsigned I);
 
   /// Checks if an element was initialized.
-  bool isInitialized(unsigned I) const;
+  bool isElementInitialized(unsigned I) const;
 
-  /// Allocates a map holding N elements.
-  static InitMap *allocate(unsigned N);
-
-private:
-  /// Number of fields initialized.
+  static constexpr size_t numFields(unsigned N) {
+    return (N + PER_FIELD - 1) / PER_FIELD;
+  }
+  /// Number of fields not initialized.
   unsigned UninitFields;
+  std::unique_ptr<T[]> Data;
 };
 
 } // namespace interp

diff  --git a/clang/lib/AST/Interp/EvalEmitter.cpp b/clang/lib/AST/Interp/EvalEmitter.cpp
index f46ef1067cf52a0..f8942291b3b162d 100644
--- a/clang/lib/AST/Interp/EvalEmitter.cpp
+++ b/clang/lib/AST/Interp/EvalEmitter.cpp
@@ -25,6 +25,14 @@ EvalEmitter::EvalEmitter(Context &Ctx, Program &P, State &Parent,
       new InterpFrame(S, /*Func=*/nullptr, /*Caller=*/nullptr, CodePtr());
 }
 
+EvalEmitter::~EvalEmitter() {
+  for (auto &[K, V] : Locals) {
+    Block *B = reinterpret_cast<Block *>(V.get());
+    if (B->isInitialized())
+      B->invokeDtor();
+  }
+}
+
 llvm::Expected<bool> EvalEmitter::interpretExpr(const Expr *E) {
   if (this->visitExpr(E))
     return true;

diff  --git a/clang/lib/AST/Interp/EvalEmitter.h b/clang/lib/AST/Interp/EvalEmitter.h
index 1128e0fb09f0de8..5a9be18c34a03bc 100644
--- a/clang/lib/AST/Interp/EvalEmitter.h
+++ b/clang/lib/AST/Interp/EvalEmitter.h
@@ -40,7 +40,7 @@ class EvalEmitter : public SourceMapper {
   EvalEmitter(Context &Ctx, Program &P, State &Parent, InterpStack &Stk,
               APValue &Result);
 
-  virtual ~EvalEmitter() {}
+  virtual ~EvalEmitter();
 
   /// Define a label.
   void emitLabel(LabelTy Label);

diff  --git a/clang/lib/AST/Interp/InterpBlock.h b/clang/lib/AST/Interp/InterpBlock.h
index 5112108e6f010a6..89f937b588fb678 100644
--- a/clang/lib/AST/Interp/InterpBlock.h
+++ b/clang/lib/AST/Interp/InterpBlock.h
@@ -71,6 +71,7 @@ class Block final {
   unsigned getSize() const { return Desc->getAllocSize(); }
   /// Returns the declaration ID.
   std::optional<unsigned> getDeclID() const { return DeclID; }
+  bool isInitialized() const { return IsInitialized; }
 
   /// Returns a pointer to the stored data.
   /// You are allowed to read Desc->getSize() bytes from this address.
@@ -104,12 +105,14 @@ class Block final {
     if (Desc->CtorFn)
       Desc->CtorFn(this, data(), Desc->IsConst, Desc->IsMutable,
                    /*isActive=*/true, Desc);
+    IsInitialized = true;
   }
 
   /// Invokes the Destructor.
   void invokeDtor() {
     if (Desc->DtorFn)
       Desc->DtorFn(this, data(), Desc);
+    IsInitialized = false;
   }
 
 protected:
@@ -139,8 +142,12 @@ class Block final {
   bool IsStatic = false;
   /// Flag indicating if the block is an extern.
   bool IsExtern = false;
-  /// Flag indicating if the pointer is dead.
+  /// Flag indicating if the pointer is dead. This is only ever
+  /// set once, when converting the Block to a DeadBlock.
   bool IsDead = false;
+  /// Flag indicating if the block contents have been initialized
+  /// via invokeCtor.
+  bool IsInitialized = false;
   /// Pointer to the stack slot descriptor.
   Descriptor *Desc;
 };

diff  --git a/clang/lib/AST/Interp/InterpFrame.cpp b/clang/lib/AST/Interp/InterpFrame.cpp
index 49db83a965ab001..b06923114c7a24e 100644
--- a/clang/lib/AST/Interp/InterpFrame.cpp
+++ b/clang/lib/AST/Interp/InterpFrame.cpp
@@ -72,6 +72,19 @@ InterpFrame::InterpFrame(InterpState &S, const Function *Func, CodePtr RetPC)
 InterpFrame::~InterpFrame() {
   for (auto &Param : Params)
     S.deallocate(reinterpret_cast<Block *>(Param.second.get()));
+
+  // When destroying the InterpFrame, call the Dtor for all block
+  // that haven't been destroyed via a destroy() op yet.
+  // This happens when the execution is interruped midway-through.
+  if (Func) {
+    for (auto &Scope : Func->scopes()) {
+      for (auto &Local : Scope.locals()) {
+        Block *B = localBlock(Local.Offset);
+        if (B->isInitialized())
+          B->invokeDtor();
+      }
+    }
+  }
 }
 
 void InterpFrame::destroy(unsigned Idx) {

diff  --git a/clang/lib/AST/Interp/InterpState.cpp b/clang/lib/AST/Interp/InterpState.cpp
index 4f050baea39e79a..2cb87ef07fe5882 100644
--- a/clang/lib/AST/Interp/InterpState.cpp
+++ b/clang/lib/AST/Interp/InterpState.cpp
@@ -65,9 +65,9 @@ void InterpState::deallocate(Block *B) {
         std::memcpy(D->rawData(), B->rawData(), Desc->getMetadataSize());
     }
 
+    // We moved the contents over to the DeadBlock.
+    B->IsInitialized = false;
   } else {
-    // Free storage, if necessary.
-    if (Desc->DtorFn)
-      Desc->DtorFn(B, B->data(), Desc);
+    B->invokeDtor();
   }
 }

diff  --git a/clang/lib/AST/Interp/Pointer.cpp b/clang/lib/AST/Interp/Pointer.cpp
index e04d29b8bd81be4..d1af58203bec64d 100644
--- a/clang/lib/AST/Interp/Pointer.cpp
+++ b/clang/lib/AST/Interp/Pointer.cpp
@@ -164,13 +164,16 @@ bool Pointer::isInitialized() const {
   if (Desc->isPrimitiveArray()) {
     if (isStatic() && Base == 0)
       return true;
-    // Primitive array field are stored in a bitset.
-    InitMap *Map = getInitMap();
-    if (!Map)
+
+    InitMapPtr &IM = getInitMap();
+
+    if (!IM)
       return false;
-    if (Map == (InitMap *)-1)
+
+    if (IM->first)
       return true;
-    return Map->isInitialized(getIndex());
+
+    return IM->second->isElementInitialized(getIndex());
   }
 
   // Field has its bit in an inline descriptor.
@@ -187,15 +190,20 @@ void Pointer::initialize() const {
     if (isStatic() && Base == 0)
       return;
 
-    // Primitive array initializer.
-    InitMap *&Map = getInitMap();
-    if (Map == (InitMap *)-1)
+    InitMapPtr &IM = getInitMap();
+    if (!IM)
+      IM =
+          std::make_pair(false, std::make_shared<InitMap>(Desc->getNumElems()));
+
+    assert(IM);
+
+    // All initialized.
+    if (IM->first)
       return;
-    if (Map == nullptr)
-      Map = InitMap::allocate(Desc->getNumElems());
-    if (Map->initialize(getIndex())) {
-      free(Map);
-      Map = (InitMap *)-1;
+
+    if (IM->second->initializeElement(getIndex())) {
+      IM->first = true;
+      IM->second.reset();
     }
     return;
   }

diff  --git a/clang/lib/AST/Interp/Pointer.h b/clang/lib/AST/Interp/Pointer.h
index d5279e757f04764..3b21290332a9d2f 100644
--- a/clang/lib/AST/Interp/Pointer.h
+++ b/clang/lib/AST/Interp/Pointer.h
@@ -108,7 +108,7 @@ class Pointer {
     if (getFieldDesc()->ElemDesc)
       Off += sizeof(InlineDescriptor);
     else
-      Off += sizeof(InitMap *);
+      Off += sizeof(InitMapPtr);
     return Pointer(Pointee, Base, Base + Off);
   }
 
@@ -147,7 +147,7 @@ class Pointer {
     if (inPrimitiveArray()) {
       if (Offset != Base)
         return *this;
-      return Pointer(Pointee, Base, Offset + sizeof(InitMap *));
+      return Pointer(Pointee, Base, Offset + sizeof(InitMapPtr));
     }
 
     // Pointer is to a field or array element - enter it.
@@ -168,7 +168,7 @@ class Pointer {
       // Revert to an outer one-past-end pointer.
       unsigned Adjust;
       if (inPrimitiveArray())
-        Adjust = sizeof(InitMap *);
+        Adjust = sizeof(InitMapPtr);
       else
         Adjust = sizeof(InlineDescriptor);
       return Pointer(Pointee, Base, Base + getSize() + Adjust);
@@ -257,7 +257,7 @@ class Pointer {
       if (getFieldDesc()->ElemDesc)
         Adjust = sizeof(InlineDescriptor);
       else
-        Adjust = sizeof(InitMap *);
+        Adjust = sizeof(InitMapPtr);
     }
     return Offset - Base - Adjust;
   }
@@ -359,7 +359,7 @@ class Pointer {
     assert(isLive() && "Invalid pointer");
     if (isArrayRoot())
       return *reinterpret_cast<T *>(Pointee->rawData() + Base +
-                                    sizeof(InitMap *));
+                                    sizeof(InitMapPtr));
 
     return *reinterpret_cast<T *>(Pointee->rawData() + Offset);
   }
@@ -367,7 +367,7 @@ class Pointer {
   /// Dereferences a primitive element.
   template <typename T> T &elem(unsigned I) const {
     assert(I < getNumElems());
-    return reinterpret_cast<T *>(Pointee->data() + sizeof(InitMap *))[I];
+    return reinterpret_cast<T *>(Pointee->data() + sizeof(InitMapPtr))[I];
   }
 
   /// Initializes a field.
@@ -418,6 +418,7 @@ class Pointer {
 private:
   friend class Block;
   friend class DeadBlock;
+  friend struct InitMap;
 
   Pointer(Block *Pointee, unsigned Base, unsigned Offset);
 
@@ -431,9 +432,9 @@ class Pointer {
            1;
   }
 
-  /// Returns a reference to the pointer which stores the initialization map.
-  InitMap *&getInitMap() const {
-    return *reinterpret_cast<InitMap **>(Pointee->rawData() + Base);
+  /// Returns a reference to the InitMapPtr which stores the initialization map.
+  InitMapPtr &getInitMap() const {
+    return *reinterpret_cast<InitMapPtr *>(Pointee->rawData() + Base);
   }
 
   /// The block the pointer is pointing to.

diff  --git a/clang/test/AST/Interp/arrays.cpp b/clang/test/AST/Interp/arrays.cpp
index ba29632e97c30a6..7110785ea4c662a 100644
--- a/clang/test/AST/Interp/arrays.cpp
+++ b/clang/test/AST/Interp/arrays.cpp
@@ -406,3 +406,52 @@ namespace StringZeroFill {
   static_assert(b[4] == '\0', "");
   static_assert(b[5] == '\0', "");
 }
+
+namespace NoInitMapLeak {
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdivision-by-zero"
+#pragma clang diagnostic ignored "-Wc++20-extensions"
+  constexpr int testLeak() { // expected-error {{never produces a constant expression}} \
+                             // ref-error {{never produces a constant expression}}
+    int a[2];
+    a[0] = 1;
+    // interrupts interpretation.
+    (void)(1 / 0); // expected-note 2{{division by zero}} \
+                   // ref-note 2{{division by zero}}
+
+
+    return 1;
+  }
+#pragma clang diagnostic pop
+  static_assert(testLeak() == 1, ""); // expected-error {{not an integral constant expression}} \
+                                      // expected-note {{in call to 'testLeak()'}} \
+                                      // ref-error {{not an integral constant expression}} \
+                                      // ref-note {{in call to 'testLeak()'}}
+
+
+  constexpr int a[] = {1,2,3,4/0,5}; // expected-error {{must be initialized by a constant expression}} \
+                                     // expected-note {{division by zero}} \
+                                     // ref-error {{must be initialized by a constant expression}} \
+                                     // ref-note {{division by zero}} \
+                                     // ref-note {{declared here}}
+
+  /// FIXME: This should fail in the new interpreter as well.
+  constexpr int b = a[0]; // ref-error {{must be initialized by a constant expression}} \
+                          // ref-note {{is not a constant expression}} \
+                          // ref-note {{declared here}}
+  static_assert(b == 1, ""); // ref-error {{not an integral constant expression}} \
+                             // ref-note {{not a constant expression}}
+
+  constexpr int f() { // expected-error {{never produces a constant expression}} \
+                      // ref-error {{never produces a constant expression}}
+    int a[] = {19,2,3/0,4}; // expected-note 2{{division by zero}} \
+                            // expected-warning {{is undefined}} \
+                            // ref-note 2{{division by zero}} \
+                            // ref-warning {{is undefined}}
+    return 1;
+  }
+  static_assert(f() == 1, ""); // expected-error {{not an integral constant expression}} \
+                               // expected-note {{in call to}} \
+                               // ref-error {{not an integral constant expression}} \
+                               // ref-note {{in call to}}
+}


        


More information about the cfe-commits mailing list