GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

[rsmith]

Summary

Fixes PR19668 and PR23034.

[rjmccall]

If this is a common algorithm across all ABIs, can we just put Sema / the AST in charge of computing it? It's not a cheap thing to recompute retroactively, especially for an imported type, and it seems like it's easily derived from the things that go into DerivedData.

[rnk]

I discussed this with @rsmith and we think the correct fix is to update the DefinitionData bits computed when adding special members. I haven't reviewed this patch in detail, but we came to the conclusion that Clang's optimization to avoid implicit special member declarations in most situations makes it impossible to get the correct answer in all situations here. The explicit loop over RD->ctors() is a bug in itself, because important ones may not be there.

I'm never able to remember the details, but we believe that's the "right" fix. This would change the values reported by some of the older pre-c++11 GCC type traits (the __has_* ones, I think), but modern versions of libstdc++ no longer use them.

We're hesitant to accept a workaround in the meantime because it means we'll have to fix the bug again later and create another ABI break with ourselves. However, we shouldn't let the perfect be the enemy of the good. In practice, I suspect we are not very good about maintaining ABI stability in corner cases this deep.

[v.g.vassilev]

Compute only once if there is a non-deleted move or copy constructor and store them in the DefinitionData of the CXXRecordDecl.

[v.g.vassilev]

Remove accidentally added change in SemaDecl.cpp

[rjmccall]

Thank you, I like this approach much better, and the IRGen changes seem fine to me. I'd like to defer to someone else (probably Richard) to review whether the changes to completeDefinition() are correct; I'm not up-to-date with how we handle lazy declarations.

[rsmith]

lib/AST/DeclCXX.cpp

1476 ↗

(On Diff #105776)

This is wrong. "Has a non-deleted copy or move constructor" is not the same thing as "does not have a deleted copy or move constructor". And you will also need to check for the case where the class notionally has such a constructor, but where we have lazily not yet declared it -- in that case, you may need to do arbitrary work (including template instantiation etc) to figure out whether the constructor should be deleted.

Please move this check into Sema and pass a bool to completeDefinition with the result of the check. That way you will have the tools available to compute the correct value in the case where you need to trigger the declaration of a constructor to determine the right answer.

[v.g.vassilev]

Move the checks in Sema.

[rsmith]

lib/CodeGen/CGCXXABI.cpp

33–47

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.

lib/Sema/SemaDecl.cpp

14816–14817 ↗

(On Diff #108505)

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.

14821–14822 ↗

(On Diff #108505)

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.

14825 ↗

(On Diff #108505)

This needs to be guarded by a getLangOpts().CPlusPlus11 check.

14831–14839 ↗

(On Diff #108505)

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.

14841–14852 ↗

(On Diff #108505)

Do we need this? The above code will have already declared as deleted the relevant operator, so this seems like it can never trigger.

15147–15148 ↗

(On Diff #108505)

This call also needs to pass in the relevant flag.

[v.g.vassilev]

Address some of the comments.

lib/Sema/SemaDecl.cpp

14841–14852 ↗

(On Diff #108505)

You are right, we do not need it.

[v.g.vassilev]

Put back accidentally removed test case.

[v.g.vassilev]

Move back the triviality checks in CGCXXABI. Explain why.

[rjmccall]

lib/CodeGen/CGCXXABI.cpp

43

Does canPassInRegisters() not subsume all of these earlier checks?

[v.g.vassilev]

lib/CodeGen/CGCXXABI.cpp

43

No, if I remove them here I get a lot of test failures. I cannot move them (yet?) in Sema, because I need to call Sema::CheckCompletedCXXClass in Sema::ActOnFields to compute the triviality of the decl. Only then it would be safe move these checks in CanPassInRegisters.

[rjmccall]

lib/CodeGen/CGCXXABI.cpp

43

I feel it's pretty unfortunate to name a property "canPassInRegisters" when it actually means something quite different. People will expect this to mean what it says.

[v.g.vassilev]

We set the record's property denoting whether we can pass the decl by registers as a last step of Sema::CheckCompletedCXXClass. We cannot do it any earlier than that because we have not computed the triviality information.

This patch still eagerly defines too many implicit members. As discussed with @rsmith, he will take care of the non-trivial refactoring of Sema::ShouldDeleteSpecialMember to fix this regression.

This patch passes all tests in codegen but still fails a few which are sensitive to the amount of implicit members being created:

Failing Tests (13):
    Clang :: CodeCompletion/ordinary-name-cxx11.cpp
    Clang :: Misc/ast-dump-color.cpp
    Clang :: Misc/ast-dump-decl.cpp
    Clang :: Misc/ast-dump-invalid.cpp
    Clang-Unit :: AST/./ASTTests/CXXMethodDecl.CXXMethodDeclWithNoExceptSpecification
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/ConstructorDeclaration.IsImplicit
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/ConstructorDeclaration.Kinds
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/DeclarationMatcher.MatchNot
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/DeclarationMatcher.hasMethod
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/DeclaratorDecl.MatchesDeclaratorDecls
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/Matcher.References
    Clang-Unit :: ASTMatchers/./ASTMatchersTests/TypeMatcher.MatchesClassType
    Clang-Unit :: ASTMatchers/Dynamic/./DynamicASTMatchersTests/RegistryTest.VariadicOp

  Expected Passes    : 10786
  Expected Failures  : 17
  Unsupported Tests  : 195
  Unexpected Failures: 13

[rjmccall]

lib/Sema/SemaDeclCXX.cpp

5742 ↗

(On Diff #109932)

This should probably be called something like "computeCanPassInRegisters" to discourage other code from calling it directly.

It should also just be a static function in this file unless it needs to be a member for some access-control reason.

[rsmith]

As requested by Vassil, I'm going to upload another version of this that avoids declaring implicit special members so frequently.

[rsmith]

Remove added calls to DeclareImplicit* and ShouldDeleteSpecialMember. In their place, figure out whether an implicit special member would be deleted or not by querying the AST.

This requires teaching CXXRecordDecl to track whether an implicit copy constructor for a class would be deleted, in the same way we do for the move constructor and move assignment operator; as with those other two cases, we fall back to an "ask Sema" state if the computation is not simple, and in that case Sema eagerly declares the special member in question to compute the answer. As a result, this can cause us to declare some copy constructors that we didn't declare previously, but in the important common cases we will still declare them lazily.

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).

[rnk]

In 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.

[rsmith]

In D35056#834705, @rnk wrote:

In 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.

I forgot to say: the new behavior matches MSVC -- at least for the testcase whose result changed (which is a new testcase I added here). The immediate bug was that we didn't consider the possibility that a user-declared copy assignment operator could cause the implicit copy constructor to be deleted, but there may have been other bugs also caused by this part of CodeGen attempting to work out deletedness for itself rather than asking.

[v.g.vassilev]

I do not feel qualified enough to review this patch but I added few minor comments.

include/clang/AST/DeclCXX.h

827 ↗

(On Diff #110115)

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.

lib/Sema/SemaDeclCXX.cpp

5731 ↗

(On Diff #110115)

It would be very useful if we somehow assert if this function is called before the class triviality is computed?

[v.g.vassilev]

And thanks for working on this!!

[rnk]

In D35056#834705, @rnk wrote:

In 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.

include/clang/AST/DeclCXX.h

420 ↗

(On Diff #110115)

Isn't this "... at least one *trivial*, non-deleted copy or move constructor..."?

lib/CodeGen/MicrosoftCXXABI.cpp

836 ↗

(On Diff #110115)

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."

test/CodeGenCXX/uncopyable-args.cpp

101

Oh dear. :(

[rsmith]

include/clang/AST/DeclCXX.h

420 ↗

(On Diff #110115)

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.

827 ↗

(On Diff #110115)

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.)

lib/CodeGen/MicrosoftCXXABI.cpp

836 ↗

(On Diff #110115)

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.

lib/Sema/SemaDeclCXX.cpp

5731 ↗

(On Diff #110115)

Many of the functions we unconditionally call below will assert if the class does not have a complete definition (eg, needsImplicitCopyConstructor).

test/CodeGenCXX/uncopyable-args.cpp

101

Can you check that MSVC 2013 is compatible with the code we produce here? (I've checked 2015 passes this indirectly on Compiler Explorer.)

[rnk]

test/CodeGenCXX/uncopyable-args.cpp

101

Yes, 2013 passes this object directly as we do here.

[rsmith]

Committed as r310401.

lib/CodeGen/CGCXXABI.cpp

 }
 
 bool CGCXXABI::canCopyArgument(const CXXRecordDecl *RD) const {
+  // 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())

 
   // We can only copy the argument if there exists at least one trivial,
   // non-deleted copy or move constructor.
-  // FIXME: This assumes that all lazily declared copy and move constructors are
-  // not deleted.  This assumption might not be true in some corner cases.
-  bool CopyDeleted = false;
-  bool MoveDeleted = false;
+  bool CopyOrMoveDeleted = false;
   for (const CXXConstructorDecl *CD : RD->ctors()) {
     if (CD->isCopyConstructor() || CD->isMoveConstructor()) {
       assert(CD->isTrivial());

       // directly.
       if (!CD->isDeleted())
         return true;
-      if (CD->isCopyConstructor())
-        CopyDeleted = true;
-      else
-        MoveDeleted = true;
+      CopyOrMoveDeleted = true;
     }
   }
+  // 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 (RD->hasUserDeclaredMoveConstructor())
+    return false;
+  if (RD->hasUserDeclaredMoveAssignment()) {
+    const LangOptions &opts = CGM.getLangOpts();
+    bool DeletesOnlyMatchingCopy =
+      opts.MSVCCompat && !opts.isCompatibleWithMSVC(LangOptions::MSVC2015);
+    if (!DeletesOnlyMatchingCopy)
+      return false;
+  }
 
   // If all trivial copy and move constructors are deleted, we cannot copy the
   // argument.
-  return !(CopyDeleted && MoveDeleted);
+  return !CopyOrMoveDeleted;
 }
 
 llvm::Constant *CGCXXABI::GetBogusMemberPointer(QualType T) {

lib/CodeGen/ItaniumCXXABI.cpp

   bool classifyReturnType(CGFunctionInfo &FI) const override;
 
   RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const override {
-    // Structures with either a non-trivial destructor or a non-trivial
-    // copy constructor are always indirect.
-    // FIXME: Use canCopyArgument() when it is fixed to handle lazily declared
-    // special members.
-    if (RD->hasNonTrivialDestructor() || RD->hasNonTrivialCopyConstructor())
+    // If C++ prohibits us from making a copy, pass by address.
+    if (!canCopyArgument(RD))
       return RAA_Indirect;
     return RAA_Default;
   }

   if (!RD)
     return false;
 
-  // Return indirectly if we have a non-trivial copy ctor or non-trivial dtor.
-  // FIXME: Use canCopyArgument() when it is fixed to handle lazily declared
-  // special members.
-  if (RD->hasNonTrivialDestructor() || RD->hasNonTrivialCopyConstructor()) {
+  // If C++ prohibits us from making a copy, return by address.
+  if (!canCopyArgument(RD)) {
     auto Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
     return true;

test/CodeGenCXX/uncopyable-args.cpp

 void bar() {
   foo({});
 }
-// FIXME: The copy ctor is implicitly deleted.
-// CHECK-DISABLED-LABEL: define void @_ZN9move_ctor3barEv()
-// CHECK-DISABLED: call void @_Z{{.*}}C1Ev(
-// CHECK-DISABLED-NOT: call
-// CHECK-DISABLED: call void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"* %{{.*}})
-// CHECK-DISABLED-LABEL: declare void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"*)
+// CHECK-LABEL: define void @_ZN9move_ctor3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK-NOT: call
+// CHECK: call void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"* %{{.*}})
+// CHECK-LABEL: declare void @_ZN9move_ctor3fooENS_1AE(%"struct.move_ctor::A"*)
 
 // WIN64-LABEL: declare void @"\01?foo@move_ctor@@YAXUA@1@@Z"(%"struct.move_ctor::A"*)
 }

 void bar() {
   foo({});
 }
-// FIXME: The copy ctor is deleted.
-// CHECK-DISABLED-LABEL: define void @_ZN11all_deleted3barEv()
-// CHECK-DISABLED: call void @_Z{{.*}}C1Ev(
-// CHECK-DISABLED-NOT: call
-// CHECK-DISABLED: call void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"* %{{.*}})
-// CHECK-DISABLED-LABEL: declare void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"*)
+// CHECK-LABEL: define void @_ZN11all_deleted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK-NOT: call
+// CHECK: call void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"* %{{.*}})
+// CHECK-LABEL: declare void @_ZN11all_deleted3fooENS_1AE(%"struct.all_deleted::A"*)
 
 // WIN64-LABEL: declare void @"\01?foo@all_deleted@@YAXUA@1@@Z"(%"struct.all_deleted::A"*)
 }

 void bar() {
   foo({});
 }
-// FIXME: The copy and move ctors are implicitly deleted.
-// CHECK-DISABLED-LABEL: define void @_ZN18implicitly_deleted3barEv()
-// CHECK-DISABLED: call void @_Z{{.*}}C1Ev(
-// CHECK-DISABLED-NOT: call
-// CHECK-DISABLED: call void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"* %{{.*}})
-// CHECK-DISABLED-LABEL: declare void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"*)
+// CHECK-LABEL: define void @_ZN18implicitly_deleted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK-NOT: call
+// CHECK: call void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"* %{{.*}})
+// CHECK-LABEL: declare void @_ZN18implicitly_deleted3fooENS_1AE(%"struct.implicitly_deleted::A"*)
 
 // WIN64-LABEL: declare void @"\01?foo@implicitly_deleted@@YAXUA@1@@Z"(%"struct.implicitly_deleted::A"*)
 }

 void bar() {
   foo({});
 }
-// FIXME: The copy constructor is implicitly deleted.
-// CHECK-DISABLED-LABEL: define void @_ZN11one_deleted3barEv()
-// CHECK-DISABLED: call void @_Z{{.*}}C1Ev(
-// CHECK-DISABLED-NOT: call
-// CHECK-DISABLED: call void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"* %{{.*}})
-// CHECK-DISABLED-LABEL: declare void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"*)
+// CHECK-LABEL: define void @_ZN11one_deleted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK-NOT: call
+// CHECK: call void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"* %{{.*}})
+// CHECK-LABEL: declare void @_ZN11one_deleted3fooENS_1AE(%"struct.one_deleted::A"*)
 
 // WIN64-LABEL: declare void @"\01?foo@one_deleted@@YAXUA@1@@Z"(%"struct.one_deleted::A"*)
 }

 void bar() {
   foo({});
 }
-// FIXME: This class has a non-trivial copy ctor and a trivial copy ctor.  It's
-// not clear whether we should pass by address or in registers.
-// CHECK-DISABLED-LABEL: define void @_ZN14two_copy_ctors3barEv()
-// CHECK-DISABLED: call void @_Z{{.*}}C1Ev(
-// CHECK-DISABLED: call void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* %{{.*}})
-// CHECK-DISABLED-LABEL: declare void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"*)
+// CHECK-LABEL: define void @_ZN14two_copy_ctors3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK: call void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"* %{{.*}})
+// CHECK-LABEL: declare void @_ZN14two_copy_ctors3fooENS_1BE(%"struct.two_copy_ctors::B"*)
 
 // WIN64-LABEL: declare void @"\01?foo@two_copy_ctors@@YAXUB@1@@Z"(%"struct.two_copy_ctors::B"*)
 }