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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline 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.
----------------
rnk wrote:
> Isn't this "... at least one *trivial*, non-deleted copy or move constructor..."?
Changed to:

```
    /// \brief True if this class can be passed in a non-address-preserving
    /// fashion (such as in registers) according to the C++ language rules.
    /// This does not imply anything about how the ABI in use will actually
    /// pass an object of this class.
```


================
Comment at: include/clang/AST/DeclCXX.h:827
+    return data().DefaultedCopyConstructorIsDeleted;
+  }
+  /// \brief \c true if a defaulted move constructor for this class would be
----------------
v.g.vassilev wrote:
> Is there a reason for not keeping the default (for the file) 1 empty line between methods? Looks like if we add one new line before and after `hasSimpleMoveAssignment` is will be all consistent.
Done. (I was following the local style, but you're right that we don't do this elsewhere in the class outside the `hasSimple` functions, excluding groups of methods that are much more closely tied together such as `*_begin`/`*_end`.)


================
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.
+    //
----------------
rnk wrote:
> 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."
You're right. I think "it is passed directly" is overspecifying, though, so how about:

```
    // If a class has at least one non-deleted, trivial copy constructor, it
    // is passed according to the C ABI. Otherwise, it is passed indirectly.
```


================
Comment at: lib/Sema/SemaDeclCXX.cpp:5731
+/// registers, per C++ [class.temporary]p3.
+static bool computeCanPassInRegisters(Sema &S, CXXRecordDecl *D) {
+  if (D->isDependentType() || D->isInvalidDecl())
----------------
v.g.vassilev wrote:
> It would be very useful if we somehow assert if this function is called before the class triviality is computed?
Many of the functions we unconditionally call below will assert if the class does not have a complete definition (eg, `needsImplicitCopyConstructor`).


================
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
----------------
rnk wrote:
> Oh dear. :(
Can you check that MSVC 2013 is compatible with the code we produce here? (I've checked 2015 passes this indirectly on Compiler Explorer.)


Repository:
  rL LLVM

https://reviews.llvm.org/D35056