[Sema] Fix crash on invalid code with parenthesized aggregate initialization

[ilya-biryukov]

Fixes #76228.
Use the same logic as braced init lists, also adds a test that puts incomplete types in various positions to check for regressions in the future.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Ilya Biryukov (ilya-biryukov)

Changes

Fixes #76228.
Use the same logic as braced init lists, also adds a test that puts incomplete types in various positions to check for regressions in the future.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaInit.cpp (+8)
• (added) clang/test/SemaCXX/crash-GH76228.cpp (+28)

diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index d5ba7fd3413718..f768d2726b0a1c 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -5512,6 +5512,14 @@ static void TryOrBuildParenListInitialization(
   } else if (auto *RT = Entity.getType()->getAs<RecordType>()) {
     bool IsUnion = RT->isUnionType();
     const CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
+    if (RD->isInvalidDecl()) {
+      // Exit early to avoid confusion when processing members.
+      // We do the same for braced list initialization in
+      // `CheckStructUnionTypes`.
+      Sequence.SetFailed(
+          clang::InitializationSequence::FK_ParenthesizedListInitFailed);
+      return;
+    }
 
     if (!IsUnion) {
       for (const CXXBaseSpecifier &Base : RD->bases()) {
diff --git a/clang/test/SemaCXX/crash-GH76228.cpp b/clang/test/SemaCXX/crash-GH76228.cpp
new file mode 100644
index 00000000000000..a10b9994c5e532
--- /dev/null
+++ b/clang/test/SemaCXX/crash-GH76228.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// Check we don't crash on incomplete members and bases when handling parenthesized initialization.
+class incomplete; // expected-note@-0 3  {{forward declaration of 'incomplete'}}
+struct foo {
+  int a;
+  incomplete b;
+  // expected-error@-1 {{incomplete type}}
+};
+foo a1(0);
+
+struct one_int {
+    int a;
+};
+struct bar : one_int, incomplete {};
+// expected-error@-1 {{incomplete type}}
+bar a2(0);
+
+incomplete a3[3](1,2,3);
+// expected-error@-1 {{incomplete type}}
+
+struct qux : foo {
+};
+qux a4(0);
+
+struct fred {
+    foo a[3];
+};
+fred a5(0);
\ No newline at end of file

[hokein]

Thanks, looks good.

[shafik]

This is missing a release note.

[vitalybuka]

Breaks multiple bots
https://lab.llvm.org/buildbot/#/builders/168/builds/17604
https://lab.llvm.org/buildbot/#/builders/74/builds/24426
https://lab.llvm.org/buildbot/#/builders/239/builds/4971
and some others

[vvereschaka]

these changes break the libc++ tests - llvm-libc++-static.cfg.in::transform_error.mandates.verify.cpp

# .---command stderr------------
# | error: 'expected-error' diagnostics expected but not seen: 
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:49): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:53): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:61): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:63): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:70): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:72): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:79): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# |   File * Line * (directive at C:\buildbot\as-builder-2\x-aarch64\llvm-project\libcxx\test\libcxx\utilities\expected\expected.void\transform_error.mandates.verify.cpp:81): {{(excess elements in struct initializer|no matching constructor for initialization of)}}{{.*}}
# | 8 errors generated.
# `-----------------------------
# error: command failed with exit status: 1

https://lab.llvm.org/buildbot/#/builders/119/builds/16381
https://lab.llvm.org/buildbot/#/builders/60/builds/15226

[ilya-biryukov]

Thanks for reverting it and sorry for the trouble, I will make sure to check libc++ tests before a reland.
Clang does report less errors now, but that's expected. The errors were spurious, caused by Clang trying to initialize an "invalid" class (more details below in case you're interested).

Failed static_assert is what made the class invalid in the first place and Clang fails parenthesized initialization of such classes early now. We should not do parenthesized initialization here as it should only be applied to aggregates, this class has constructors and is not an aggregate in the first place. The only reason we attempted aggregate init is because invalid classes are left internally in inconsistent states sometimes (there are many reasons why class is invalid). So less errors is better here, they were only adding noise and confusion in the first place.

I will reland the commit with updated libc++ tests. The commit did not change the set of correct programs that we compile, it's just showing less spurious errors.