Don't copy objects with trivial, deleted copy ctors

[rnk]

Summary

This affects both the Itanium and Microsoft C++ ABIs.

Our C++ ABI code was checking if the object had a non-trivial copy ctor
or non-trivial dtor. Now we check if there are any trivial, non-deleted
copy or move ctors before passing an argument in registers.

On x86 Windows, we can mostly use the same logic, where we use inalloca
instead of passing by address. However, on Win64, there are register
parameters, and we have to do what MSVC does. MSVC ignores the presence
of non-trivial move constructors and only considers the presence of
non-trivial or deleted copy constructors. If a non-trivial or deleted
copy ctor is present, it passes the argument indirectly.

This change fixes bugs and makes us more ABI compatible with both GCC
and MSVC.

Fixes PR19668.

[rsmith]

Richard, you had concerns that this loop over ctors was incorrect, and that
I should really be making some change to CXXRecordDecl::DefinitionData.
Can you imagine some test cases that might be problematic?

Something like:

struct A {

A(const A&) = default;
A(const A&, int = 0);
void *p;

};
struct B : A {};

void foo(B); // B should be passed indirect, because its copy ctor is deleted, but it's also trivial and not-yet-declared so we might not realize this.

If so, do you think it's worth pursuing a more complete fix that
updates the flags, or is it OK to accept this as an incremental fix?

It's an extremely obscure case, so the incremental fix seems reasonable to me.

lib/CodeGen/CGCXXABI.cpp

49–50 ↗

(On Diff #9197)

Please add a FIXME saying that we assume that all lazily-declared trivial copy/move constructors are not deleted, and that this assumption might not actually be true in some corner cases.

lib/CodeGen/ItaniumCXXABI.cpp

65–66 ↗

(On Diff #9197)

This should probably be part of the 'canCopy' check; the proposed fix for DR1590 also requires a trivial destructor.

lib/CodeGen/MicrosoftCXXABI.cpp

431–435 ↗

(On Diff #9197)

As an optimization, you could skip this if !hasTrivialCopyConstructor().

[rnk]

OK. I added a two-copy ctor test case, but it's not enough of a corner case. We appear to eagerly declare the implicit special members for it.

PTAL

lib/CodeGen/CGCXXABI.cpp

49–50 ↗

(On Diff #9197)

done

lib/CodeGen/ItaniumCXXABI.cpp

65–66 ↗

(On Diff #9197)

I wasn't thinking about C++11 braced, copyless initialization when I named this. I intended to share it with win64, but those rules are more complicated. I'll fold this in.

lib/CodeGen/MicrosoftCXXABI.cpp

431–435 ↗

(On Diff #9197)

done

[rnk]

• Review stuff and add a test case for multiple copy ctors

[rnk]

• Fix zero ctor case

[rnk]

• Simplify by exiting early

[rnk]

Closed by commit rL208786 (authored by @rnk).

cfe/trunk/lib/CodeGen/CGCXXABI.h

enum RecordArgABI {
     /// evaluated directly into the correct stack position in the arguments area,
     /// and the call machinery must not move it or introduce extra copies.
     RAA_DirectInMemory,
 
     /// Pass it as a pointer to temporary memory.
     RAA_Indirect
   };
 
+  /// Returns true if C++ allows us to copy the memory of an object of type RD
+  /// when it is passed as an argument.
+  bool canCopyArgument(const CXXRecordDecl *RD) const;
+
   /// Returns how an argument of the given record type should be passed.
   virtual RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const = 0;
 
   /// Returns true if the implicit 'sret' parameter comes after the implicit
   /// 'this' parameter of C++ instance methods.
   virtual bool isSRetParameterAfterThis() const { return false; }
 
   /// Find the LLVM type used to represent the given member pointer

cfe/trunk/lib/CodeGen/CGCXXABI.cpp

void CGCXXABI::ErrorUnsupportedABI(CodeGenFunction &CGF, StringRef S) {
   DiagnosticsEngine &Diags = CGF.CGM.getDiags();
   unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
                                           "cannot yet compile %0 in this ABI");
   Diags.Report(CGF.getContext().getFullLoc(CGF.CurCodeDecl->getLocation()),
                DiagID)
     << S;
 }
 
+bool CGCXXABI::canCopyArgument(const CXXRecordDecl *RD) const {
+  // If RD has a non-trivial move or copy constructor, we cannot copy the
+  // argument.
+  if (RD->hasNonTrivialCopyConstructor() || RD->hasNonTrivialMoveConstructor())
+    return false;
+
+  // If RD has a non-trivial destructor, we cannot copy the argument.
+  if (RD->hasNonTrivialDestructor())
+    return false;
+
+  // 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 CopyOrMoveDeleted = false;
+  for (const CXXConstructorDecl *CD : RD->ctors()) {
+    if (CD->isCopyConstructor() || CD->isMoveConstructor()) {
+      assert(CD->isTrivial());
+      // We had at least one undeleted trivial copy or move ctor.  Return
+      // directly.
+      if (!CD->isDeleted())
+        return true;
+      CopyOrMoveDeleted = true;
+    }
+  }
+
+  // If all trivial copy and move constructors are deleted, we cannot copy the
+  // argument.
+  return !CopyOrMoveDeleted;
+}
+
 llvm::Constant *CGCXXABI::GetBogusMemberPointer(QualType T) {
   return llvm::Constant::getNullValue(CGM.getTypes().ConvertType(T));
 }
 
 llvm::Type *
 CGCXXABI::ConvertMemberPointerType(const MemberPointerType *MPT) {
   return CGM.getTypes().ConvertType(CGM.getContext().getPointerDiffType());
 }

cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

ItaniumCXXABI(CodeGen::CodeGenModule &CGM,
                 bool UseARMMethodPtrABI = false,
                 bool UseARMGuardVarABI = false) :
     CGCXXABI(CGM), UseARMMethodPtrABI(UseARMMethodPtrABI),
     UseARMGuardVarABI(UseARMGuardVarABI) { }
 
   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.
-    if (!RD->hasTrivialDestructor() || RD->hasNonTrivialCopyConstructor())
+    // If C++ prohibits us from making a copy, pass by address.
+    if (!canCopyArgument(RD))
       return RAA_Indirect;
     return RAA_Default;
   }
 
   bool isZeroInitializable(const MemberPointerType *MPT) override;
 
   llvm::Type *ConvertMemberPointerType(const MemberPointerType *MPT) override;
 
ItaniumCXXABI::EmitMemberPointerIsNotNull(CodeGenFunction &CGF,
   return Result;
 }
 
 bool ItaniumCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
   const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
   if (!RD)
     return false;
 
-  // Return indirectly if we have a non-trivial copy ctor or non-trivial dtor.
-  if (RD->hasNonTrivialDestructor() || RD->hasNonTrivialCopyConstructor()) {
+  // If C++ prohibits us from making a copy, return by address.
+  if (!canCopyArgument(RD)) {
     FI.getReturnInfo() = ABIArgInfo::getIndirect(0, /*ByVal=*/false);
     return true;
   }
-
-  // Otherwise, use the normal C ABI rules.
   return false;
 }
 
 /// The Itanium ABI requires non-zero initialization only for data
 /// member pointers, for which '0' is a valid offset.
 bool ItaniumCXXABI::isZeroInitializable(const MemberPointerType *MPT) {
   return MPT->getPointeeType()->isFunctionType();
 }

cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp

 CGCXXABI::RecordArgABI
 MicrosoftCXXABI::getRecordArgABI(const CXXRecordDecl *RD) const {
   switch (CGM.getTarget().getTriple().getArch()) {
   default:
     // FIXME: Implement for other architectures.
     return RAA_Default;
 
   case llvm::Triple::x86:
-    // 32-bit x86 constructs non-trivial objects directly in outgoing argument
-    // slots.  LLVM uses the inalloca attribute to implement this.
-    if (RD->hasNonTrivialCopyConstructor() || RD->hasNonTrivialDestructor())
+    // All record arguments are passed in memory on x86.  Decide whether to
+    // construct the object directly in argument memory, or to construct the
+    // argument elsewhere and copy the bytes during the call.
+
+    // If C++ prohibits us from making a copy, construct the arguments directly
+    // into argument memory.
+    if (!canCopyArgument(RD))
       return RAA_DirectInMemory;
+
+    // Otherwise, construct the argument into a temporary and copy the bytes
+    // into the outgoing argument memory.
     return RAA_Default;
 
   case llvm::Triple::x86_64:
     // Win64 passes objects with non-trivial copy ctors indirectly.
     if (RD->hasNonTrivialCopyConstructor())
       return RAA_Indirect;
+
     // Win64 passes objects larger than 8 bytes indirectly.
     if (getContext().getTypeSize(RD->getTypeForDecl()) > 64)
       return RAA_Indirect;
+
+    // We have a trivial copy constructor or no copy constructors, but we have
+    // to make sure it isn't deleted.
+    bool CopyDeleted = false;
+    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;
+        CopyDeleted = true;
+      }
+    }
+
+    // The trivial copy constructor was deleted.  Return indirectly.
+    if (CopyDeleted)
+      return RAA_Indirect;
+
+    // There were no copy ctors.  Return in RAX.
     return RAA_Default;
   }
 
   llvm_unreachable("invalid enum");
 }
 
 llvm::Value *MicrosoftCXXABI::adjustToCompleteObject(CodeGenFunction &CGF,
                                                      llvm::Value *ptr,

cfe/trunk/test/CodeGenCXX/uncopyable-args.cpp

+// RUN: %clang_cc1 -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-windows-msvc -emit-llvm -o - %s | FileCheck %s -check-prefix=WIN64
+
+namespace trivial {
+// Trivial structs should be passed directly.
+struct A {
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// CHECK-LABEL: define void @_ZN7trivial3barEv()
+// CHECK: alloca %"struct.trivial::A"
+// CHECK: load i8**
+// CHECK: call void @_ZN7trivial3fooENS_1AE(i8* %{{.*}})
+// CHECK-LABEL: declare void @_ZN7trivial3fooENS_1AE(i8*)
+
+// WIN64-LABEL: declare void @"\01?foo@trivial@@YAXUA@1@@Z"(i64)
+}
+
+namespace default_ctor {
+struct A {
+  A();
+  void *p;
+};
+void foo(A);
+void bar() {
+  // Core issue 1590.  We can pass this type in registers, even though C++
+  // normally doesn't permit copies when using braced initialization.
+  foo({});
+}
+// CHECK-LABEL: define void @_ZN12default_ctor3barEv()
+// CHECK: alloca %"struct.default_ctor::A"
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK: load i8**
+// CHECK: call void @_ZN12default_ctor3fooENS_1AE(i8* %{{.*}})
+// CHECK-LABEL: declare void @_ZN12default_ctor3fooENS_1AE(i8*)
+
+// WIN64-LABEL: declare void @"\01?foo@default_ctor@@YAXUA@1@@Z"(i64)
+}
+
+namespace move_ctor {
+// The presence of a move constructor implicitly deletes the trivial copy ctor
+// and means that we have to pass this struct by address.
+struct A {
+  A();
+  A(A &&o);
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// 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"*)
+}
+
+namespace all_deleted {
+struct A {
+  A();
+  A(const A &o) = delete;
+  A(A &&o) = delete;
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// 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"*)
+}
+
+namespace implicitly_deleted {
+struct A {
+  A();
+  A &operator=(A &&o);
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// 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"*)
+}
+
+namespace one_deleted {
+struct A {
+  A();
+  A(A &&o) = delete;
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// 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"*)
+}
+
+namespace copy_defaulted {
+struct A {
+  A();
+  A(const A &o) = default;
+  A(A &&o) = delete;
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// CHECK-LABEL: define void @_ZN14copy_defaulted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK: load i8**
+// CHECK: call void @_ZN14copy_defaulted3fooENS_1AE(i8* %{{.*}})
+// CHECK-LABEL: declare void @_ZN14copy_defaulted3fooENS_1AE(i8*)
+
+// WIN64-LABEL: declare void @"\01?foo@copy_defaulted@@YAXUA@1@@Z"(i64)
+}
+
+namespace move_defaulted {
+struct A {
+  A();
+  A(const A &o) = delete;
+  A(A &&o) = default;
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// CHECK-LABEL: define void @_ZN14move_defaulted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK: load i8**
+// CHECK: call void @_ZN14move_defaulted3fooENS_1AE(i8* %{{.*}})
+// CHECK-LABEL: declare void @_ZN14move_defaulted3fooENS_1AE(i8*)
+
+// WIN64-LABEL: declare void @"\01?foo@move_defaulted@@YAXUA@1@@Z"(%"struct.move_defaulted::A"*)
+}
+
+namespace trivial_defaulted {
+struct A {
+  A();
+  A(const A &o) = default;
+  void *p;
+};
+void foo(A);
+void bar() {
+  foo({});
+}
+// CHECK-LABEL: define void @_ZN17trivial_defaulted3barEv()
+// CHECK: call void @_Z{{.*}}C1Ev(
+// CHECK: load i8**
+// CHECK: call void @_ZN17trivial_defaulted3fooENS_1AE(i8* %{{.*}})
+// CHECK-LABEL: declare void @_ZN17trivial_defaulted3fooENS_1AE(i8*)
+
+// WIN64-LABEL: declare void @"\01?foo@trivial_defaulted@@YAXUA@1@@Z"(i64)
+}
+
+namespace two_copy_ctors {
+struct A {
+  A();
+  A(const A &) = default;
+  A(const A &, int = 0);
+  void *p;
+};
+struct B : A {};
+
+void foo(B);
+void bar() {
+  foo({});
+}
+// 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"*)
+}