[Sema][ObjC] Disallow non-trivial C struct fields in unions

[ahatanak]

Summary

This patch fixes a bug where clang doesn’t reject union fields of non-trivial struct types:

struct S0 {
  id x;
};

struct S1 {
  id y;
};

union U0 {
  struct S0 s0;  // no diagnostics.
  struct S1 s1;  // no diagnostics.
};

union U1 {
  id x;  // clang rejects ObjC pointer fields in unions.
};

void test(union U0 a) {
  // Both ‘S0::x’ and ‘S1::y' are destructed in the IR.
}

rdar://problem/46677858

[ahatanak]

Just to clarify, this patch doesn't change clang's handling of C++ unions that have non-static data members with non-trivial special member functions.

[ahatanak]

This patch is unnecessarily complicated and is not correct.

[ahatanak]

Make sure that volatile trivial fields in a struct don't cause Sema to emit a diagnostic. Simplify the code in Sema::ActOnFields .

[rjmccall]

Two minor tweaks, but otherwise LGTM.

lib/AST/Type.cpp

2273 ↗

(On Diff #185466)

Might be good to spell this out: we don't want to apply the C restriction in C++ because C++ (1) can apply the restriction at a finer grain by banning copying/destroying the union and (2) allows users to override these restrictions by declaring explicit constructors/etc., which we're not proposing to add to C.

test/SemaObjC/arc-decls.m

23

spacing

include/clang/AST/Type.h

enum PrimitiveCopyKind {
     PCK_Struct
   };
 
   /// Check if this is a non-trivial type that would cause a C struct
   /// transitively containing this type to be non-trivial to copy and return the
   /// kind.
   PrimitiveCopyKind isNonTrivialToPrimitiveCopy() const;
 
+  bool isTrivialOrTrivialVolatileToPrimitiveCopy() const {
+    PrimitiveCopyKind Kind = isNonTrivialToPrimitiveCopy();
+    return Kind == PCK_Trivial || Kind == PCK_VolatileTrivial;
+  }
+
   /// Check if this is a non-trivial type that would cause a C struct
   /// transitively containing this type to be non-trivial to destructively
   /// move and return the kind. Destructive move in this context is a C++-style
   /// move in which the source object is placed in a valid but unspecified state
   /// after it is moved, as opposed to a truly destructive move in which the
   /// source object is placed in an uninitialized state.
   PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;
 
+  bool isTrivialOrTrivialVolatileToPrimitiveMove() const {
+    return isTrivialOrTrivialVolatileToPrimitiveCopy();
+  }
+
   enum DestructionKind {
     DK_none,
     DK_cxx_destructor,
     DK_objc_strong_lifetime,
     DK_objc_weak_lifetime,
     DK_nontrivial_c_struct
   };
 

include/clang/Basic/DiagnosticSemaKinds.td

def err_arc_mismatched_cast : Error<
   "%select{%2|a non-Objective-C pointer type %2|a block pointer|"
   "an Objective-C pointer|an indirect pointer to an Objective-C pointer}1"
   " to %3 is disallowed with ARC">;
 def err_arc_nolifetime_behavior : Error<
   "explicit ownership qualifier on cast result has no effect">;
 def err_arc_objc_object_in_tag : Error<
   "ARC forbids %select{Objective-C objects|blocks}0 in "
   "%select{struct|interface|union|<<ERROR>>|enum}1">;
+def err_arc_nontrivial_primitive_type_in_union : Error<
+  "ARC forbids non-trivial C types in union">;
 def err_arc_objc_property_default_assign_on_object : Error<
   "ARC forbids synthesizing a property of an Objective-C object "
   "with unspecified ownership or storage attribute">;
 def err_arc_illegal_selector : Error<
   "ARC forbids use of %0 in a @selector">;
 def err_arc_illegal_method_def : Error<
   "ARC forbids %select{implementation|synthesis}0 of %1">;
 def warn_arc_strong_pointer_objc_pointer : Warning<

lib/Sema/SemaDecl.cpp

for (const auto *I : Record->decls()) {
         if (IFD->getDeclName())
           ++NumNamedMembers;
     }
   }
 
   // Verify that all the fields are okay.
   SmallVector<FieldDecl*, 32> RecFields;
 
-  bool ObjCFieldLifetimeErrReported = false;
+  bool NonTrivialPrimitiveFieldErrReported = false;
   for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
        i != end; ++i) {
     FieldDecl *FD = cast<FieldDecl>(*i);
 
     // Get the type for the field.
-    const Type *FDTy = FD->getType().getTypePtr();
+    QualType FDTy = FD->getType();
 
     if (!FD->isAnonymousStructOrUnion()) {
       // Remember all fields written by the user.
       RecFields.push_back(FD);
     }
 
     // If the field is already invalid for some reason, don't emit more
     // diagnostics about it.
if (FDTy->isFunctionType()) {
       }
     } else if (!FDTy->isDependentType() &&
                RequireCompleteType(FD->getLocation(), FD->getType(),
                                    diag::err_field_incomplete)) {
       // Incomplete type
       FD->setInvalidDecl();
       EnclosingDecl->setInvalidDecl();
       continue;
+    } 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 && !NonTrivialPrimitiveFieldErrReported &&
+               Record->isUnion() &&
+               (FDTy.hasNonTrivialObjCLifetime() ||
+                (!getLangOpts().CPlusPlus &&
+                 (FDTy.isNonTrivialToPrimitiveDefaultInitialize() ||
+                  !FDTy.isTrivialOrTrivialVolatileToPrimitiveCopy() ||
+                  !FDTy.isTrivialOrTrivialVolatileToPrimitiveMove() ||
+                  FDTy.isDestructedType())))) {
+      // It's an error to have a field that has a non-trivial ObjC lifetime or
+      // a non-trivial C type in a union.
+      // We don't want to report this in a system header, though,
+      // so we just make the field unavailable.
+      // FIXME: that's really not sufficient; we need to make the type
+      // itself invalid to, say, initialize or copy.
+      SourceLocation loc = FD->getLocation();
+      if (getSourceManager().isInSystemHeader(loc)) {
+        if (!FD->hasAttr<UnavailableAttr>()) {
+          FD->addAttr(UnavailableAttr::CreateImplicit(Context, "",
+                        UnavailableAttr::IR_ARCFieldWithOwnership, loc));
+        }
+      } else if (FDTy.hasNonTrivialObjCLifetime()) {
+        Diag(FD->getLocation(), diag::err_arc_objc_object_in_tag)
+          << FDTy->isBlockPointerType() << Record->getTagKind();
+      } else {
+        Diag(FD->getLocation(),
+             diag::err_arc_nontrivial_primitive_type_in_union);
+      }
+      NonTrivialPrimitiveFieldErrReported = true;
     } else if (const RecordType *FDTTy = FDTy->getAs<RecordType>()) {
       if (Record && FDTTy->getDecl()->hasFlexibleArrayMember()) {
         // A type which contains a flexible array member is considered to be a
         // flexible array member.
         Record->setHasFlexibleArrayMember(true);
         if (!Record->isUnion()) {
           // If this is a struct/class and this is not the last element, reject
           // it.  Note that GCC supports variable sized arrays in the middle of
if (FDTy->isFunctionType()) {
                                  AbstractIvarType)) {
         // Ivars can not have abstract class types
         FD->setInvalidDecl();
       }
       if (Record && FDTTy->getDecl()->hasObjectMember())
         Record->setHasObjectMember(true);
       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 (getLangOpts().allowsNonTrivialObjCLifetimeQualifiers() &&
-               Record && !ObjCFieldLifetimeErrReported && Record->isUnion()) {
-      // It's an error in ARC or Weak if a field has lifetime.
-      // We don't want to report this in a system header, though,
-      // so we just make the field unavailable.
-      // FIXME: that's really not sufficient; we need to make the type
-      // itself invalid to, say, initialize or copy.
-      QualType T = FD->getType();
-      if (T.hasNonTrivialObjCLifetime()) {
-        SourceLocation loc = FD->getLocation();
-        if (getSourceManager().isInSystemHeader(loc)) {
-          if (!FD->hasAttr<UnavailableAttr>()) {
-            FD->addAttr(UnavailableAttr::CreateImplicit(Context, "",
-                          UnavailableAttr::IR_ARCFieldWithOwnership, loc));
-          }
-        } else {
-          Diag(FD->getLocation(), diag::err_arc_objc_object_in_tag)
-            << T->isBlockPointerType() << Record->getTagKind();
-        }
-        ObjCFieldLifetimeErrReported = true;
-      }
     } 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());

test/SemaObjC/arc-decls.m

 // RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -verify -Wno-objc-root-class %s
 
 // rdar://8843524
 
 struct A {
     id x;
 };
 
 union u {
     id u; // expected-error {{ARC forbids Objective-C objects in union}}
 };
 
+union u_nontrivial_c {
+    struct A a; // expected-error {{ARC forbids non-trivial C types in union}}
+};
+
 @interface I {
    struct A a; 
    struct B {
     id y[10][20];
     id z;
    } b;
 
    union u c; 
 };
 @end
 
 // rdar://10260525
 struct r10260525 {
   id (^block) ();
 };

test/SemaObjCXX/objc-weak.mm

 // RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++98 -Wno-c++0x-extensions -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime-has-weak -fobjc-weak -fblocks -Wno-objc-root-class -std=c++11 -Wno-c++0x-extensions -verify %s
 
 @interface AnObject
 @property(weak) id value;
 @end
 
 __attribute__((objc_arc_weak_reference_unavailable))
 @interface NOWEAK : AnObject // expected-note 2 {{class is declared here}}
 @end
 
 struct S {
-  __weak id a; // expected-note {{because type 'S' has a member with __weak ownership}}
+  __weak id a;
 };
 
 union U {
   __weak id a; // expected-error {{ARC forbids Objective-C objects in union}}
-  S b;         // expected-error {{union member 'b' has a non-trivial copy constructor}}
+  S b;
 };
 
+#if __cplusplus < 201103L
+// expected-note@-9 {{because type 'S' has a member with __weak ownership}}
+// expected-error@-5 {{union member 'b' has a non-trivial copy constructor}}
+#endif
+
 void testCast(AnObject *o) {
   __weak id a = reinterpret_cast<__weak NOWEAK *>(o); // expected-error {{class is incompatible with __weak references}} \
                                                       // expected-error {{explicit ownership qualifier on cast result has no effect}} \
                                                       // expected-error {{assignment of a weak-unavailable object to a __weak object}}
 
   __weak id b = static_cast<__weak NOWEAK *>(o); // expected-error {{class is incompatible with __weak references}} \
                                                  // expected-error {{explicit ownership qualifier on cast result has no effect}} \
                                                  // expected-error {{assignment of a weak-unavailable object to a __weak object}}
 }