[clang] [clang][bytecode] Fix conditional operator scoping wrt. local variables (PR #169030)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 21 06:50:44 PST 2025
https://github.com/tbaederr updated https://github.com/llvm/llvm-project/pull/169030
>From 480df19e28fcd631528d79802d5e23f5c788e05d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 21 Nov 2025 11:15:12 +0100
Subject: [PATCH] local activate
---
clang/lib/AST/ByteCode/ByteCodeEmitter.h | 2 +-
clang/lib/AST/ByteCode/Compiler.cpp | 34 +++++++++++++++------
clang/lib/AST/ByteCode/Compiler.h | 39 +++++++++++++++++++-----
clang/lib/AST/ByteCode/EvalEmitter.cpp | 29 +++++++++++++++++-
clang/lib/AST/ByteCode/Function.h | 2 ++
clang/lib/AST/ByteCode/Interp.h | 12 ++++++++
clang/lib/AST/ByteCode/InterpFrame.cpp | 12 ++++++++
clang/lib/AST/ByteCode/InterpFrame.h | 4 +++
clang/lib/AST/ByteCode/Opcodes.td | 10 ++++++
clang/test/AST/ByteCode/cxx23.cpp | 23 ++++++++++++++
10 files changed, 148 insertions(+), 19 deletions(-)
diff --git a/clang/lib/AST/ByteCode/ByteCodeEmitter.h b/clang/lib/AST/ByteCode/ByteCodeEmitter.h
index ca8dc38e65246..dd18341d52a09 100644
--- a/clang/lib/AST/ByteCode/ByteCodeEmitter.h
+++ b/clang/lib/AST/ByteCode/ByteCodeEmitter.h
@@ -25,11 +25,11 @@ enum Opcode : uint32_t;
/// An emitter which links the program to bytecode for later use.
class ByteCodeEmitter {
protected:
- using LabelTy = uint32_t;
using AddrTy = uintptr_t;
using Local = Scope::Local;
public:
+ using LabelTy = uint32_t;
/// Compiles the function into the module.
void compileFunc(const FunctionDecl *FuncDecl, Function *Func = nullptr);
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 8f8f3d6a05820..e221d71ea9f2f 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -16,6 +16,7 @@
#include "PrimType.h"
#include "Program.h"
#include "clang/AST/Attr.h"
+#include "llvm/Support/SaveAndRestore.h"
using namespace clang;
using namespace clang::interp;
@@ -2500,17 +2501,18 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
const Expr *TrueExpr = E->getTrueExpr();
const Expr *FalseExpr = E->getFalseExpr();
- auto visitChildExpr = [&](const Expr *E) -> bool {
- LocalScope<Emitter> S(this);
- if (!this->delegate(E))
- return false;
- return S.destroyLocals();
- };
+ // The TrueExpr and FalseExpr of a conditional operator do _not_ create a
+ // scope, which means the local variables created within them unconditionally
+ // always exist. However, we need to later differentiate which branch was
+ // taken and only destroy the varibles of the active branch. This is what the
+ // "enabled" flags on local variables are used for.
+ llvm::SaveAndRestore LAAA(this->VarScope->LocalsAlwaysEnabled,
+ /*NewValue=*/false);
if (std::optional<bool> BoolValue = getBoolValue(Condition)) {
if (*BoolValue)
- return visitChildExpr(TrueExpr);
- return visitChildExpr(FalseExpr);
+ return this->delegate(TrueExpr);
+ return this->delegate(FalseExpr);
}
bool IsBcpCall = false;
@@ -2542,13 +2544,15 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
if (!this->jumpFalse(LabelFalse))
return false;
- if (!visitChildExpr(TrueExpr))
+ if (!this->delegate(TrueExpr))
return false;
+
if (!this->jump(LabelEnd))
return false;
this->emitLabel(LabelFalse);
- if (!visitChildExpr(FalseExpr))
+ if (!this->delegate(FalseExpr))
return false;
+
this->fallthrough(LabelEnd);
this->emitLabel(LabelEnd);
@@ -2955,10 +2959,15 @@ bool Compiler<Emitter>::VisitMaterializeTemporaryExpr(
bool IsVolatile = SubExpr->getType().isVolatileQualified();
unsigned LocalIndex = allocateLocalPrimitive(
E, *SubExprT, IsConst, IsVolatile, E->getExtendingDecl());
+ if (!this->VarScope->LocalsAlwaysEnabled &&
+ !this->emitEnableLocal(LocalIndex, E))
+ return false;
+
if (!this->visit(SubExpr))
return false;
if (!this->emitSetLocal(*SubExprT, LocalIndex, E))
return false;
+
return this->emitGetPtrLocal(LocalIndex, E);
}
@@ -2968,6 +2977,11 @@ bool Compiler<Emitter>::VisitMaterializeTemporaryExpr(
if (UnsignedOrNone LocalIndex =
allocateLocal(E, Inner->getType(), E->getExtendingDecl())) {
InitLinkScope<Emitter> ILS(this, InitLink::Temp(*LocalIndex));
+
+ if (!this->VarScope->LocalsAlwaysEnabled &&
+ !this->emitEnableLocal(*LocalIndex, E))
+ return false;
+
if (!this->emitGetPtrLocal(*LocalIndex, E))
return false;
return this->visitInitializer(SubExpr) && this->emitFinishInit(E);
diff --git a/clang/lib/AST/ByteCode/Compiler.h b/clang/lib/AST/ByteCode/Compiler.h
index 0c6cab9276531..2da06ef7793ed 100644
--- a/clang/lib/AST/ByteCode/Compiler.h
+++ b/clang/lib/AST/ByteCode/Compiler.h
@@ -477,12 +477,14 @@ template <class Emitter> class VariableScope {
VariableScope(Compiler<Emitter> *Ctx, const ValueDecl *VD,
ScopeKind Kind = ScopeKind::Block)
: Ctx(Ctx), Parent(Ctx->VarScope), ValDecl(VD), Kind(Kind) {
+ if (Parent)
+ this->LocalsAlwaysEnabled = Parent->LocalsAlwaysEnabled;
Ctx->VarScope = this;
}
virtual ~VariableScope() { Ctx->VarScope = this->Parent; }
- virtual void addLocal(const Scope::Local &Local) {
+ virtual void addLocal(Scope::Local Local) {
llvm_unreachable("Shouldn't be called");
}
@@ -519,7 +521,6 @@ template <class Emitter> class VariableScope {
if (!P)
break;
}
-
// Add to this scope.
this->addLocal(Local);
}
@@ -530,6 +531,11 @@ template <class Emitter> class VariableScope {
VariableScope *getParent() const { return Parent; }
ScopeKind getKind() const { return Kind; }
+ /// Whether locals added to this scope are enabled by default.
+ /// This is almost always true, except for the two branches
+ /// of a conditional operator.
+ bool LocalsAlwaysEnabled = true;
+
protected:
/// Compiler instance.
Compiler<Emitter> *Ctx;
@@ -576,29 +582,48 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
return Success;
}
- void addLocal(const Scope::Local &Local) override {
+ void addLocal(Scope::Local Local) override {
if (!Idx) {
Idx = static_cast<unsigned>(this->Ctx->Descriptors.size());
this->Ctx->Descriptors.emplace_back();
this->Ctx->emitInitScope(*Idx, {});
}
+ Local.EnabledByDefault = this->LocalsAlwaysEnabled;
this->Ctx->Descriptors[*Idx].emplace_back(Local);
}
bool emitDestructors(const Expr *E = nullptr) override {
if (!Idx)
return true;
+ assert(!this->Ctx->Descriptors[*Idx].empty());
+
// Emit destructor calls for local variables of record
// type with a destructor.
for (Scope::Local &Local : llvm::reverse(this->Ctx->Descriptors[*Idx])) {
if (Local.Desc->hasTrivialDtor())
continue;
- if (!this->Ctx->emitGetPtrLocal(Local.Offset, E))
- return false;
- if (!this->Ctx->emitDestructionPop(Local.Desc, Local.Desc->getLoc()))
- return false;
+ if (!Local.EnabledByDefault) {
+ typename Emitter::LabelTy EndLabel = this->Ctx->getLabel();
+ if (!this->Ctx->emitGetLocalEnabled(Local.Offset, E))
+ return false;
+ if (!this->Ctx->jumpFalse(EndLabel))
+ return false;
+
+ if (!this->Ctx->emitGetPtrLocal(Local.Offset, E))
+ return false;
+
+ if (!this->Ctx->emitDestructionPop(Local.Desc, Local.Desc->getLoc()))
+ return false;
+
+ this->Ctx->emitLabel(EndLabel);
+ } else {
+ if (!this->Ctx->emitGetPtrLocal(Local.Offset, E))
+ return false;
+ if (!this->Ctx->emitDestructionPop(Local.Desc, Local.Desc->getLoc()))
+ return false;
+ }
removeIfStoredOpaqueValue(Local);
}
diff --git a/clang/lib/AST/ByteCode/EvalEmitter.cpp b/clang/lib/AST/ByteCode/EvalEmitter.cpp
index 007321791fdd4..a2e01efc8ffd9 100644
--- a/clang/lib/AST/ByteCode/EvalEmitter.cpp
+++ b/clang/lib/AST/ByteCode/EvalEmitter.cpp
@@ -113,7 +113,7 @@ Scope::Local EvalEmitter::createLocal(Descriptor *D) {
InlineDescriptor &Desc = *reinterpret_cast<InlineDescriptor *>(B->rawData());
Desc.Desc = D;
Desc.Offset = sizeof(InlineDescriptor);
- Desc.IsActive = true;
+ Desc.IsActive = false;
Desc.IsBase = false;
Desc.IsFieldMutable = false;
Desc.IsConst = false;
@@ -322,6 +322,33 @@ bool EvalEmitter::emitDestroy(uint32_t I, SourceInfo Info) {
return true;
}
+bool EvalEmitter::emitGetLocalEnabled(uint32_t I, SourceInfo Info) {
+ if (!isActive())
+ return true;
+
+ Block *B = getLocal(I);
+ const InlineDescriptor &Desc =
+ *reinterpret_cast<InlineDescriptor *>(B->rawData());
+
+ S.Stk.push<bool>(Desc.IsActive);
+ return true;
+}
+
+bool EvalEmitter::emitEnableLocal(uint32_t I, SourceInfo Info) {
+ if (!isActive())
+ return true;
+
+ // FIXME: This is a little dirty, but to avoid adding a flag to
+ // InlineDescriptor that's only ever useful on the toplevel of local
+ // variables, we reuse the IsActive flag for the enabled state. We should
+ // probably use a different struct than InlineDescriptor for the block-level
+ // inline descriptor of local varaibles.
+ Block *B = getLocal(I);
+ InlineDescriptor &Desc = *reinterpret_cast<InlineDescriptor *>(B->rawData());
+ Desc.IsActive = true;
+ return true;
+}
+
/// Global temporaries (LifetimeExtendedTemporary) carry their value
/// around as an APValue, which codegen accesses.
/// We set their value once when creating them, but we don't update it
diff --git a/clang/lib/AST/ByteCode/Function.h b/clang/lib/AST/ByteCode/Function.h
index 95add5809afcc..8c309c921afa9 100644
--- a/clang/lib/AST/ByteCode/Function.h
+++ b/clang/lib/AST/ByteCode/Function.h
@@ -41,6 +41,8 @@ class Scope final {
unsigned Offset;
/// Descriptor of the local.
Descriptor *Desc;
+ /// If the cleanup for this local should be emitted.
+ bool EnabledByDefault = true;
};
using LocalVectorTy = llvm::SmallVector<Local, 8>;
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 3e869c1ee5f2c..86b1ba88ca9d4 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -2474,6 +2474,18 @@ inline bool InitScope(InterpState &S, CodePtr OpPC, uint32_t I) {
return true;
}
+inline bool EnableLocal(InterpState &S, CodePtr OpPC, uint32_t I) {
+ assert(!S.Current->isLocalEnabled(I));
+ S.Current->enableLocal(I);
+ return true;
+}
+
+inline bool GetLocalEnabled(InterpState &S, CodePtr OpPC, uint32_t I) {
+ assert(S.Current);
+ S.Stk.push<bool>(S.Current->isLocalEnabled(I));
+ return true;
+}
+
//===----------------------------------------------------------------------===//
// Cast, CastFP
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/AST/ByteCode/InterpFrame.cpp b/clang/lib/AST/ByteCode/InterpFrame.cpp
index 039acb5d72b2c..3b883761ad001 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.cpp
+++ b/clang/lib/AST/ByteCode/InterpFrame.cpp
@@ -89,11 +89,23 @@ void InterpFrame::destroyScopes() {
void InterpFrame::initScope(unsigned Idx) {
if (!Func)
return;
+
for (auto &Local : Func->getScope(Idx).locals()) {
localBlock(Local.Offset)->invokeCtor();
}
}
+void InterpFrame::enableLocal(unsigned Idx) {
+ assert(Func);
+
+ // FIXME: This is a little dirty, but to avoid adding a flag to
+ // InlineDescriptor that's only ever useful on the toplevel of local
+ // variables, we reuse the IsActive flag for the enabled state. We should
+ // probably use a different struct than InlineDescriptor for the block-level
+ // inline descriptor of local varaibles.
+ localInlineDesc(Idx)->IsActive = true;
+}
+
void InterpFrame::destroy(unsigned Idx) {
for (auto &Local : Func->getScope(Idx).locals_reverse()) {
S.deallocate(localBlock(Local.Offset));
diff --git a/clang/lib/AST/ByteCode/InterpFrame.h b/clang/lib/AST/ByteCode/InterpFrame.h
index fa9de2e1e7c6d..8f563c8b9348c 100644
--- a/clang/lib/AST/ByteCode/InterpFrame.h
+++ b/clang/lib/AST/ByteCode/InterpFrame.h
@@ -55,6 +55,10 @@ class InterpFrame final : public Frame {
void destroy(unsigned Idx);
void initScope(unsigned Idx);
void destroyScopes();
+ void enableLocal(unsigned Idx);
+ bool isLocalEnabled(unsigned Idx) const {
+ return localInlineDesc(Idx)->IsActive;
+ }
/// Describes the frame with arguments for diagnostic purposes.
void describe(llvm::raw_ostream &OS) const override;
diff --git a/clang/lib/AST/ByteCode/Opcodes.td b/clang/lib/AST/ByteCode/Opcodes.td
index a236f89dcf78b..6e768793fcfcf 100644
--- a/clang/lib/AST/ByteCode/Opcodes.td
+++ b/clang/lib/AST/ByteCode/Opcodes.td
@@ -251,6 +251,16 @@ def InitScope : Opcode {
let Args = [ArgUint32];
}
+def GetLocalEnabled : Opcode {
+ let Args = [ArgUint32];
+ let HasCustomEval = 1;
+}
+
+def EnableLocal : Opcode {
+ let Args = [ArgUint32];
+ let HasCustomEval = 1;
+}
+
//===----------------------------------------------------------------------===//
// Constants
//===----------------------------------------------------------------------===//
diff --git a/clang/test/AST/ByteCode/cxx23.cpp b/clang/test/AST/ByteCode/cxx23.cpp
index c5d26925ce11b..819460628c1b7 100644
--- a/clang/test/AST/ByteCode/cxx23.cpp
+++ b/clang/test/AST/ByteCode/cxx23.cpp
@@ -473,3 +473,26 @@ namespace AIEWithIndex0Narrows {
}
static_assert(test());
}
+
+#if __cplusplus >= 202302L
+namespace InactiveLocalsInConditionalOp {
+ struct A { constexpr A(){}; ~A(); constexpr int get() { return 10; } }; // all-note 2{{declared here}}
+ constexpr int get(bool b) {
+ return b ? A().get() : 1; // all-note {{non-constexpr function '~A' cannot be used in a constant expression}}
+ }
+ static_assert(get(false) == 1, "");
+ static_assert(get(true) == 10, ""); // all-error {{not an integral constant expression}} \
+ // all-note {{in call to}}
+
+ static_assert( (false ? A().get() : 1) == 1);
+ static_assert( (true ? A().get() : 1) == 1); // all-error {{not an integral constant expression}} \
+ // all-note {{non-constexpr function '~A' cannot be used in a constant expression}}
+
+ constexpr bool test2(bool b) {
+ unsigned long __ms = b ? (const unsigned long &)0 : __ms;
+ return true;
+ }
+ static_assert(test2(true));
+
+}
+#endif
More information about the cfe-commits
mailing list