[ObjC++] Never pass structs with `__weak` fields in registers

[ahatanak]

Summary

This patch fixes a bug in r328731 that caused structs with __weak fields to be passed in registers. This happens when a struct doesn't have a __weak field but one of its subobjects does. To fix this, I added flag CXXRecordDecl::CannotPassInRegisters that propagates outwards and tracks whether a __weak field was seen. I added a new flag instead of reusing the CanPassInRegisters flag since in C++ we cannot always determine the value of a class' CanPassInRegisters flag by looking at its subobject's CanPassInRegisters flag (CanPassInRegisters=true in the subclass doesn't mean CanPassInRegisters=true in the derived class in C++).

rdar://problem/39194693

[rjmccall]

Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.

[ahatanak]

In D45384#1060164, @rjmccall wrote:

Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.

I think there are cases that is not true. If I compile the following code, S0 (base class) is passed indirectly and S1 (derived class) is passed directly.

struct S0 {
  S0();
  S0(const S0 &) = default;
  S0(S0 &&);
  ~S0() = default;
  int *p;
};

struct S1 : S0 {
  S1();
  S1(const S1 &) = default;
  S1(S1 &&) = delete;
  ~S1() = default;
  int a;
};

void foo1(S0);
void foo1(S1);

void test0(S0 *a) {
  foo1(*a);
}

void test1(S1 *a) {
  foo1(*a);
}

By the way, I meant to say "CanPassInRegisters=true in the *base class*" not "CanPassInRegisters=true in the subclass".

[rjmccall]

In D45384#1060192, @ahatanak wrote:

In D45384#1060164, @rjmccall wrote:

Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass.

I think there are cases that is not true. If I compile the following code, S0 (base class) is passed indirectly and S1 (derived class) is passed directly.

Yes, I think you're right. That's exciting.

So "cannot pass in registers" is tracking whether there's a primitive reason that the class cannot be passed in registers, something that's above and beyond the C++ rules? Okay.

include/clang/AST/DeclCXX.h

486

You should definitely explain the semantic difference with CanPassInRegisters in this comment, and I think the field/method names should underline that. Maybe this is a place to pull out "Primitive" again?

[ahatanak]

Yes. I intended it as a property that propagates to classes that contain or derive from the type.

Would it make it less confusing if I merged CXXRecordDecl::CanPassInRegisters and RecordDecl::CannotPassInRegisters into a single enum? For example, the enum could have three enumerators, "CanPass", "CannotPass", "CanNeverPass", or something. Both "CannotPass" and "CanNeverPass" would force the type to be passed indirectly, and the only difference is that "CanNeverPass" propagates its property outwards and "CannotPass" doesn't. C structs are either "CanPass" or "CanNeverPass" while C++ structs can take any of the three values.

[rjmccall]

In D45384#1060369, @ahatanak wrote:

Yes. I intended it as a property that propagates to classes that contain or derive from the type.

Would it make it less confusing if I merged CXXRecordDecl::CanPassInRegisters and RecordDecl::CannotPassInRegisters into a single enum? For example, the enum could have three enumerators, "CanPass", "CannotPass", "CanNeverPass", or something. Both "CannotPass" and "CanNeverPass" would force the type to be passed indirectly, and the only difference is that "CanNeverPass" propagates its property outwards and "CannotPass" doesn't. C structs are either "CanPass" or "CanNeverPass" while C++ structs can take any of the three values.

Yes, I think that would help a lot.

[ahatanak]

Replace flags CanPassInRegisters and CannotPassInRegisters with a 2-bit enum in RecordDecl.

[rjmccall]

Just a couple minor requests; if you accept them, feel free to commit.

include/clang/AST/Decl.h

3556 ↗

(On Diff #141471)

I think it's probably worth spelling out why this can happen in C++, something like:

This value in 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).

3601 ↗

(On Diff #141471)

I think maybe "ArgPassingRestrictions" would be better, since we don't necessarily honor this in the ABI.

[ahatanak]

Address review comments.

[ahatanak]

Committed in r329617.

include/clang/AST/DeclCXX.h

struct DefinitionData {
     /// \brief True when this class has at least one user-declared constexpr
     /// constructor which is neither the copy nor move constructor.
     unsigned HasConstexprNonCopyMoveConstructor : 1;
 
     /// \brief True if this class has a (possibly implicit) defaulted default
     /// constructor.
     unsigned HasDefaultedDefaultConstructor : 1;
 
+    /// Indicates that this class can never be passed or returned in registers.
+    /// This flag is set to true only when this class or one of its subobjects
+    /// has a field (e.g., ObjC __weak pointer field) that makes it impossible
+    /// for this class to be passed in registers.
+    unsigned CannotPassInRegisters : 1;
+
     /// \brief True if a defaulted default constructor for this class would
     /// be constexpr.
     unsigned DefaultedDefaultConstructorIsConstexpr : 1;
 
     /// \brief True if this class has a constexpr default constructor.
     ///
     /// This is true for either a user-declared constexpr default constructor
     /// or an implicitly declared constexpr default constructor.
public:
   /// semantic effect.
   ///
   /// Any such destructor will be trivial, public, defaulted and not deleted,
   /// and will call only irrelevant destructors.
   bool hasIrrelevantDestructor() const {
     return data().HasIrrelevantDestructor;
   }
 
+  /// Indicates that this class can never be passed or returned in registers.
+  bool cannotPassInRegisters() const {
+    return data().CannotPassInRegisters;
+  }
+
+  void setCannotPassInRegisters(bool CannotPass) {
+    data().CannotPassInRegisters = CannotPass;
+  }
+
   /// \brief Determine whether this class has a non-literal or/ volatile type
   /// non-static data member or base class.
   bool hasNonLiteralTypeFieldsOrBases() const {
     return data().HasNonLiteralTypeFieldsOrBases;
   }
 
   /// \brief Determine whether this class has a using-declaration that names
   /// a user-declared base class constructor.

lib/AST/ASTImporter.cpp

ToData.DefaultedMoveAssignmentIsDeleted
       = FromData.DefaultedMoveAssignmentIsDeleted;
     ToData.DefaultedDestructorIsDeleted = FromData.DefaultedDestructorIsDeleted;
     ToData.HasTrivialSpecialMembers = FromData.HasTrivialSpecialMembers;
     ToData.HasIrrelevantDestructor = FromData.HasIrrelevantDestructor;
     ToData.HasConstexprNonCopyMoveConstructor
       = FromData.HasConstexprNonCopyMoveConstructor;
     ToData.HasDefaultedDefaultConstructor
       = FromData.HasDefaultedDefaultConstructor;
+    ToData.CannotPassInRegisters = FromData.CannotPassInRegisters;
     ToData.DefaultedDefaultConstructorIsConstexpr
       = FromData.DefaultedDefaultConstructorIsConstexpr;
     ToData.HasConstexprDefaultConstructor
       = FromData.HasConstexprDefaultConstructor;
     ToData.HasNonLiteralTypeFieldsOrBases
       = FromData.HasNonLiteralTypeFieldsOrBases;
     // ComputedVisibleConversions not imported.
     ToData.UserProvidedDefaultConstructor

lib/AST/DeclCXX.cpp

: UserDeclaredConstructor(false), UserDeclaredSpecialMembers(0),
       DefaultedCopyConstructorIsDeleted(false),
       DefaultedMoveConstructorIsDeleted(false),
       DefaultedMoveAssignmentIsDeleted(false),
       DefaultedDestructorIsDeleted(false), HasTrivialSpecialMembers(SMF_All),
       HasTrivialSpecialMembersForCall(SMF_All),
       DeclaredNonTrivialSpecialMembers(0),
       DeclaredNonTrivialSpecialMembersForCall(0), HasIrrelevantDestructor(true),
       HasConstexprNonCopyMoveConstructor(false),
-      HasDefaultedDefaultConstructor(false),
+      HasDefaultedDefaultConstructor(false), CannotPassInRegisters(false),
       DefaultedDefaultConstructorIsConstexpr(true),
       HasConstexprDefaultConstructor(false),
       HasNonLiteralTypeFieldsOrBases(false), ComputedVisibleConversions(false),
       UserProvidedDefaultConstructor(false), DeclaredSpecialMembers(0),
       ImplicitCopyConstructorCanHaveConstParamForVBase(true),
       ImplicitCopyConstructorCanHaveConstParamForNonVBase(true),
       ImplicitCopyAssignmentHasConstParam(true),
       HasDeclaredCopyConstructorWithConstParam(false),
for (unsigned i = 0; i < NumBases; ++i) {
     // A class has an Objective-C object member if... or any of its bases
     // has an Objective-C object member.
     if (BaseClassDecl->hasObjectMember())
       setHasObjectMember(true);
     
     if (BaseClassDecl->hasVolatileMember())
       setHasVolatileMember(true);
 
+    if (BaseClassDecl->cannotPassInRegisters())
+      setCannotPassInRegisters(true);
+
     // Keep track of the presence of mutable fields.
     if (BaseClassDecl->hasMutableFields()) {
       data().HasMutableFields = true;
       data().NeedOverloadResolutionForCopyConstructor = true;
     }
 
     if (BaseClassDecl->hasUninitializedReferenceMember())
       data().HasUninitializedReferenceMember = true;
if (T->isObjCRetainableType() || T.isObjCGCStrong()) {
         // __strong or __weak fields do not make special functions non-trivial
         // for the purpose of calls.
         Qualifiers::ObjCLifetime LT = T.getQualifiers().getObjCLifetime();
         if (LT != Qualifiers::OCL_Strong && LT != Qualifiers::OCL_Weak)
           data().HasTrivialSpecialMembersForCall = 0;
 
         // Structs with __weak fields should never be passed directly.
         if (LT == Qualifiers::OCL_Weak)
-          setCanPassInRegisters(false);
+          setCannotPassInRegisters(true);
 
         Data.HasIrrelevantDestructor = false;
       } else if (!Context.getLangOpts().ObjCAutoRefCount) {
         setHasObjectMember(true);
       }
     } else if (!T.isCXX98PODType(Context))
       data().PlainOldData = false;
     
if (const auto *RecordTy = T->getAs<RecordType>()) {
         if (!FieldRec->hasTrivialDestructorForCall())
           data().HasTrivialSpecialMembersForCall &= ~SMF_Destructor;
         if (!FieldRec->hasIrrelevantDestructor())
           data().HasIrrelevantDestructor = false;
         if (FieldRec->hasObjectMember())
           setHasObjectMember(true);
         if (FieldRec->hasVolatileMember())
           setHasVolatileMember(true);
+        if (FieldRec->cannotPassInRegisters())
+          setCannotPassInRegisters(true);
 
         // C++0x [class]p7:
         //   A standard-layout class is a class that:
         //    -- has no non-static data members of type non-standard-layout
         //       class (or array of such types) [...]
         if (!FieldRec->isStandardLayout())
           data().IsStandardLayout = false;
         if (!FieldRec->isCXX11StandardLayout())

lib/Sema/SemaDeclCXX.cpp

 
 static bool computeCanPassInRegister(bool DestroyedInCallee,
                                      const CXXRecordDecl *RD,
                                      TargetInfo::CallingConvKind CCK,
                                      Sema &S) {
   if (RD->isDependentType() || RD->isInvalidDecl())
     return true;
 
-  // The param cannot be passed in registers if CanPassInRegisters is already
-  // set to false.
-  if (!RD->canPassInRegisters())
+  // The param cannot be passed in registers if CannotPassInRegisters is set to
+  // true.
+  if (RD->cannotPassInRegisters())
     return false;
 
   if (CCK != TargetInfo::CCK_MicrosoftX86_64)
     return DestroyedInCallee;
 
   bool CopyCtorIsTrivial = false, CopyCtorIsTrivialForCall = false;
   bool DtorIsTrivialForCall = false;
 

lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::ReadCXXDefinitionData(
   Data.DefaultedDestructorIsDeleted = Record.readInt();
   Data.HasTrivialSpecialMembers = Record.readInt();
   Data.HasTrivialSpecialMembersForCall = Record.readInt();
   Data.DeclaredNonTrivialSpecialMembers = Record.readInt();
   Data.DeclaredNonTrivialSpecialMembersForCall = Record.readInt();
   Data.HasIrrelevantDestructor = Record.readInt();
   Data.HasConstexprNonCopyMoveConstructor = Record.readInt();
   Data.HasDefaultedDefaultConstructor = Record.readInt();
+  Data.CannotPassInRegisters = Record.readInt();
   Data.DefaultedDefaultConstructorIsConstexpr = Record.readInt();
   Data.HasConstexprDefaultConstructor = Record.readInt();
   Data.HasNonLiteralTypeFieldsOrBases = Record.readInt();
   Data.ComputedVisibleConversions = Record.readInt();
   Data.UserProvidedDefaultConstructor = Record.readInt();
   Data.DeclaredSpecialMembers = Record.readInt();
   Data.ImplicitCopyConstructorCanHaveConstParamForVBase = Record.readInt();
   Data.ImplicitCopyConstructorCanHaveConstParamForNonVBase = Record.readInt();
#define MATCH_FIELD(Field) \
   MATCH_FIELD(DefaultedDestructorIsDeleted)
   OR_FIELD(HasTrivialSpecialMembers)
   OR_FIELD(HasTrivialSpecialMembersForCall)
   OR_FIELD(DeclaredNonTrivialSpecialMembers)
   OR_FIELD(DeclaredNonTrivialSpecialMembersForCall)
   MATCH_FIELD(HasIrrelevantDestructor)
   OR_FIELD(HasConstexprNonCopyMoveConstructor)
   OR_FIELD(HasDefaultedDefaultConstructor)
+  MATCH_FIELD(CannotPassInRegisters)
   MATCH_FIELD(DefaultedDefaultConstructorIsConstexpr)
   OR_FIELD(HasConstexprDefaultConstructor)
   MATCH_FIELD(HasNonLiteralTypeFieldsOrBases)
   // ComputedVisibleConversions is handled below.
   MATCH_FIELD(UserProvidedDefaultConstructor)
   OR_FIELD(DeclaredSpecialMembers)
   MATCH_FIELD(ImplicitCopyConstructorCanHaveConstParamForVBase)
   MATCH_FIELD(ImplicitCopyConstructorCanHaveConstParamForNonVBase)

lib/Serialization/ASTWriter.cpp

void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
   Record->push_back(Data.DefaultedDestructorIsDeleted);
   Record->push_back(Data.HasTrivialSpecialMembers);
   Record->push_back(Data.HasTrivialSpecialMembersForCall);
   Record->push_back(Data.DeclaredNonTrivialSpecialMembers);
   Record->push_back(Data.DeclaredNonTrivialSpecialMembersForCall);
   Record->push_back(Data.HasIrrelevantDestructor);
   Record->push_back(Data.HasConstexprNonCopyMoveConstructor);
   Record->push_back(Data.HasDefaultedDefaultConstructor);
+  Record->push_back(Data.CannotPassInRegisters);
   Record->push_back(Data.DefaultedDefaultConstructorIsConstexpr);
   Record->push_back(Data.HasConstexprDefaultConstructor);
   Record->push_back(Data.HasNonLiteralTypeFieldsOrBases);
   Record->push_back(Data.ComputedVisibleConversions);
   Record->push_back(Data.UserProvidedDefaultConstructor);
   Record->push_back(Data.DeclaredSpecialMembers);
   Record->push_back(Data.ImplicitCopyConstructorCanHaveConstParamForVBase);
   Record->push_back(Data.ImplicitCopyConstructorCanHaveConstParamForNonVBase);

test/CodeGenObjCXX/objc-struct-cxx-abi.mm

 // RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o - -DTRIVIALABI %s | FileCheck %s
 // RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -fclang-abi-compat=4.0 -emit-llvm -o - -DTRIVIALABI %s | FileCheck %s
 
 // Check that structs consisting solely of __strong or __weak pointer fields are
 // destructed in the callee function and structs consisting solely of __strong
 // 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* }
 
 #ifdef TRIVIALABI
 struct __attribute__((trivial_abi)) StrongWeak {
 #else
 struct StrongWeak {
 #endif
   id fstrong;
   __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
 struct Strong {
 #endif
   id fstrong;
 };
 
 template<class T>

 // CHECK: %[[V0:.*]] = load %[[STRUCT_STRONGWEAK]]*, %[[STRUCT_STRONGWEAK]]** %[[A_ADDR]], align 8
 // CHECK: %[[CALL:.*]] = call %[[STRUCT_STRONGWEAK]]* @_ZN10StrongWeakC1ERKS_(%[[STRUCT_STRONGWEAK]]* %[[AGG_RESULT]], %[[STRUCT_STRONGWEAK]]* dereferenceable(16) %[[V0]])
 // CHECK: ret void
 
 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
 // CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i8*
 // CHECK: store i8* %[[COERCE_VAL_IP]], i8** %[[COERCE_DIVE]], align 8
 // CHECK: %[[CALL:.*]] = call %[[STRUCT_STRONG]]* @_ZN6StrongD1Ev(%[[STRUCT_STRONG]]* %[[A]])
 // CHECK: ret void