[clang][bytecode] Fix an inconsistency with loop condition jumps

[tbaederr]

When emitting the jump for e.g. a for loop condition, we used to jump out of the CondScope, leaving the scope initialized, because we skipped the corresponding Destroy opcode. If that loop was in a loop itself, that outer loop could then iterate once more, leading to us initializing a scope that was still initialized.
Fix this by also destroying the scope after the EndLabel.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

When emitting the jump for e.g. a for loop condition, we used to jump out of the CondScope, leaving the scope initialized, because we skipped the corresponding Destroy opcode. If that loop was in a loop itself, that outer loop could then iterate once more, leading to us initializing a scope that was still initialized.
Fix this by also destroying the scope after the EndLabel.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ByteCode/Compiler.cpp (+16-15)
• (modified) clang/lib/AST/ByteCode/Compiler.h (+2-1)

diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 86b43585cd292..376daec5cd0d2 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -5431,21 +5431,21 @@ bool Compiler<Emitter>::visitForStmt(const ForStmt *S) {
   this->fallthrough(CondLabel);
   this->emitLabel(CondLabel);
 
-  {
-    LocalScope<Emitter> CondScope(this);
-    if (const DeclStmt *CondDecl = S->getConditionVariableDeclStmt())
-      if (!visitDeclStmt(CondDecl))
-        return false;
+  // Start of loop body.
+  LocalScope<Emitter> CondScope(this);
+  if (const DeclStmt *CondDecl = S->getConditionVariableDeclStmt())
+    if (!visitDeclStmt(CondDecl))
+      return false;
 
-    if (Cond) {
-      if (!this->visitBool(Cond))
-        return false;
-      if (!this->jumpFalse(EndLabel))
-        return false;
-    }
+  if (!this->maybeEmitDeferredVarInit(S->getConditionVariable()))
+    return false;
 
-    if (!this->maybeEmitDeferredVarInit(S->getConditionVariable()))
+  if (Cond) {
+    if (!this->visitBool(Cond))
+      return false;
+    if (!this->jumpFalse(EndLabel))
       return false;
+  }
 
     if (Body && !this->visitStmt(Body))
       return false;
@@ -5457,13 +5457,14 @@ bool Compiler<Emitter>::visitForStmt(const ForStmt *S) {
 
     if (!CondScope.destroyLocals())
       return false;
-  }
   if (!this->jump(CondLabel))
     return false;
+  // End of loop body.
 
-  this->fallthrough(EndLabel);
   this->emitLabel(EndLabel);
-  return true;
+  // If we jumped out of the loop above, we still need to clean up the condition
+  // scope.
+  return CondScope.destroyLocals();
 }
 
 template <class Emitter>
diff --git a/clang/lib/AST/ByteCode/Compiler.h b/clang/lib/AST/ByteCode/Compiler.h
index 256e917728886..858957367d85d 100644
--- a/clang/lib/AST/ByteCode/Compiler.h
+++ b/clang/lib/AST/ByteCode/Compiler.h
@@ -531,9 +531,10 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
     if (!Idx)
       return true;
 
+    // NB: We are *not* resetting Idx here as to allow multiple
+    // calls to destroyLocals().
     bool Success = this->emitDestructors(E);
     this->Ctx->emitDestroy(*Idx, E);
-    this->Idx = std::nullopt;
     return Success;
   }