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

7886

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 ↗

(On Diff #185157)

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

7883

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 ↗

(On Diff #185157)

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

7883

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 ↗

(On Diff #185157)

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 ↗

(On Diff #185157)

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
   }];
 }

lib/Sema/SemaDeclCXX.cpp

void Sema::checkIllFormedTrivialABIStruct(CXXRecordDecl &RD) {
   auto PrintDiagAndRemoveAttr = [&]() {
     // No diagnostics if this is a template instantiation.
     if (!isTemplateInstantiation(RD.getTemplateSpecializationKind()))
       Diag(RD.getAttr<TrivialABIAttr>()->getLocation(),
            diag::ext_cannot_use_trivial_abi) << &RD;
     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();
+    return;
+  }
+
   // Ill-formed if the struct has virtual functions.
   if (RD.isPolymorphic()) {
     PrintDiagAndRemoveAttr();
     return;
   }
 
   for (const auto &B : RD.bases()) {
     // Ill-formed if the base class is non-trivial for the purpose of calls or a

test/SemaObjCXX/attr-trivial-abi.mm

 S16<int> s16;
 
 template<class T>
 struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
   __weak id a;
 };
 
 S17<int> s17;
+
+namespace deletedCopyMoveConstructor {
+  struct __attribute__((trivial_abi)) CopyMoveDeleted { // expected-warning {{'trivial_abi' cannot be applied to 'CopyMoveDeleted'}}
+    CopyMoveDeleted(const CopyMoveDeleted &) = delete;
+    CopyMoveDeleted(CopyMoveDeleted &&) = delete;
+  };
+
+  struct __attribute__((trivial_abi)) S18 { // expected-warning {{'trivial_abi' cannot be applied to 'S18'}}
+    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'}}
+    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.
+  };
+}