[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: lib/CodeGen/CGCXXABI.cpp:33-47
+  // See also Sema::ShouldDeleteSpecialMember. These two functions
+  // should be kept consistent.
+
   // If RD has a non-trivial move or copy constructor, we cannot copy the
   // argument.
   if (RD->hasNonTrivialCopyConstructor() || RD->hasNonTrivialMoveConstructor())
     return false;
----------------
Perhaps it would make sense to put this whole "does the language permit passing in registers?" calculation in Sema and store that in the DefinitionData rather than storing the "HasNonDeletedCopyOrMoveConstructor" flag.


================
Comment at: lib/Sema/SemaDecl.cpp:14816-14817
+static bool HasNonDeletedCopyOrMoveCtor(CXXRecordDecl *CXXRD, Sema &SemaRef) {
+  // FIXME: Handle the cases in which CXXRD is dependent. We need to instantiate
+  // the class to figure out if the constructor should be deleted.
+  assert(!CXXRD->isDependentType() && "Not implemented yet");
----------------
This seems like a good motivation for the flag to be a "can pass in registers" flag: that makes it much more obvious that it's meaningless on a dependent class, since such a class will never be passed / returned at all.


================
Comment at: lib/Sema/SemaDecl.cpp:14821-14822
+    CXXConstructorDecl *CopyCtor = SemaRef.DeclareImplicitCopyConstructor(CXXRD);
+    if (SemaRef.ShouldDeleteSpecialMember(CopyCtor, Sema::CXXCopyConstructor))
+      SemaRef.SetDeclDeleted(CopyCtor, CXXRD->getLocation());
+  }
----------------
You don't need to do this: triggering the declaration will delete the member if relevant.

However, we should refactor `ShouldDeleteSpecialMember` so that it can be called without actually having built a special member declaration: this patch removes all laziness in declaring copy or move constructors.


================
Comment at: lib/Sema/SemaDecl.cpp:14825
+
+  if (CXXRD->needsImplicitMoveConstructor()) {
+    CXXConstructorDecl *MoveCtor = SemaRef.DeclareImplicitMoveConstructor(CXXRD);
----------------
This needs to be guarded by a `getLangOpts().CPlusPlus11` check.


================
Comment at: lib/Sema/SemaDecl.cpp:14831-14839
+  bool CopyOrMoveDeleted = false;
+  for (const CXXConstructorDecl *CD : CXXRD->ctors()) {
+    if (CD->isMoveConstructor() || CD->isCopyConstructor()) {
+      if (!CD->isDeleted()) {
+        return true;
+      }
+      CopyOrMoveDeleted = true;
----------------
We should do this check before we call `ShouldDeleteSpecialMember` to get the benefit of the early exit. If we switch to tracking the ABI flag, we can also bail out early if we see a non-trivial copy ctor / move ctor / dtor.


================
Comment at: lib/Sema/SemaDecl.cpp:14841-14852
+  // If a move constructor or move assignment operator was declared, the
+  // default copy constructors are implicitly deleted, except in one case
+  // related to compatibility with MSVC pre-2015.
+  if (CXXRD->hasUserDeclaredMoveConstructor())
+    CopyOrMoveDeleted = true;
+  if (CXXRD->hasUserDeclaredMoveAssignment()) {
+    const LangOptions &opts = SemaRef.getASTContext().getLangOpts();
----------------
Do we need this? The above code will have already declared as deleted the relevant operator, so this seems like it can never trigger.


================
Comment at: lib/Sema/SemaDecl.cpp:15147-15148
 
     if (!Completed)
       Record->completeDefinition();
 
----------------
This call also needs to pass in the relevant flag.


https://reviews.llvm.org/D35056