Skip to content

[clang] Allow class with anonymous union member to be const-default-constructible even if a union member has a default member initializer (#95854) #96301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

Rajveer100
Copy link
Contributor

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 21, 2024

@llvm/pr-subscribers-clang

Author: Rajveer Singh Bharadwaj (Rajveer100)

Changes

Resolves #95854

-- As per https://eel.is/c++draft/dcl.init#general-8.3


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

2 Files Affected:

  • (modified) clang/include/clang/AST/DeclCXX.h (+2-1)
  • (added) clang/test/Sema/debug-95854.cpp (+11)
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index fb52ac804849d..1d1083a5d6205 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1392,7 +1392,8 @@ class CXXRecordDecl : public RecordDecl {
   bool allowConstDefaultInit() const {
     return !data().HasUninitializedFields ||
            !(data().HasDefaultedDefaultConstructor ||
-             needsImplicitDefaultConstructor());
+             needsImplicitDefaultConstructor()) ||
+           hasInClassInitializer();
   }
 
   /// Determine whether this class has a destructor which has no
diff --git a/clang/test/Sema/debug-95854.cpp b/clang/test/Sema/debug-95854.cpp
new file mode 100644
index 0000000000000..1fb976558650d
--- /dev/null
+++ b/clang/test/Sema/debug-95854.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify %s
+//
+// expected-no-diagnostics
+
+struct A {
+  union {
+    int n = 0;
+    int m;
+  };
+};
+const A a;

@Rajveer100
Copy link
Contributor Author

@zygoloid

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this fix. It needs a release note and your summary should have some more details on what the cause of the bug is and how your PR fixes it. efriedma-quic's comment provides a good framework for explaining the cause.

It also looks like your change broke some tests:

Clang :: AST/Interp/records.cpp
Clang :: SemaCXX/cxx0x-cursory-default-delete.cpp

Are these expected breaks? If so then please update these tests

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from c8f2496 to b964923 Compare June 22, 2024 19:06
@Rajveer100 Rajveer100 requested review from shafik and zygoloid June 22, 2024 19:21
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch 2 times, most recently from 3fb29e5 to 751e630 Compare July 5, 2024 11:10
@Rajveer100 Rajveer100 requested a review from ofAlpaca July 5, 2024 11:11
@Rajveer100
Copy link
Contributor Author

@zygoloid
Let me know if the new changes work well.

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch 2 times, most recently from 1a25f02 to 7efe2bd Compare August 10, 2024 11:26
@Rajveer100
Copy link
Contributor Author

@zygoloid
Could you review this?

Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes generally make sense.

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 7efe2bd to 03c02ea Compare August 13, 2024 10:28
@Rajveer100 Rajveer100 requested review from Sirraide and jrtc27 August 13, 2024 10:29
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch 9 times, most recently from 49e6fb5 to 3ccb1c9 Compare August 13, 2024 13:36
@Rajveer100
Copy link
Contributor Author

@Sirraide The tests pass, not sure about the failure though.

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the test failures look related to the changes can you push and empty change to see if rerunning the tests comes up clean?

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 3ccb1c9 to 8796f60 Compare August 27, 2024 05:57
@Rajveer100
Copy link
Contributor Author

Rajveer100 commented Aug 27, 2024

Only windows failed.

PS: Pushed again to resolve merge conflict.

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 8796f60 to 387e838 Compare August 27, 2024 08:05
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are more tests to consider https://godbolt.org/z/M6fj5zTfq
Thanks!

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 387e838 to dfc4445 Compare August 27, 2024 09:14
@Rajveer100 Rajveer100 requested a review from cor3ntin August 27, 2024 09:22
@Rajveer100
Copy link
Contributor Author

@cor3ntin
CI looks good.

@cor3ntin
Copy link
Contributor

Neither GCC, EDG nor MSVC agree with us https://godbolt.org/z/Wqrhnz63a
The behavior of GCC is a bit weird but I am pretty sure the last two tests should pass
@hubert-reinterpretcast

@Rajveer100 Rajveer100 requested a review from shafik September 17, 2024 11:21
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from dfc4445 to f42cea2 Compare September 25, 2024 12:22
@Rajveer100 Rajveer100 requested a review from shafik September 25, 2024 12:27
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch 2 times, most recently from d6b3358 to 1e1e446 Compare October 1, 2024 11:04
@Rajveer100
Copy link
Contributor Author

@shafik
Anything else that's needed?

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!
Give a couple of days to @shafik before merging

@Rajveer100
Copy link
Contributor Author

Thanks for the approval @cor3ntin, could you land this for me?!

@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 1e1e446 to 38ded4c Compare October 16, 2024 10:44
@Rajveer100 Rajveer100 requested a review from cor3ntin October 16, 2024 10:45
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from 38ded4c to c12be47 Compare October 19, 2024 10:44
…onstructible even if a union member has a default member initializer (llvm#95854)

Resolves llvm#95854

  Clang incorrectly considers a class with an anonymous union member to not be
  const-default-constructible even if a union member has a default member initializer.

```
struct A {
  union {
    int n = 0;
    int m;
  };
};
const A a;
```

-- As per https://eel.is/c++draft/dcl.init#general-8.3
@Rajveer100 Rajveer100 force-pushed the branch-for-issue-95854 branch from c12be47 to cbd6e83 Compare October 19, 2024 10:46
@cor3ntin cor3ntin merged commit 7305734 into llvm:main Oct 22, 2024
7 of 9 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 22, 2024

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building clang at step 11 "Add check check-libc-amdgcn-amd-amdhsa".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7389

Here is the relevant piece of the build log for the reference
Step 11 (Add check check-libc-amdgcn-amd-amdhsa) failure: 1200 seconds without output running [b'ninja', b'-j 32', b'check-libc-amdgcn-amd-amdhsa'], attempting to kill
...
[2442/2681] Linking CXX executable libc/test/src/string/libc.test.src.string.strncat_test.__hermetic__.__build__
[2443/2681] Linking CXX executable libc/test/src/locale/libc.test.src.locale.localeconv_test.__hermetic__.__build__
[2444/2681] Linking CXX executable libc/test/src/inttypes/libc.test.src.inttypes.strtoimax_test.__hermetic__.__build__
[2445/2681] Linking CXX executable libc/test/src/inttypes/libc.test.src.inttypes.strtoumax_test.__hermetic__.__build__
[2446/2681] Linking CXX executable libc/test/src/string/libc.test.src.string.memcmp_test.__hermetic__.__build__
[2447/2681] Linking CXX executable libc/test/src/time/libc.test.src.time.clock_test.__hermetic__.__build__
[2448/2681] Linking CXX executable libc/test/src/string/libc.test.src.string.memmove_test.__hermetic__.__build__
[2449/2681] Linking CXX executable libc/test/src/time/libc.test.src.time.clock_gettime_test.__hermetic__.__build__
[2450/2681] Linking CXX executable libc/test/src/stdio/libc.test.src.stdio.asprintf_test.__hermetic__.__build__
[2451/2681] Linking CXX executable libc/test/src/stdio/libc.test.src.stdio.vasprintf_test.__hermetic__.__build__
command timed out: 1200 seconds without output running [b'ninja', b'-j 32', b'check-libc-amdgcn-amd-amdhsa'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1470.210201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
9 participants