Consider aggregate bases when checking if an InitListExpr is constant

[rnk]

This code was correct as written prior to C++17, which allowed bases to appear in the initializer list.

This was observable by creating non-constant aggregate initialization at file scope in a compound literal, but since that behavior will change soon if we implement support for dynamic initialization, I also added a unit test for isConstantInitializer.

This fixes at least one part of issue #80510 .

[llvmbot]

@llvm/pr-subscribers-clang

Author: Reid Kleckner (rnk)

Changes

This code was correct as written prior to C++17, which allowed bases to appear in the initializer list.

Clang currently requires compound literal initializers at file scope to be constants, which is how I tested this behavior change, but I am open to other testing ideas.

This fixes at least one part of issue #80510 .

---

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

2 Files Affected:

• (modified) clang/lib/AST/Expr.cpp (+12)
• (modified) clang/test/SemaCXX/compound-literal.cpp (+20)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index d665a08deb47e..8852fadf79b9a 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -3342,6 +3342,18 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
     if (ILE->getType()->isRecordType()) {
       unsigned ElementNo = 0;
       RecordDecl *RD = ILE->getType()->castAs<RecordType>()->getDecl();
+
+      // Check bases for C++17 aggregate initializers.
+      if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+        for (unsigned i = 0, e = CXXRD->getNumBases(); i < e; i++) {
+          if (ElementNo < ILE->getNumInits()) {
+            const Expr *Elt = ILE->getInit(ElementNo++);
+            if (!Elt->isConstantInitializer(Ctx, false, Culprit))
+              return false;
+          }
+        }
+      }
+
       for (const auto *Field : RD->fields()) {
         // If this is a union, skip all the fields that aren't being initialized.
         if (RD->isUnion() && ILE->getInitializedFieldInUnion() != Field)
diff --git a/clang/test/SemaCXX/compound-literal.cpp b/clang/test/SemaCXX/compound-literal.cpp
index 5957099de53af..81f8b41ff0313 100644
--- a/clang/test/SemaCXX/compound-literal.cpp
+++ b/clang/test/SemaCXX/compound-literal.cpp
@@ -3,6 +3,7 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify -ast-dump %s > %t-11
 // RUN: FileCheck --input-file=%t-11 %s
 // RUN: FileCheck --input-file=%t-11 %s --check-prefix=CHECK-CXX11
+// RUN: %clang_cc1 -verify -std=c++17 %s
 
 // http://llvm.org/PR7905
 namespace PR7905 {
@@ -108,3 +109,22 @@ int computed_with_lambda = [] {
   return result;
 }();
 #endif
+
+#if __cplusplus >= 201703L
+namespace DynamicFileScopeLiteral {
+// This covers the case where we have a file-scope compound literal with a
+// non-constant initializer in C++. Previously, we had a bug where Clang forgot
+// to consider initializer list elements for bases.
+struct Empty {};
+struct Foo : Empty {
+  int x;
+  int y;
+};
+int f();
+Foo o = (Foo){
+  {},
+  1,
+  f() // expected-error {{initializer element is not a compile-time constant}}
+};
+}
+#endif

[shafik]

gcc accepts this: https://godbolt.org/z/enWrG56je

Why do we believe this should be rejected?

[rnk]

That is correct and I did discover that in my testing, but I think fixing that goes beyond the scope of this patch. Clang currently rejects this case if you remove the empty base: https://godbolt.org/z/1x8hshM75 Adding support for non-constant compound literals at file scope requires codegen changes, and this is fixing a small bug to make more isConstantInitializer correct.

The real issue is that I couldn't find a better way to test isConstantInitializer that will be resilient to that future bug fix, and I'm not sure what to do about that.

[rnk]

I went ahead and make a targeted unit test for isConstantInitializer to see if that is nicer

[shafik]

Should we be checking CPlusPlus17 here?

Also I would like to see a standard quote in the comments. I know we are not doing this locally but these are really helpful and we should be doing this more consistently.

[shafik]

Also in the issue: #80510 you pointed out a crash bug. We should have the bug covered in the test case as well.

[rnk]

Also in the issue: #80510 you pointed out a crash bug. We should have the bug covered in the test case as well.

I think that is covered by the existing test case, instead of crashing, we now reject with an error. In the future if we want to align with GCC as you suggest, we'll need to have a codegen test case for non-constant compound literals of aggregates with bases.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shafik]

@AaronBallman since this involves compound literals a C-ism, I would like you to review this as well.

[AaronBallman]

Please be sure to add a release note so users know about the fix, but aside from a bad standards reference, this LGTM (no concerns about C behavior either).

[rnk]

Thanks! I added notes.