llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Timm Baeder (tbaederr)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/169030.diff
10 Files Affected:
- (modified) clang/lib/AST/ByteCode/ByteCodeEmitter.h (+1-1)
- (modified) clang/lib/AST/ByteCode/Compiler.cpp (+24-10)
- (modified) clang/lib/AST/ByteCode/Compiler.h (+32-6)
- (modified) clang/lib/AST/ByteCode/EvalEmitter.cpp (+28-1)
- (modified) clang/lib/AST/ByteCode/Function.h (+2)
- (modified) clang/lib/AST/ByteCode/Interp.h (+12)
- (modified) clang/lib/AST/ByteCode/InterpFrame.cpp (+12)
- (modified) clang/lib/AST/ByteCode/InterpFrame.h (+4)
- (modified) clang/lib/AST/ByteCode/Opcodes.td (+10)
- (modified) clang/test/AST/ByteCode/cxx23.cpp (+17)
``````````diff
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..f9d31b5e73f89 100644
--- a/clang/lib/AST/ByteCode/Compiler.h
+++ b/clang/lib/AST/ByteCode/Compiler.h
@@ -477,6 +477,8 @@ 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;
}
@@ -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;
@@ -583,22 +589,42 @@ template <class Emitter> class LocalScope : public
VariableScope<Emitter> {
this->Ctx->emitInitScope(*Idx, {});
}
- this->Ctx->Descriptors[*Idx].emplace_back(Local);
+ Scope::Local L = Local;
+ L.EnabledByDefault = this->LocalsAlwaysEnabled;
+ this->Ctx->Descriptors[*Idx].emplace_back(L);
}
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..ef92ff9218fba 100644
--- a/clang/test/AST/ByteCode/cxx23.cpp
+++ b/clang/test/AST/ByteCode/cxx23.cpp
@@ -473,3 +473,20 @@ 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}}
+
+}
+#endif
``````````
</details>
https://github.com/llvm/llvm-project/pull/169030
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
