From 189f54bbe75921a3ef69f3a66c3a2a4eebdc794a Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Mon, 9 Apr 2018 20:39:47 +0000 Subject: [PATCH] [ObjC++] Never pass structs that transitively contain __weak fields in registers. This patch fixes a bug in r328731 that caused structs transitively containing __weak fields to be passed in registers. The patch replaces the flag RecordDecl::CanPassInRegisters with a 2-bit enum that indicates whether the struct or structs containing the struct are forced to be passed indirectly. rdar://problem/39194693 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@329617 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 45 ++++++++++++++++++----- include/clang/AST/Type.h | 2 - lib/AST/Decl.cpp | 2 +- lib/AST/DeclCXX.cpp | 9 ++++- lib/AST/Type.cpp | 11 ------ lib/Sema/SemaDecl.cpp | 9 ++++- lib/Sema/SemaDeclCXX.cpp | 30 +++++++-------- lib/Serialization/ASTReaderDecl.cpp | 5 ++- lib/Serialization/ASTWriter.cpp | 2 +- lib/Serialization/ASTWriterDecl.cpp | 5 ++- test/CodeGenObjCXX/objc-struct-cxx-abi.mm | 29 +++++++++++++++ 11 files changed, 102 insertions(+), 47 deletions(-) diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index f9f5cf858d..515ab3b711 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -3541,6 +3541,31 @@ public: /// union Y { int A, B; }; // Has body with members A and B (FieldDecls). /// This decl will be marked invalid if *any* members are invalid. class RecordDecl : public TagDecl { +public: + /// Enum that represents the different ways arguments are passed to and + /// returned from function calls. This takes into account the target-specific + /// and version-specific rules along with the rules determined by the + /// language. + enum ArgPassingKind { + /// The argument of this type can be passed directly in registers. + APK_CanPassInRegs, + + /// The argument of this type cannot be passed directly in registers. + /// Records containing this type as a subobject are not forced to be passed + /// indirectly. This value is used only in C++. This value is required by + /// C++ because, in uncommon situations, it is possible for a class to have + /// only trivial copy/move constructors even when one of its subobjects has + /// a non-trivial copy/move constructor (if e.g. the corresponding copy/move + /// constructor in the derived class is deleted). + APK_CannotPassInRegs, + + /// The argument of this type cannot be passed directly in registers. + /// Records containing this type as a subobject are forced to be passed + /// indirectly. + APK_CanNeverPassInRegs + }; + +private: friend class DeclContext; // FIXME: This can be packed into the bitfields in Decl. @@ -3571,17 +3596,14 @@ class RecordDecl : public TagDecl { bool NonTrivialToPrimitiveCopy : 1; bool NonTrivialToPrimitiveDestroy : 1; - /// True if this class can be passed in a non-address-preserving fashion - /// (such as in registers). - /// This does not imply anything about how the ABI in use will actually - /// pass an object of this class. - bool CanPassInRegisters : 1; - /// Indicates whether this struct is destroyed in the callee. This flag is /// meaningless when Microsoft ABI is used since parameters are always /// destroyed in the callee. bool ParamDestroyedInCallee : 1; + /// Represents the way this type is passed to a function. + ArgPassingKind ArgPassingRestrictions : 2; + protected: RecordDecl(Kind DK, TagKind TK, const ASTContext &C, DeclContext *DC, SourceLocation StartLoc, SourceLocation IdLoc, @@ -3669,12 +3691,15 @@ public: /// it must have at least one trivial, non-deleted copy or move constructor. /// FIXME: This should be set as part of completeDefinition. bool canPassInRegisters() const { - return CanPassInRegisters; + return ArgPassingRestrictions == APK_CanPassInRegs; + } + + ArgPassingKind getArgPassingRestrictions() const { + return ArgPassingRestrictions; } - /// Set that we can pass this RecordDecl in registers. - void setCanPassInRegisters(bool CanPass) { - CanPassInRegisters = CanPass; + void setArgPassingRestrictions(ArgPassingKind Kind) { + ArgPassingRestrictions = Kind; } bool isParamDestroyedInCallee() const { diff --git a/include/clang/AST/Type.h b/include/clang/AST/Type.h index a97f5af5ce..ab6e113f2b 100644 --- a/include/clang/AST/Type.h +++ b/include/clang/AST/Type.h @@ -1149,8 +1149,6 @@ public: /// source object is placed in an uninitialized state. PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const; - bool canPassInRegisters() const; - enum DestructionKind { DK_none, DK_cxx_destructor, diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index b49fda08a7..18ef94ba22 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -3956,7 +3956,7 @@ RecordDecl::RecordDecl(Kind DK, TagKind TK, const ASTContext &C, LoadedFieldsFromExternalStorage(false), NonTrivialToPrimitiveDefaultInitialize(false), NonTrivialToPrimitiveCopy(false), NonTrivialToPrimitiveDestroy(false), - CanPassInRegisters(true), ParamDestroyedInCallee(false) { + ParamDestroyedInCallee(false), ArgPassingRestrictions(APK_CanPassInRegs) { assert(classof(static_cast(this)) && "Invalid Kind!"); } diff --git a/lib/AST/DeclCXX.cpp b/lib/AST/DeclCXX.cpp index 99ff1ca31e..dd653d8f57 100644 --- a/lib/AST/DeclCXX.cpp +++ b/lib/AST/DeclCXX.cpp @@ -421,6 +421,10 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases, if (BaseClassDecl->hasVolatileMember()) setHasVolatileMember(true); + if (BaseClassDecl->getArgPassingRestrictions() == + RecordDecl::APK_CanNeverPassInRegs) + setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs); + // Keep track of the presence of mutable fields. if (BaseClassDecl->hasMutableFields()) { data().HasMutableFields = true; @@ -950,7 +954,7 @@ void CXXRecordDecl::addedMember(Decl *D) { // Structs with __weak fields should never be passed directly. if (LT == Qualifiers::OCL_Weak) - setCanPassInRegisters(false); + setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs); Data.HasIrrelevantDestructor = false; } else if (!Context.getLangOpts().ObjCAutoRefCount) { @@ -1117,6 +1121,9 @@ void CXXRecordDecl::addedMember(Decl *D) { setHasObjectMember(true); if (FieldRec->hasVolatileMember()) setHasVolatileMember(true); + if (FieldRec->getArgPassingRestrictions() == + RecordDecl::APK_CanNeverPassInRegs) + setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs); // C++0x [class]p7: // A standard-layout class is a class that: diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp index cca5ddc1e4..a2a60772b7 100644 --- a/lib/AST/Type.cpp +++ b/lib/AST/Type.cpp @@ -2239,17 +2239,6 @@ QualType::isNonTrivialToPrimitiveDestructiveMove() const { return isNonTrivialToPrimitiveCopy(); } -bool QualType::canPassInRegisters() const { - if (const auto *RT = - getTypePtr()->getBaseElementTypeUnsafe()->getAs()) - return RT->getDecl()->canPassInRegisters(); - - if (getQualifiers().getObjCLifetime() == Qualifiers::OCL_Weak) - return false; - - return true; -} - bool Type::isLiteralType(const ASTContext &Ctx) const { if (isDependentType()) return false; diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index f650e046c0..9228d65250 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -15465,8 +15465,13 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, Record->setNonTrivialToPrimitiveDestroy(true); Record->setParamDestroyedInCallee(true); } - if (!FT.canPassInRegisters()) - Record->setCanPassInRegisters(false); + + if (const auto *RT = FT->getAs()) { + if (RT->getDecl()->getArgPassingRestrictions() == + RecordDecl::APK_CanNeverPassInRegs) + Record->setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs); + } else if (FT.getQualifiers().getObjCLifetime() == Qualifiers::OCL_Weak) + Record->setArgPassingRestrictions(RecordDecl::APK_CanNeverPassInRegs); } if (Record && FD->getType().isVolatileQualified()) diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index 55c7a9ae24..08d3bc21a2 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -5847,20 +5847,20 @@ static bool paramCanBeDestroyedInCallee(Sema &S, CXXRecordDecl *D, return HasNonDeletedCopyOrMove; } -static bool computeCanPassInRegister(bool DestroyedInCallee, - const CXXRecordDecl *RD, - TargetInfo::CallingConvKind CCK, - Sema &S) { +static RecordDecl::ArgPassingKind +computeArgPassingRestrictions(bool DestroyedInCallee, const CXXRecordDecl *RD, + TargetInfo::CallingConvKind CCK, Sema &S) { if (RD->isDependentType() || RD->isInvalidDecl()) - return true; + return RecordDecl::APK_CanPassInRegs; - // The param cannot be passed in registers if CanPassInRegisters is already - // set to false. - if (!RD->canPassInRegisters()) - return false; + // The param cannot be passed in registers if ArgPassingRestrictions is set to + // APK_CanNeverPassInRegs. + if (RD->getArgPassingRestrictions() == RecordDecl::APK_CanNeverPassInRegs) + return RecordDecl::APK_CanNeverPassInRegs; if (CCK != TargetInfo::CCK_MicrosoftX86_64) - return DestroyedInCallee; + return DestroyedInCallee ? RecordDecl::APK_CanPassInRegs + : RecordDecl::APK_CannotPassInRegs; bool CopyCtorIsTrivial = false, CopyCtorIsTrivialForCall = false; bool DtorIsTrivialForCall = false; @@ -5900,7 +5900,7 @@ static bool computeCanPassInRegister(bool DestroyedInCallee, // If the copy ctor and dtor are both trivial-for-calls, pass direct. if (CopyCtorIsTrivialForCall && DtorIsTrivialForCall) - return true; + return RecordDecl::APK_CanPassInRegs; // If a class has a destructor, we'd really like to pass it indirectly // because it allows us to elide copies. Unfortunately, MSVC makes that @@ -5914,8 +5914,8 @@ static bool computeCanPassInRegister(bool DestroyedInCallee, // passed in registers, which is non-conforming. if (CopyCtorIsTrivial && S.getASTContext().getTypeSize(RD->getTypeForDecl()) <= 64) - return true; - return false; + return RecordDecl::APK_CanPassInRegs; + return RecordDecl::APK_CannotPassInRegs; } /// \brief Perform semantic checks on a class definition that has been @@ -6090,8 +6090,8 @@ void Sema::CheckCompletedCXXClass(CXXRecordDecl *Record) { if (Record->hasNonTrivialDestructor()) Record->setParamDestroyedInCallee(DestroyedInCallee); - Record->setCanPassInRegisters( - computeCanPassInRegister(DestroyedInCallee, Record, CCK, *this)); + Record->setArgPassingRestrictions( + computeArgPassingRestrictions(DestroyedInCallee, Record, CCK, *this)); } /// Look up the special member function that would be called by a special diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index a6cc69d623..af4cf84b9b 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -742,8 +742,8 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) { RD->setNonTrivialToPrimitiveDefaultInitialize(Record.readInt()); RD->setNonTrivialToPrimitiveCopy(Record.readInt()); RD->setNonTrivialToPrimitiveDestroy(Record.readInt()); - RD->setCanPassInRegisters(Record.readInt()); RD->setParamDestroyedInCallee(Record.readInt()); + RD->setArgPassingRestrictions((RecordDecl::ArgPassingKind)Record.readInt()); return Redecl; } @@ -4114,8 +4114,9 @@ void ASTDeclReader::UpdateDecl(Decl *D, bool HadRealDefinition = OldDD && (OldDD->Definition != RD || !Reader.PendingFakeDefinitionData.count(OldDD)); - RD->setCanPassInRegisters(Record.readInt()); RD->setParamDestroyedInCallee(Record.readInt()); + RD->setArgPassingRestrictions( + (RecordDecl::ArgPassingKind)Record.readInt()); ReadCXXRecordDefinition(RD, /*Update*/true); // Visible update is handled separately. diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index 3369a54368..7f1199f1b6 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -5193,8 +5193,8 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) { case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: { auto *RD = cast(D); UpdatedDeclContexts.insert(RD->getPrimaryContext()); - Record.push_back(RD->canPassInRegisters()); Record.push_back(RD->isParamDestroyedInCallee()); + Record.push_back(RD->getArgPassingRestrictions()); Record.AddCXXDefinitionData(RD); Record.AddOffset(WriteDeclContextLexicalBlock( *Context, const_cast(RD))); diff --git a/lib/Serialization/ASTWriterDecl.cpp b/lib/Serialization/ASTWriterDecl.cpp index 06ea8e7f2b..189de14cff 100644 --- a/lib/Serialization/ASTWriterDecl.cpp +++ b/lib/Serialization/ASTWriterDecl.cpp @@ -469,8 +469,8 @@ void ASTDeclWriter::VisitRecordDecl(RecordDecl *D) { Record.push_back(D->isNonTrivialToPrimitiveDefaultInitialize()); Record.push_back(D->isNonTrivialToPrimitiveCopy()); Record.push_back(D->isNonTrivialToPrimitiveDestroy()); - Record.push_back(D->canPassInRegisters()); Record.push_back(D->isParamDestroyedInCallee()); + Record.push_back(D->getArgPassingRestrictions()); if (D->getDeclContext() == D->getLexicalDeclContext() && !D->hasAttrs() && @@ -1913,9 +1913,10 @@ void ASTWriter::WriteDeclAbbrevs() { Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNonTrivialToPrimitiveDestroy Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); - Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // canPassInRegisters // isParamDestroyedInCallee Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); + // getArgPassingRestrictions + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 2)); // DC Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // LexicalOffset diff --git a/test/CodeGenObjCXX/objc-struct-cxx-abi.mm b/test/CodeGenObjCXX/objc-struct-cxx-abi.mm index de8c5f9fcb..dd9b88b023 100644 --- a/test/CodeGenObjCXX/objc-struct-cxx-abi.mm +++ b/test/CodeGenObjCXX/objc-struct-cxx-abi.mm @@ -8,6 +8,8 @@ // pointer fields are passed directly. // CHECK: %[[STRUCT_STRONGWEAK:.*]] = type { i8*, i8* } +// CHECK: %[[STRUCT_CONTAINSSTRONGWEAK:.*]] = type { %[[STRUCT_STRONGWEAK]] } +// CHECK: %[[STRUCT_DERIVEDSTRONGWEAK:.*]] = type { %[[STRUCT_STRONGWEAK]] } // CHECK: %[[STRUCT_STRONG:.*]] = type { i8* } // CHECK: %[[STRUCT_S:.*]] = type { i8* } // CHECK: %[[STRUCT_CONTAINSNONTRIVIAL:.*]] = type { %{{.*}}, i8* } @@ -21,6 +23,21 @@ struct StrongWeak { __weak id fweak; }; +#ifdef TRIVIALABI +struct __attribute__((trivial_abi)) ContainsStrongWeak { +#else +struct ContainsStrongWeak { +#endif + StrongWeak sw; +}; + +#ifdef TRIVIALABI +struct __attribute__((trivial_abi)) DerivedStrongWeak : StrongWeak { +#else +struct DerivedStrongWeak : StrongWeak { +#endif +}; + #ifdef TRIVIALABI struct __attribute__((trivial_abi)) Strong { #else @@ -84,6 +101,18 @@ StrongWeak testReturnStrongWeak(StrongWeak *a) { return *a; } +// CHECK: define void @_Z27testParamContainsStrongWeak18ContainsStrongWeak(%[[STRUCT_CONTAINSSTRONGWEAK]]* %[[A:.*]]) +// CHECK: call %[[STRUCT_CONTAINSSTRONGWEAK]]* @_ZN18ContainsStrongWeakD1Ev(%[[STRUCT_CONTAINSSTRONGWEAK]]* %[[A]]) + +void testParamContainsStrongWeak(ContainsStrongWeak a) { +} + +// CHECK: define void @_Z26testParamDerivedStrongWeak17DerivedStrongWeak(%[[STRUCT_DERIVEDSTRONGWEAK]]* %[[A:.*]]) +// CHECK: call %[[STRUCT_DERIVEDSTRONGWEAK]]* @_ZN17DerivedStrongWeakD1Ev(%[[STRUCT_DERIVEDSTRONGWEAK]]* %[[A]]) + +void testParamDerivedStrongWeak(DerivedStrongWeak a) { +} + // CHECK: define void @_Z15testParamStrong6Strong(i64 %[[A_COERCE:.*]]) // CHECK: %[[A:.*]] = alloca %[[STRUCT_STRONG]], align 8 // CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_STRONG]], %[[STRUCT_STRONG]]* %[[A]], i32 0, i32 0 -- 2.40.0