"Reapply "[Sema] Fix crash on invalid code with parenthesized aggrega… #76833
Conversation
…te initialization" (llvm#76272)"" With updates the libc++ tests. This reverts commit 2205d23 and relands 86dc6e1 and 7ab16fb. Original commit was reverted because of failing libc++ tests, see llvm#76232 for the discussion. The errors in the tests are spurious in the first place (coming from initialization of invalid classes), so update the tests to match new behavior that does not show those errors.
|
@ilya-biryukov this is the new attempt to land this patch, the version you committed breaks the libc++ CI. I'll try to fix it this week. |
Wow, I didn't expect that. Is libc++ ok with disabling this test until the nightly build catches up? (The PR is in the Draft state, so it's hard to tell if that's acceptable). In the long-term, how should people land changes like this? Compiler diagnostics are expected to change sometimes, so we should figure out some way that allows landing these without reverts. Any thoughts? |
It's not ideal, but I can't think of a better solution. Typically nightly apt builds are built nightly so then we can update the Docker CI quickly. If the nightly builds are red it might take a bit longer. The patch is in draft since I wanted to test with the bootstrap build which required temporary disabling other builds (the second commit). When the CI is green I'll do a final review and publish the patch.
Typically we work together with whom changes the tests. When there are libc++ changes it is visible in our review queue. For changes in diagnostics it's often possible to use a regex matcher. For example, the messages of |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-libcxx Author: Mark de Wever (mordante) Changes…te initialization" (#76272)"" With updates the libc++ tests. This reverts commit 2205d23 and relands Original commit was reverted because of failing libc++ tests, see #76232 for the discussion. The errors in the tests are spurious in the first place (coming from initialization of invalid classes), so update the tests to match new behavior that does not show those errors. Full diff: https://github.com/llvm/llvm-project/pull/76833.diff 5 Files Affected:
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 61d244f3bb9798..cc9db5ded1149a 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..33a9395823127e
--- /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);
diff --git a/clang/test/SemaCXX/paren-list-agg-init.cpp b/clang/test/SemaCXX/paren-list-agg-init.cpp
index f60b20e0d46568..c1964a5a9eb005 100644
--- a/clang/test/SemaCXX/paren-list-agg-init.cpp
+++ b/clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -289,7 +289,7 @@ int test() {
// used to crash
S a(0, 1);
S b(0);
- S c(0, 0, 1); // beforecxx20-warning {{aggregate initialization of type 'S' from a parenthesized list of values is a C++20 extension}}
+ S c(0, 0, 1);
S d {0, 1};
S e {0};
diff --git a/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp b/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
index 965e82a7b40346..318435660c3652 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp
@@ -6,6 +6,15 @@
//
//===----------------------------------------------------------------------===//
+// https://github.com/llvm/llvm-project/pull/76232 breaks this libc++ test.
+// The fix would be to update this file. The issue is that the CI uses 2
+// versions of Clang-18
+// - An older nightly build as the main compiler
+// - A freshly bootstrap build
+// This means the test can't be used until the nightly build is updated.
+// TODO(mordante) Reenable clang-18.
+// UNSUPPORTED: clang-18
+
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// Test the mandates
diff --git a/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp b/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp
index 09aa1332e98000..d9f65e9f5919cd 100644
--- a/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp
+++ b/libcxx/test/libcxx/utilities/expected/expected.void/transform_error.mandates.verify.cpp
@@ -6,6 +6,15 @@
//
//===----------------------------------------------------------------------===//
+// https://github.com/llvm/llvm-project/pull/76232 breaks this libc++ test.
+// The fix would be to update this file. The issue is that the CI uses 2
+// versions of Clang-18
+// - An older nightly build as the main compiler
+// - A freshly bootstrap build
+// This means the test can't be used until the nightly build is updated.
+// TODO(mordante) Reenable clang-18.
+// UNSUPPORTED: clang-18
+
// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
// Test the mandates
|
It's been 2 days, maybe we should just submit this and see if anything goes wrong?
This use-case is different, we need a way to change the set of reported diagnostics in Clang even if the diagnostic is produced in libc++ tests. Blocking changes to Clang because libc++ tests against nightly and head builds looks too harsh. I think we need to find some alternative solution that works here. The obvious solution that I see is to run the nightly Clang as post-submits and allow them to be broken for this reasons (as it will fix itself after nightly Clang gets rebuilt). @AaronBallman, @cor3ntin what are your thoughts as Clang maintainers? TLDR; is that libc++ runs tests in CI with both head Clang and nightly Clang builds. Therefore, if we happen to change the set of reported diagnostics in Clang (in this case it was a spurious error in initialization of invalid classes that got silenced with a Clang change), we cannot land this change without breaking the libc++ CI. |
I'll land this soon. Yesterday evening I didn't have time.
All but one of the libc++ pre-commit CI jobs uses pre-built clang binaries. We heavily depend on a pre-commit CI to test changes on platforms we don't have access to. So changing it to a post-commit CI is not an option.
Note that the original patch also breaks clang-16 and clang-17. So usually there's a regex matcher used and that works with clang-16, clang-17, clang-18 nightly, and clang-18 ToT. This is a rare case where it's not possible to use a regex. (In general clang patches affecting libc++ are rare.) As mentioned before typically we work together with Clang to see how to fix this. As far as I can tell the changes to libc++ were not reviewed by the a libc++ reviewer and not tested in the pre-commit CI. Please correct me if I'm wrong. After a previous breakage both @ldionne and I have discussed this with @AaronBallman and that resulted in documentation to create awareness and we helped to get a clang pre-commit CI up and running. |
|
@mordante The lines could be changed to |
Totally forgot using 0 occurrences are valid in this syntax. I'll make a new patch. I'll keep the redundant part, the entire expected will be redundant in about a year. |
llvm#76833) …te initialization" (llvm#76272)"" With updates the libc++ tests. This reverts commit 2205d23 and relands 86dc6e1 and 7ab16fb. Original commit was reverted because of failing libc++ tests, see llvm#76232 for the discussion. The errors in the tests are spurious in the first place (coming from initialization of invalid classes), so update the tests to match new behavior that does not show those errors. The original patch was written by @ilya-biryukov To fix the CI two libc++ tests are temporary disabled for clang-18.
…te initialization" (#76272)""
With updates the libc++ tests.
This reverts commit 2205d23 and relands
86dc6e1 and
7ab16fb.
Original commit was reverted because of failing libc++ tests, see #76232 for the discussion.
The errors in the tests are spurious in the first place (coming from initialization of invalid classes), so update the tests to match new behavior that does not show those errors.