From 37be3363b54b5bf30e3c76ab48c8206da45dcd0e Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Sat, 4 May 2019 04:00:45 +0000 Subject: [PATCH] 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). llvm-svn: 359958 --- clang/lib/AST/ExprConstant.cpp | 43 ++++++++++++------- clang/test/SemaCXX/builtin-constant-p.cpp | 71 +++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 15 deletions(-) diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 6b3f4dc..11e753c 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/clang/lib/AST/ExprConstant.cpp @@ -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 + 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 *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(EvalInfo &Info, const Expr *E, } 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(); @@ -3330,7 +3339,7 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, // 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 EvaluateBuiltinConstantPForLValue(const APValue &LV) { /// 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). diff --git a/clang/test/SemaCXX/builtin-constant-p.cpp b/clang/test/SemaCXX/builtin-constant-p.cpp index 252c7bb..21cbaf7 100644 --- a/clang/test/SemaCXX/builtin-constant-p.cpp +++ b/clang/test/SemaCXX/builtin-constant-p.cpp @@ -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); -- 2.7.4