[Sema] Don't mark deleted special member functions as non-trivial

[royjacobson]

Summary

As noted in https://github.com/llvm/llvm-project/issues/59624, we sometimes mark implicitly
deleted special member functions as non-trivial. This is unnecessary work and leads to some
weird type traits errors.

This fixes the problem by making the implicitly deleted special member functions always
trivial.

[chriselrod]

clang/test/SemaCXX/GH59624.cpp

10–12

This isn't needed for the test.

[erichkeane]

Code changes LGTM. Dead CFoo obviously should be used or deleted.

[royjacobson]

Rebase + remove extra class

[royjacobson]

format

[royjacobson]

So, I had a quick look at this. Turns out this fix changed the argument passing ABI for those classes on the Sony PS4. This is probably not acceptable :)

Putting this behind an ABI compat/Sony flag is probably good enough in this case, will try to write it up next week.

clang/docs/ReleaseNotes.rst

- GCC doesn't pack non-POD members in packed structs unless the packed
   attribute is also specified on the member. Clang historically did perform
   such packing. Clang now matches the gcc behavior (except on Darwin and PS4).
   You can switch back to the old ABI behavior with the flag:
   ``-fclang-abi-compat=15.0``.
 - GCC allows POD types to have defaulted special members. Clang historically
   classified such types as non-POD (for the purposes of Itanium ABI). Clang now
   matches the gcc behavior (except on Darwin and PS4). You can switch back to
   the old ABI behavior with the flag: ``-fclang-abi-compat=15.0``.
+- Some types with implicitly deleted special member functions were accidentally
+  marked as non-trivially copyable. This has been fixed
+  (`#59624 <https://github.com/llvm/llvm-project/issues/59624>`_).
 
 OpenMP Support in Clang
 -----------------------
 
 ...
 
 CUDA Support in Clang
 ---------------------

clang/lib/Sema/SemaDeclCXX.cpp

CXXMethodDecl *Sema::DeclareImplicitCopyAssignment(CXXRecordDecl *ClassDecl) {
   // Add the parameter to the operator.
   ParmVarDecl *FromParam = ParmVarDecl::Create(Context, CopyAssignment,
                                                ClassLoc, ClassLoc,
                                                /*Id=*/nullptr, ArgType,
                                                /*TInfo=*/nullptr, SC_None,
                                                nullptr);
   CopyAssignment->setParams(FromParam);
 
-  CopyAssignment->setTrivial(
-    ClassDecl->needsOverloadResolutionForCopyAssignment()
-      ? SpecialMemberIsTrivial(CopyAssignment, CXXCopyAssignment)
-      : ClassDecl->hasTrivialCopyAssignment());
-
   // Note that we have added this copy-assignment operator.
   ++getASTContext().NumImplicitCopyAssignmentOperatorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, CopyAssignment);
 
   if (ShouldDeleteSpecialMember(CopyAssignment, CXXCopyAssignment)) {
     ClassDecl->setImplicitCopyAssignmentIsDeleted();
     SetDeclDeleted(CopyAssignment, ClassLoc);
+  } else {
+    CopyAssignment->setTrivial(
+      ClassDecl->needsOverloadResolutionForCopyAssignment()
+        ? SpecialMemberIsTrivial(CopyAssignment, CXXCopyAssignment)
+        : ClassDecl->hasTrivialCopyAssignment());
   }
 
   if (S)
     PushOnScopeChains(CopyAssignment, S, false);
   ClassDecl->addDecl(CopyAssignment);
 
   return CopyAssignment;
 }
CXXMethodDecl *Sema::DeclareImplicitMoveAssignment(CXXRecordDecl *ClassDecl) {
   // Add the parameter to the operator.
   ParmVarDecl *FromParam = ParmVarDecl::Create(Context, MoveAssignment,
                                                ClassLoc, ClassLoc,
                                                /*Id=*/nullptr, ArgType,
                                                /*TInfo=*/nullptr, SC_None,
                                                nullptr);
   MoveAssignment->setParams(FromParam);
 
-  MoveAssignment->setTrivial(
-    ClassDecl->needsOverloadResolutionForMoveAssignment()
-      ? SpecialMemberIsTrivial(MoveAssignment, CXXMoveAssignment)
-      : ClassDecl->hasTrivialMoveAssignment());
 
   // Note that we have added this copy-assignment operator.
   ++getASTContext().NumImplicitMoveAssignmentOperatorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, MoveAssignment);
 
   if (ShouldDeleteSpecialMember(MoveAssignment, CXXMoveAssignment)) {
     ClassDecl->setImplicitMoveAssignmentIsDeleted();
     SetDeclDeleted(MoveAssignment, ClassLoc);
+  } else {
+    MoveAssignment->setTrivial(
+      ClassDecl->needsOverloadResolutionForMoveAssignment()
+        ? SpecialMemberIsTrivial(MoveAssignment, CXXMoveAssignment)
+        : ClassDecl->hasTrivialMoveAssignment());
   }
 
   if (S)
     PushOnScopeChains(MoveAssignment, S, false);
   ClassDecl->addDecl(MoveAssignment);
 
   return MoveAssignment;
 }
CXXConstructorDecl *Sema::DeclareImplicitCopyConstructor(
 
   // Add the parameter to the constructor.
   ParmVarDecl *FromParam =
       ParmVarDecl::Create(Context, CopyConstructor, ClassLoc, ClassLoc,
                           /*IdentifierInfo=*/nullptr, ArgType,
                           /*TInfo=*/TSI, SC_None, nullptr);
   CopyConstructor->setParams(FromParam);
 
+  // Note that we have declared this constructor.
+  ++getASTContext().NumImplicitCopyConstructorsDeclared;
+
+  Scope *S = getScopeForContext(ClassDecl);
+  CheckImplicitSpecialMemberDeclaration(S, CopyConstructor);
+
+  if (ShouldDeleteSpecialMember(CopyConstructor, CXXCopyConstructor)) {
+    ClassDecl->setImplicitCopyConstructorIsDeleted();
+    SetDeclDeleted(CopyConstructor, ClassLoc);
+  } else {
     CopyConstructor->setTrivial(
         ClassDecl->needsOverloadResolutionForCopyConstructor()
             ? SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor)
             : ClassDecl->hasTrivialCopyConstructor());
 
     CopyConstructor->setTrivialForCall(
         ClassDecl->hasAttr<TrivialABIAttr>() ||
         (ClassDecl->needsOverloadResolutionForCopyConstructor()
              ? SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor,
                                       TAH_ConsiderTrivialABI)
              : ClassDecl->hasTrivialCopyConstructorForCall()));
-
-  // Note that we have declared this constructor.
-  ++getASTContext().NumImplicitCopyConstructorsDeclared;
-
-  Scope *S = getScopeForContext(ClassDecl);
-  CheckImplicitSpecialMemberDeclaration(S, CopyConstructor);
-
-  if (ShouldDeleteSpecialMember(CopyConstructor, CXXCopyConstructor)) {
-    ClassDecl->setImplicitCopyConstructorIsDeleted();
-    SetDeclDeleted(CopyConstructor, ClassLoc);
   }
 
   if (S)
     PushOnScopeChains(CopyConstructor, S, false);
   ClassDecl->addDecl(CopyConstructor);
 
   return CopyConstructor;
 }
CXXConstructorDecl *Sema::DeclareImplicitMoveConstructor(
   // Add the parameter to the constructor.
   ParmVarDecl *FromParam = ParmVarDecl::Create(Context, MoveConstructor,
                                                ClassLoc, ClassLoc,
                                                /*IdentifierInfo=*/nullptr,
                                                ArgType, /*TInfo=*/nullptr,
                                                SC_None, nullptr);
   MoveConstructor->setParams(FromParam);
 
+  // Note that we have declared this constructor.
+  ++getASTContext().NumImplicitMoveConstructorsDeclared;
+
+  Scope *S = getScopeForContext(ClassDecl);
+  CheckImplicitSpecialMemberDeclaration(S, MoveConstructor);
+
+  if (ShouldDeleteSpecialMember(MoveConstructor, CXXMoveConstructor)) {
+    ClassDecl->setImplicitMoveConstructorIsDeleted();
+    SetDeclDeleted(MoveConstructor, ClassLoc);
+  } else {
     MoveConstructor->setTrivial(
         ClassDecl->needsOverloadResolutionForMoveConstructor()
             ? SpecialMemberIsTrivial(MoveConstructor, CXXMoveConstructor)
             : ClassDecl->hasTrivialMoveConstructor());
 
     MoveConstructor->setTrivialForCall(
         ClassDecl->hasAttr<TrivialABIAttr>() ||
         (ClassDecl->needsOverloadResolutionForMoveConstructor()
              ? SpecialMemberIsTrivial(MoveConstructor, CXXMoveConstructor,
                                       TAH_ConsiderTrivialABI)
              : ClassDecl->hasTrivialMoveConstructorForCall()));
-
-  // Note that we have declared this constructor.
-  ++getASTContext().NumImplicitMoveConstructorsDeclared;
-
-  Scope *S = getScopeForContext(ClassDecl);
-  CheckImplicitSpecialMemberDeclaration(S, MoveConstructor);
-
-  if (ShouldDeleteSpecialMember(MoveConstructor, CXXMoveConstructor)) {
-    ClassDecl->setImplicitMoveConstructorIsDeleted();
-    SetDeclDeleted(MoveConstructor, ClassLoc);
   }
 
   if (S)
     PushOnScopeChains(MoveConstructor, S, false);
   ClassDecl->addDecl(MoveConstructor);
 
   return MoveConstructor;
 }
void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
   //   A program that defines main as deleted [...] is ill-formed.
   if (Fn->isMain())
     Diag(DelLoc, diag::err_deleted_main);
 
   // C++11 [dcl.fct.def.delete]p4:
   //  A deleted function is implicitly inline.
   Fn->setImplicitlyInline();
   Fn->setDeletedAsWritten();
+
+  Fn->setTrivial(true);
+  Fn->setTrivialForCall(true);
 }
 
 void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
   if (!Dcl || Dcl->isInvalidDecl())
     return;
 
   auto *FD = dyn_cast<FunctionDecl>(Dcl);
   if (!FD) {

clang/test/AST/ast-dump-funcs.cpp

struct S {
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:33> col:16 f 'void (float, int)' virtual
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:18> col:23 'float'
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:25, col:31> col:29 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}} <col:31> 'int' 12
 
   virtual void g() = 0;
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:22> col:16 g 'void ()' virtual pure
 
-  // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <line:[[@LINE-33]]:8> col:8 implicit S 'void (const S &)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <line:[[@LINE-33]]:8> col:8 implicit S 'void (const S &)' inline default_delete trivial noexcept-unevaluated
   // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <col:8> col:8 implicit constexpr S 'void (S &&)' inline default noexcept-unevaluated
-  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(const S &)' inline default_delete noexcept-unevaluated
-  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(S &&)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(const S &)' inline default_delete trivial noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'S &(S &&)' inline default_delete trivial noexcept-unevaluated
   // CHECK: CXXDestructorDecl 0x{{[^ ]*}} <col:8> col:8 implicit ~S 'void ()' inline default noexcept-unevaluated
 };
 
 struct T : S { // T is not referenced, but S is
   void f(float, int = 100) override;
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:28> col:8 f 'void (float, int)'
   // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:10> col:15 'float'
   // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}} <col:17, col:23> col:21 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}} <col:23> 'int' 100
   // CHECK-NEXT: OverrideAttr
 
-  // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <line:[[@LINE-9]]:8> col:8 implicit T 'void (const T &)' inline default_delete noexcept-unevaluated
-  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(const T &)' inline default_delete noexcept-unevaluated
-  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(T &&)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}} <line:[[@LINE-9]]:8> col:8 implicit T 'void (const T &)' inline default_delete trivial noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(const T &)' inline default_delete trivial noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}} <col:8> col:8 implicit operator= 'T &(T &&)' inline default_delete trivial noexcept-unevaluated
   // CHECK: CXXDestructorDecl 0x{{[^ ]*}} <col:8> col:8 implicit ~T 'void ()' inline default noexcept-unevaluated
 };
 
 struct U {
   void f();
   // CHECK: CXXMethodDecl 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:10> col:8 f 'void ()'
 };
 void U::f() {} // parent

clang/test/SemaCXX/GH59624.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++20
+// expected-no-diagnostics
+
+namespace GH59624 {
+
+struct Foo{
+    int x{0};
+};
+struct CFoo{
+    const int x{0};
+};
+struct Bar{
+    const Foo y;
+};
+
+// Deleted move assignment shouldn't make type non-trivially copyable:
+
+static_assert(__is_trivially_copyable(Bar), "");
+
+}
+