From e6313ace66129a8fd91238ec960031ed2fbab810 Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Mon, 9 Apr 2018 22:48:22 +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. This reapplies r329617. r329617 didn't specify the underlying type for enum ArgPassingKind, which caused regression tests to fail on a windows bot. rdar://problem/39194693 Differential Revision: https://reviews.llvm.org/D45384 llvm-svn: 329635 --- clang/include/clang/AST/Decl.h | 45 ++++++++++++++----- clang/include/clang/AST/Type.h | 2 - clang/lib/AST/Decl.cpp | 2 +- clang/lib/AST/DeclCXX.cpp | 9 +++- clang/lib/AST/Type.cpp | 11 ----- clang/lib/Sema/SemaDecl.cpp | 9 +++- clang/lib/Sema/SemaDeclCXX.cpp | 30 ++++++------- clang/lib/Serialization/ASTReaderDecl.cpp | 5 ++- clang/lib/Serialization/ASTWriter.cpp | 2 +- clang/lib/Serialization/ASTWriterDecl.cpp | 5 ++- .../test/CodeGenObjCXX/objc-struct-cxx-abi.mm | 29 ++++++++++++ 11 files changed, 102 insertions(+), 47 deletions(-) diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index f9f5cf858deb..8dcf70722421 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/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 : unsigned { + /// 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; } - /// Set that we can pass this RecordDecl in registers. - void setCanPassInRegisters(bool CanPass) { - CanPassInRegisters = CanPass; + ArgPassingKind getArgPassingRestrictions() const { + return ArgPassingRestrictions; + } + + void setArgPassingRestrictions(ArgPassingKind Kind) { + ArgPassingRestrictions = Kind; } bool isParamDestroyedInCallee() const { diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index a97f5af5cefa..ab6e113f2bdc 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/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/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index b49fda08a7c1..18ef94ba2251 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/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/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp index 99ff1ca31e77..dd653d8f5730 100644 --- a/clang/lib/AST/DeclCXX.cpp +++ b/clang/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/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index cca5ddc1e480..a2a60772b770 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/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/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index f650e046c0d7..9228d65250cd 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/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/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 55c7a9ae24d4..08d3bc21a246 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/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/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index a6cc69d623fc..af4cf84b9b39 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/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/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 3369a543685e..7f1199f1b6c8 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/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/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp index 06ea8e7f2bcd..189de14cff14 100644 --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/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/clang/test/CodeGenObjCXX/objc-struct-cxx-abi.mm b/clang/test/CodeGenObjCXX/objc-struct-cxx-abi.mm index de8c5f9fcb80..dd9b88b0234d 100644 --- a/clang/test/CodeGenObjCXX/objc-struct-cxx-abi.mm +++ b/clang/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