[clang] fc65a96 - [clang][Interp] Run record destructors when deallocating dynamic memory

[Timm Bäder via cfe-commits]

Author: Timm Bäder
Date: 2024-07-18T15:07:29+02:00
New Revision: fc65a9603bf16ed1fe98fbee6933bca9e2083384

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

LOG: [clang][Interp] Run record destructors when deallocating dynamic memory

Added: 
    

Modified: 
    clang/lib/AST/Interp/Interp.cpp
    clang/lib/AST/Interp/Interp.h
    clang/test/AST/Interp/new-delete.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/AST/Interp/Interp.cpp b/clang/lib/AST/Interp/Interp.cpp
index fb63228f8aea8..2be9b5360d055 100644
--- a/clang/lib/AST/Interp/Interp.cpp
+++ b/clang/lib/AST/Interp/Interp.cpp
@@ -819,6 +819,81 @@ bool CheckNonNullArgs(InterpState &S, CodePtr OpPC, const Function *F,
   return true;
 }
 
+// FIXME: This is similar to code we already have in Compiler.cpp.
+// I think it makes sense to instead add the field and base destruction stuff
+// to the destructor Function itself. Then destroying a record would really
+// _just_ be calling its destructor. That would also help with the diagnostic
+// 
diff erence when the destructor or a field/base fails.
+static bool runRecordDestructor(InterpState &S, CodePtr OpPC,
+                                const Pointer &BasePtr,
+                                const Descriptor *Desc) {
+  assert(Desc->isRecord());
+  const Record *R = Desc->ElemRecord;
+  assert(R);
+
+  // Fields.
+  for (const Record::Field &Field : llvm::reverse(R->fields())) {
+    const Descriptor *D = Field.Desc;
+    if (D->isRecord()) {
+      if (!runRecordDestructor(S, OpPC, BasePtr.atField(Field.Offset), D))
+        return false;
+    } else if (D->isCompositeArray()) {
+      const Descriptor *ElemDesc = Desc->ElemDesc;
+      assert(ElemDesc->isRecord());
+      for (unsigned I = 0; I != Desc->getNumElems(); ++I) {
+        if (!runRecordDestructor(S, OpPC, BasePtr.atIndex(I).narrow(),
+                                 ElemDesc))
+          return false;
+      }
+    }
+  }
+
+  // Destructor of this record.
+  if (const CXXDestructorDecl *Dtor = R->getDestructor();
+      Dtor && !Dtor->isTrivial()) {
+    const Function *DtorFunc = S.getContext().getOrCreateFunction(Dtor);
+    if (!DtorFunc)
+      return false;
+
+    S.Stk.push<Pointer>(BasePtr);
+    if (!Call(S, OpPC, DtorFunc, 0))
+      return false;
+  }
+
+  // Bases.
+  for (const Record::Base &Base : llvm::reverse(R->bases())) {
+    if (!runRecordDestructor(S, OpPC, BasePtr.atField(Base.Offset), Base.Desc))
+      return false;
+  }
+
+  return true;
+}
+
+bool RunDestructors(InterpState &S, CodePtr OpPC, const Block *B) {
+  assert(B);
+  const Descriptor *Desc = B->getDescriptor();
+
+  if (Desc->isPrimitive() || Desc->isPrimitiveArray())
+    return true;
+
+  assert(Desc->isRecord() || Desc->isCompositeArray());
+
+  if (Desc->isCompositeArray()) {
+    const Descriptor *ElemDesc = Desc->ElemDesc;
+    assert(ElemDesc->isRecord());
+
+    Pointer RP(const_cast<Block *>(B));
+    for (unsigned I = 0; I != Desc->getNumElems(); ++I) {
+      if (!runRecordDestructor(S, OpPC, RP.atIndex(I).narrow(), ElemDesc))
+        return false;
+    }
+    return true;
+  }
+
+  assert(Desc->isRecord());
+  return runRecordDestructor(S, OpPC, Pointer(const_cast<Block *>(B)), Desc);
+}
+
 bool Interpret(InterpState &S, APValue &Result) {
   // The current stack frame when we started Interpret().
   // This is being used by the ops to determine wheter

diff  --git a/clang/lib/AST/Interp/Interp.h b/clang/lib/AST/Interp/Interp.h
index b4f8c03280c85..17b3157cb40a9 100644
--- a/clang/lib/AST/Interp/Interp.h
+++ b/clang/lib/AST/Interp/Interp.h
@@ -2872,8 +2872,8 @@ inline bool AllocCN(InterpState &S, CodePtr OpPC, const Descriptor *ElementDesc,
   return true;
 }
 
+bool RunDestructors(InterpState &S, CodePtr OpPC, const Block *B);
 static inline bool Free(InterpState &S, CodePtr OpPC, bool DeleteIsArrayForm) {
-
   if (!CheckDynamicMemoryAllocation(S, OpPC))
     return false;
 
@@ -2904,6 +2904,10 @@ static inline bool Free(InterpState &S, CodePtr OpPC, bool DeleteIsArrayForm) {
   assert(Source);
   assert(BlockToDelete);
 
+  // Invoke destructors before deallocating the memory.
+  if (!RunDestructors(S, OpPC, BlockToDelete))
+    return false;
+
   DynamicAllocator &Allocator = S.getAllocator();
   bool WasArrayAlloc = Allocator.isArrayAllocation(Source);
   const Descriptor *BlockDesc = BlockToDelete->getDescriptor();

diff  --git a/clang/test/AST/Interp/new-delete.cpp b/clang/test/AST/Interp/new-delete.cpp
index 04ce3ae5f6637..cb46426c0e3be 100644
--- a/clang/test/AST/Interp/new-delete.cpp
+++ b/clang/test/AST/Interp/new-delete.cpp
@@ -476,7 +476,80 @@ constexpr Sp ss[] = {Sp{new int{154}}}; // both-error {{must be initialized by a
                                         // both-note {{pointer to heap-allocated object}} \
                                         // both-note {{allocation performed here}}
 
+namespace DeleteRunsDtors {
+  struct InnerFoo {
+    int *mem;
+    constexpr ~InnerFoo() {
+      delete mem;
+    }
+  };
+
+  struct Foo {
+    int *a;
+    InnerFoo IF;
+
+    constexpr Foo() {
+      a = new int(13);
+      IF.mem = new int(100);
+    }
+    constexpr ~Foo() { delete a; }
+  };
+
+  constexpr int abc() {
+    Foo *F = new Foo();
+    int n = *F->a;
+    delete F;
+
+    return n;
+  }
+  static_assert(abc() == 13);
+
+  constexpr int abc2() {
+    Foo *f = new Foo[3];
+
+    delete[] f;
+
+    return 1;
+  }
+  static_assert(abc2() == 1);
+}
+
+/// FIXME: There is a slight 
diff erence in diagnostics here, because we don't
+/// create a new frame when we delete record fields or bases at all.
+namespace FaultyDtorCalledByDelete {
+  struct InnerFoo {
+    int *mem;
+    constexpr ~InnerFoo() {
+      if (mem) {
+        (void)(1/0); // both-warning {{division by zero is undefined}} \
+                     // both-note {{division by zero}}
+      }
+      delete mem;
+    }
+  };
+
+  struct Foo {
+    int *a;
+    InnerFoo IF;
 
+    constexpr Foo() {
+      a = new int(13);
+      IF.mem = new int(100);
+    }
+    constexpr ~Foo() { delete a; }
+  };
+
+  constexpr int abc() {
+    Foo *F = new Foo();
+    int n = *F->a;
+    delete F; // both-note {{in call to}} \
+              // ref-note {{in call to}}
+
+    return n;
+  }
+  static_assert(abc() == 13); // both-error {{not an integral constant expression}} \
+                              // both-note {{in call to 'abc()'}}
+}
 
 
 #else