[clang][Interp] Fix local variable (destructor) management in loop bodies

[tbaeder]

Summary

Ok, let me explain.

PREVIOUSLY:

When generating bytecode for a loop, we visitStmt(Body), which will create its own local scope (that makes sense) and at the end of said scope, it will emit Destroy ops for all local variables, as well as emit code for the destructors of those local variables. However, the Destroy op calls the destructor for all data in the Block, and converts the blocks to DeadBlocks. That means the contents aren't usable anymore after it, and when we jump to the beginning of the loop and try to assign something to the block, we might end up trying to use the unusable data. This happens in the copy constructor of our Pointer class for example.

In pseudo code:

loop {
   create locals...

   local destructors
   Destroy Scope
   jump to beginning
}

NOW:

I've split emitting the destructors for locals and emitting the Destroy op for a scope up into two separate parts. They are mostly still the same, i.e.when using a AutoScope or any of its subclasses, which emits the destructors and the Destroy op in its own destructor.

When visiting a loop, we create our own LocalScope to manage the lifetimes of locals created within the body and then call emitDestructors of that scope right before the jmp to the beginning of the loop. The Destroy op for the scope is emitted in the LocalScope destructor, so will only be emitted once.

In pseudo code:

loop {
    create locals...

   local destructors
   jump to beginning
}
Destroy Scope

The changes in this patch are on top of https://reviews.llvm.org/D137070, but the patch needs to go in before that or be merged with it.

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping. Please prioritize this patch over the other interpreter patches since the others depend on it.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.h

338

Should we be setting Idx = std::nullopt; after this so that the LocalScope destructor does not also emit a destroy for the same Idx?

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

199–200

Errr, I'm surprised it isn't UB to call this with anything but a CompoundStmt given the function name.

209

It's a bit of a surprise that we visit the entire body of the compound statement before we visit the compound statement itself. I was thinking the compound statement could potentially have prologue work it needs to do, but now I'm second-guessing that. But this design still feels a little bit off... it's basically doing a post-order traversal just for this one node, and I wonder if we want something more general like a preVisitFoo() followed by visitFoo() followed by postVisitFoo() that applies to any AST node.

320–325

AutoScope and some curly braces to delimit the scope object lifetime?

341

Similar question here and elsewhere. The concern I have with this form is that it's pretty easy to accidentally miss that we've emitted the destructors in later code, whereas using the RAII object makes that impossible.

[tbaeder]

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

199–200

Yeah, I'm not too happy about that either. I'll see what I can do.

209

I think you're overthinking this, this just for the !isa<CompoundStmt>(S) case (which IIRC happens at least for comma operator bin ops?)

320–325

The problem is that we want to emit the destructors before the jump, but not destroy the scope. That should happen after the end label, when the loop is finished altogether so an AutoScope doesn't work.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

209

Oh! Yeah, I totally overthought that. I got tripped up by this accepting things other than a CompoundStmt because of the function name. Sorry for the noise!

320–325

Ahh yeah, that's a good point... but this still worries me a bit because of how easy it is to misuse the scope after emitting the destructors. Notionally, we want two scopes, right? One scope for the loop construct guts and a second (inner) scope for the loop body. That's how the construct is modeled in the standard: http://eel.is/c++draft/stmt.while#2

[tbaeder]

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

320–325

Add a DestructorScope that just emits the destructors of a given scope.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

338

Yeah I think that makes sense, thanks.

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

199–200

Renamed it to visitLoopBody and added a doc comment explaining its purpose.

[aaron.ballman]

LGTM!

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

320–325

Thank you, I like this!

[tbaeder]

Thanks, I'll verify the patch once again with an msan build before I push, just to be sure.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   }
 
   virtual void addExtended(const Scope::Local &Local) {
     if (this->Parent)
       this->Parent->addExtended(Local);
   }
 
   virtual void emitDestruction() {}
+  /// Emit destructors for local variables in this scope.
+  virtual void emitDestructors() {}
 
   VariableScope *getParent() const { return Parent; }
 
 protected:
   /// ByteCodeExprGen instance.
   ByteCodeExprGen<Emitter> *Ctx;
   /// Link to the parent scope.
   VariableScope *Parent;
 };
 
-/// Scope for local variables.
-///
-/// When the scope is destroyed, instructions are emitted to tear down
-/// all variables declared in this scope.
+/// Generic scope for local variables.
 template <class Emitter> class LocalScope : public VariableScope<Emitter> {
 public:
   LocalScope(ByteCodeExprGen<Emitter> *Ctx) : VariableScope<Emitter>(Ctx) {}
 
-  ~LocalScope() override { this->emitDestruction(); }
+  /// Emit a Destroy op for this scope.
+  ~LocalScope() override {
+    if (!Idx)
+      return;
+    this->Ctx->emitDestroy(*Idx, SourceInfo{});
+  }
+
+  /// Overriden to support explicit destruction.
+  void emitDestruction() override {
+    if (!Idx)
+      return;
+    this->emitDestructors();
+    this->Ctx->emitDestroy(*Idx, SourceInfo{});
+  }
 
   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 {
+  void emitDestructors() 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;
 };
 
+/// Like a regular LocalScope, except that the destructors of all local
+/// variables are automatically emitted when the AutoScope is destroyed.
+template <class Emitter> class AutoScope : public LocalScope<Emitter> {
+public:
+  AutoScope(ByteCodeExprGen<Emitter> *Ctx) : LocalScope<Emitter>(Ctx) {}
+
+  ~AutoScope() override { this->emitDestructors(); }
+};
+
 /// Scope for storage declared in a compound statement.
-template <class Emitter> class BlockScope final : public LocalScope<Emitter> {
+template <class Emitter> class BlockScope final : public AutoScope<Emitter> {
 public:
-  BlockScope(ByteCodeExprGen<Emitter> *Ctx) : LocalScope<Emitter>(Ctx) {}
+  BlockScope(ByteCodeExprGen<Emitter> *Ctx) : AutoScope<Emitter>(Ctx) {}
 
   void addExtended(const Scope::Local &Local) override {
     // If we to this point, just add the variable as a normal local
     // variable. It will be destroyed at the end of the block just
     // like all others.
     this->addLocal(Local);
   }
 };
 
 /// Expression scope which tracks potentially lifetime extended
 /// temporaries which are hoisted to the parent scope on exit.
-template <class Emitter> class ExprScope final : public LocalScope<Emitter> {
+template <class Emitter> class ExprScope final : public AutoScope<Emitter> {
 public:
-  ExprScope(ByteCodeExprGen<Emitter> *Ctx) : LocalScope<Emitter>(Ctx) {}
+  ExprScope(ByteCodeExprGen<Emitter> *Ctx) : AutoScope<Emitter>(Ctx) {}
 
   void addExtended(const Scope::Local &Local) override {
     if (this->Parent)
       this->Parent->addLocal(Local);
   }
 };
 
 template <class Emitter> class ArrayIndexScope final {

clang/lib/AST/Interp/ByteCodeStmtGen.h

 private:
   friend class LabelScope<Emitter>;
   friend class LoopScope<Emitter>;
   friend class SwitchScope<Emitter>;
 
   // Statement visitors.
   bool visitStmt(const Stmt *S);
   bool visitCompoundStmt(const CompoundStmt *S);
+  bool visitUnscopedCompoundStmt(const Stmt *S);
   bool visitDeclStmt(const DeclStmt *DS);
   bool visitReturnStmt(const ReturnStmt *RS);
   bool visitIfStmt(const IfStmt *IS);
   bool visitWhileStmt(const WhileStmt *S);
   bool visitDoStmt(const DoStmt *S);
   bool visitForStmt(const ForStmt *S);
   bool visitCXXForRangeStmt(const CXXForRangeStmt *S);
   bool visitBreakStmt(const BreakStmt *S);

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

default: {
     if (auto *Exp = dyn_cast<Expr>(S))
       return this->discard(Exp);
     return this->bail(S);
   }
   }
 }
 
 template <class Emitter>
+bool ByteCodeStmtGen<Emitter>::visitUnscopedCompoundStmt(const Stmt *S) {
+  if (isa<NullStmt>(S))
+    return true;
+
+  for (auto *InnerStmt : cast<CompoundStmt>(S)->body())
+    if (!visitStmt(InnerStmt))
+      return false;
+  return true;
+}
+
+template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitCompoundStmt(
     const CompoundStmt *CompoundStmt) {
   BlockScope<Emitter> Scope(this);
   for (auto *InnerStmt : CompoundStmt->body())
     if (!visitStmt(InnerStmt))
       return false;
   return true;
 }
 
bool ByteCodeStmtGen<Emitter>::visitWhileStmt(const WhileStmt *S) {
   LoopScope<Emitter> LS(this, EndLabel, CondLabel);
 
   this->emitLabel(CondLabel);
   if (!this->visitBool(Cond))
     return false;
   if (!this->jumpFalse(EndLabel))
     return false;
 
-  if (!this->visitStmt(Body))
+  LocalScope<Emitter> Scope(this);
+  if (!this->visitUnscopedCompoundStmt(Body))
     return false;
+
+  Scope.emitDestructors();
   if (!this->jump(CondLabel))
     return false;
-
   this->emitLabel(EndLabel);
 
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitDoStmt(const DoStmt *S) {
   const Expr *Cond = S->getCond();
   const Stmt *Body = S->getBody();
 
   LabelTy StartLabel = this->getLabel();
   LabelTy EndLabel = this->getLabel();
   LabelTy CondLabel = this->getLabel();
   LoopScope<Emitter> LS(this, EndLabel, CondLabel);
+  LocalScope<Emitter> Scope(this);
 
   this->emitLabel(StartLabel);
-  if (!this->visitStmt(Body))
+  if (!this->visitUnscopedCompoundStmt(Body))
     return false;
   this->emitLabel(CondLabel);
   if (!this->visitBool(Cond))
     return false;
+
+  Scope.emitDestructors();
   if (!this->jumpTrue(StartLabel))
     return false;
   this->emitLabel(EndLabel);
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitForStmt(const ForStmt *S) {
   // for (Init; Cond; Inc) { Body }
   const Stmt *Init = S->getInit();
   const Expr *Cond = S->getCond();
   const Expr *Inc = S->getInc();
   const Stmt *Body = S->getBody();
 
   LabelTy EndLabel = this->getLabel();
   LabelTy CondLabel = this->getLabel();
   LabelTy IncLabel = this->getLabel();
   LoopScope<Emitter> LS(this, EndLabel, IncLabel);
+  LocalScope<Emitter> Scope(this);
 
   if (Init && !this->visitStmt(Init))
     return false;
   this->emitLabel(CondLabel);
   if (Cond) {
     if (!this->visitBool(Cond))
       return false;
     if (!this->jumpFalse(EndLabel))
       return false;
   }
-  if (Body && !this->visitStmt(Body))
+  if (Body && !this->visitUnscopedCompoundStmt(Body))
     return false;
   this->emitLabel(IncLabel);
   if (Inc && !this->discard(Inc))
     return false;
+
+  Scope.emitDestructors();
   if (!this->jump(CondLabel))
     return false;
   this->emitLabel(EndLabel);
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitCXXForRangeStmt(const CXXForRangeStmt *S) {
   const Stmt *Init = S->getInit();
   const Expr *Cond = S->getCond();
   const Expr *Inc = S->getInc();
   const Stmt *Body = S->getBody();
   const Stmt *BeginStmt = S->getBeginStmt();
   const Stmt *RangeStmt = S->getRangeStmt();
   const Stmt *EndStmt = S->getEndStmt();
   const VarDecl *LoopVar = S->getLoopVariable();
 
   LabelTy EndLabel = this->getLabel();
   LabelTy CondLabel = this->getLabel();
   LabelTy IncLabel = this->getLabel();
   LoopScope<Emitter> LS(this, EndLabel, IncLabel);
-  {
-    ExprScope<Emitter> ES(this);
 
-    // Emit declarations needed in the loop.
-    if (Init && !this->visitStmt(Init))
-      return false;
-    if (!this->visitStmt(RangeStmt))
-      return false;
-    if (!this->visitStmt(BeginStmt))
-      return false;
-    if (!this->visitStmt(EndStmt))
-      return false;
 
-    // Now the condition as well as the loop variable assignment.
-    this->emitLabel(CondLabel);
-    if (!this->visitBool(Cond))
-      return false;
-    if (!this->jumpFalse(EndLabel))
-      return false;
 
-    if (!this->visitVarDecl(LoopVar))
-      return false;
 
-    // Body.
-    if (!this->visitStmt(Body))
-      return false;
-    this->emitLabel(IncLabel);
-    if (!this->discard(Inc))
-      return false;
-    if (!this->jump(CondLabel))
-      return false;
-  }
+  LocalScope<Emitter> Scope(this);
+  if (!this->visitUnscopedCompoundStmt(Body))
+
+  Scope.emitDestructors();
 
   this->emitLabel(EndLabel);
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitBreakStmt(const BreakStmt *S) {
   if (!BreakLabel)
     return false;
 
-  this->emitCleanup();
+  this->VarScope->emitDestructors();
   return this->jump(*BreakLabel);
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitContinueStmt(const ContinueStmt *S) {
   if (!ContinueLabel)
     return false;
 
-  this->emitCleanup();
+  this->VarScope->emitDestructors();
   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());