tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, tahonermann, erichkeane, shafik.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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
`DeadBlock`s. 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
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
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D145545
Files:
clang/lib/AST/Interp/ByteCodeExprGen.h
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
clang/lib/AST/Interp/ByteCodeStmtGen.h
Index: clang/lib/AST/Interp/ByteCodeStmtGen.h
===================================================================
--- clang/lib/AST/Interp/ByteCodeStmtGen.h
+++ clang/lib/AST/Interp/ByteCodeStmtGen.h
@@ -54,6 +54,7 @@
// 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);
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===================================================================
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -194,6 +194,17 @@
}
}
+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) {
@@ -306,11 +317,13 @@
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;
@@ -325,13 +338,16 @@
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);
@@ -350,6 +366,7 @@
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;
@@ -360,11 +377,13 @@
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);
@@ -386,38 +405,38 @@
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;
+ // 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;
+ // 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;
+ 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;
- }
+ // Body.
+ LocalScope<Emitter> Scope(this);
+ if (!this->visitUnscopedCompoundStmt(Body))
+ return false;
+ this->emitLabel(IncLabel);
+ if (!this->discard(Inc))
+ return false;
+
+ Scope.emitDestructors();
+ if (!this->jump(CondLabel))
+ return false;
this->emitLabel(EndLabel);
return true;
@@ -428,7 +447,7 @@
if (!BreakLabel)
return false;
- this->emitCleanup();
+ this->VarScope->emitDestructors();
return this->jump(*BreakLabel);
}
@@ -437,7 +456,7 @@
if (!ContinueLabel)
return false;
- this->emitCleanup();
+ this->VarScope->emitDestructors();
return this->jump(*ContinueLabel);
}
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===================================================================
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -306,6 +306,8 @@
}
virtual void emitDestruction() {}
+ /// Emit destructors for local variables in this scope.
+ virtual void emitDestructors() {}
VariableScope *getParent() const { return Parent; }
@@ -316,15 +318,25 @@
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) {
@@ -335,9 +347,7 @@
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
@@ -348,8 +358,6 @@
this->Ctx->emitRecordDestruction(Local.Desc);
}
}
-
- this->Ctx->emitDestroy(*Idx, SourceInfo{});
}
protected:
@@ -357,10 +365,19 @@
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
@@ -372,9 +389,9 @@
/// 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)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
