Skip to content

Commit

Permalink
PR19668, PR23034: Fix handling of move constructors and deleted copy
Browse files Browse the repository at this point in the history 
constructors when deciding whether classes should be passed indirectly.

This fixes ABI differences between Clang and GCC:

 * Previously, Clang ignored the move constructor when making this
   determination. It now takes the move constructor into account, per
   itanium-cxx-abi/cxx-abi#17 (this change may
   seem recent, but the ABI change was agreed on the Itanium C++ ABI
   list a long time ago).

 * Previously, Clang's behavior when the copy constructor was deleted
   was unstable -- depending on whether the lazy declaration of the
   copy constructor had been triggered, you might get different behavior.
   We now eagerly declare the copy constructor whenever its deletedness
   is unclear, and ignore deleted copy/move constructors when looking for
   a trivial such constructor.

This also fixes an ABI difference between Clang and MSVC:

 * If the copy constructor would be implicitly deleted (but has not been
   lazily declared yet), for instance because the class has an rvalue
   reference member, we would pass it directly. We now pass such a class
   indirectly, matching MSVC.

llvm-svn: 310401
  • Loading branch information
@zygoloid
zygoloid committed Aug 8, 2017
1 parent f5266f0 commit f1a425e
Show file tree
Hide file tree
Showing 11 changed files with 332 additions and 128 deletions.
74 changes: 72 additions & 2 deletions clang/include/clang/AST/DeclCXX.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ class CXXRecordDecl : public RecordDecl {
/// \brief These flags are \c true if a defaulted corresponding special
/// member can't be fully analyzed without performing overload resolution.
/// @{
unsigned NeedOverloadResolutionForCopyConstructor : 1;
unsigned NeedOverloadResolutionForMoveConstructor : 1;
unsigned NeedOverloadResolutionForMoveAssignment : 1;
unsigned NeedOverloadResolutionForDestructor : 1;
Expand All @@ -383,6 +384,7 @@ class CXXRecordDecl : public RecordDecl {
/// \brief These flags are \c true if an implicit defaulted corresponding
/// special member would be defined as deleted.
/// @{
unsigned DefaultedCopyConstructorIsDeleted : 1;
unsigned DefaultedMoveConstructorIsDeleted : 1;
unsigned DefaultedMoveAssignmentIsDeleted : 1;
unsigned DefaultedDestructorIsDeleted : 1;
Expand Down Expand Up @@ -415,6 +417,12 @@ class CXXRecordDecl : public RecordDecl {
/// constructor.
unsigned HasDefaultedDefaultConstructor : 1;

/// \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.
unsigned CanPassInRegisters : 1;

/// \brief True if a defaulted default constructor for this class would
/// be constexpr.
unsigned DefaultedDefaultConstructorIsConstexpr : 1;
Expand Down Expand Up @@ -811,18 +819,50 @@ class CXXRecordDecl : public RecordDecl {
return data().FirstFriend.isValid();
}

/// \brief \c true if a defaulted copy constructor for this class would be
/// deleted.
bool defaultedCopyConstructorIsDeleted() const {
assert((!needsOverloadResolutionForCopyConstructor() ||
(data().DeclaredSpecialMembers & SMF_CopyConstructor)) &&
"this property has not yet been computed by Sema");
return data().DefaultedCopyConstructorIsDeleted;
}

/// \brief \c true if a defaulted move constructor for this class would be
/// deleted.
bool defaultedMoveConstructorIsDeleted() const {
assert((!needsOverloadResolutionForMoveConstructor() ||
(data().DeclaredSpecialMembers & SMF_MoveConstructor)) &&
"this property has not yet been computed by Sema");
return data().DefaultedMoveConstructorIsDeleted;
}

/// \brief \c true if a defaulted destructor for this class would be deleted.
bool defaultedDestructorIsDeleted() const {
return !data().DefaultedDestructorIsDeleted;
}

/// \brief \c true if we know for sure that this class has a single,
/// accessible, unambiguous copy constructor that is not deleted.
bool hasSimpleCopyConstructor() const {
return !hasUserDeclaredCopyConstructor() &&
!data().DefaultedCopyConstructorIsDeleted;
}

/// \brief \c true if we know for sure that this class has a single,
/// accessible, unambiguous move constructor that is not deleted.
bool hasSimpleMoveConstructor() const {
return !hasUserDeclaredMoveConstructor() && hasMoveConstructor() &&
!data().DefaultedMoveConstructorIsDeleted;
}

/// \brief \c true if we know for sure that this class has a single,
/// accessible, unambiguous move assignment operator that is not deleted.
bool hasSimpleMoveAssignment() const {
return !hasUserDeclaredMoveAssignment() && hasMoveAssignment() &&
!data().DefaultedMoveAssignmentIsDeleted;
}

/// \brief \c true if we know for sure that this class has an accessible
/// destructor that is not deleted.
bool hasSimpleDestructor() const {
Expand Down Expand Up @@ -878,7 +918,16 @@ class CXXRecordDecl : public RecordDecl {
/// \brief Determine whether we need to eagerly declare a defaulted copy
/// constructor for this class.
bool needsOverloadResolutionForCopyConstructor() const {
return data().HasMutableFields;
// C++17 [class.copy.ctor]p6:
// If the class definition declares a move constructor or move assignment
// operator, the implicitly declared copy constructor is defined as
// deleted.
// In MSVC mode, sometimes a declared move assignment does not delete an
// implicit copy constructor, so defer this choice to Sema.
if (data().UserDeclaredSpecialMembers &
(SMF_MoveConstructor | SMF_MoveAssignment))
return true;
return data().NeedOverloadResolutionForCopyConstructor;
}

/// \brief Determine whether an implicit copy constructor for this type
Expand Down Expand Up @@ -919,7 +968,16 @@ class CXXRecordDecl : public RecordDecl {
needsImplicitMoveConstructor();
}

/// \brief Set that we attempted to declare an implicitly move
/// \brief Set that we attempted to declare an implicit copy
/// constructor, but overload resolution failed so we deleted it.
void setImplicitCopyConstructorIsDeleted() {
assert((data().DefaultedCopyConstructorIsDeleted ||
needsOverloadResolutionForCopyConstructor()) &&
"Copy constructor should not be deleted");
data().DefaultedCopyConstructorIsDeleted = true;
}

/// \brief Set that we attempted to declare an implicit move
/// constructor, but overload resolution failed so we deleted it.
void setImplicitMoveConstructorIsDeleted() {
assert((data().DefaultedMoveConstructorIsDeleted ||
Expand Down Expand Up @@ -1316,6 +1374,18 @@ class CXXRecordDecl : public RecordDecl {
return data().HasIrrelevantDestructor;
}

/// \brief Determine whether this class has at least one trivial, non-deleted
/// copy or move constructor.
bool canPassInRegisters() const {
return data().CanPassInRegisters;
}

/// \brief Set that we can pass this RecordDecl in registers.
// FIXME: This should be set as part of completeDefinition.
void setCanPassInRegisters(bool CanPass) {
data().CanPassInRegisters = CanPass;
}

/// \brief Determine whether this class has a non-literal or/ volatile type
/// non-static data member or base class.
bool hasNonLiteralTypeFieldsOrBases() const {
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/AST/ASTImporter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -956,12 +956,16 @@ bool ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To,
ToData.HasUninitializedFields = FromData.HasUninitializedFields;
ToData.HasInheritedConstructor = FromData.HasInheritedConstructor;
ToData.HasInheritedAssignment = FromData.HasInheritedAssignment;
ToData.NeedOverloadResolutionForCopyConstructor
= FromData.NeedOverloadResolutionForCopyConstructor;
ToData.NeedOverloadResolutionForMoveConstructor
= FromData.NeedOverloadResolutionForMoveConstructor;
ToData.NeedOverloadResolutionForMoveAssignment
= FromData.NeedOverloadResolutionForMoveAssignment;
ToData.NeedOverloadResolutionForDestructor
= FromData.NeedOverloadResolutionForDestructor;
ToData.DefaultedCopyConstructorIsDeleted
= FromData.DefaultedCopyConstructorIsDeleted;
ToData.DefaultedMoveConstructorIsDeleted
= FromData.DefaultedMoveConstructorIsDeleted;
ToData.DefaultedMoveAssignmentIsDeleted
Expand All @@ -973,6 +977,7 @@ bool ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To,
= FromData.HasConstexprNonCopyMoveConstructor;
ToData.HasDefaultedDefaultConstructor
= FromData.HasDefaultedDefaultConstructor;
ToData.CanPassInRegisters = FromData.CanPassInRegisters;
ToData.DefaultedDefaultConstructorIsConstexpr
= FromData.DefaultedDefaultConstructorIsConstexpr;
ToData.HasConstexprDefaultConstructor
Expand Down
34 changes: 30 additions & 4 deletions clang/lib/AST/DeclCXX.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,18 @@ CXXRecordDecl::DefinitionData::DefinitionData(CXXRecordDecl *D)
HasOnlyCMembers(true), HasInClassInitializer(false),
HasUninitializedReferenceMember(false), HasUninitializedFields(false),
HasInheritedConstructor(false), HasInheritedAssignment(false),
NeedOverloadResolutionForCopyConstructor(false),
NeedOverloadResolutionForMoveConstructor(false),
NeedOverloadResolutionForMoveAssignment(false),
NeedOverloadResolutionForDestructor(false),
DefaultedCopyConstructorIsDeleted(false),
DefaultedMoveConstructorIsDeleted(false),
DefaultedMoveAssignmentIsDeleted(false),
DefaultedDestructorIsDeleted(false), HasTrivialSpecialMembers(SMF_All),
DeclaredNonTrivialSpecialMembers(0), HasIrrelevantDestructor(true),
HasConstexprNonCopyMoveConstructor(false),
HasDefaultedDefaultConstructor(false),
CanPassInRegisters(false),
DefaultedDefaultConstructorIsConstexpr(true),
HasConstexprDefaultConstructor(false),
HasNonLiteralTypeFieldsOrBases(false), ComputedVisibleConversions(false),
Expand Down Expand Up @@ -352,8 +355,10 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases,
setHasVolatileMember(true);

// Keep track of the presence of mutable fields.
if (BaseClassDecl->hasMutableFields())
if (BaseClassDecl->hasMutableFields()) {
data().HasMutableFields = true;
data().NeedOverloadResolutionForCopyConstructor = true;
}

if (BaseClassDecl->hasUninitializedReferenceMember())
data().HasUninitializedReferenceMember = true;
Expand Down Expand Up @@ -406,6 +411,8 @@ void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
// -- a direct or virtual base class B that cannot be copied/moved [...]
// -- a non-static data member of class type M (or array thereof)
// that cannot be copied or moved [...]
if (!Subobj->hasSimpleCopyConstructor())
data().NeedOverloadResolutionForCopyConstructor = true;
if (!Subobj->hasSimpleMoveConstructor())
data().NeedOverloadResolutionForMoveConstructor = true;

Expand All @@ -426,6 +433,7 @@ void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
// -- any non-static data member has a type with a destructor
// that is deleted or inaccessible from the defaulted [ctor or dtor].
if (!Subobj->hasSimpleDestructor()) {
data().NeedOverloadResolutionForCopyConstructor = true;
data().NeedOverloadResolutionForMoveConstructor = true;
data().NeedOverloadResolutionForDestructor = true;
}
Expand Down Expand Up @@ -711,8 +719,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
data().IsStandardLayout = false;

// Keep track of the presence of mutable fields.
if (Field->isMutable())
if (Field->isMutable()) {
data().HasMutableFields = true;
data().NeedOverloadResolutionForCopyConstructor = true;
}

// C++11 [class.union]p8, DR1460:
// If X is a union, a non-static data member of X that is not an anonymous
Expand Down Expand Up @@ -756,6 +766,12 @@ void CXXRecordDecl::addedMember(Decl *D) {
// A standard-layout class is a class that:
// -- has no non-static data members of type [...] reference,
data().IsStandardLayout = false;

// C++1z [class.copy.ctor]p10:
// A defaulted copy constructor for a class X is defined as deleted if X has:
// -- a non-static data member of rvalue reference type
if (T->isRValueReferenceType())
data().DefaultedCopyConstructorIsDeleted = true;
}

if (!Field->hasInClassInitializer() && !Field->isMutable()) {
Expand Down Expand Up @@ -809,6 +825,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
// We may need to perform overload resolution to determine whether a
// field can be moved if it's const or volatile qualified.
if (T.getCVRQualifiers() & (Qualifiers::Const | Qualifiers::Volatile)) {
// We need to care about 'const' for the copy constructor because an
// implicit copy constructor might be declared with a non-const
// parameter.
data().NeedOverloadResolutionForCopyConstructor = true;
data().NeedOverloadResolutionForMoveConstructor = true;
data().NeedOverloadResolutionForMoveAssignment = true;
}
Expand All @@ -819,6 +839,8 @@ void CXXRecordDecl::addedMember(Decl *D) {
// -- X is a union-like class that has a variant member with a
// non-trivial [corresponding special member]
if (isUnion()) {
if (FieldRec->hasNonTrivialCopyConstructor())
data().DefaultedCopyConstructorIsDeleted = true;
if (FieldRec->hasNonTrivialMoveConstructor())
data().DefaultedMoveConstructorIsDeleted = true;
if (FieldRec->hasNonTrivialMoveAssignment())
Expand All @@ -830,6 +852,8 @@ void CXXRecordDecl::addedMember(Decl *D) {
// For an anonymous union member, our overload resolution will perform
// overload resolution for its members.
if (Field->isAnonymousStructOrUnion()) {
data().NeedOverloadResolutionForCopyConstructor |=
FieldRec->data().NeedOverloadResolutionForCopyConstructor;
data().NeedOverloadResolutionForMoveConstructor |=
FieldRec->data().NeedOverloadResolutionForMoveConstructor;
data().NeedOverloadResolutionForMoveAssignment |=
Expand Down Expand Up @@ -915,8 +939,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
}

// Keep track of the presence of mutable fields.
if (FieldRec->hasMutableFields())
if (FieldRec->hasMutableFields()) {
data().HasMutableFields = true;
data().NeedOverloadResolutionForCopyConstructor = true;
}

// C++11 [class.copy]p13:
// If the implicitly-defined constructor would satisfy the
Expand Down Expand Up @@ -1450,7 +1476,7 @@ void CXXRecordDecl::completeDefinition() {

void CXXRecordDecl::completeDefinition(CXXFinalOverriderMap *FinalOverriders) {
RecordDecl::completeDefinition();

// If the class may be abstract (but hasn't been marked as such), check for
// any pure final overriders.
if (mayBeAbstract()) {
Expand Down
31 changes: 1 addition & 30 deletions clang/lib/CodeGen/CGCXXABI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,38 +30,9 @@ void CGCXXABI::ErrorUnsupportedABI(CodeGenFunction &CGF, StringRef 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 CopyDeleted = false;
bool MoveDeleted = 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;
if (CD->isCopyConstructor())
CopyDeleted = true;
else
MoveDeleted = true;
}
}

// If all trivial copy and move constructors are deleted, we cannot copy the
// argument.
return !(CopyDeleted && MoveDeleted);
return RD->canPassInRegisters();
}

llvm::Constant *CGCXXABI::GetBogusMemberPointer(QualType T) {
Expand Down
13 changes: 4 additions & 9 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,8 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
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;
}
Expand Down Expand Up @@ -1014,10 +1011,8 @@ bool ItaniumCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
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;
Expand Down
Loading

0 comments on commit f1a425e

Please sign in to comment.