[clang][ExprConst] Allow mutation in __builtin_constant_p's argument

[tbaederr]

Allow side effects and mutation of local variables in the argument of a __builtin_constant_p call.

This aligns clang's behavior with GCC's current behavior: https://godbolt.org/z/8xhMxY6rx

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Allow side effects and mutation of local variables in the argument of a __builtin_constant_p call.

This aligns clang's behavior with GCC's current behavior: https://godbolt.org/z/8xhMxY6rx

---

Full diff: https://github.com/llvm/llvm-project/pull/159599.diff

3 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+1-6)
• (modified) clang/test/AST/ByteCode/builtin-constant-p.cpp (+2-4)
• (modified) clang/test/SemaCXX/builtin-constant-p.cpp (+6-16)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index bea592d04c2df..ae53e921ffe8c 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -4566,13 +4566,8 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E,
   //
   // FIXME: Not all local state is mutable. Allow local constant subobjects
   // to be read here (but take care with 'mutable' fields).
-  unsigned VisibleDepth = Depth;
-  if (llvm::isa_and_nonnull<ParmVarDecl>(
-          LVal.Base.dyn_cast<const ValueDecl *>()))
-    ++VisibleDepth;
   if ((Frame && Info.getLangOpts().CPlusPlus14 &&
-       Info.EvalStatus.HasSideEffects) ||
-      (isModification(AK) && VisibleDepth < Info.SpeculativeEvaluationDepth))
+       Info.EvalStatus.HasSideEffects))
     return CompleteObject();
 
   return CompleteObject(LVal.getLValueBase(), BaseVal, BaseType);
diff --git a/clang/test/AST/ByteCode/builtin-constant-p.cpp b/clang/test/AST/ByteCode/builtin-constant-p.cpp
index 315a907949c34..3f0f203735640 100644
--- a/clang/test/AST/ByteCode/builtin-constant-p.cpp
+++ b/clang/test/AST/ByteCode/builtin-constant-p.cpp
@@ -103,8 +103,7 @@ constexpr int mutate1() {
   int m = __builtin_constant_p(++n);
   return n * 10 + m;
 }
-static_assert(mutate1() == 21); // ref-error {{static assertion failed}} \
-                                // ref-note {{evaluates to '10 == 21'}}
+static_assert(mutate1() == 21);
 
 /// Similar for this. GCC agrees with the bytecode interpreter.
 constexpr int mutate_param(bool mutate, int &param) {
@@ -119,8 +118,7 @@ constexpr int mutate6(bool mutate) {
   return n * 10 + m;
 }
 static_assert(mutate6(false) == 11);
-static_assert(mutate6(true) == 21); // ref-error {{static assertion failed}} \
-                                    // ref-note {{evaluates to '10 == 21'}}
+static_assert(mutate6(true) == 21);
 
 #define fold(x) (__builtin_constant_p(x) ? (x) : (x))
 void g() {
diff --git a/clang/test/SemaCXX/builtin-constant-p.cpp b/clang/test/SemaCXX/builtin-constant-p.cpp
index 71c38c5c63c71..af56a805bd0d4 100644
--- a/clang/test/SemaCXX/builtin-constant-p.cpp
+++ b/clang/test/SemaCXX/builtin-constant-p.cpp
@@ -61,25 +61,14 @@ static_assert(bcp(int_to_ptr(123)));      // GCC rejects these due to not recogn
 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.
+// State mutations in the operand are permitted.
 constexpr int mutate1() {
   int n = 1;
   int m = __builtin_constant_p(++n);
   return n * 10 + m;
 }
-static_assert(mutate1() == 10);
+static_assert(mutate1() == 21);
 
-// 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);
@@ -107,8 +96,6 @@ constexpr int mutate4() {
 }
 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);
@@ -130,7 +117,7 @@ constexpr int mutate6(bool mutate) {
 // 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);
+static_assert(mutate6(true) == 21);
 
 // GCC strangely returns true for the address of a type_info object, despite it
 // not being a pointer to the start of a string literal.
@@ -151,6 +138,9 @@ namespace dtor_side_effect {
 }
 
 #if __cplusplus >= 202002L
+
+static_assert(__builtin_constant_p((delete new int, 0))); // expected-error {{failed}}
+
 namespace constexpr_dtor {
   struct A {
     int *p;

[tbaederr]

Not sure what the reason is we have the same behavior in __builtin_object_size. GCC seems to evaluate the side effects as well.

[shafik]

I think I would feel more comfortable if there was a justification of why this is allowed.

Looking at the documentation they talk about "constant folding" and I know we diverge from gcc in a some ways here.

It also says "Any expression that has side-effects makes the function return 0" which seems inconsistent w/ this behavior.

Offline someone was saying the behavior has changed a few times on gcc's side and I don't want to be targeting a moving target without some more discussions about this.

[cor3ntin]

I agree with @shafik here, this needs more motivation.
I'm not sure we want to follow GCC's behavior here.
In general __builtin_constant_p is pretty confusing and I would prefer to be conservative - or at least changing it might warrant a wider discussion / rfc