r359958 - Disallow the operand of __builtin_constant_p from modifying enclosing
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri May 3 21:00:45 PDT 2019
Author: rsmith
Date: Fri May 3 21:00:45 2019
New Revision: 359958
URL: http://llvm.org/viewvc/llvm-project?rev=359958&view=rev
Log:
Disallow the operand of __builtin_constant_p from modifying enclosing
state when it's encountered while evaluating a constexpr function.
We attempt to follow GCC trunk's behavior here, but it is somewhat
inscrutible, so our behavior is only approximately the same for now.
Specifically, we only permit modification of objects whose lifetime
began within the operand of the __builtin_constant_p. GCC appears to
have effectively the same restriction, but also some unknown restriction
based on where and how the local state of the constexpr function is
mentioned within the operand (see added testcases).
Modified:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/SemaCXX/builtin-constant-p.cpp
Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=359958&r1=359957&r2=359958&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Fri May 3 21:00:45 2019
@@ -716,6 +716,10 @@ namespace {
EvaluatingObject(Decl, {CallIndex, Version}));
}
+ /// If we're currently speculatively evaluating, the outermost call stack
+ /// depth at which we can mutate state, otherwise 0.
+ unsigned SpeculativeEvaluationDepth = 0;
+
/// The current array initialization index, if we're performing array
/// initialization.
uint64_t ArrayInitIndex = -1;
@@ -728,9 +732,6 @@ namespace {
/// fold (not just why it's not strictly a constant expression)?
bool HasFoldFailureDiagnostic;
- /// Whether or not we're currently speculatively evaluating.
- bool IsSpeculativelyEvaluating;
-
/// Whether or not we're in a context where the front end requires a
/// constant value.
bool InConstantContext;
@@ -795,7 +796,7 @@ namespace {
BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
EvaluatingDecl((const ValueDecl *)nullptr),
EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
- HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
+ HasFoldFailureDiagnostic(false),
InConstantContext(false), EvalMode(Mode) {}
void setEvaluatingDecl(APValue::LValueBase Base, APValue &Value) {
@@ -823,14 +824,20 @@ namespace {
return false;
}
- CallStackFrame *getCallFrame(unsigned CallIndex) {
- assert(CallIndex && "no call index in getCallFrame");
+ std::pair<CallStackFrame *, unsigned>
+ getCallFrameAndDepth(unsigned CallIndex) {
+ assert(CallIndex && "no call index in getCallFrameAndDepth");
// We will eventually hit BottomFrame, which has Index 1, so Frame can't
// be null in this loop.
+ unsigned Depth = CallStackDepth;
CallStackFrame *Frame = CurrentCall;
- while (Frame->Index > CallIndex)
+ while (Frame->Index > CallIndex) {
Frame = Frame->Caller;
- return (Frame->Index == CallIndex) ? Frame : nullptr;
+ --Depth;
+ }
+ if (Frame->Index == CallIndex)
+ return {Frame, Depth};
+ return {nullptr, 0};
}
bool nextStep(const Stmt *S) {
@@ -1111,12 +1118,12 @@ namespace {
class SpeculativeEvaluationRAII {
EvalInfo *Info = nullptr;
Expr::EvalStatus OldStatus;
- bool OldIsSpeculativelyEvaluating;
+ unsigned OldSpeculativeEvaluationDepth;
void moveFromAndCancel(SpeculativeEvaluationRAII &&Other) {
Info = Other.Info;
OldStatus = Other.OldStatus;
- OldIsSpeculativelyEvaluating = Other.OldIsSpeculativelyEvaluating;
+ OldSpeculativeEvaluationDepth = Other.OldSpeculativeEvaluationDepth;
Other.Info = nullptr;
}
@@ -1125,7 +1132,7 @@ namespace {
return;
Info->EvalStatus = OldStatus;
- Info->IsSpeculativelyEvaluating = OldIsSpeculativelyEvaluating;
+ Info->SpeculativeEvaluationDepth = OldSpeculativeEvaluationDepth;
}
public:
@@ -1134,9 +1141,9 @@ namespace {
SpeculativeEvaluationRAII(
EvalInfo &Info, SmallVectorImpl<PartialDiagnosticAt> *NewDiag = nullptr)
: Info(&Info), OldStatus(Info.EvalStatus),
- OldIsSpeculativelyEvaluating(Info.IsSpeculativelyEvaluating) {
+ OldSpeculativeEvaluationDepth(Info.SpeculativeEvaluationDepth) {
Info.EvalStatus.Diag = NewDiag;
- Info.IsSpeculativelyEvaluating = true;
+ Info.SpeculativeEvaluationDepth = Info.CallStackDepth + 1;
}
SpeculativeEvaluationRAII(const SpeculativeEvaluationRAII &Other) = delete;
@@ -3138,8 +3145,10 @@ static CompleteObject findCompleteObject
}
CallStackFrame *Frame = nullptr;
+ unsigned Depth = 0;
if (LVal.getLValueCallIndex()) {
- Frame = Info.getCallFrame(LVal.getLValueCallIndex());
+ std::tie(Frame, Depth) =
+ Info.getCallFrameAndDepth(LVal.getLValueCallIndex());
if (!Frame) {
Info.FFDiag(E, diag::note_constexpr_lifetime_ended, 1)
<< AK << LVal.Base.is<const ValueDecl*>();
@@ -3330,7 +3339,7 @@ static CompleteObject findCompleteObject
// to be read here (but take care with 'mutable' fields).
if ((Frame && Info.getLangOpts().CPlusPlus14 &&
Info.EvalStatus.HasSideEffects) ||
- (AK != AK_Read && Info.IsSpeculativelyEvaluating))
+ (AK != AK_Read && Depth < Info.SpeculativeEvaluationDepth))
return CompleteObject();
return CompleteObject(BaseVal, BaseType, LifetimeStartedInEvaluation);
@@ -7823,6 +7832,10 @@ static bool EvaluateBuiltinConstantPForL
/// EvaluateBuiltinConstantP - Evaluate __builtin_constant_p as similarly to
/// GCC as we can manage.
static bool EvaluateBuiltinConstantP(EvalInfo &Info, const Expr *Arg) {
+ // This evaluation is not permitted to have side-effects, so evaluate it in
+ // a speculative evaluation context.
+ SpeculativeEvaluationRAII SpeculativeEval(Info);
+
// Constant-folding is always enabled for the operand of __builtin_constant_p
// (even when the enclosing evaluation context otherwise requires a strict
// language-specific constant expression).
Modified: cfe/trunk/test/SemaCXX/builtin-constant-p.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-constant-p.cpp?rev=359958&r1=359957&r2=359958&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/builtin-constant-p.cpp (original)
+++ cfe/trunk/test/SemaCXX/builtin-constant-p.cpp Fri May 3 21:00:45 2019
@@ -59,3 +59,74 @@ static_assert(bcp_fold(int_to_ptr(0)));
static_assert(bcp(int_to_ptr(123))); // GCC rejects these due to not recognizing
static_assert(bcp_fold(int_to_ptr(123))); // the bcp conditional in 'int_to_ptr' ...
static_assert(__builtin_constant_p((int*)123)); // ... but GCC accepts this
+
+// State mutations in the operand are not permitted.
+//
+// The rule GCC uses for this is not entirely understood, but seems to depend
+// in some way on what local state is mentioned in the operand of
+// __builtin_constant_p and where.
+//
+// We approximate GCC's rule by evaluating the operand in a speculative
+// evaluation context; only state created within the evaluation can be
+// modified.
+constexpr int mutate1() {
+ int n = 1;
+ int m = __builtin_constant_p(++n);
+ return n * 10 + m;
+}
+static_assert(mutate1() == 10);
+
+// FIXME: GCC treats this as being non-constant because of the "n = 2", even
+// though evaluation in the context of the enclosing constant expression
+// succeeds without mutating any state.
+constexpr int mutate2() {
+ int n = 1;
+ int m = __builtin_constant_p(n ? n + 1 : n = 2);
+ return n * 10 + m;
+}
+static_assert(mutate2() == 11);
+
+constexpr int internal_mutation(int unused) {
+ int x = 1;
+ ++x;
+ return x;
+}
+
+constexpr int mutate3() {
+ int n = 1;
+ int m = __builtin_constant_p(internal_mutation(0));
+ return n * 10 + m;
+}
+static_assert(mutate3() == 11);
+
+constexpr int mutate4() {
+ int n = 1;
+ int m = __builtin_constant_p(n ? internal_mutation(0) : 0);
+ return n * 10 + m;
+}
+static_assert(mutate4() == 11);
+
+// FIXME: GCC treats this as being non-constant because of something to do with
+// the 'n' in the argument to internal_mutation.
+constexpr int mutate5() {
+ int n = 1;
+ int m = __builtin_constant_p(n ? internal_mutation(n) : 0);
+ return n * 10 + m;
+}
+static_assert(mutate5() == 11);
+
+constexpr int mutate_param(bool mutate, int ¶m) {
+ mutate = mutate; // Mutation of internal state is OK
+ if (mutate)
+ ++param;
+ return param;
+}
+constexpr int mutate6(bool mutate) {
+ int n = 1;
+ int m = __builtin_constant_p(mutate_param(mutate, n));
+ return n * 10 + m;
+}
+// No mutation of state outside __builtin_constant_p: evaluates to true.
+static_assert(mutate6(false) == 11);
+// Mutation of state outside __builtin_constant_p: evaluates to false.
+static_assert(mutate6(true) == 10);
More information about the cfe-commits
mailing list