[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D35056#834705, @rnk wrote:

> In https://reviews.llvm.org/D35056#834689, @rsmith wrote:
>
> > I also removed some incorrect assumptions from the Win64 ABI code; this changed the behavior of one testcase from uncopyable-args.cpp (`implicitly_deleted_copy_ctor::A` is now passed indirect).
>
>
> That's probably not correct, let me take a look... I remember breaking the rules for small types here.


Nevermind, everything looks good there. Thanks for untangling the mess. I only have comments on comments.



================
Comment at: include/clang/AST/DeclCXX.h:420
 
+    /// \brief True if this class has at least one non-deleted copy or move
+    /// constructor. That would allow passing it by registers.
----------------
Isn't this "... at least one *trivial*, non-deleted copy or move constructor..."?


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:836
 
-    // If this is true, the implicit copy constructor that Sema would have
-    // created would not be deleted. FIXME: We should provide a more direct way
-    // for CodeGen to ask whether the constructor was deleted.
-    if (!RD->hasUserDeclaredCopyConstructor() &&
-        !RD->hasUserDeclaredMoveConstructor() &&
-        !RD->needsOverloadResolutionForMoveConstructor() &&
-        !RD->hasUserDeclaredMoveAssignment() &&
-        !RD->needsOverloadResolutionForMoveAssignment())
-      return RAA_Default;
-
-    // Otherwise, Sema should have created an implicit copy constructor if
-    // needed.
-    assert(!RD->needsImplicitCopyConstructor());
-
-    // We have to make sure the trivial copy constructor isn't deleted.
-    for (const CXXConstructorDecl *CD : RD->ctors()) {
-      if (CD->isCopyConstructor()) {
-        assert(CD->isTrivial());
-        // We had at least one undeleted trivial copy ctor.  Return directly.
-        if (!CD->isDeleted())
-          return RAA_Default;
+    // Win64 passes objects with non-deleted, non-trivial copy ctors indirectly.
+    //
----------------
This doesn't seem to match what we've computing, and it doesn't seem quite right. MSVC will pass a class with deleted, trivial copy ctors indirectly. Would it be correct to rephrase like this?
"If RD has at least one trivial, non-deleted copy constructor, it is passed directly. Otherwise, it is passed indirectly."


================
Comment at: test/CodeGenCXX/uncopyable-args.cpp:101
+
+// In MSVC 2013, the copy ctor is not deleted by a move assignment. In MSVC 2015, it is.
+// WIN64-18-LABEL: declare void @"\01?foo at implicitly_deleted@@YAXUA at 1@@Z"(i64
----------------
Oh dear. :(


Repository:
  rL LLVM

https://reviews.llvm.org/D35056