[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

24 ↗

(On Diff #185466)

spacing

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

enum PrimitiveCopyKind {
     /// Note that a C++ struct type does not necessarily match this; C++ copying
     /// semantics are too complex to express here, in part because they depend
     /// on the exact constructor or assignment operator that is chosen by
     /// overload resolution to do the copy.
     PCK_Struct
   };
 
   /// Check if this is a non-trivial type that would cause a C struct
+  /// transitively containing this type to be non-trivial. This function can be
+  /// used to determine whether a field of this type can be declared inside a C
+  /// union.
+  bool isNonTrivialPrimitiveCType(const ASTContext &Ctx) const;
+
+  /// 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;
 
   /// 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

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
   InGroup<ImplicitFunctionDeclare>, DefaultError;
 def warn_cstruct_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this "
   "%1 call is a pointer to record %2 that is not trivial to "
   "%select{primitive-default-initialize|primitive-copy}3">,
   InGroup<NonTrivialMemaccess>;
 def note_nontrivial_field : Note<
   "field is non-trivial to %select{copy|default-initialize}0">;
+def err_nontrivial_primitive_type_in_union : Error<
+  "non-trivial C types are disallowed in union">;
 def warn_dyn_class_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this "
   "%1 call is a pointer to %select{|class containing a }2dynamic class %3; "
   "vtable pointer will be %select{overwritten|copied|moved|compared}4">,
   InGroup<DynamicClassMemaccess>;
 def note_bad_memaccess_silence : Note<
   "explicitly cast the pointer to silence this warning">;
 def warn_sizeof_pointer_expr_memaccess : Warning<

cfe/trunk/lib/AST/Type.cpp

 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/NonTrivialTypeVisitor.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/ExceptionSpecificationType.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"

 }
 
 bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const {
   return !Context.getLangOpts().ObjCAutoRefCount &&
          Context.getLangOpts().ObjCWeak &&
          getObjCLifetime() != Qualifiers::OCL_Weak;
 }
 
+namespace {
+// Helper class that determines whether this is a type that is non-trivial to
+// primitive copy or move, or is a struct type that has a field of such type.
+template <bool IsMove>
+struct IsNonTrivialCopyMoveVisitor
+    : CopiedTypeVisitor<IsNonTrivialCopyMoveVisitor<IsMove>, IsMove, bool> {
+  using Super =
+      CopiedTypeVisitor<IsNonTrivialCopyMoveVisitor<IsMove>, IsMove, bool>;
+  IsNonTrivialCopyMoveVisitor(const ASTContext &C) : Ctx(C) {}
+  void preVisit(QualType::PrimitiveCopyKind PCK, QualType QT) {}
+
+  bool visitWithKind(QualType::PrimitiveCopyKind PCK, QualType QT) {
+    if (const auto *AT = this->Ctx.getAsArrayType(QT))
+      return this->asDerived().visit(QualType(AT, 0));
+    return Super::visitWithKind(PCK, QT);
+  }
+
+  bool visitARCStrong(QualType QT) { return true; }
+  bool visitARCWeak(QualType QT) { return true; }
+  bool visitTrivial(QualType QT) { return false; }
+  // Volatile fields are considered trivial.
+  bool visitVolatileTrivial(QualType QT) { return false; }
+
+  bool visitStruct(QualType QT) {
+    const RecordDecl *RD = QT->castAs<RecordType>()->getDecl();
+    // 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 or
+    //     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.
+    if (isa<CXXRecordDecl>(RD))
+      return false;
+    for (const FieldDecl *FD : RD->fields())
+      if (this->asDerived().visit(FD->getType()))
+        return true;
+    return false;
+  }
+
+  const ASTContext &Ctx;
+};
+
+} // namespace
+
+bool QualType::isNonTrivialPrimitiveCType(const ASTContext &Ctx) const {
+  if (isNonTrivialToPrimitiveDefaultInitialize())
+    return true;
+  DestructionKind DK = isDestructedType();
+  if (DK != DK_none && DK != DK_cxx_destructor)
+    return true;
+  if (IsNonTrivialCopyMoveVisitor<false>(Ctx).visit(*this))
+    return true;
+  if (IsNonTrivialCopyMoveVisitor<true>(Ctx).visit(*this))
+    return true;
+  return false;
+}
+
 QualType::PrimitiveDefaultInitializeKind
 QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
   if (const auto *RT =
           getTypePtr()->getBaseElementTypeUnsafe()->getAs<RecordType>())
     if (RT->getDecl()->isNonTrivialToPrimitiveDefaultInitialize())
       return PDIK_Struct;
 
   switch (getQualifiers().getObjCLifetime()) {

cfe/trunk/lib/Sema/SemaDecl.cpp

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);
+      if (Record && Record->isUnion() &&
+          FD->getType().isNonTrivialPrimitiveCType(Context))
+        Diag(FD->getLocation(),
+             diag::err_nontrivial_primitive_type_in_union);
     } 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() &&

cfe/trunk/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;
+  id x[4];
+  id y;
 };
 
 union u {
     id u; // expected-error {{ARC forbids Objective-C objects in union}}
 };
 
+union u_nontrivial_c {
+  struct A a; // expected-error {{non-trivial C types are disallowed in union}}
+};
+
+// Volatile fields are fine.
+struct C {
+  volatile int x[4];
+  volatile int y;
+};
+
+union u_trivial_c {
+  volatile int b;
+  struct C c;
+};
+
 @interface I {
    struct A a; 
    struct B {
     id y[10][20];
     id z;
    } b;
 
    union u c;