Skip to content

[clang][bytecode] Fix local destructor order #107951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 10, 2024
Merged

[clang][bytecode] Fix local destructor order #107951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 51 additions & 31 deletions clang/lib/AST/ByteCode/Compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,22 +115,26 @@ template <class Emitter> class LoopScope final : public LabelScope<Emitter> {
LoopScope(Compiler<Emitter> *Ctx, LabelTy BreakLabel, LabelTy ContinueLabel)
: LabelScope<Emitter>(Ctx), OldBreakLabel(Ctx->BreakLabel),
OldContinueLabel(Ctx->ContinueLabel),
OldLabelVarScope(Ctx->LabelVarScope) {
OldBreakVarScope(Ctx->BreakVarScope),
OldContinueVarScope(Ctx->ContinueVarScope) {
this->Ctx->BreakLabel = BreakLabel;
this->Ctx->ContinueLabel = ContinueLabel;
this->Ctx->LabelVarScope = this->Ctx->VarScope;
this->Ctx->BreakVarScope = this->Ctx->VarScope;
this->Ctx->ContinueVarScope = this->Ctx->VarScope;
}

~LoopScope() {
this->Ctx->BreakLabel = OldBreakLabel;
this->Ctx->ContinueLabel = OldContinueLabel;
this->Ctx->LabelVarScope = OldLabelVarScope;
this->Ctx->ContinueVarScope = OldContinueVarScope;
this->Ctx->BreakVarScope = OldBreakVarScope;
}

private:
OptLabelTy OldBreakLabel;
OptLabelTy OldContinueLabel;
VariableScope<Emitter> *OldLabelVarScope;
VariableScope<Emitter> *OldBreakVarScope;
VariableScope<Emitter> *OldContinueVarScope;
};

// Sets the context for a switch scope, mapping labels.
Expand All @@ -145,18 +149,18 @@ template <class Emitter> class SwitchScope final : public LabelScope<Emitter> {
: LabelScope<Emitter>(Ctx), OldBreakLabel(Ctx->BreakLabel),
OldDefaultLabel(this->Ctx->DefaultLabel),
OldCaseLabels(std::move(this->Ctx->CaseLabels)),
OldLabelVarScope(Ctx->LabelVarScope) {
OldLabelVarScope(Ctx->BreakVarScope) {
this->Ctx->BreakLabel = BreakLabel;
this->Ctx->DefaultLabel = DefaultLabel;
this->Ctx->CaseLabels = std::move(CaseLabels);
this->Ctx->LabelVarScope = this->Ctx->VarScope;
this->Ctx->BreakVarScope = this->Ctx->VarScope;
}

~SwitchScope() {
this->Ctx->BreakLabel = OldBreakLabel;
this->Ctx->DefaultLabel = OldDefaultLabel;
this->Ctx->CaseLabels = std::move(OldCaseLabels);
this->Ctx->LabelVarScope = OldLabelVarScope;
this->Ctx->BreakVarScope = OldLabelVarScope;
}

private:
Expand Down Expand Up @@ -4605,18 +4609,23 @@ bool Compiler<Emitter>::visitWhileStmt(const WhileStmt *S) {
this->fallthrough(CondLabel);
this->emitLabel(CondLabel);

if (const DeclStmt *CondDecl = S->getConditionVariableDeclStmt())
if (!visitDeclStmt(CondDecl))
return false;
{
LocalScope<Emitter> CondScope(this);
if (const DeclStmt *CondDecl = S->getConditionVariableDeclStmt())
if (!visitDeclStmt(CondDecl))
return false;

if (!this->visitBool(Cond))
return false;
if (!this->jumpFalse(EndLabel))
return false;
if (!this->visitBool(Cond))
return false;
if (!this->jumpFalse(EndLabel))
return false;

if (!this->visitStmt(Body))
return false;
if (!this->visitStmt(Body))
return false;

if (!CondScope.destroyLocals())
return false;
}
if (!this->jump(CondLabel))
return false;
this->fallthrough(EndLabel);
Expand All @@ -4636,13 +4645,18 @@ template <class Emitter> bool Compiler<Emitter>::visitDoStmt(const DoStmt *S) {

this->fallthrough(StartLabel);
this->emitLabel(StartLabel);

{
LocalScope<Emitter> CondScope(this);
if (!this->visitStmt(Body))
return false;
this->fallthrough(CondLabel);
this->emitLabel(CondLabel);
if (!this->visitBool(Cond))
return false;

if (!CondScope.destroyLocals())
return false;
}
if (!this->jumpTrue(StartLabel))
return false;
Expand Down Expand Up @@ -4671,29 +4685,33 @@ bool Compiler<Emitter>::visitForStmt(const ForStmt *S) {
this->fallthrough(CondLabel);
this->emitLabel(CondLabel);

if (const DeclStmt *CondDecl = S->getConditionVariableDeclStmt())
if (!visitDeclStmt(CondDecl))
return false;
{
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 (Cond) {
if (!this->visitBool(Cond))
return false;
if (!this->jumpFalse(EndLabel))
return false;
}

{
if (Body && !this->visitStmt(Body))
return false;

this->fallthrough(IncLabel);
this->emitLabel(IncLabel);
if (Inc && !this->discard(Inc))
return false;
}

if (!CondScope.destroyLocals())
return false;
}
if (!this->jump(CondLabel))
return false;

this->fallthrough(EndLabel);
this->emitLabel(EndLabel);
return true;
Expand Down Expand Up @@ -4760,7 +4778,7 @@ bool Compiler<Emitter>::visitBreakStmt(const BreakStmt *S) {
if (!BreakLabel)
return false;

for (VariableScope<Emitter> *C = VarScope; C != LabelVarScope;
for (VariableScope<Emitter> *C = VarScope; C != BreakVarScope;
C = C->getParent())
C->emitDestruction();
return this->jump(*BreakLabel);
Expand All @@ -4771,8 +4789,8 @@ bool Compiler<Emitter>::visitContinueStmt(const ContinueStmt *S) {
if (!ContinueLabel)
return false;

for (VariableScope<Emitter> *C = VarScope; C != LabelVarScope;
C = C->getParent())
for (VariableScope<Emitter> *C = VarScope;
C && C->getParent() != ContinueVarScope; C = C->getParent())
C->emitDestruction();
return this->jump(*ContinueLabel);
}
Expand All @@ -4781,6 +4799,7 @@ template <class Emitter>
bool Compiler<Emitter>::visitSwitchStmt(const SwitchStmt *S) {
const Expr *Cond = S->getCond();
PrimType CondT = this->classifyPrim(Cond->getType());
LocalScope<Emitter> LS(this);

LabelTy EndLabel = this->getLabel();
OptLabelTy DefaultLabel = std::nullopt;
Expand Down Expand Up @@ -4844,7 +4863,8 @@ bool Compiler<Emitter>::visitSwitchStmt(const SwitchStmt *S) {
if (!this->visitStmt(S->getBody()))
return false;
this->emitLabel(EndLabel);
return true;

return LS.destroyLocals();
}

template <class Emitter>
Expand Down
8 changes: 5 additions & 3 deletions clang/lib/AST/ByteCode/Compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,10 +409,12 @@ class Compiler : public ConstStmtVisitor<Compiler<Emitter>, bool>,
/// Switch case mapping.
CaseMap CaseLabels;

/// Scope to cleanup until when chumping to one of the labels.
VariableScope<Emitter> *LabelVarScope = nullptr;
/// Scope to cleanup until when we see a break statement.
VariableScope<Emitter> *BreakVarScope = nullptr;
/// Point to break to.
OptLabelTy BreakLabel;
/// Scope to cleanup until when we see a continue statement.
VariableScope<Emitter> *ContinueVarScope = nullptr;
/// Point to continue to.
OptLabelTy ContinueLabel;
/// Default case label.
Expand Down Expand Up @@ -533,7 +535,7 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
return true;
// Emit destructor calls for local variables of record
// type with a destructor.
for (Scope::Local &Local : this->Ctx->Descriptors[*Idx]) {
for (Scope::Local &Local : llvm::reverse(this->Ctx->Descriptors[*Idx])) {
if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray()) {
if (!this->Ctx->emitGetPtrLocal(Local.Offset, E))
return false;
Expand Down
76 changes: 76 additions & 0 deletions clang/test/AST/ByteCode/cxx2a.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,79 @@ namespace Covariant {
constexpr const Covariant1 *cb1 = &cb;
static_assert(cb1->f()->a == 'Z');
}

namespace DtorOrder {
struct Buf {
char buf[64];
int n = 0;
constexpr void operator+=(char c) { buf[n++] = c; }
constexpr bool operator==(const char *str) const {
if (str[n] != 0)
return false;

for (int i = 0; i < n; ++i) {
if (buf[n] != str[n])
return false;
}
return true;

return __builtin_memcmp(str, buf, n) == 0;
}
constexpr bool operator!=(const char *str) const { return !operator==(str); }
};

struct A {
constexpr A(Buf &buf, char c) : buf(buf), c(c) { buf += c; }
constexpr ~A() { buf += (c - 32);}
constexpr operator bool() const { return true; }
Buf &buf;
char c;
};

constexpr void abnormal_termination(Buf &buf) {
struct Indestructible {
constexpr ~Indestructible(); // not defined
};
A a(buf, 'a');
A(buf, 'b');
int n = 0;

for (A &&c = A(buf, 'c'); A d = A(buf, 'd'); A(buf, 'e')) {
switch (A f(buf, 'f'); A g = A(buf, 'g')) { // both-warning {{boolean}}
case false: {
A x(buf, 'x');
}

case true: {
A h(buf, 'h');
switch (n++) {
case 0:
break;
case 1:
continue;
case 2:
return;
}
break;
}

default:
Indestructible indest;
}

A j = (A(buf, 'i'), A(buf, 'j'));
}
}

constexpr bool check_abnormal_termination() {
Buf buf = {};
abnormal_termination(buf);
return buf ==
"abBc"
"dfgh" /*break*/ "HGFijIJeED"
"dfgh" /*continue*/ "HGFeED"
"dfgh" /*return*/ "HGFD"
"CA";
}
static_assert(check_abnormal_termination());
}
Loading