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

2019-02-07 Thread Akira Hatanaka via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
ahatanak marked 2 inline comments as done.
Closed by commit rL353459: [Sema][ObjC] Disallow non-trivial C struct fields in 
unions. (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55659?vs=185466=185846#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55659/new/

https://reviews.llvm.org/D55659

Files:
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/test/SemaObjC/arc-decls.m

Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -22,6 +22,7 @@
 #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"
@@ -2244,6 +2245,62 @@
  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 
+struct IsNonTrivialCopyMoveVisitor
+: CopiedTypeVisitor, IsMove, bool> {
+  using Super =
+  CopiedTypeVisitor, IsMove, bool>;
+  IsNonTrivialCopyMoveVisitor(const ASTContext ) : 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()->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(RD))
+  return false;
+for (const FieldDecl *FD : RD->fields())
+  if (this->asDerived().visit(FD->getType()))
+return true;
+return false;
+  }
+
+  const ASTContext 
+};
+
+} // namespace
+
+bool QualType::isNonTrivialPrimitiveCType(const ASTContext ) const {
+  if (isNonTrivialToPrimitiveDefaultInitialize())
+return true;
+  DestructionKind DK = isDestructedType();
+  if (DK != DK_none && DK != DK_cxx_destructor)
+return true;
+  if (IsNonTrivialCopyMoveVisitor(Ctx).visit(*this))
+return true;
+  if (IsNonTrivialCopyMoveVisitor(Ctx).visit(*this))
+return true;
+  return false;
+}
+
 QualType::PrimitiveDefaultInitializeKind
 QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
   if (const auto *RT =
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -15916,6 +15916,10 @@
 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)
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -1121,6 +1121,12 @@
   };
 
   /// 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 ) 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;
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- 

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

2019-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Two minor tweaks, but otherwise LGTM.




Comment at: lib/AST/Type.cpp:2273
+const RecordDecl *RD = QT->castAs()->getDecl();
+// C++ classes are ignored.
+if (isa(RD))

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.



Comment at: test/SemaObjC/arc-decls.m:24
+
+union  u_trivial_c {
+  volatile int b;

spacing


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55659/new/

https://reviews.llvm.org/D55659



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2019-02-05 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak updated this revision to Diff 185466.
ahatanak added a comment.

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55659/new/

https://reviews.llvm.org/D55659

Files:
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/Type.cpp
  lib/Sema/SemaDecl.cpp
  test/SemaObjC/arc-decls.m

Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,13 +3,29 @@
 // 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 {
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15916,6 +15916,10 @@
 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)
Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -22,6 +22,7 @@
 #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"
@@ -2244,6 +2245,58 @@
  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 
+struct IsNonTrivialCopyMoveVisitor
+: CopiedTypeVisitor, IsMove, bool> {
+  using Super =
+  CopiedTypeVisitor, IsMove, bool>;
+  IsNonTrivialCopyMoveVisitor(const ASTContext ) : 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()->getDecl();
+// C++ classes are ignored.
+if (isa(RD))
+  return false;
+for (const FieldDecl *FD : RD->fields())
+  if (this->asDerived().visit(FD->getType()))
+return true;
+return false;
+  }
+
+  const ASTContext 
+};
+
+} // namespace
+
+bool QualType::isNonTrivialPrimitiveCType(const ASTContext ) const {
+  if (isNonTrivialToPrimitiveDefaultInitialize())
+return true;
+  DestructionKind DK = isDestructedType();
+  if (DK != DK_none && DK != DK_cxx_destructor)
+return true;
+  if (IsNonTrivialCopyMoveVisitor(Ctx).visit(*this))
+return true;
+  if (IsNonTrivialCopyMoveVisitor(Ctx).visit(*this))
+return true;
+  return false;
+}
+
 QualType::PrimitiveDefaultInitializeKind
 QualType::isNonTrivialToPrimitiveDefaultInitialize() const {
   if (const auto *RT =
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -609,6 +609,8 @@
   InGroup;
 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; "
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1120,6 +1120,12 @@
 

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

2019-02-05 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak planned changes to this revision.
ahatanak added a comment.

This patch is unnecessarily complicated and is not correct.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55659/new/

https://reviews.llvm.org/D55659



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-12-13 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55659/new/

https://reviews.llvm.org/D55659



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-12-13 Thread Akira Hatanaka via Phabricator via cfe-commits
ahatanak created this revision.
ahatanak added a reviewer: rjmccall.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.

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


Repository:
  rC Clang

https://reviews.llvm.org/D55659

Files:
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDecl.cpp
  test/SemaObjC/arc-decls.m
  test/SemaObjCXX/objc-weak.mm

Index: test/SemaObjCXX/objc-weak.mm
===
--- test/SemaObjCXX/objc-weak.mm
+++ test/SemaObjCXX/objc-weak.mm
@@ -1,4 +1,5 @@
 // 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;
@@ -9,14 +10,19 @@
 @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}} \
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -10,6 +10,10 @@
 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 {
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -15760,13 +15760,13 @@
   // Verify that all the fields are okay.
   SmallVector RecFields;
 
-  bool ObjCFieldLifetimeErrReported = false;
+  bool NonTrivialPrimitiveFieldErrReported = false;
   for (ArrayRef::iterator i = Fields.begin(), end = Fields.end();
i != end; ++i) {
 FieldDecl *FD = cast(*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.
@@ -15868,6 +15868,40 @@
   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()) {
+  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() <<