[Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[ahatanak]

Summary

r365985 stopped marking those fields as unavailable, which caused the union's NonTrivialToPrimitive* bits to be set to true. This patch restores the behavior prior to r365985, except that users can explicitly specify the ownership qualification of the field to instruct the compiler not to mark it as unavailable.

rdar://problem/53420753

[jordan_rose]

lib/Sema/SemaDecl.cpp

16379 ↗

(On Diff #211632)

This check seems a little worrisome since we could have some other kind of attributed type. Is there an easy way to check specifically for a lifetime qualifier?

[ahatanak]

lib/Sema/SemaDecl.cpp

16379 ↗

(On Diff #211632)

I think I can make IsObjCOwnershipAttributedType in SemaType.cpp a method of ASTContext or Sema and call it here.

[ahatanak]

Check that the attributed type is an ObjC qualified type. Do not diagnose fields that are unavailable.

[ahatanak]

Mark fields that don't have an explicit __strong qualifier as unavailable.

[ahatanak]

test/SemaObjC/Inputs/non-trivial-c-union.h

10 ↗

(On Diff #211778)

Rather than making an exception for explicit __strong fields, should we have an attribute that can be used to instruct the compiler not to mark the field as unavailable?

[rjmccall]

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

[jordan_rose]

In D65256#1601410, @rjmccall wrote:

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

That's the right answer on paper, but it's source-breaking in practice, for both Swift and Objective-C. On the Objective-C side, someone could have been using, say, glob_t in a copyable way in their ARC code, never touching the block field, and it would have been working fine. On the Swift side, we won't be able to import such a union at all when previously we would have.

[jordan_rose]

I'm personally still of the opinion that allowing non-trivial fields in unions was a mistake, but it's too late to change that as well.

[rjmccall]

In D65256#1601415, @jordan_rose wrote:

In D65256#1601410, @rjmccall wrote:

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

That's the right answer on paper, but it's source-breaking in practice, for both Swift and Objective-C. On the Objective-C side, someone could have been using, say, glob_t in a copyable way in their ARC code, never touching the block field, and it would have been working fine. On the Swift side, we won't be able to import such a union at all when previously we would have.

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

[jordan_rose]

In D65256#1601509, @rjmccall wrote:

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Apparently that was not the case if it was in a system header. Instead, Clang marked the member unavailable rather than the entire union.

[rjmccall]

In D65256#1601510, @jordan_rose wrote:

In D65256#1601509, @rjmccall wrote:

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Apparently that was not the case if it was in a system header. Instead, Clang marked the member unavailable rather than the entire union.

Ah, that's unfortunate. It also just seems like a bug.

I guess my questions are whether we're fixing a specific source-compatibility problem here and, if so, whether this is the only reasonable approach for solving it.

[rjmccall]

Okay, now that I understand the source-compatibility issues a little better, I think this approach is probably okay. If it causes trouble, we can consider special-casing these headers to treat the member as __unsafe_unretained implicitly — the special case isn't great, but it's better than the potential unsoundness.

include/clang/AST/ASTContext.h

2060 ↗

(On Diff #211778)

How about something like: "Return true if the type has been explicitly qualified with ObjC ownership. A type may be implicitly qualified with ownership under ObjC ARC, and in some cases the compiler treats these differently".

Could you look for other places where we look for an explicit qualifier? I'm pretty sure I remember that happening once or twice.

lib/Sema/SemaDecl.cpp

11176 ↗

(On Diff #211778)

Can we make these exceptions only apply to the attributes we synthesize rather than arbitrary uses of __attribute__((unavailable))? These aren't really good semantics in general.

You can do the check in a well-named function like isSuppressedNonTrivialMember, which would be a good place for a comment explaining what's going on here and why this seemed like the most reasonable solution.

[ahatanak]

Address review comments.

include/clang/AST/ASTContext.h

2060 ↗

(On Diff #211778)

I found that there is a function named hasDirectOwnershipQualifier in SemaType.cpp which also detects explicit ownership qualifiers, so I'm using that instead of isObjCOwnershipAttributedType. The difference between isObjCOwnershipAttributedType is that it detects paren types like I*(__strong x) and doesn't look through typedefs (see the test case in non-trivial-c-union.h).

lib/Sema/SemaDecl.cpp

11176 ↗

(On Diff #211778)

We are ignoring unavailable fields here since they don't make the containing union non-trivial and we don't want the user to think that the unavailable field has to be a trivial field in order to fix a compile error. For example, without the check for UnavailableAttr, clang will diagnose the unavailable field f0 when the following code is compiled:

union U3 {
  id f0 __attribute__((unavailable)); // expected-note {{f0 has type '__strong id' that is non-trivial to default-initialize}}
  __weak id f1; // expected-note {{f1 has type '__weak id' that is non-trivial to default-initialize}}
};

void test(void) {
  union U3 b; // expected-error {{cannot default-initialize an object of type 'union U3' since it is a union that is non-trivial to default-initialize}}
}

In that case, does it matter whether the field is explicitly annotated unavailable in the source code or implicitly by the compiler?

[rjmccall]

include/clang/AST/ASTContext.h

2060 ↗

(On Diff #211778)

Okay. I don't know what the right rule here is, but I agree we should be using the same rule in both places.

lib/Sema/SemaDecl.cpp

11176 ↗

(On Diff #211778)

Oh, I didn't realize we were already honoring the ordinary attribute in the ABI, but from the code it looks we do. In that case, yeah, it makes sense to ignore it here. Should we extract a function to ask whether a FieldDecl should be ignored for triviality computations so that it's more self-documenting that there's a consistent logic here?

[ahatanak]

Address review comments.

lib/Sema/SemaDecl.cpp

11176 ↗

(On Diff #211778)

I added function shouldDiagnoseNonTrivialField, which returns false if the field is unavailable.

[rjmccall]

Could you give it a slightly more general name and then use it in the main semantic check in ActOnFields?

[ahatanak]

Rename function to ignoreForTrivialityComputation.

[rjmccall]

lib/Sema/SemaDecl.cpp

11142 ↗

(On Diff #219008)

shouldIgnoreForRecordTriviality or something like that, please.

11144 ↗

(On Diff #219008)

The "since" clause here is circular: this function *defines* what affects the triviality. The comment should talk about the motivations (e.g. fields that aren't available in particular language modes) and why this might be okay or is the best-available option (despite e.g. the potential for ABI incompatibility across language modes),

[ahatanak]

Rename function and fix comments.

lib/Sema/SemaDecl.cpp

11144 ↗

(On Diff #219008)

I commented on the ABI incompatibility issue in ActOnFields where the unavailable attribute is added to the field.

[rjmccall]

Okay, thanks. LGTM.

cfe/trunk/include/clang/AST/ASTContext.h

public:
 
   /// Return true if the given vector types are of the same unqualified
   /// type or if they are equivalent to the same GCC vector type.
   ///
   /// \note This ignores whether they are target-specific (AltiVec or Neon)
   /// types.
   bool areCompatibleVectorTypes(QualType FirstVec, QualType SecondVec);
 
+  /// Return true if the type has been explicitly qualified with ObjC ownership.
+  /// A type may be implicitly qualified with ownership under ObjC ARC, and in
+  /// some cases the compiler treats these differently.
+  bool hasDirectOwnershipQualifier(QualType Ty) const;
+
   /// Return true if this is an \c NSObject object with its \c NSObject
   /// attribute set.
   static bool isObjCNSObjectType(QualType Ty) {
     return Ty->isObjCNSObjectType();
   }
 
   //===--------------------------------------------------------------------===//
   //                         Type Sizing and Analysis

cfe/trunk/lib/AST/ASTContext.cpp

if (First->getNumElements() == Second->getNumElements() &&
       First->getVectorKind() != VectorType::AltiVecBool &&
       Second->getVectorKind() != VectorType::AltiVecPixel &&
       Second->getVectorKind() != VectorType::AltiVecBool)
     return true;
 
   return false;
 }
 
+bool ASTContext::hasDirectOwnershipQualifier(QualType Ty) const {
+  while (true) {
+    // __strong id
+    if (const AttributedType *Attr = dyn_cast<AttributedType>(Ty)) {
+      if (Attr->getAttrKind() == attr::ObjCOwnership)
+        return true;
+
+      Ty = Attr->getModifiedType();
+
+    // X *__strong (...)
+    } else if (const ParenType *Paren = dyn_cast<ParenType>(Ty)) {
+      Ty = Paren->getInnerType();
+
+    // We do not want to look through typedefs, typeof(expr),
+    // typeof(type), or any other way that the type is somehow
+    // abstracted.
+    } else {
+      return false;
+    }
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // ObjCQualifiedIdTypesAreCompatible - Compatibility testing for qualified id's.
 //===----------------------------------------------------------------------===//
 
 /// ProtocolCompatibleWithProtocol - return 'true' if 'lProto' is in the
 /// inheritance hierarchy of 'rProto'.
 bool
 ASTContext::ProtocolCompatibleWithProtocol(ObjCProtocolDecl *lProto,

cfe/trunk/lib/Sema/SemaDecl.cpp

if (isa<ImplicitValueInitExpr>(Init)) {
     // copy-elision.
     if (InitType.hasNonTrivialToPrimitiveCopyCUnion())
       checkNonTrivialCUnion(InitType, Loc, NTCUC_CopyInit, NTCUK_Copy);
   }
 }
 
 namespace {
 
+bool shouldIgnoreForRecordTriviality(const FieldDecl *FD) {
+  // Ignore unavailable fields. A field can be marked as unavailable explicitly
+  // in the source code or implicitly by the compiler if it is in a union
+  // defined in a system header and has non-trivial ObjC ownership
+  // qualifications. We don't want those fields to participate in determining
+  // whether the containing union is non-trivial.
+  return FD->hasAttr<UnavailableAttr>();
+}
+
 struct DiagNonTrivalCUnionDefaultInitializeVisitor
     : DefaultInitializedTypeVisitor<DiagNonTrivalCUnionDefaultInitializeVisitor,
                                     void> {
   using Super =
       DefaultInitializedTypeVisitor<DiagNonTrivalCUnionDefaultInitializeVisitor,
                                     void>;
 
   DiagNonTrivalCUnionDefaultInitializeVisitor(
if (RD->isUnion()) {
       InNonTrivialUnion = true;
     }
 
     if (InNonTrivialUnion)
       S.Diag(RD->getLocation(), diag::note_non_trivial_c_union)
           << 0 << 0 << QT.getUnqualifiedType() << "";
 
     for (const FieldDecl *FD : RD->fields())
+      if (!shouldIgnoreForRecordTriviality(FD))
         asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
   }
 
   void visitTrivial(QualType QT, const FieldDecl *FD, bool InNonTrivialUnion) {}
 
   // The non-trivial C union type or the struct/union type that contains a
   // non-trivial C union.
   QualType OrigTy;
   SourceLocation OrigLoc;
if (RD->isUnion()) {
       InNonTrivialUnion = true;
     }
 
     if (InNonTrivialUnion)
       S.Diag(RD->getLocation(), diag::note_non_trivial_c_union)
           << 0 << 1 << QT.getUnqualifiedType() << "";
 
     for (const FieldDecl *FD : RD->fields())
+      if (!shouldIgnoreForRecordTriviality(FD))
         asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
   }
 
   void visitTrivial(QualType QT, const FieldDecl *FD, bool InNonTrivialUnion) {}
   void visitCXXDestructor(QualType QT, const FieldDecl *FD,
                           bool InNonTrivialUnion) {}
 
   // The non-trivial C union type or the struct/union type that contains a
   // non-trivial C union.
if (RD->isUnion()) {
       InNonTrivialUnion = true;
     }
 
     if (InNonTrivialUnion)
       S.Diag(RD->getLocation(), diag::note_non_trivial_c_union)
           << 0 << 2 << QT.getUnqualifiedType() << "";
 
     for (const FieldDecl *FD : RD->fields())
+      if (!shouldIgnoreForRecordTriviality(FD))
         asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
   }
 
   void preVisit(QualType::PrimitiveCopyKind PCK, QualType QT,
                 const FieldDecl *FD, bool InNonTrivialUnion) {}
   void visitTrivial(QualType QT, const FieldDecl *FD, bool InNonTrivialUnion) {}
   void visitVolatileTrivial(QualType QT, const FieldDecl *FD,
                             bool InNonTrivialUnion) {}
 
if (FDTy->isFunctionType()) {
       if (Record && FDTTy->getDecl()->hasVolatileMember())
         Record->setHasVolatileMember(true);
     } else if (FDTy->isObjCObjectType()) {
       /// A field cannot be an Objective-c object
       Diag(FD->getLocation(), diag::err_statically_allocated_object)
         << FixItHint::CreateInsertion(FD->getLocation(), "*");
       QualType T = Context.getObjCObjectPointerType(FD->getType());
       FD->setType(T);
+    } else if (Record && Record->isUnion() &&
+               FD->getType().hasNonTrivialObjCLifetime() &&
+               getSourceManager().isInSystemHeader(FD->getLocation()) &&
+               !getLangOpts().CPlusPlus && !FD->hasAttr<UnavailableAttr>() &&
+               (FD->getType().getObjCLifetime() != Qualifiers::OCL_Strong ||
+                !Context.hasDirectOwnershipQualifier(FD->getType()))) {
+      // For backward compatibility, fields of C unions declared in system
+      // headers that have non-trivial ObjC ownership qualifications are marked
+      // as unavailable unless the qualifier is explicit and __strong. This can
+      // break ABI compatibility between programs compiled with ARC and MRR, but
+      // is a better option than rejecting programs using those unions under
+      // ARC.
+      FD->addAttr(UnavailableAttr::CreateImplicit(
+          Context, "", UnavailableAttr::IR_ARCFieldWithOwnership,
+          FD->getLocation()));
     } else if (getLangOpts().ObjC &&
                getLangOpts().getGC() != LangOptions::NonGC &&
                Record && !Record->hasObjectMember()) {
       if (FD->getType()->isObjCObjectPointerType() ||
           FD->getType().isObjCGCStrong())
         Record->setHasObjectMember(true);
       else if (Context.getAsArrayType(FD->getType())) {
         QualType BaseType = Context.getBaseElementType(FD->getType());
         if (BaseType->isRecordType() &&
             BaseType->getAs<RecordType>()->getDecl()->hasObjectMember())
           Record->setHasObjectMember(true);
         else if (BaseType->isObjCObjectPointerType() ||
                  BaseType.isObjCGCStrong())
                Record->setHasObjectMember(true);
       }
     }
 
-    if (Record && !getLangOpts().CPlusPlus && !FD->hasAttr<UnavailableAttr>()) {
+    if (Record && !getLangOpts().CPlusPlus &&
+        !shouldIgnoreForRecordTriviality(FD)) {
       QualType FT = FD->getType();
       if (FT.isNonTrivialToPrimitiveDefaultInitialize()) {
         Record->setNonTrivialToPrimitiveDefaultInitialize(true);
         if (FT.hasNonTrivialToPrimitiveDefaultInitializeCUnion() ||
             Record->isUnion())
           Record->setHasNonTrivialToPrimitiveDefaultInitializeCUnion(true);
       }
       QualType::PrimitiveCopyKind PCK = FT.isNonTrivialToPrimitiveCopy();

cfe/trunk/lib/Sema/SemaExpr.cpp

if (BuildAndDiagnose) {
       Invalid = true;
     } else {
       return false;
     }
   }
 
   // Warn about implicitly autoreleasing indirect parameters captured by blocks.
   if (const auto *PT = CaptureType->getAs<PointerType>()) {
-    // This function finds out whether there is an AttributedType of kind
-    // attr::ObjCOwnership in Ty. The existence of AttributedType of kind
-    // attr::ObjCOwnership implies __autoreleasing was explicitly specified
-    // rather than being added implicitly by the compiler.
-    auto IsObjCOwnershipAttributedType = [](QualType Ty) {
-      while (const auto *AttrTy = Ty->getAs<AttributedType>()) {
-        if (AttrTy->getAttrKind() == attr::ObjCOwnership)
-          return true;
-
-        // Peel off AttributedTypes that are not of kind ObjCOwnership.
-        Ty = AttrTy->getModifiedType();
-      }
-
-      return false;
-    };
-
     QualType PointeeTy = PT->getPointeeType();
 
     if (!Invalid && PointeeTy->getAs<ObjCObjectPointerType>() &&
         PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing &&
-        !IsObjCOwnershipAttributedType(PointeeTy)) {
+        !S.Context.hasDirectOwnershipQualifier(PointeeTy)) {
       if (BuildAndDiagnose) {
         SourceLocation VarLoc = Var->getLocation();
         S.Diag(Loc, diag::warn_block_capture_autoreleasing);
         S.Diag(VarLoc, diag::note_declare_parameter_strong);
       }
     }
   }
 

cfe/trunk/lib/Sema/SemaType.cpp

if (DiagnoseMultipleAddrSpaceAttributes(S, Type.getAddressSpace(), ASIdx,
       Attr.setInvalid();
       return;
     }
 
     Type = S.Context.getAddrSpaceQualType(Type, ASIdx);
   }
 }
 
-/// Does this type have a "direct" ownership qualifier?  That is,
-/// is it written like "__strong id", as opposed to something like
-/// "typeof(foo)", where that happens to be strong?
-static bool hasDirectOwnershipQualifier(QualType type) {
-  // Fast path: no qualifier at all.
-  assert(type.getQualifiers().hasObjCLifetime());
-
-  while (true) {
-    // __strong id
-    if (const AttributedType *attr = dyn_cast<AttributedType>(type)) {
-      if (attr->getAttrKind() == attr::ObjCOwnership)
-        return true;
-
-      type = attr->getModifiedType();
-
-    // X *__strong (...)
-    } else if (const ParenType *paren = dyn_cast<ParenType>(type)) {
-      type = paren->getInnerType();
-
-    // That's it for things we want to complain about.  In particular,
-    // we do not want to look through typedefs, typeof(expr),
-    // typeof(type), or any other way that the type is somehow
-    // abstracted.
-    } else {
-
-      return false;
-    }
-  }
-}
-
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
 /// Returns 'true' if the attribute was handled.
 static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
                                         ParsedAttr &attr, QualType &type) {
   bool NonObjCPointer = false;
 
static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
   }
 
   SplitQualType underlyingType = type.split();
 
   // Check for redundant/conflicting ownership qualifiers.
   if (Qualifiers::ObjCLifetime previousLifetime
         = type.getQualifiers().getObjCLifetime()) {
     // If it's written directly, that's an error.
-    if (hasDirectOwnershipQualifier(type)) {
+    if (S.Context.hasDirectOwnershipQualifier(type)) {
       S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
         << type;
       return true;
     }
 
     // Otherwise, if the qualifiers actually conflict, pull sugar off
     // and remove the ObjCLifetime qualifiers.
     if (previousLifetime != lifetime) {

cfe/trunk/test/SemaObjC/Inputs/non-trivial-c-union.h

+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;

cfe/trunk/test/SemaObjC/non-trivial-c-union.m

-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
   __weak id f1; // expected-note 12 {{f1 has type '__weak id' that is non-trivial to default-initialize}} expected-note 36 {{f1 has type '__weak id' that is non-trivial to destruct}} expected-note 28 {{f1 has type '__weak id' that is non-trivial to copy}}
 } U0;
 
 typedef struct {
   U0 f0;
void testBlockCapture(void) {
   noescapingFunc(^{ g0 = t1.f0.f0; }); // expected-error {{cannot capture a variable of type 'S0' since it contains a union that is non-trivial to destruct}} expected-error {{cannot capture a variable of type 'S0' since it contains a union that is non-trivial to copy}}
   noescapingFunc(^{ g0 = t2.f0; });
   noescapingFunc(^{ g0 = t3.f0.f0; });
 }
 
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}