[Clang][Sema] Fix incorrect deletion of default constructors for some unions

[royjacobson]

Summary

If a union has explicit initializers for some members, we shouldn't delete
its default constructor.
Fixes https://github.com/llvm/llvm-project/issues/48416.

[royjacobson]

Small cleanup

[erichkeane]

Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?

clang/lib/Sema/SemaDeclCXX.cpp

9163

This way you don't need the assert!

9167

else needs curleys, since its associated 'if' has them.

[royjacobson]

Add more tests, small nit.

[royjacobson]

In D145851#4189158, @erichkeane wrote:

Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?

Added constexpr + codegen tests.

If we have multiple initializers it's an error that's already diagnosed, I assume the error recovery just drops any initializer after the first.

[erichkeane]

In D145851#4189916, @royjacobson wrote:

In D145851#4189158, @erichkeane wrote:

Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)?

Added constexpr + codegen tests.

If we have multiple initializers it's an error that's already diagnosed, I assume the error recovery just drops any initializer after the first.

Got it, thanks! LGTM!

[shafik]

LGTM, I wlll let @royjacobson give the final approval after reviewing codegen tests.

clang/lib/Sema/SemaDeclCXX.cpp

9161–9162

[aaron.ballman]

Precommit CI found related failures:

/var/lib/buildkite-agent/builds/llvm-project/clang/test/CodeGen/union-non-trivial-member.cpp:29:16: error: found empty check string with prefix 'CHECK:'
// CHECK-NEXT: 
               ^
error: unknown argument: '-g0'

[erichkeane]

clang/test/CodeGen/union-non-trivial-member.cpp

31

Looking more closely, this test is a little over specific. First, you can just do CHECK: to start each function, that way you don't have the blank lines.

In the top function, all we care about is the call void <name>s, so the rest of the lines can go away. Since you're checking function's define line, you can count on ordering that way rather than check-next.

This middle function should use more wildcard/placeholders, checking fully specific names like this or this.addr is a mistake, as those aren't guaranteed to be there. Also, all the 'align' checks are likely to make this fail in post-commit.

on the last function, we don't even care about the parameters list, so you just need the call void <name> part.

[royjacobson]

Fix the codegen test, add a standard ref to the comment.

[royjacobson]

Oops, sorry for the bad codegen test and thanks for the comments.

[royjacobson]

clang/test/CodeGen/union-non-trivial-member.cpp

31

removed the non-essential parts.

[erichkeane]

clang/test/CodeGen/union-non-trivial-member.cpp

23

24

Don't need the entry: line,

25

Wildcards are implicit at the end of a line.

26

27

This and the line after it aren't really necessary/valuable, particularly in case we get debug info/builtin/etc added later.

30

I typically suggest not requiring a space either before or after a {{.*}} block, since that wont match 'empty'.

31

Same here, skip the entry , ret, and closing curley brace line.

32

same advise as above on the following ones too.

[erichkeane]

In D145851#4194154, @royjacobson wrote:

Fix the codegen test, add a standard ref to the comment.

No worries, there is definitely somewhat of an art to writing a codegen test that won't be fragile/overspecified, I've made a few more suggestions to make it more clear/easier, else this LGTM. Feel free to put the rest of the codegen test suggestions up for further review, but if you're comfortable with them, you may commit instead.

[royjacobson]

slimmer codegen test

[erichkeane]

Still LGTM!

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fix crash on invalid code when looking up a destructor in a templated class
   inside a namespace.
   (`#59446 <https://github.com/llvm/llvm-project/issues/59446>`_)
 - Fix crash when evaluating consteval constructor of derived class whose base
   has more than one field.
   (`#60166 <https://github.com/llvm/llvm-project/issues/60166>`_)
+- Fix incorrect deletion of the default constructor of unions in some
+  cases. (`#48416 <https://github.com/llvm/llvm-project/issues/48416>`_)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed

clang/lib/Sema/SemaDeclCXX.cpp

else if (!isAccessible(Subobj, Decl))
     DiagKind = 3;
   else if (!IsDtorCallInCtor && Field && Field->getParent()->isUnion() &&
            !Decl->isTrivial()) {
     // A member of a union must have a trivial corresponding special member.
     // As a weird special case, a destructor call from a union's constructor
     // must be accessible and non-deleted, but need not be trivial. Such a
     // destructor is never actually called, but is semantically checked as
     // if it were.
+    if (CSM == Sema::CXXDefaultConstructor) {
+      // [class.default.ctor]p2:
+      //   A defaulted default constructor for class X is defined as deleted if
+      //   - X is a union that has a variant member with a non-trivial default
+      //     constructor and no variant member of X has a default member
+      //     initializer
+      const auto *RD = cast<CXXRecordDecl>(Field->getParent());
+      if (!RD->hasInClassInitializer())
+        DiagKind = 4;
+    } else {
       DiagKind = 4;
     }
+  }
 
   if (DiagKind == -1)
     return false;
 
   if (Diagnose) {
     if (Field) {
       S.Diag(Field->getLocation(),
              diag::note_deleted_special_member_class_subobject)

clang/test/CodeGen/union-non-trivial-member.cpp

+// RUN: %clang_cc1 --std=c++17 -emit-llvm %s -o - -triple x86_64-unknown-linux-gnu | FileCheck %s
+
+struct non_trivial_constructor {
+    constexpr non_trivial_constructor() : x(100) { }
+    int x;
+};
+
+union UnionInt {
+    int a{1000};
+    non_trivial_constructor b;
+};
+
+union UnionNonTrivial {
+    int a;
+    non_trivial_constructor b{};
+};
+
+void f() {
+    UnionInt u1;
+    UnionNonTrivial u2;
+}
+
+// CHECK:      define dso_local void @_Z1fv() {{.*}} {
+// CHECK-NEXT: entry:
+// CHECK:        call void @_ZN8UnionIntC1Ev{{.*}}
+// CHECK-NEXT:   call void @_ZN15UnionNonTrivialC1Ev{{.*}}
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }
+
+// CHECK:      define {{.*}} void @_ZN8UnionIntC1Ev{{.*}}
+// CHECK-NEXT: entry:
+// CHECK:        call void @_ZN8UnionIntC2Ev{{.*}}
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }
+
+// CHECK:      define {{.*}} void @_ZN15UnionNonTrivialC1Ev{{.*}}
+// CHECK-NEXT: entry:
+// CHECK:        call void @_ZN15UnionNonTrivialC2Ev{{.*}}
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }
+
+// CHECK:      define {{.*}} void @_ZN8UnionIntC2Ev{{.*}}
+// CHECK-NEXT: entry:
+// CHECK:        store i32 1000{{.*}}
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }
+
+// CHECK:      define {{.*}} void @_ZN15UnionNonTrivialC2Ev{{.*}}
+// CHECK-NEXT: entry:
+// CHECK:        call void @_ZN23non_trivial_constructorC1Ev{{.*}}
+// CHECK-NEXT:   ret void
+// CHECK-NEXT: }

clang/test/SemaCXX/cxx0x-nontrivial-union.cpp

union {
       struct {  // expected-note {{default constructor of 'Test2<pr16061::X>' is implicitly deleted because variant field '' has a non-trivial default constructor}}
         T x;
       };
     };
   };
 
   Test2<X> t2x;  // expected-error {{call to implicitly-deleted default constructor of 'Test2<X>'}}
 }
+
+namespace GH48416 {
+
+struct non_trivial_constructor {
+    constexpr non_trivial_constructor() : x(100) {}
+    int x;
+};
+
+
+union U1 {
+    int a;
+    non_trivial_constructor b; // expected-note {{has a non-trivial default constructor}}
+};
+
+union U2 {
+    int a{1000};
+    non_trivial_constructor b;
+};
+
+union U3 {
+    int a;
+    non_trivial_constructor b{};
+};
+
+union U4 {
+    int a{}; // expected-note {{previous initialization is here}}
+    non_trivial_constructor b{}; // expected-error {{initializing multiple members of union}}
+};
+
+U1 u1; // expected-error {{call to implicitly-deleted default constructor}}
+U2 u2;
+U3 u3;
+U4 u4;
+
+static_assert(U2().a == 1000, "");
+static_assert(U3().a == 1000, "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+// expected-note@-2 {{read of member 'a' of union with active member 'b'}}
+static_assert(U2().b.x == 100, "");
+// expected-error@-1 {{static assertion expression is not an integral constant expression}}
+// expected-note@-2 {{read of member 'b' of union with active member 'a'}}
+static_assert(U3().b.x == 100, "");
+
+} // namespace GH48416