[clang][Interp] Support destructors

[tbaeder]

Summary

Try to use the existing cleanup machinery to implement calling destructors.

[tbaeder]

Ping

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

29

Why the change from LocalScope to VariableScope?

clang/lib/AST/Interp/ByteCodeExprGen.h

356

clang/test/AST/Interp/cxx20.cpp

330

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

29

I changed VariableScope to do the destructor handling, bu that's not needed for "temporaries in toplevel variable desclarations" (also, but unrelated, inherting from something called "local scope" but then explaining in a comment that it's only used for toplevel declarations is kinda weird).

[shafik]

LGTM

[tbaeder]

Looks like I have to actually go up the class chain and emit all destructors manually.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1893–1895

As a FIXME: you should also handle virtual destructors at some point, whenever you get around to handling virtual functions in general.

1908–1909

isTrivial() only works once the class has been fully built up by Sema IIRC; we should have a test case for that situation.

1910

This looks like it will destroy the array elements in order instead of in reverse order -- need test coverage for that. See https://eel.is/c++draft/class.dtor#13.sentence-5

1927

Won't this also destroy static data members? (Needs a test case for that.)

Also, what if the record is a union and not a structure? We only want to destroy the active member in that case, not all of the variant members, right? (Also needs a test case.)

See http://eel.is/c++draft/class.dtor#13

1951–1956

I don't think we handle virtual functions yet so I doubt we handle virtual bases, but that's something that might need a FIXME comment.

clang/lib/AST/Interp/ByteCodeExprGen.h

328

No need to add the virtual here as the override already signals that (can't override a non-virtual function).

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1908–1909

Are you saying that isTrivial() cannot be used like this, or just that it can, but needs a test case to ensure that this is true?

Also, how would such a test case look like?

1910

Right, that makes a lot of sense, good catch.

1927

I saw

// A union destructor does not implicitly destroy its members.
if (RD->isUnion())
  return true;

in ExprConstant.cpp, but since we don't handle unions at all in the new interpreter right now, I didn't add anything for them. I added a comment.

[tbaeder]

Ping

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1908–1909

Sema::DeclareImplicitDestructor() decides whether the destructor is trivial or not, and that is based on information that the class collects as the class is being declared. While the class is being parsed, the DeclarationData for the class is updated as we go and we use that to decide if we need the destructor, whether it's trivial, etc. So it's possible for us to have not seen a part of the class yet that would cause the special member function to be (non)trivial and so asking the method "are you trivial" may give a different answer depending on when the question is asked.

In terms of a test case, I think it would be trying to hit one of these cases http://eel.is/c++draft/class.mem#class.dtor-8 by using a constexpr function that needs to be evaluated before we get to something that causes the dtor to no longer be trivial.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1908–1909

Hm, I can't come up with a reproducer for this. The class of a member variable must be fully defined when the member is declared, so I can't forward-declare it and then introduce a non-trivial destructor later. And as soon as I add a destructor declaration (and try to define i later), the destructor is automatically not trivial anymore.

[aaron.ballman]

LGTM assuming no surprises with the new test request.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1908–1909

Yeah, I'm struggling to make a test case as well, so let's move on.

clang/test/AST/Interp/cxx20.cpp

439

Another test case that I think would be interesting is with a static member that is not constexpr (to show it doesn't cause problems) and an out-of-line destructor just because it's a bit of an oddity: https://godbolt.org/z/YqrPMEEr4

[tbaeder]

No surprises I think.

clang/lib/AST/Interp/ByteCodeExprGen.h

llvm::RoundingMode getRoundingMode(const Expr *E) const {
     FPOptions FPO = E->getFPFeaturesInEffect(Ctx.getLangOpts());
 
     if (FPO.getRoundingMode() == llvm::RoundingMode::Dynamic)
       return llvm::RoundingMode::NearestTiesToEven;
 
     return FPO.getRoundingMode();
   }
 
+  bool emitRecordDestruction(const Descriptor *Desc);
+
 protected:
   /// Variable to storage mapping.
   llvm::DenseMap<const ValueDecl *, Scope::Local> Locals;
 
   /// OpaqueValueExpr to location mapping.
   llvm::DenseMap<const OpaqueValueExpr *, unsigned> OpaqueExprs;
 
   /// Current scope.

 /// Scope for local variables.
 ///
 /// When the scope is destroyed, instructions are emitted to tear down
 /// all variables declared in this scope.
 template <class Emitter> class LocalScope : public VariableScope<Emitter> {
 public:
   LocalScope(ByteCodeExprGen<Emitter> *Ctx) : VariableScope<Emitter>(Ctx) {}
 
   ~LocalScope() override { this->emitDestruction(); }
 
   void addLocal(const Scope::Local &Local) override {
     if (!Idx) {
       Idx = this->Ctx->Descriptors.size();
       this->Ctx->Descriptors.emplace_back();
     }
 
     this->Ctx->Descriptors[*Idx].emplace_back(Local);
   }
 
+  /// Emit destruction of the local variable. This includes
+  /// object destructors.
   void emitDestruction() override {
     if (!Idx)
       return;
+    // Emit destructor calls for local variables of record
+    // type with a destructor.
+    for (Scope::Local &Local : this->Ctx->Descriptors[*Idx]) {
+      if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray()) {
+        this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{});
+        this->Ctx->emitRecordDestruction(Local.Desc);
+      }
+    }
+
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
   }
 
 protected:
   /// Index of the scope in the chain.
   std::optional<unsigned> Idx;
 };
 
 /// Scope for storage declared in a compound statement.
 template <class Emitter> class BlockScope final : public LocalScope<Emitter> {
 public:
   BlockScope(ByteCodeExprGen<Emitter> *Ctx) : LocalScope<Emitter>(Ctx) {}

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 using namespace clang::interp;
 
 using APSInt = llvm::APSInt;
 
 namespace clang {
 namespace interp {
 
 /// Scope used to handle temporaries in toplevel variable declarations.
-template <class Emitter> class DeclScope final : public LocalScope<Emitter> {
+template <class Emitter> class DeclScope final : public VariableScope<Emitter> {
 public:
   DeclScope(ByteCodeExprGen<Emitter> *Ctx, const ValueDecl *VD)
-      : LocalScope<Emitter>(Ctx), Scope(Ctx->P, VD) {}
+      : VariableScope<Emitter>(Ctx), Scope(Ctx->P, VD) {}
 
   virtual ~DeclScope() override { this->emitDestruction(); }
 
   void addExtended(const Scope::Local &Local) override {
     return this->addLocal(Local);
   }
 
 private:

 }
 
 template <class Emitter>
 void ByteCodeExprGen<Emitter>::emitCleanup() {
   for (VariableScope<Emitter> *C = VarScope; C; C = C->getParent())
     C->emitDestruction();
 }
 
+/// When calling this, we have a pointer of the local-to-destroy
+/// on the stack.
+/// Emit destruction of record types (or arrays of record types).
+/// FIXME: Handle virtual destructors.
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::emitRecordDestruction(const Descriptor *Desc) {
+  assert(Desc);
+  assert(!Desc->isPrimitive());
+  assert(!Desc->isPrimitiveArray());
+
+  // Arrays.
+  if (Desc->isArray()) {
+    const Descriptor *ElemDesc = Desc->ElemDesc;
+    const Record *ElemRecord = ElemDesc->ElemRecord;
+    assert(ElemRecord); // This is not a primitive array.
+
+    if (const CXXDestructorDecl *Dtor = ElemRecord->getDestructor();
+        Dtor && !Dtor->isTrivial()) {
+      for (ssize_t I = Desc->getNumElems() - 1; I >= 0; --I) {
+        if (!this->emitConstUint64(I, SourceInfo{}))
+          return false;
+        if (!this->emitArrayElemPtrUint64(SourceInfo{}))
+          return false;
+        if (!this->emitRecordDestruction(Desc->ElemDesc))
+          return false;
+      }
+    }
+    return this->emitPopPtr(SourceInfo{});
+  }
+
+  const Record *R = Desc->ElemRecord;
+  assert(R);
+  // First, destroy all fields.
+  for (const Record::Field &Field : llvm::reverse(R->fields())) {
+    const Descriptor *D = Field.Desc;
+    if (!D->isPrimitive() && !D->isPrimitiveArray()) {
+      if (!this->emitDupPtr(SourceInfo{}))
+        return false;
+      if (!this->emitGetPtrField(Field.Offset, SourceInfo{}))
+        return false;
+      if (!this->emitRecordDestruction(D))
+        return false;
+    }
+  }
+
+  // FIXME: Unions need to be handled differently here. We don't want to
+  //   call the destructor of its members.
+
+  // Now emit the destructor and recurse into base classes.
+  if (const CXXDestructorDecl *Dtor = R->getDestructor();
+      Dtor && !Dtor->isTrivial()) {
+    const Function *DtorFunc = getFunction(Dtor);
+    if (DtorFunc && DtorFunc->isConstexpr()) {
+      assert(DtorFunc->hasThisPointer());
+      assert(DtorFunc->getNumParams() == 1);
+      if (!this->emitDupPtr(SourceInfo{}))
+        return false;
+      if (!this->emitCall(DtorFunc, SourceInfo{}))
+        return false;
+    }
+  }
+
+  for (const Record::Base &Base : llvm::reverse(R->bases())) {
+    if (!this->emitGetPtrBase(Base.Offset, SourceInfo{}))
+      return false;
+    if (!this->emitRecordDestruction(Base.Desc))
+      return false;
+  }
+  // FIXME: Virtual bases.
+
+  // Remove the instance pointer.
+  return this->emitPopPtr(SourceInfo{});
+}
+
 namespace clang {
 namespace interp {
 
 template class ByteCodeExprGen<ByteCodeEmitter>;
 template class ByteCodeExprGen<EvalEmitter>;
 
 } // namespace interp
 } // namespace clang

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

bool ByteCodeStmtGen<Emitter>::visitCXXForRangeStmt(const CXXForRangeStmt *S) {
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitBreakStmt(const BreakStmt *S) {
   if (!BreakLabel)
     return false;
 
+  this->emitCleanup();
   return this->jump(*BreakLabel);
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitContinueStmt(const ContinueStmt *S) {
   if (!ContinueLabel)
     return false;
 
+  this->emitCleanup();
   return this->jump(*ContinueLabel);
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitSwitchStmt(const SwitchStmt *S) {
   const Expr *Cond = S->getCond();
   PrimType CondT = this->classifyPrim(Cond->getType());
 

clang/test/AST/Interp/cxx20.cpp

constexpr Foo() {
       T = 13; // expected-error {{cannot assign to non-static data member 'T' with const-qualified type}} \
               // ref-error {{cannot assign to non-static data member 'T' with const-qualified type}}
     }
   };
   constexpr Foo F; // expected-error {{must be initialized by a constant expression}} \
                    // ref-error {{must be initialized by a constant expression}}
 
 };
+
+namespace Destructors {
+
+  class Inc final {
+  public:
+    int &I;
+    constexpr Inc(int &I) : I(I) {}
+    constexpr ~Inc() {
+      I++;
+    }
+  };
+
+  class Dec final {
+  public:
+    int &I;
+    constexpr Dec(int &I) : I(I) {}
+    constexpr ~Dec() {
+      I--;
+    }
+  };
+
+
+
+  constexpr int m() {
+    int i = 0;
+    {
+      Inc f1(i);
+      Inc f2(i);
+      Inc f3(i);
+    }
+    return i;
+  }
+  static_assert(m() == 3, "");
+
+
+  constexpr int C() {
+    int i = 0;
+
+    while (i < 10) {
+      Inc inc(i);
+      continue;
+      Dec dec(i);
+    }
+    return i;
+  }
+  static_assert(C() == 10, "");
+
+
+  constexpr int D() {
+    int i = 0;
+
+    {
+      Inc i1(i);
+      {
+        Inc i2(i);
+        return i;
+      }
+    }
+
+    return i;
+  }
+  static_assert(D() == 0, "");
+
+  constexpr int E() {
+    int i = 0;
+
+    for(;;) {
+      Inc i1(i);
+      break;
+    }
+    return i;
+  }
+  static_assert(E() == 1, "");
+
+
+  /// FIXME: This should be rejected, since we call the destructor
+  ///   twice. However, GCC doesn't care either.
+  constexpr int ManualDtor() {
+    int i = 0;
+    {
+      Inc I(i); // ref-note {{destroying object 'I' whose lifetime has already ended}}
+      I.~Inc();
+    }
+    return i;
+  }
+  static_assert(ManualDtor() == 1, ""); // expected-error {{static assertion failed}} \
+                                        // expected-note {{evaluates to '2 == 1'}} \
+                                        // ref-error {{not an integral constant expression}} \
+                                        // ref-note {{in call to 'ManualDtor()'}}
+
+  constexpr void doInc(int &i) {
+    Inc I(i);
+    return;
+  }
+  constexpr int testInc() {
+    int i = 0;
+    doInc(i);
+    return i;
+  }
+  static_assert(testInc() == 1, "");
+  constexpr void doInc2(int &i) {
+    Inc I(i);
+    // No return statement.
+  }
+   constexpr int testInc2() {
+    int i = 0;
+    doInc2(i);
+    return i;
+  }
+  static_assert(testInc2() == 1, "");
+
+
+  namespace DtorOrder {
+    class A {
+      public:
+      int &I;
+      constexpr A(int &I) : I(I) {}
+      constexpr ~A() {
+        I = 1337;
+      }
+    };
+
+    class B : public A {
+      public:
+      constexpr B(int &I) : A(I) {}
+      constexpr ~B() {
+        I = 42;
+      }
+    };
+
+    constexpr int foo() {
+      int i = 0;
+      {
+        B b(i);
+      }
+      return i;
+    }
+
+    static_assert(foo() == 1337);
+  }
+
+  class FieldDtor1 {
+  public:
+    Inc I1;
+    Inc I2;
+    constexpr FieldDtor1(int &I) : I1(I), I2(I){}
+  };
+
+  constexpr int foo2() {
+    int i = 0;
+    {
+      FieldDtor1 FD1(i);
+    }
+    return i;
+  }
+
+  static_assert(foo2() == 2);
+
+  class FieldDtor2 {
+  public:
+    Inc Incs[3];
+    constexpr FieldDtor2(int &I)  : Incs{Inc(I), Inc(I), Inc(I)} {}
+  };
+
+  constexpr int foo3() {
+    int i = 0;
+    {
+      FieldDtor2 FD2(i);
+    }
+    return i;
+  }
+
+  static_assert(foo3() == 3);
+
+  struct ArrD {
+    int index;
+    int *arr;
+    int &p;
+    constexpr ~ArrD() {
+      arr[p] = index;
+      ++p;
+    }
+  };
+  constexpr bool ArrayOrder() {
+    int order[3] = {0, 0, 0};
+    int p = 0;
+    {
+      ArrD ds[3] = {
+        {1, order, p},
+        {2, order, p},
+        {3, order, p},
+      };
+      // ds will be destroyed.
+    }
+    return order[0] == 3 && order[1] == 2 && order[2] == 1;
+  }
+  static_assert(ArrayOrder());
+
+
+  // Static members aren't destroyed.
+  class Dec2 {
+  public:
+    int A = 0;
+    constexpr ~Dec() {
+      A++;
+    }
+  };
+  class Foo {
+  public:
+    static constexpr Dec2 a;
+  };
+  static_assert(Foo::a.A == 0);
+  constexpr bool f() {
+    Foo f;
+    return true;
+  }
+  static_assert(Foo::a.A == 0);
+  static_assert(f());
+  static_assert(Foo::a.A == 0);
+
+
+}