[clang] Prevent folding of non-const compound expr

[serge-sans-paille]

Summary

When a non-const compound statement is used to initialize a constexpr pointer,
the pointed value is not const itself and cannot be folded at codegen time.

This matches GCC behavior for compund literal array

Fix issue #39324.

[efriedma]

The fix doesn't look right.

A CompoundLiteralExpr is itself an lvalue; we should catch any issues when we visit it, not a VarDecl that contains a pointer to it.

[serge-sans-paille]

In D124038#3460731, @efriedma wrote:

The fix doesn't look right.

A CompoundLiteralExpr is itself an lvalue; we should catch any issues when we visit it, not a VarDecl that contains a pointer to it.

Well, the visitor for CompoundLiteralExpr explicitly states

// Defer visiting the literal until the lvalue-to-rvalue conversion. We can
// only see this when folding in C, so there's no standard to follow here.

Which is what I was trying to do in this patch.
We only visit CompoundLiteralExpr once, during initialization of the VarDecl, and that initialization is correct. We don't visit it again during checking of expression constness because we stop at the VarDecl level. I'm checking if we could peform an extra check at that point. But maybe I'm totally taking a wrong turn, feel free to advise :-)

[serge-sans-paille]

Alternatively, the following also works, but it splits the logic into anotherplace, while current patch is at least consistent with existing state

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 498f0d4..233307f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3352,6 +3352,17 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
     NoteLValueLocation(Info, Base);
   }
 
+  // In the particular case of CompoundLiteralExpr initialization, check that it is itself
+  // constant.
+  if (Info.InConstantContext)
+    if (const CompoundLiteralExpr *CLE = dyn_cast_or_null<CompoundLiteralExpr>(
+            VD->getAnyInitializer()->IgnoreCasts())) {
+      QualType CLET = CLE->getType().getCanonicalType();
+      if (!CLET.isConstant(Info.Ctx)) {
+        Info.FFDiag(E);
+      }
+    }
+

[efriedma]

The VD->getAnyInitializer()->IgnoreCasts() is the part that's wrong. We want to allow something like the following:

constexpr int *q = (int *)(int[1]){3};
constexpr int *q2 = q;

To catch the bad cases specifically, we have to wait until we actually reach the compound literal. See line 4262, or something like that.

[serge-sans-paille]

Match GCC behavior here: some test case were previously accepted while having the opposite behavior. This pacth both fixes the original issue and adopt gcc behavior.

[efriedma]

clang/lib/AST/ExprConstant.cpp

4267

Is the "isArrayType()" check here necessary?

[serge-sans-paille]

clang/lib/AST/ExprConstant.cpp

4267

Yeah, otherwise we have an issue with

typedef __attribute__(( ext_vector_type(4) )) float float4;
float4 foo = (float4){ 1.0, 2.0, 3.0, 4.0 };

error: cannot compile this static initializer yet

[efriedma]

clang/lib/AST/ExprConstant.cpp

4267

Hmm, I see... in C++, the compound literal rules are weird. C compound literals are lvalues, but C++ ones are not. So normally, we don't allow taking the address of a compound literal or any of its members. But if it's an array, somehow different rules (what rules?) apply.

If you can add a comment explaining what's going on here, maybe it's okay?

I'm tempted to say we should reject the testcase, though. For example, in the following, it doesn't really make sense to reject the first variable, but accept the second.

typedef int arr[2];
constexpr int *x = arr{1};
constexpr int *y = (arr){1};

---

Can you add a testcase involving an array of structs with a "mutable" member?

[serge-sans-paille]

Added reg to GCC info page to explain current behavior, and make the test more explicit with respect to that quote.

clang/test/SemaCXX/constant-expression-cxx11.cpp

1609

@efriedma this is the test you asked for. it's already implemented, and my patch doesn't change its behavior, which matches the GCC behavior.

[efriedma]

I think you're looking at old documentation? Here's what the current page (https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html) has to say:

In C, a compound literal designates an unnamed object with static or automatic storage duration. In C++, a compound literal designates a temporary object that only lives until the end of its full-expression. As a result, well-defined C code that takes the address of a subobject of a compound literal can be undefined in C++, so G++ rejects the conversion of a temporary array to a pointer. For instance, if the array compound literal example above appeared inside a function, any subsequent use of foo in C++ would have undefined behavior because the lifetime of the array ends after the declaration of foo.

As an optimization, G++ sometimes gives array compound literals longer lifetimes: when the array either appears outside a function or has a const-qualified type. If foo and its initializer had elements of type char *const rather than char *, or if foo were a global variable, the array would have static storage duration. But it is probably safest just to avoid the use of array compound literals in C++ code.

[serge-sans-paille]

In D124038#3504371, @efriedma wrote:

I think you're looking at old documentation? Here's what the current page (https://gcc.gnu.org/onlinedocs/gcc/Compound-Literals.html) has to say:

Indeed! I was looking at my local Info Page. thanks for the extra pointer.

As an optimization, G++ sometimes gives array compound literals longer lifetimes: when the array either appears outside a function or has a const-qualified type. If foo and its initializer had elements of type char *const rather than char *, or if foo were a global variable, the array would have static storage duration. But it is probably safest just to avoid the use of array compound literals in C++ code.

I can quote that part instead. I don't think this invalidates the actual code, right? At least with that commit the observable behavior gets closer to GCC, and it fixes https://github.com/llvm/llvm-project/issues/39324

[efriedma]

I think so, yes.

[serge-sans-paille]

Update GCC manual quote

[efriedma]

Updated messed up formatting?

Otherwise, I guess this is fine.

[serge-sans-paille]

Update messed up format

[efriedma]

LGTM

clang/lib/AST/ExprConstant.cpp

if (Base && !LVal.getLValueCallIndex() && !Type.isVolatileQualified()) {
     if (const CompoundLiteralExpr *CLE = dyn_cast<CompoundLiteralExpr>(Base)) {
       // In C99, a CompoundLiteralExpr is an lvalue, and we defer evaluating the
       // initializer until now for such expressions. Such an expression can't be
       // an ICE in C, so this only matters for fold.
       if (Type.isVolatileQualified()) {
         Info.FFDiag(Conv);
         return false;
       }
+
       APValue Lit;
       if (!Evaluate(Lit, Info, CLE->getInitializer()))
         return false;
+
+      // According to GCC info page:
+      //
+      // 6.28 Compound Literals
+      //
+      // As an optimization, G++ sometimes gives array compound literals longer
+      // lifetimes: when the array either appears outside a function or has a
+      // const-qualified type. If foo and its initializer had elements of type
+      // char *const rather than char *, or if foo were a global variable, the
+      // array would have static storage duration. But it is probably safest
+      // just to avoid the use of array compound literals in C++ code.
+      //
+      // Obey that rule by checking constness for converted array types.
+
+      QualType CLETy = CLE->getType();
+      if (CLETy->isArrayType() && !Type->isArrayType()) {
+        if (!CLETy.isConstant(Info.Ctx)) {
+          Info.FFDiag(Conv);
+          Info.Note(CLE->getExprLoc(), diag::note_declared_at);
+          return false;
+        }
+      }
+
       CompleteObject LitObj(LVal.Base, &Lit, Base->getType());
       return extractSubobject(Info, Conv, LitObj, LVal.Designator, RVal, AK);
     } else if (isa<StringLiteral>(Base) || isa<PredefinedExpr>(Base)) {
       // Special-case character extraction so we don't have to construct an
       // APValue for the whole string.
       assert(LVal.Designator.Entries.size() <= 1 &&
              "Can only read characters from string literals");
       if (LVal.Designator.Entries.empty()) {

clang/test/SemaCXX/constant-expression-cxx11.cpp

constexpr int n = 1 +
         * 3;
   }
 }
 
 namespace CompoundLiteral {
   // Matching GCC, file-scope array compound literals initialized by constants
   // are lifetime-extended.
   constexpr int *p = (int*)(int[1]){3}; // expected-warning {{C99}}
-  static_assert(*p == 3, "");
+  static_assert(*p == 3, "");           // expected-error {{static_assert expression is not an integral constant expression}}
+                                        // expected-note@-1 {{subexpression not valid}}
+                                        // expected-note@-3 {{declared here}}
   static_assert((int[2]){1, 2}[1] == 2, ""); // expected-warning {{C99}}
+  // expected-error@-1 {{static_assert expression is not an integral constant expression}}
+  // expected-note@-2 {{subexpression not valid}}
+  // expected-note@-3 {{declared here}}
 
   // Other kinds are not.
   struct X { int a[2]; };
   constexpr int *n = (X){1, 2}.a; // expected-warning {{C99}} expected-warning {{temporary}}
   // expected-error@-1 {{constant expression}}
   // expected-note@-2 {{pointer to subobject of temporary}}
   // expected-note@-3 {{temporary created here}}
 
   void f() {
     static constexpr int *p = (int*)(int[1]){3}; // expected-warning {{C99}} expected-warning {{temporary}}
     // expected-error@-1 {{constant expression}}
     // expected-note@-2 {{pointer to subobject of temporary}}

clang/test/SemaTemplate/constexpr-instantiate.cpp

namespace Unevaluated {
   // instantiations would odr-use, or used in list initialization), if they
   // can be used as a constant (const integral or constexpr).
   namespace Variables {
     template<int N> struct A {
       static const int k;
       static int n;
     };
     template<const int *N> struct B {};
-    template<int N> constexpr int A<N>::k = *(int[N]){N}; // expected-error 1+{{negative}}
+    template <int N> constexpr int A<N>::k = *(int[N]){N}; // expected-error 1+{{negative}} expected-note 1+{{not valid in a constant expression}} expected-note 1+{{declared here}}
+    // expected-error@-1 1+{{must be initialized by a constant expression}}
+
     template<int N> int A<N>::n = *(int[N]){0};
 
     template <typename> void f() {
       (void)A<-1>::n; // ok
       (void)A<-1>::k; // expected-note {{instantiation of }}
       B<&A<-2>::n> b1; // ok
       B<&A<-2>::k> b2; // expected-note {{instantiation of }}
     };
 
     decltype(A<-3>::k) d1 = 0; // ok
-    decltype(char{A<-4>::k}) d2 = 0; // expected-note {{instantiation of }} expected-error {{narrow}} expected-note {{cast}}
-    decltype(char{A<1>::k}) d3 = 0; // ok
-    decltype(char{A<1 + (unsigned char)-1>::k}) d4 = 0; // expected-error {{narrow}} expected-note {{cast}}
+    decltype(char{A<-4>::k}) d2 = 0;                    // expected-note 1+{{instantiation of }} expected-error {{narrow}} expected-note {{cast}}
+    decltype(char{A<1>::k}) d3 = 0;                     // expected-note 1+{{instantiation of }} expected-error {{narrow}} expected-note {{cast}}
+    decltype(char{A<1 + (unsigned char)-1>::k}) d4 = 0; // expected-error {{narrow}} expected-note {{cast}}  expected-note {{instantiation of}}
   }
 }
 
 namespace NoInstantiationWhenSelectingOverload {
   // Check that we don't instantiate conversion functions when we're checking
   // for the existence of an implicit conversion sequence, only when a function
   // is actually chosen by overload resolution.
   struct S {