[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

16382

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

16382

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

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

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

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

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

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

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

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

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

11123

shouldIgnoreForRecordTriviality or something like that, please.

11125

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

11125

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

[rjmccall]

Okay, thanks. LGTM.

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);
 
+  /// This function finds out whether there is an AttributedType of kind
+  /// attr::ObjCOwnership in Ty. The existence of AttributedType of kind
+  /// attr::ObjCOwnership implies an ownership qualifier was explicitly
+  /// specified rather than being added implicitly by the compiler.
+  bool isObjCOwnershipAttributedType(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

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::isObjCOwnershipAttributedType(QualType Ty) const {
+  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;
+}
+
 //===----------------------------------------------------------------------===//
 // ObjCQualifiedIdTypesAreCompatible - Compatibility testing for qualified id's.
 //===----------------------------------------------------------------------===//
 
 /// ProtocolCompatibleWithProtocol - return 'true' if 'lProto' is in the
 /// inheritance hierarchy of 'rProto'.
 bool
 ASTContext::ProtocolCompatibleWithProtocol(ObjCProtocolDecl *lProto,

lib/Sema/SemaDecl.cpp

if (isa<ImplicitValueInitExpr>(Init)) {
     // copy-elision.
     if (InitType.hasNonTrivialToPrimitiveCopyCUnion())
       checkNonTrivialCUnion(InitType, Loc, NTCUC_CopyInit, NTCUK_Copy);
   }
 }
 
 namespace {
 
 struct DiagNonTrivalCUnionDefaultInitializeVisitor
     : DefaultInitializedTypeVisitor<DiagNonTrivalCUnionDefaultInitializeVisitor,
                                     void> {
   using Super =
       DefaultInitializedTypeVisitor<DiagNonTrivalCUnionDefaultInitializeVisitor,
                                     void>;
 
   DiagNonTrivalCUnionDefaultInitializeVisitor(
       QualType OrigTy, SourceLocation OrigLoc,
       Sema::NonTrivialCUnionContext UseContext, Sema &S)
       : OrigTy(OrigTy), OrigLoc(OrigLoc), UseContext(UseContext), S(S) {}
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 (!FD->hasAttr<UnavailableAttr>())
         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 (!FD->hasAttr<UnavailableAttr>())
         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 (!FD->hasAttr<UnavailableAttr>())
         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.isObjCOwnershipAttributedType(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.
+      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());

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.isObjCOwnershipAttributedType(PointeeTy)) {
       if (BuildAndDiagnose) {
         SourceLocation VarLoc = Var->getLocation();
         S.Diag(Loc, diag::warn_block_capture_autoreleasing);
         S.Diag(VarLoc, diag::note_declare_parameter_strong);
       }
     }
   }
 

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 union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+} 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;

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}}