Disallow trivial_abi on a class if all copy and move constructors are deleted

[ahatanak]

Summary

Instead of forcing the class to be passed in registers, which was what r350920 did, issue a warning and inform the user that the attribute cannot be used.

For more background, see this discussion: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20190128/259907.html

This fixes PR39683.

rdar://problem/47308221

[rjmccall]

lib/Sema/SemaDeclCXX.cpp

7889

This is not a very useful diagnostic. We have several different reasons why we reject the attribute, some of which are pretty subtle, and it's not reasonable to expect users to puzzle it out. At the very least we can tell them the immediate cause for all of these rejections.

[• Quuxplusone]

Can you give more intuition on why classes with no copy/move operations should be forced non-trivial-abi? Let's take this specific example:

struct [[clang::trivial_abi]] lock_guard {
    mutex *m;
    explicit lock_guard(mutex *m) : m(m) { m->lock(); }
    ~lock_guard() { m->unlock(); }
    lock_guard(const lock_guard&) = delete;
    lock_guard(lock_guard&&) = delete;
};

void foo(lock_guard g) { ... }
void bar() { mutex m; foo(lock_guard(&m)); }

With C++17 "guaranteed copy elision," there's no reason this code would need copy/move operations. But equally I can't see any reason that g should not be passed in a register when possible — it's just a pointer, after all.
I admit that this lock_guard example is contrived and generally ill-advised, but its ill-advisedness seems like a higher-level concern that shouldn't be "enforced" by fiddling with the rules of [[trivial_abi]], so I hope that's not what's going on here.

[rjmccall]

Richard felt that this was an odd special case, and I was happy to use the old language-designer's dodge of banning something today so that we can decide to allow it tomorrow. This isn't SFINAE-able.

...of course, if it's just a *warning* that this isn't allowed, that dodge doesn't quite work.

[ahatanak]

Add a note diagnostic to inform the user of the reason for not allowing annotating a class with trivial_abi.

[• Quuxplusone]

include/clang/Basic/DiagnosticSemaKinds.td

2953

nit: "of non-trivial class types" should be "of non-trivial class type" in both places.

And I would write "are not move-constructible" rather than "don't have non-deleted copy/move constructors". Double negations aren't non-bad.

Actually I would rephrase this as 'trivial_abi' is disallowed on this class because it %select{is not move-constructible|is polymorphic|has a base of non-trivial class type|has a virtual base|has a __weak field|has a field of non-trivial class type}, i.e., we're not just giving information about "classes" in general, we're talking about "this class" specifically. We could even name the class if we're feeling generous.

lib/Sema/SemaDeclCXX.cpp

7886

How confident are we that this logic is correct? I ask because I need something similar for my own diagnostic in D50119. If this logic is rock-solid (no lurking corner-case bugs), I should copy it — and/or it should be moved into a helper member function on CXXRecordDecl so that other people can call it too.

[rsmith]

In D57626#1382391, @Quuxplusone wrote:

I admit that this lock_guard example is contrived and generally ill-advised, but its ill-advisedness seems like a higher-level concern that shouldn't be "enforced" by fiddling with the rules of [[trivial_abi]], so I hope that's not what's going on here.

[[trivial_abi]] (at least right now) only affects whether user-provided special member functions are considered to be trivial for ABI purposes. A class whose copy and move constructor are both deleted is not passed in registers by the ABI; that has nothing to do with triviality, so it's unaffected by [[trivial_abi]] as currently specified. We could "fiddle with the rules of [[trivial_abi]]" to *make* that work (that's what the previous approach for this case did), but as you note, this is an ill-advised case, and fiddling with the rules to give it special behavior doesn't seem like worthwhile complexity. Our design intent was to produce a diagnostic if [[trivial_abi]] is specified on a non-template class that we can't actually pass in registers; this patch fixes a hole in our implementation of that design by adding a missing diagnostic.

[ahatanak]

include/clang/Basic/DiagnosticSemaKinds.td

2953

Does not being move-constructible imply that the class doesn't have a *public* copy or move constructor that isn't deleted? If it does, that is slightly different than saying the copy and move constructors of the class are all deleted. When the users see the message "is not move-constructible", they might think the copy or move constructor that isn't deleted has to be public in order to annotate the class with trivial_abi. For trivial_abi, a private one is good enough.

I think your other suggestions are all good ideas.

lib/Sema/SemaDeclCXX.cpp

7886

I think it's correct. The first part looks for implicit constructors that are not deleted, and the for loop looks for the explicitly declared ones that aren't deleted.

[• Quuxplusone]

include/clang/Basic/DiagnosticSemaKinds.td

2953

Does not being move-constructible imply that the class doesn't have a *public* copy or move constructor that isn't deleted?

Yeah, I suppose so. Okay.

[ahatanak]

Improve diagnostic messages.

[ahatanak]

include/clang/Basic/DiagnosticSemaKinds.td

2953

I changed the message to "its copy constructors and move constructors are all deleted".

test/SemaObjCXX/attr-trivial-abi.mm

108

The diagnostic note printed here was actually "have fields of non-trivial class types", not "don't have non-deleted copy/move constructors", because the class had explicitly declared copy and move constructors. I removed both declarations to make clang print the latter.

[rsmith]

Are you still interested in pursuing this? This change looks good to me if so.

[ahatanak]

Yes, I'm still interested in pursuing this. I'll rebase the patch and commit it if I don't get any more feedback.

[ahatanak]

Rebase patch.

include/clang/Basic/AttrDocs.td

 
 If a type is trivial for the purposes of calls, has a non-trivial destructor,
 and is passed as an argument by value, the convention is that the callee will
 destroy the object before returning.
 
 Attribute ``trivial_abi`` has no effect in the following cases:
 
 - The class directly declares a virtual base or virtual methods.
+- The class doesn't have a copy or move constructor that isn't deleted.
 - The class has a base class that is non-trivial for the purposes of calls.
 - The class has a non-static data member whose type is non-trivial for the purposes of calls, which includes:
 
   - classes that are non-trivial for the purposes of calls
   - __weak-qualified types in Objective-C++
   - arrays of any of the above
   }];
 }

include/clang/Basic/DiagnosticSemaKinds.td

def warn_attribute_not_on_decl : Warning<
   "%0 attribute ignored when parsing type">, InGroup<IgnoredAttributes>;
 def err_base_specifier_attribute : Error<
   "%0 attribute cannot be applied to a base specifier">;
 def err_invalid_attribute_on_virtual_function : Error<
   "%0 attribute cannot be applied to virtual functions">;
 
 def ext_cannot_use_trivial_abi : ExtWarn<
   "'trivial_abi' cannot be applied to %0">, InGroup<IgnoredAttributes>;
+def note_cannot_use_trivial_abi_reason : Note<
+  "'trivial_abi' is disallowed on classes that %select{"
+  "don't have non-deleted copy/move constructors|are polymorphic|"
+  "have bases of non-trivial class types|have virtual bases|"
+  "have __weak fields under ARC|have fields of non-trivial class types}0">;
 
 // Availability attribute
 def warn_availability_unknown_platform : Warning<
   "unknown platform %0 in availability macro">, InGroup<Availability>;
 def warn_availability_version_ordering : Warning<
   "feature cannot be %select{introduced|deprecated|obsoleted}0 in %1 version "
   "%2 before it was %select{introduced|deprecated|obsoleted}3 in version %4; "
   "attribute ignored">, InGroup<Availability>;

lib/Sema/SemaDeclCXX.cpp

if (!OverloadedMethods.empty()) {
     Diag(MD->getLocation(), diag::warn_overloaded_virtual)
       << MD << (OverloadedMethods.size() > 1);
 
     NoteHiddenVirtualMethods(MD, OverloadedMethods);
   }
 }
 
 void Sema::checkIllFormedTrivialABIStruct(CXXRecordDecl &RD) {
-  auto PrintDiagAndRemoveAttr = [&]() {
+  auto PrintDiagAndRemoveAttr = [&](unsigned N) {
     // No diagnostics if this is a template instantiation.
-    if (!isTemplateInstantiation(RD.getTemplateSpecializationKind()))
+    if (!isTemplateInstantiation(RD.getTemplateSpecializationKind())) {
       Diag(RD.getAttr<TrivialABIAttr>()->getLocation(),
            diag::ext_cannot_use_trivial_abi) << &RD;
+      Diag(RD.getAttr<TrivialABIAttr>()->getLocation(),
+           diag::note_cannot_use_trivial_abi_reason) << N;
+    }
     RD.dropAttr<TrivialABIAttr>();
   };
 
+  // Ill-formed if the copy and move constructors are deleted.
+  auto HasNonDeletedCopyOrMoveConstructor = [&]() {
+    if (RD.needsImplicitCopyConstructor() &&
+        !RD.defaultedCopyConstructorIsDeleted())
+      return true;
+    if (RD.needsImplicitMoveConstructor() &&
+        !RD.defaultedMoveConstructorIsDeleted())
+      return true;
+    for (const CXXConstructorDecl *CD : RD.ctors())
+      if (CD->isCopyOrMoveConstructor() && !CD->isDeleted())
+        return true;
+    return false;
+  };
+
+  if (!HasNonDeletedCopyOrMoveConstructor()) {
+    PrintDiagAndRemoveAttr(0);
+    return;
+  }
+
   // Ill-formed if the struct has virtual functions.
   if (RD.isPolymorphic()) {
-    PrintDiagAndRemoveAttr();
+    PrintDiagAndRemoveAttr(1);
     return;
   }
 
   for (const auto &B : RD.bases()) {
     // Ill-formed if the base class is non-trivial for the purpose of calls or a
     // virtual base.
-    if ((!B.getType()->isDependentType() &&
-         !B.getType()->getAsCXXRecordDecl()->canPassInRegisters()) ||
-        B.isVirtual()) {
-      PrintDiagAndRemoveAttr();
+    if (!B.getType()->isDependentType() &&
+        !B.getType()->getAsCXXRecordDecl()->canPassInRegisters()) {
+      PrintDiagAndRemoveAttr(2);
+      return;
+    }
+
+    if (B.isVirtual()) {
+      PrintDiagAndRemoveAttr(3);
       return;
     }
   }
 
   for (const auto *FD : RD.fields()) {
     // Ill-formed if the field is an ObjectiveC pointer or of a type that is
     // non-trivial for the purpose of calls.
     QualType FT = FD->getType();
     if (FT.getObjCLifetime() == Qualifiers::OCL_Weak) {
-      PrintDiagAndRemoveAttr();
+      PrintDiagAndRemoveAttr(4);
       return;
     }
 
     if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs<RecordType>())
       if (!RT->isDependentType() &&
           !cast<CXXRecordDecl>(RT->getDecl())->canPassInRegisters()) {
-        PrintDiagAndRemoveAttr();
+        PrintDiagAndRemoveAttr(5);
         return;
       }
   }
 }
 
 void Sema::ActOnFinishCXXMemberSpecification(
     Scope *S, SourceLocation RLoc, Decl *TagDecl, SourceLocation LBrac,
     SourceLocation RBrac, const ParsedAttributesView &AttrList) {

test/SemaObjCXX/attr-trivial-abi.mm

 // RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
 struct [[clang::trivial_abi]] S0 {
   int a;
 };
 
 struct __attribute__((trivial_abi)) S1 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}} expected-note {{are polymorphic}}
   virtual void m();
 };
 
 struct S3_2 {
   virtual void m();
-} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}}
+} __attribute__((trivial_abi)); // expected-warning {{'trivial_abi' cannot be applied to 'S3_2'}} expected-note {{are polymorphic}}
 
 struct S4 {
   int a;
 };
 
-struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}} expected-note {{have virtual bases}}
 };
 
 struct __attribute__((trivial_abi)) S9 : public S4 {
 };
 
 struct S6 {
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
-struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}} expected-note {{have __weak fields under ARC}}
   __weak id a[2];
 };
 
-struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}} expected-note {{have fields of non-trivial class types}}
   S6 a;
 };
 
-struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}} expected-note {{have fields of non-trivial class types}}
   S6 a[2];
 };
 
 struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
   int a;
 };
 
 // Do not warn when 'trivial_abi' is used to annotate a template class.
 template<class T>
 struct __attribute__((trivial_abi)) S10 {
   T p;
 };
 
 S10<int *> p1;
 S10<__weak id> p2;
 
 template<>
-struct __attribute__((trivial_abi)) S10<id> { // expected-warning {{'trivial_abi' cannot be applied to 'S10<id>'}}
+struct __attribute__((trivial_abi)) S10<id> { // expected-warning {{'trivial_abi' cannot be applied to 'S10<id>'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
 template<class T>
 struct S14 {
   T a;
   __weak id b;
 };
 
 template<class T>
 struct __attribute__((trivial_abi)) S15 : S14<T> {
 };
 
 S15<int> s15;
 
 template<class T>
 struct __attribute__((trivial_abi)) S16 {
   S14<T> a;
 };
 
 S16<int> s16;
 
 template<class T>
-struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}} expected-note {{have __weak fields under ARC}}
   __weak id a;
 };
 
 S17<int> s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}} expected-note {{"don't have non-deleted copy/move constructors}}
+    CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+    CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}} expected-note {{"don't have non-deleted copy/move constructors}}
+    CopyMoveDeleted a;
+    S18(const S18 &);
+    S18(S18 &&);
+  };
+
+  struct __attribute__((trivial_abi)) CopyDeleted {
+    CopyDeleted(const CopyDeleted &) = delete;
+    CopyDeleted(CopyDeleted &&) = default;
+  };
+
+  struct __attribute__((trivial_abi)) MoveDeleted {
+    MoveDeleted(const MoveDeleted &) = default;
+    MoveDeleted(MoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S19 { // expected-warning {{'trivial_abi' cannot be applied to 'S19'}} expected-note {{"don't have non-deleted copy/move constructors}}
+    CopyDeleted a;
+    MoveDeleted b;
+  };
+
+  // This is fine since the move constructor isn't deleted.
+  struct __attribute__((trivial_abi)) S20 {
+    int &&a; // a member of rvalue reference type deletes the copy constructor.
+  };
+}