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).

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@359958 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/AST/ExprConstant.cpp b/lib/AST/ExprConstant.cpp
index 6b3f4dcefa68ddfbdb591e8840fb1a734b84074b..11e753c07713a347464c8288889ec35e25cfce18 100644 (file)
--- a/lib/AST/ExprConstant.cpp
+++ b/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<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(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<const ValueDecl*>();
@@ -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/test/SemaCXX/builtin-constant-p.cpp b/test/SemaCXX/builtin-constant-p.cpp
index 252c7bbac57562c432e3d313311533f5838c5a79..21cbaf7c699a8b740bf2a8958cf69a620b9d1dc2 100644 (file)
--- a/test/SemaCXX/builtin-constant-p.cpp
+++ b/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 &param) {
+  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);