[clang][bytecode] Diagnose class-specific operator delete calls

[tbaederr]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/111700.diff

5 Files Affected:

• (modified) clang/lib/AST/ByteCode/Compiler.cpp (+1-1)
• (modified) clang/lib/AST/ByteCode/Interp.cpp (+84-1)
• (modified) clang/lib/AST/ByteCode/Interp.h (+2-55)
• (modified) clang/lib/AST/ByteCode/Opcodes.td (+1-1)
• (modified) clang/test/AST/ByteCode/new-delete.cpp (+24-5)

diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index f270de1054c61b..fe44238ea11869 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -3406,7 +3406,7 @@ bool Compiler<Emitter>::VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
   if (!this->visit(Arg))
     return false;
 
-  return this->emitFree(E->isArrayForm(), E);
+  return this->emitFree(E->isArrayForm(), E->isGlobalDelete(), E);
 }
 
 template <class Emitter>
diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp
index 050de67c2e77dd..9f26f54c1abdc1 100644
--- a/clang/lib/AST/ByteCode/Interp.cpp
+++ b/clang/lib/AST/ByteCode/Interp.cpp
@@ -963,7 +963,7 @@ static bool runRecordDestructor(InterpState &S, CodePtr OpPC,
   return true;
 }
 
-bool RunDestructors(InterpState &S, CodePtr OpPC, const Block *B) {
+static bool RunDestructors(InterpState &S, CodePtr OpPC, const Block *B) {
   assert(B);
   const Descriptor *Desc = B->getDescriptor();
 
@@ -988,6 +988,89 @@ bool RunDestructors(InterpState &S, CodePtr OpPC, const Block *B) {
   return runRecordDestructor(S, OpPC, Pointer(const_cast<Block *>(B)), Desc);
 }
 
+bool Free(InterpState &S, CodePtr OpPC, bool DeleteIsArrayForm,
+          bool IsGlobalDelete) {
+  if (!CheckDynamicMemoryAllocation(S, OpPC))
+    return false;
+
+  const Expr *Source = nullptr;
+  const Block *BlockToDelete = nullptr;
+  {
+    // Extra scope for this so the block doesn't have this pointer
+    // pointing to it when we destroy it.
+    Pointer Ptr = S.Stk.pop<Pointer>();
+
+    // Deleteing nullptr is always fine.
+    if (Ptr.isZero())
+      return true;
+
+    // Remove base casts.
+    while (Ptr.isBaseClass())
+      Ptr = Ptr.getBase();
+
+    if (!Ptr.isRoot() || Ptr.isOnePastEnd() || Ptr.isArrayElement()) {
+      const SourceInfo &Loc = S.Current->getSource(OpPC);
+      S.FFDiag(Loc, diag::note_constexpr_delete_subobject)
+          << Ptr.toDiagnosticString(S.getASTContext()) << Ptr.isOnePastEnd();
+      return false;
+    }
+
+    Source = Ptr.getDeclDesc()->asExpr();
+    BlockToDelete = Ptr.block();
+
+    if (!CheckDeleteSource(S, OpPC, Source, Ptr))
+      return false;
+
+    // For a class type with a virtual destructor, the selected operator delete
+    // is the one looked up when building the destructor.
+    QualType AllocType = Ptr.getType();
+    if (!DeleteIsArrayForm && !IsGlobalDelete) {
+      auto getVirtualOperatorDelete = [](QualType T) -> const FunctionDecl * {
+        if (const CXXRecordDecl *RD = T->getAsCXXRecordDecl())
+          if (const CXXDestructorDecl *DD = RD->getDestructor())
+            return DD->isVirtual() ? DD->getOperatorDelete() : nullptr;
+        return nullptr;
+      };
+
+      AllocType->dump();
+      if (const FunctionDecl *VirtualDelete =
+              getVirtualOperatorDelete(AllocType);
+          VirtualDelete &&
+          !VirtualDelete->isReplaceableGlobalAllocationFunction()) {
+        S.FFDiag(S.Current->getSource(OpPC),
+                 diag::note_constexpr_new_non_replaceable)
+            << isa<CXXMethodDecl>(VirtualDelete) << VirtualDelete;
+        return false;
+      }
+    }
+  }
+  assert(Source);
+  assert(BlockToDelete);
+
+  // Invoke destructors before deallocating the memory.
+  if (!RunDestructors(S, OpPC, BlockToDelete))
+    return false;
+
+  DynamicAllocator &Allocator = S.getAllocator();
+  const Descriptor *BlockDesc = BlockToDelete->getDescriptor();
+  std::optional<DynamicAllocator::Form> AllocForm =
+      Allocator.getAllocationForm(Source);
+
+  if (!Allocator.deallocate(Source, BlockToDelete, S)) {
+    // Nothing has been deallocated, this must be a double-delete.
+    const SourceInfo &Loc = S.Current->getSource(OpPC);
+    S.FFDiag(Loc, diag::note_constexpr_double_delete);
+    return false;
+  }
+
+  assert(AllocForm);
+  DynamicAllocator::Form DeleteForm = DeleteIsArrayForm
+                                          ? DynamicAllocator::Form::Array
+                                          : DynamicAllocator::Form::NonArray;
+  return CheckNewDeleteForms(S, OpPC, *AllocForm, DeleteForm, BlockDesc,
+                             Source);
+}
+
 void diagnoseEnumValue(InterpState &S, CodePtr OpPC, const EnumDecl *ED,
                        const APSInt &Value) {
   llvm::APInt Min;
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 2c5538d221bf0b..3ebf73213b8cf3 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -3007,61 +3007,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;
-
-  const Expr *Source = nullptr;
-  const Block *BlockToDelete = nullptr;
-  {
-    // Extra scope for this so the block doesn't have this pointer
-    // pointing to it when we destroy it.
-    const Pointer &Ptr = S.Stk.pop<Pointer>();
-
-    // Deleteing nullptr is always fine.
-    if (Ptr.isZero())
-      return true;
-
-    if (!Ptr.isRoot() || Ptr.isOnePastEnd() || Ptr.isArrayElement()) {
-      const SourceInfo &Loc = S.Current->getSource(OpPC);
-      S.FFDiag(Loc, diag::note_constexpr_delete_subobject)
-          << Ptr.toDiagnosticString(S.getASTContext()) << Ptr.isOnePastEnd();
-      return false;
-    }
-
-    Source = Ptr.getDeclDesc()->asExpr();
-    BlockToDelete = Ptr.block();
-
-    if (!CheckDeleteSource(S, OpPC, Source, Ptr))
-      return false;
-  }
-  assert(Source);
-  assert(BlockToDelete);
-
-  // Invoke destructors before deallocating the memory.
-  if (!RunDestructors(S, OpPC, BlockToDelete))
-    return false;
-
-  DynamicAllocator &Allocator = S.getAllocator();
-  const Descriptor *BlockDesc = BlockToDelete->getDescriptor();
-  std::optional<DynamicAllocator::Form> AllocForm =
-      Allocator.getAllocationForm(Source);
-
-  if (!Allocator.deallocate(Source, BlockToDelete, S)) {
-    // Nothing has been deallocated, this must be a double-delete.
-    const SourceInfo &Loc = S.Current->getSource(OpPC);
-    S.FFDiag(Loc, diag::note_constexpr_double_delete);
-    return false;
-  }
-
-  assert(AllocForm);
-  DynamicAllocator::Form DeleteForm = DeleteIsArrayForm
-                                          ? DynamicAllocator::Form::Array
-                                          : DynamicAllocator::Form::NonArray;
-  return CheckNewDeleteForms(S, OpPC, *AllocForm, DeleteForm, BlockDesc,
-                             Source);
-}
+bool Free(InterpState &S, CodePtr OpPC, bool DeleteIsArrayForm,
+          bool IsGlobalDelete);
 
 static inline bool IsConstantContext(InterpState &S, CodePtr OpPC) {
   S.Stk.push<Boolean>(Boolean::from(S.inConstantContext()));
diff --git a/clang/lib/AST/ByteCode/Opcodes.td b/clang/lib/AST/ByteCode/Opcodes.td
index 7b65138e5a3c94..4fa9b6d61d5ab9 100644
--- a/clang/lib/AST/ByteCode/Opcodes.td
+++ b/clang/lib/AST/ByteCode/Opcodes.td
@@ -818,7 +818,7 @@ def AllocCN : Opcode {
 }
 
 def Free : Opcode {
-  let Args = [ArgBool];
+  let Args = [ArgBool, ArgBool];
 }
 
 def CheckNewTypeMismatch : Opcode {
diff --git a/clang/test/AST/ByteCode/new-delete.cpp b/clang/test/AST/ByteCode/new-delete.cpp
index 8c9d5d9c9b1d7c..8bcbed1aba21e9 100644
--- a/clang/test/AST/ByteCode/new-delete.cpp
+++ b/clang/test/AST/ByteCode/new-delete.cpp
@@ -552,8 +552,6 @@ namespace DeleteThis {
                                                // both-note {{in call to 'super_secret_double_delete()'}}
 }
 
-/// FIXME: This is currently diagnosed, but should work.
-/// If the destructor for S is _not_ virtual however, it should fail.
 namespace CastedDelete {
   struct S {
     constexpr S(int *p) : p(p) {}
@@ -567,11 +565,10 @@ namespace CastedDelete {
 
   constexpr int vdtor_1() {
     int a;
-    delete (S*)new T(&a); // expected-note {{delete of pointer to subobject}}
+    delete (S*)new T(&a);
     return a;
   }
-  static_assert(vdtor_1() == 1); // expected-error {{not an integral constant expression}} \
-                                 // expected-note {{in call to}}
+  static_assert(vdtor_1() == 1);
 }
 
 constexpr void use_after_free_2() { // both-error {{never produces a constant expression}}
@@ -778,6 +775,28 @@ namespace Placement {
   static_assert(ok2()== 0);
 }
 
+constexpr bool virt_delete(bool global) {
+  struct A {
+    virtual constexpr ~A() {}
+  };
+  struct B : A {
+    void operator delete(void *);
+    constexpr ~B() {}
+  };
+
+  A *p = new B;
+  if (global)
+    ::delete p;
+  else
+    delete p; // both-note {{call to class-specific 'operator delete'}}
+  return true;
+}
+static_assert(virt_delete(true));
+static_assert(virt_delete(false)); // both-error {{not an integral constant expression}} \
+                                   // both-note {{in call to}}
+
+
+
 #else
 /// Make sure we reject this prior to C++20
 constexpr int a() { // both-error {{never produces a constant expression}}