[clang] Fix default initializers being ignored when initializing templated aggregate types

[ayzhao]

Summary

Previously, when checking whether an in-class initializer exists when
performing parenthesized aggregate initialization, Clang checks that the
output of FieldDecl::getInClassInitializer() is non-null. This is
incorrect; if the field is part of a templated type, then
getInClassInitializer() will return nullptr if we haven't called
Sem::BuildCXXDefaultInitExpr(...) before, even if
FieldDecl::hasInClassInitializer() returns true. The end result is that
Clang incorrectly ignores the in class initializer and
value-initializes the field. The fix therefore is to instead call
FieldDecl::hasInClassInitializer(), which is what we do for braced init
lists [0].

Before this patch, Clang does correctly recognize the in-class field
initializer in certain cases. This is Sema::BuildCXXDefaultInitExpr(...)
populates the in class initializer of the corresponding FieldDecl
object. Therefore, if that method was previously called with the same
FieldDecl object, as can happen with a decltype(...) or a braced list
initialization, FieldDecl::getInClassInitializer() will return a
non-null expression, and the field becomes properly initialized.

Fixes 62266

[0]: https://github.com/llvm/llvm-project/blob/be5f35e24f4c15caf3c4aeccddc54c52560c28a0/clang/lib/Sema/SemaInit.cpp#L685

[shafik]

So the change makes sense but the original issue: https://github.com/llvm/llvm-project/issues/62266 it worked for the first once but not the second, it also worked for the non-paren aggregate init. So I feel like the explanation is missing some details.

It seems like the codegen test should be mimicing the test from the bug report more closely w/ two paren inits.

[shafik]

Please also add a release note.

[ayzhao]

rebase + add release notes

[ayzhao]

In D149389#4303966, @shafik wrote:

So the change makes sense but the original issue: https://github.com/llvm/llvm-project/issues/62266 it worked for the first once but not the second, it also worked for the non-paren aggregate init. So I feel like the explanation is missing some details.

Patch description has been updated with an explanation.

It seems like the codegen test should be mimicing the test from the bug report more closely w/ two paren inits.

The bug was that Clang wasn't calling the default-member intializer in the first initializer, which the CodeGen test asserts. The second initializer is irrelevant - it's only function is to provide the expected value of the initialized field. In fact, changing the test assertion to be the same as that in the bug report (asserting that the values of the fields in the two objects are equal) is worse because that test would pass if we value-initialize the fields instead of calling the default initialization expression for both objects.

In D149389#4304092, @shafik wrote:

Please also add a release note.

Done

[shafik]

In D149389#4306605, @ayzhao wrote:

In D149389#4303966, @shafik wrote:

So the change makes sense but the original issue: https://github.com/llvm/llvm-project/issues/62266 it worked for the first once but not the second, it also worked for the non-paren aggregate init. So I feel like the explanation is missing some details.

Patch description has been updated with an explanation.

It seems like the codegen test should be mimicing the test from the bug report more closely w/ two paren inits.

The bug was that Clang wasn't calling the default-member intializer in the first initializer, which the CodeGen test asserts. The second initializer is irrelevant - it's only function is to provide the expected value of the initialized field. In fact, changing the test assertion to be the same as that in the bug report (asserting that the values of the fields in the two objects are equal) is worse because that test would pass if we value-initialize the fields instead of calling the default initialization expression for both objects.

Apologies, I was not alluding to the comparing of the results of the two initializations but ensuring that it does the right thing for not just the first but also the second. So we did not change the bug in some strange way where the first now succeeds but now the second fails.

[shafik]

LGTM

clang/docs/ReleaseNotes.rst

- Stop stripping CV qualifiers from the type of ``this`` when capturing it by value in
   (`#50866 <https://github.com/llvm/llvm-project/issues/50866>`_)
 - Fix ordering of function templates by constraints when they have template
   template parameters with different nested constraints.
 - Fix type equivalence comparison between auto types to take constraints into
   account.
 - Fix bug in the computation of the ``__has_unique_object_representations``
   builtin for types with unnamed bitfields.
   (`#61336 <https://github.com/llvm/llvm-project/issues/61336>`_)
+- Fix default member initializers sometimes being ignored when performing
+  parenthesized aggregate initialization of templated types.
+  (`#62266 <https://github.com/llvm/llvm-project/issues/62266>`_)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed

clang/lib/Sema/SemaInit.cpp

for (InitializedEntity SubEntity : Range) {
       } else {
         // We've processed all of the args, but there are still entities that
         // have to be initialized.
         if (IsMember) {
           // C++ [dcl.init]p17.6.2.2
           //   The remaining elements are initialized with their default member
           //   initializers, if any
           auto *FD = cast<FieldDecl>(SubEntity.getDecl());
-          if (Expr *ICE = FD->getInClassInitializer(); ICE && !VerifyOnly) {
+          if (FD->hasInClassInitializer()) {
+            if (!VerifyOnly) {
               ExprResult DIE = S.BuildCXXDefaultInitExpr(FD->getLocation(), FD);
               if (DIE.isInvalid())
                 return false;
               S.checkInitializerLifetime(SubEntity, DIE.get());
               InitExprs.push_back(DIE.get());
+            }
             continue;
-          };
+          }
         }
         // Remaining class elements without default member initializers and
         // array elements are value initialized:
         //
         // C++ [dcl.init]p17.6.2.2
         //   The remaining elements...otherwise are value initialzed
         //
         // C++ [dcl.init]p17.5

clang/test/CodeGen/paren-list-agg-init.cpp

namespace gh61145 {
 
   // CHECK-DAG: [[STRUCT_S2:%.*]] = type { [[STRUCT_VEC]], i8 }
   struct S2 {
     Vec v;
     char c;
   };
 }
 
+namespace gh62266 {
+  // CHECK-DAG: [[STRUCT_H:%.*H.*]] = type { i32, i32 }
+  template <int J>
+  struct H {
+    int i;
+    int j = J;
+  };
+}
+
 // CHECK-DAG: [[A1:@.*a1.*]] = internal constant [[STRUCT_A]] { i8 3, double 2.000000e+00 }, align 8
 constexpr A a1(3.1, 2.0);
 // CHECK-DAG: [[A2:@.*a2.*]] = internal constant [[STRUCT_A]] { i8 99, double 0.000000e+00 }, align 8
 constexpr auto a2 = static_cast<A>('c');
 // CHECK-DAG: [[B1:@.*b1.*]] = internal constant [[STRUCT_B]] { [[STRUCT_A]] { i8 99, double 0.000000e+00 }, i32 0 }, align 8
 constexpr B b1(A('c'));
 // CHECK-DAG: [[C1:@.*c1.*]] = internal constant { [[STRUCT_A]], i32, [4 x i8], i8, double, i32 } { [[STRUCT_A]] { i8 99, double 0.000000e+00 }, i32 0, [4 x i8] undef, i8 3, double 2.000000e+00, i32 0 }, align
 constexpr C c1(b1, a1);
void make2() {
     S2((Vec&&) v, 0);
   }
 
   void foo() {
     make1<0>();
     make2<0>();
   }
 }
+
+namespace gh62266 {
+  // CHECK: define {{.*}} void {{.*foo20.*}}
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT: [[H:%.*h.*]] = alloca [[STRUCT_H]], align 4
+  // CHECK-NEXT: [[I:%.*i.*]] = getelementptr inbounds [[STRUCT_H]], ptr [[H]], i32 0, i32 0
+  // CHECK-NEXT: store i32 1, ptr [[I]], align 4
+  // CHECK-NEXT: [[J:%.*j.*]] = getelementptr inbounds [[STRUCT_H]], ptr [[H]], i32 0, i32 1
+  // CHECK-NEXT: store i32 2, ptr [[J]], align 4
+  // CHECK-NEXT: ret void
+  void foo20() {
+    H<2> h(1);
+  }
+}