Author: Timm Baeder Date: 2025-11-24T07:34:48+01:00 New Revision: 95f0fab7fab48bbf37d3c02c0ea8b01ca73c30dd
URL: https://github.com/llvm/llvm-project/commit/95f0fab7fab48bbf37d3c02c0ea8b01ca73c30dd DIFF: https://github.com/llvm/llvm-project/commit/95f0fab7fab48bbf37d3c02c0ea8b01ca73c30dd.diff LOG: [clang][bytecode] Fix conditional operator scoping wrt. local variables (#169030) We used to create a scope for the true- and false expression of a conditional operator. This was done so e.g. in this example: ```c++ struct A { constexpr A(){}; ~A(); constexpr int get() { return 10; } }; // all-note 2{{declared here}} static_assert( (false ? A().get() : 1) == 1); ``` we did _not_ evaluate the true branch at all, meaning we did not register the local variable for the temporary of type `A`, which means we also didn't call it destructor. However, this breaks the case where the temporary needs to outlive the conditional operator and instead be destroyed via the surrounding `ExprWithCleanups`: ``` constexpr bool test2(bool b) { unsigned long __ms = b ? (const unsigned long &)0 : __ms; return true; } static_assert(test2(true)); ``` Before this patch, we diagnosed this example: ```console ./array.cpp:180:15: error: static assertion expression is not an integral constant expression 180 | static_assert(test2(true)); | ^~~~~~~~~~~ ./array.cpp:177:24: note: read of temporary whose lifetime has ended 177 | unsigned long __ms = b ? (const unsigned long &)0 : __ms; | ^ ./array.cpp:180:15: note: in call to 'test2(true)' 180 | static_assert(test2(true)); | ^~~~~~~~~~~ ./array.cpp:177:51: note: temporary created here 177 | unsigned long __ms = b ? (const unsigned long &)0 : __ms; | ^ 1 error generated. ``` because the temporary created for the true branch got immediately destroyed. The problem in essence is that since the conditional operator doesn't create a scope at all, we register the local variables for both its branches, but we later only execute one of them, which means we should also only destroy the locals of one of the branches. We fix this similar to clang codgen's `is_active` flag: In the case of a conditional operator (which is so far the only case where this is problematic, and this also helps minimize the performance impact of this change), we make local variables as disabled-by-default and then emit a `EnableLocal` opcode later, which marks them as enabled. The code calling their destructors checks whether the local was enabled at all. Added: Modified: clang/lib/AST/ByteCode/ByteCodeEmitter.h clang/lib/AST/ByteCode/Compiler.cpp clang/lib/AST/ByteCode/Compiler.h clang/lib/AST/ByteCode/EvalEmitter.cpp clang/lib/AST/ByteCode/Function.h clang/lib/AST/ByteCode/Interp.h clang/lib/AST/ByteCode/InterpFrame.cpp clang/lib/AST/ByteCode/InterpFrame.h clang/lib/AST/ByteCode/Opcodes.td clang/test/AST/ByteCode/cxx23.cpp Removed: ################################################################################ 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 725db1f77f29c..958373ed94fe3 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 diff erentiate 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 359bf28a51c6e..6a9fe1e954530 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); } @@ -529,6 +530,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; @@ -566,29 +572,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 diff erent 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 diff erent 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 febef1097ea8a..e150e9279a6ef 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
