[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

30

Why the change from LocalScope to VariableScope?

clang/lib/AST/Interp/ByteCodeExprGen.h

369

clang/test/AST/Interp/cxx20.cpp

279

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

30

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

1513–1515

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

1528–1529

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

1530

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

1547

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

1571–1576

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

343

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

1528–1529

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?

1530

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

1547

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

1528–1529

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

1528–1529

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

1528–1529

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

clang/test/AST/Interp/cxx20.cpp

388

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

 /// 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(); }
+  virtual ~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);
   }
 
   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]) {
+      const Record *TypeRecord = Local.Desc->ElemRecord;
+      if (!TypeRecord)
+        continue;
+
+      const auto *RDecl = dyn_cast<CXXRecordDecl>(TypeRecord->getDecl());
+      if (!RDecl)
+        continue;
+
+      if (const auto *DtorDecl = RDecl->getDestructor()) {
+        const Function *Dtor = this->Ctx->getFunction(DtorDecl);
+        if (Dtor && Dtor->isConstexpr()) {
+          assert(Dtor->hasThisPointer());
+          assert(Dtor->getNumParams() == 1);
+          // Emit destructor call.
+          this->Ctx->emitGetPtrLocal(Local.Offset, DtorDecl);
+          this->Ctx->emitCall(Dtor, DtorDecl);
+        }
+      }
+    }
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
   }
 
 protected:
   /// Index of the scope in the chain.
   Optional<unsigned> Idx;
 };
 

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 using APSInt = llvm::APSInt;
 template <typename T> using Expected = llvm::Expected<T>;
 template <typename T> using Optional = llvm::Optional<T>;
 
 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 VarDecl *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:
   Program::DeclScope Scope;
 };

 }
 
 template <class Emitter>
 void ByteCodeExprGen<Emitter>::emitCleanup() {
   for (VariableScope<Emitter> *C = VarScope; C; C = C->getParent())
     C->emitDestruction();
 }
 
 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>::visitForStmt(const ForStmt *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);
 }
 
 namespace clang {
 namespace interp {
 
 template class ByteCodeStmtGen<ByteCodeEmitter>;
 
 } // namespace interp
 } // namespace clang

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()'}}
+};