Page MenuHomePhabricator

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

Authored by ahatanak on Feb 1 2019, 3:09 PM.

Details

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

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.Feb 1 2019, 3:09 PM
rjmccall added inline comments.Feb 1 2019, 3:49 PM
lib/Sema/SemaDeclCXX.cpp
7923

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.

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.

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 updated this revision to Diff 185157.Feb 4 2019, 2:38 PM

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

Quuxplusone added inline comments.Feb 4 2019, 3:03 PM
include/clang/Basic/DiagnosticSemaKinds.td
2956

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
7920

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 added a comment.Feb 4 2019, 3:09 PM

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 marked 2 inline comments as done.Feb 5 2019, 3:42 PM
ahatanak added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
2956

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
7920

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 added inline comments.Feb 6 2019, 10:38 AM
include/clang/Basic/DiagnosticSemaKinds.td
2956

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 updated this revision to Diff 186083.Feb 8 2019, 5:32 PM
ahatanak marked 4 inline comments as done.

Improve diagnostic messages.

ahatanak added inline comments.Feb 8 2019, 5:33 PM
include/clang/Basic/DiagnosticSemaKinds.td
2956

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.