[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365985: [Sema] Diagnose default-initialization, destruction, 
and copying of (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63753?vs=209626=209663#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63753

Files:
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/include/clang/AST/DeclBase.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/Sema/Sema.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
  cfe/trunk/test/CodeGenObjC/Inputs/strong_in_union.h
  cfe/trunk/test/CodeGenObjC/strong-in-c-struct.m
  cfe/trunk/test/PCH/non-trivial-c-union.m
  cfe/trunk/test/SemaObjC/arc-decls.m
  cfe/trunk/test/SemaObjC/non-trivial-c-union.m

Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -1130,12 +1130,6 @@
   };
 
   /// 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;
@@ -1164,6 +1158,22 @@
 return isDestructedTypeImpl(*this);
   }
 
+  /// Check if this is or contains a C union that is non-trivial to
+  /// default-initialize, which is a union that has a member that is non-trivial
+  /// to default-initialize. If this returns true,
+  /// isNonTrivialToPrimitiveDefaultInitialize returns PDIK_Struct.
+  bool hasNonTrivialToPrimitiveDefaultInitializeCUnion() const;
+
+  /// Check if this is or contains a C union that is non-trivial to destruct,
+  /// which is a union that has a member that is non-trivial to destruct. If
+  /// this returns true, isDestructedType returns DK_nontrivial_c_struct.
+  bool hasNonTrivialToPrimitiveDestructCUnion() const;
+
+  /// Check if this is or contains a C union that is non-trivial to copy, which
+  /// is a union that has a member that is non-trivial to copy. If this returns
+  /// true, isNonTrivialToPrimitiveCopy returns PCK_Struct.
+  bool hasNonTrivialToPrimitiveCopyCUnion() const;
+
   /// Determine whether expressions of the given type are forbidden
   /// from being lvalues in C.
   ///
@@ -1236,6 +1246,11 @@
  const ASTContext );
   static QualType IgnoreParens(QualType T);
   static DestructionKind isDestructedTypeImpl(QualType type);
+
+  /// Check if \param RD is or contains a non-trivial C union.
+  static bool hasNonTrivialToPrimitiveDefaultInitializeCUnion(const RecordDecl *RD);
+  static bool hasNonTrivialToPrimitiveDestructCUnion(const RecordDecl *RD);
+  static bool hasNonTrivialToPrimitiveCopyCUnion(const RecordDecl *RD);
 };
 
 } // namespace clang
@@ -6249,6 +6264,24 @@
   return getQualifiers().getObjCGCAttr();
 }
 
+inline bool QualType::hasNonTrivialToPrimitiveDefaultInitializeCUnion() const {
+  if (auto *RD = getTypePtr()->getBaseElementTypeUnsafe()->getAsRecordDecl())
+return hasNonTrivialToPrimitiveDefaultInitializeCUnion(RD);
+  return false;
+}
+
+inline bool QualType::hasNonTrivialToPrimitiveDestructCUnion() const {
+  if (auto *RD = getTypePtr()->getBaseElementTypeUnsafe()->getAsRecordDecl())
+return hasNonTrivialToPrimitiveDestructCUnion(RD);
+  return false;
+}
+
+inline bool QualType::hasNonTrivialToPrimitiveCopyCUnion() const {
+  if (auto *RD = getTypePtr()->getBaseElementTypeUnsafe()->getAsRecordDecl())
+return hasNonTrivialToPrimitiveCopyCUnion(RD);
+  return false;
+}
+
 inline FunctionType::ExtInfo getFunctionExtInfo(const Type ) {
   if (const auto *PT = t.getAs()) {
 if (const auto *FT = PT->getPointeeType()->getAs())
Index: cfe/trunk/include/clang/AST/DeclBase.h
===
--- cfe/trunk/include/clang/AST/DeclBase.h
+++ cfe/trunk/include/clang/AST/DeclBase.h
@@ -1440,6 +1440,13 @@
 uint64_t NonTrivialToPrimitiveCopy : 1;
 uint64_t NonTrivialToPrimitiveDestroy : 1;
 
+/// The following bits indicate whether this is or contains a C union that
+/// is non-trivial to default-initialize, destruct, or copy. These bits
+/// imply the associated basic 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 209626.
ahatanak added a comment.

In Type.h, move method declarations down and mention that the predicates imply 
the associated basic non-triviality predicates.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/PCH/non-trivial-c-union.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+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;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+U0 ug1 = { .f0 = 0 };
+S0 sg0; // expected-error {{cannot default-initialize an object of type 'S0' since it contains a union that is non-trivial to default-initialize}}
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+S0 sg2 = { .f1 = 0 }; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 209622.
ahatanak marked 3 inline comments as done.
ahatanak added a comment.

Improve comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/PCH/non-trivial-c-union.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+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;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+U0 ug1 = { .f0 = 0 };
+S0 sg0; // expected-error {{cannot default-initialize an object of type 'S0' since it contains a union that is non-trivial to default-initialize}}
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+S0 sg2 = { .f1 = 0 }; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, just a few minor comment requests now.




Comment at: include/clang/AST/DeclBase.h:1453
+/// copy.
+uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
+

Please include in these comments that these imply the associated basic 
non-triviality predicates.



Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 

rjmccall wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > You only want these checks to trigger on unions with non-trivial 
> > > > members (or structs containing them), right?  How about something like 
> > > > `hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more 
> > > > descriptive for the use sites, like `isPrimitiveCRestrictedType()`?
> > > > 
> > > > Also, it would be nice if the fast path of this could be inlined so 
> > > > that clients usually didn't had to make a call at all.  You can write 
> > > > the `getBaseElementTypeUnsafe()->getAs()` part in an 
> > > > `inline` implementation at the bottom this file.
> > > Since we don't keep track of whether a struct or union is or contains 
> > > unions with non-trivial members, we'll have to use the visitors to detect 
> > > such structs or unions or, to do it faster, add a bit to `RecordDeclBits` 
> > > that indicates the presence of non-trivial unions. I guess it's okay to 
> > > add another bit to `RecordDeclBits`?
> > It looks like there's plenty of space in `RecordDeclBits`, yeah.
> This comment seems like the right place to explain what makes a union 
> non-trivial in C (that it contains a member which is non-trivial for *any* of 
> the reasons that a type might be non-trivial).
Okay, if we're tracking these separately, please put separate comments on each. 
 Also, please mention in each comment that this implies the associated basic 
non-triviality predicate.



Comment at: lib/Sema/SemaDecl.cpp:12053
+NTCUC_UninitAutoVar);
 }
+

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Please add a comment explaining why this is specific to local variables.
> > > I was trying to explain why this should be specific to local variables 
> > > and realized that it's not clear to me whether it should be.
> > > 
> > > Suppose there is a union with two fields that are both non-trivial:
> > > 
> > > ```
> > > union U {
> > >   Type A a;
> > >   Type B a;
> > > };
> > > 
> > > U global;
> > > ```
> > > 
> > > In this case, is value-initialization (which is essentially 
> > > default-initialization plus a bunch of zero-initialization as per our 
> > > previous discussion) used to initialize `global`? If so, should we reject 
> > > the code since it requires default-initialization? It should be fine if 
> > > we can assume default-initialization means zero-initialization for 
> > > non-trivial types in C, but what if `TypeA` or `TypeB` requires 
> > > initializing to a non-zero value?
> > Yeah, the default-initialization dimension of this problem is interesting.  
> > The C++ rule makes sense for C++ because default initialization of a C++ 
> > class requires an actual, arbitrary-side-effects constructor call, which of 
> > course you can't reasonably do implicitly for a union member.  As discussed 
> > previously, non-trivial C types can presumably always be 
> > default-initialized with a constant bit pattern.  That means that, as long 
> > as we can do any initialization work at all, then it's in principle not a 
> > problem as long as the bit pattern is the same for all the union members 
> > requiring non-trivial initialization (and in particular if there's only one 
> > such member).  So it's just like you say, we *could* just initialize such 
> > unions conservatively as long as two different members don't require 
> > inconsistent patterns, which in practice they currently never do.  That's 
> > all true independent of storage duration — if we can write that pattern 
> > into a global, we can write into a local.  The only caveat is that a 
> > semantic need for non-trivial default initialization almost certainly means 
> > that there's a semantic need for non-trivial destruction as well, which of 
> > course can't be done on a local union (but isn't a problem for a global 
> > because we just don't destroy them).
> > 
> > On the other hand, on a language level it's much simpler to just say that 
> > we can't default-initialize a union of any storage duration if it has a 
> > non-trivial member, and then the language rule doesn't depend on bit-level 
> > representations.  If there's interest, we can look into weakening that rule 
> > later by saying that e.g. it's possible to default-initialize a union with 
> > at most one non-trivial member.
> > 
> > Apropos, do we consider unions with non-trivial members 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:12053
+NTCUC_UninitAutoVar);
 }
+

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Please add a comment explaining why this is specific to local variables.
> > I was trying to explain why this should be specific to local variables and 
> > realized that it's not clear to me whether it should be.
> > 
> > Suppose there is a union with two fields that are both non-trivial:
> > 
> > ```
> > union U {
> >   Type A a;
> >   Type B a;
> > };
> > 
> > U global;
> > ```
> > 
> > In this case, is value-initialization (which is essentially 
> > default-initialization plus a bunch of zero-initialization as per our 
> > previous discussion) used to initialize `global`? If so, should we reject 
> > the code since it requires default-initialization? It should be fine if we 
> > can assume default-initialization means zero-initialization for non-trivial 
> > types in C, but what if `TypeA` or `TypeB` requires initializing to a 
> > non-zero value?
> Yeah, the default-initialization dimension of this problem is interesting.  
> The C++ rule makes sense for C++ because default initialization of a C++ 
> class requires an actual, arbitrary-side-effects constructor call, which of 
> course you can't reasonably do implicitly for a union member.  As discussed 
> previously, non-trivial C types can presumably always be default-initialized 
> with a constant bit pattern.  That means that, as long as we can do any 
> initialization work at all, then it's in principle not a problem as long as 
> the bit pattern is the same for all the union members requiring non-trivial 
> initialization (and in particular if there's only one such member).  So it's 
> just like you say, we *could* just initialize such unions conservatively as 
> long as two different members don't require inconsistent patterns, which in 
> practice they currently never do.  That's all true independent of storage 
> duration — if we can write that pattern into a global, we can write into a 
> local.  The only caveat is that a semantic need for non-trivial default 
> initialization almost certainly means that there's a semantic need for 
> non-trivial destruction as well, which of course can't be done on a local 
> union (but isn't a problem for a global because we just don't destroy them).
> 
> On the other hand, on a language level it's much simpler to just say that we 
> can't default-initialize a union of any storage duration if it has a 
> non-trivial member, and then the language rule doesn't depend on bit-level 
> representations.  If there's interest, we can look into weakening that rule 
> later by saying that e.g. it's possible to default-initialize a union with at 
> most one non-trivial member.
> 
> Apropos, do we consider unions with non-trivial members to be non-trivial 
> members for the purposes of enclosing unions?  Seems like we should.  
> Probably the most sensible way to handle that is to also flag the union as 
> being non-trivial in a dimension if it has a member that's non-trivial in 
> that dimension (which might also let you fast-path some of the checking you 
> need to do).  Essentially, we'd consider the case where copying is impossible 
> to be a subset of the case where copying is non-trivial.
Yes, this patch does consider unions with non-trivial members to be non-trivial 
members for the purposes of enclosing unions.

I've made changes that make clang diagnose global variables that are or have C 
union types that are non-trivial to default-initialize. This disallows 
declaring global C union variables that have ObjC ARC pointer fields, but we 
can relax this later if users want them.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 209565.
ahatanak marked an inline comment as done.
ahatanak added a comment.

Diagnose C union globals that are non-trivial to default-initialize. Add 3 bits 
to RecordDeclBitfields for default-initialize, destruct, and copy to fast-path 
checking.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/PCH/non-trivial-c-union.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+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;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+U0 ug1 = { .f0 = 0 };
+S0 sg0; // expected-error {{cannot default-initialize an object of type 'S0' since it contains a union that is non-trivial to default-initialize}}
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+S0 sg2 = { .f1 = 0 }; // expected-error {{cannot default-initialize an object of type 'U0' since it is a union that is non-trivial to default-initialize}}
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:12053
+NTCUC_UninitAutoVar);
 }
+

ahatanak wrote:
> rjmccall wrote:
> > Please add a comment explaining why this is specific to local variables.
> I was trying to explain why this should be specific to local variables and 
> realized that it's not clear to me whether it should be.
> 
> Suppose there is a union with two fields that are both non-trivial:
> 
> ```
> union U {
>   Type A a;
>   Type B a;
> };
> 
> U global;
> ```
> 
> In this case, is value-initialization (which is essentially 
> default-initialization plus a bunch of zero-initialization as per our 
> previous discussion) used to initialize `global`? If so, should we reject the 
> code since it requires default-initialization? It should be fine if we can 
> assume default-initialization means zero-initialization for non-trivial types 
> in C, but what if `TypeA` or `TypeB` requires initializing to a non-zero 
> value?
Yeah, the default-initialization dimension of this problem is interesting.  The 
C++ rule makes sense for C++ because default initialization of a C++ class 
requires an actual, arbitrary-side-effects constructor call, which of course 
you can't reasonably do implicitly for a union member.  As discussed 
previously, non-trivial C types can presumably always be default-initialized 
with a constant bit pattern.  That means that, as long as we can do any 
initialization work at all, then it's in principle not a problem as long as the 
bit pattern is the same for all the union members requiring non-trivial 
initialization (and in particular if there's only one such member).  So it's 
just like you say, we *could* just initialize such unions conservatively as 
long as two different members don't require inconsistent patterns, which in 
practice they currently never do.  That's all true independent of storage 
duration — if we can write that pattern into a global, we can write into a 
local.  The only caveat is that a semantic need for non-trivial default 
initialization almost certainly means that there's a semantic need for 
non-trivial destruction as well, which of course can't be done on a local union 
(but isn't a problem for a global because we just don't destroy them).

On the other hand, on a language level it's much simpler to just say that we 
can't default-initialize a union of any storage duration if it has a 
non-trivial member, and then the language rule doesn't depend on bit-level 
representations.  If there's interest, we can look into weakening that rule 
later by saying that e.g. it's possible to default-initialize a union with at 
most one non-trivial member.

Apropos, do we consider unions with non-trivial members to be non-trivial 
members for the purposes of enclosing unions?  Seems like we should.  Probably 
the most sensible way to handle that is to also flag the union as being 
non-trivial in a dimension if it has a member that's non-trivial in that 
dimension (which might also let you fast-path some of the checking you need to 
do).  Essentially, we'd consider the case where copying is impossible to be a 
subset of the case where copying is non-trivial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:12053
+NTCUC_UninitAutoVar);
 }
+

rjmccall wrote:
> Please add a comment explaining why this is specific to local variables.
I was trying to explain why this should be specific to local variables and 
realized that it's not clear to me whether it should be.

Suppose there is a union with two fields that are both non-trivial:

```
union U {
  Type A a;
  Type B a;
};

U global;
```

In this case, is value-initialization (which is essentially 
default-initialization plus a bunch of zero-initialization as per our previous 
discussion) used to initialize `global`? If so, should we reject the code since 
it requires default-initialization? It should be fine if we can assume 
default-initialization means zero-initialization for non-trivial types in C, 
but what if `TypeA` or `TypeB` requires initializing to a non-zero value?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 209108.
ahatanak marked 3 inline comments as done.
ahatanak added a comment.

Address review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/PCH/non-trivial-c-union.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+typedef union { // expected-note 8 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 26 {{'U0' has subobjects that are non-trivial to copy}}
+  id f0; // expected-note 8 {{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 26 {{f0 has type '__strong id' that is non-trivial to copy}}
+  __weak id f1; // expected-note 8 {{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 26 {{f1 has type '__weak id' that is non-trivial to copy}}
+} U0;
+
+typedef struct {
+  U0 f0;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0;
+U0 ug1 = { .f0 = 0 };
+S0 sg0;
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+  (void)^(S0 a){ return sg0; }; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:6097
+  // non-trivial to copy or default-initialize.
+  checkNonTrivialCUnionInInitList(ILE);
+  }

ahatanak wrote:
> rjmccall wrote:
> > Can we extract a common function that checks the initializer expression of 
> > a non-trivial C union?  I think there are at least three separate places 
> > that do that in this patch, and it's not too hard to imagine that we might 
> > want to add more cases to the common analysis.
> Do you mean we should have something like `bool 
> checkNonTrivialCUnionInInitList(const Expr *Init)` so that we don't have to 
> call `dyn_cast`? If `Init` is an `InitListExpr`, the function diagnoses 
> non-trivial C unions types and returns true, otherwise it returns false.
Well, the goal is not to eliminate a `dyn_cast`, it's to generally perform 
whatever checking is necessary for an initializer.  Currently all of our 
checking is specific to `InitListExpr`, but there might be other cases we'll 
want to care about, in which case we'll probably care about them in all three 
of these places.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 3 inline comments as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:6097
+  // non-trivial to copy or default-initialize.
+  checkNonTrivialCUnionInInitList(ILE);
+  }

rjmccall wrote:
> Can we extract a common function that checks the initializer expression of a 
> non-trivial C union?  I think there are at least three separate places that 
> do that in this patch, and it's not too hard to imagine that we might want to 
> add more cases to the common analysis.
Do you mean we should have something like `bool 
checkNonTrivialCUnionInInitList(const Expr *Init)` so that we don't have to 
call `dyn_cast`? If `Init` is an `InitListExpr`, the function diagnoses 
non-trivial C unions types and returns true, otherwise it returns false.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks, that looks great.  A few more requests and then this will be ready to 
go, I think.




Comment at: include/clang/AST/DeclBase.h:1454
   /// Number of non-inherited bits in RecordDeclBitfields.
   enum { NumRecordDeclBits = 11 };
 

This needs to be updated.



Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > You only want these checks to trigger on unions with non-trivial members 
> > > (or structs containing them), right?  How about something like 
> > > `hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more 
> > > descriptive for the use sites, like `isPrimitiveCRestrictedType()`?
> > > 
> > > Also, it would be nice if the fast path of this could be inlined so that 
> > > clients usually didn't had to make a call at all.  You can write the 
> > > `getBaseElementTypeUnsafe()->getAs()` part in an `inline` 
> > > implementation at the bottom this file.
> > Since we don't keep track of whether a struct or union is or contains 
> > unions with non-trivial members, we'll have to use the visitors to detect 
> > such structs or unions or, to do it faster, add a bit to `RecordDeclBits` 
> > that indicates the presence of non-trivial unions. I guess it's okay to add 
> > another bit to `RecordDeclBits`?
> It looks like there's plenty of space in `RecordDeclBits`, yeah.
This comment seems like the right place to explain what makes a union 
non-trivial in C (that it contains a member which is non-trivial for *any* of 
the reasons that a type might be non-trivial).



Comment at: include/clang/AST/Type.h:1238
+  /// Check if \param RD is or contains a non-trivial C union.
+  bool hasNonTrivialPrimitiveCUnionMember(const RecordDecl *RD) const;
 };

This should be `static`, right?



Comment at: lib/Sema/SemaDecl.cpp:11648
+if (VDecl->getType().hasNonTrivialPrimitiveCUnionMember() &&
+VDecl->hasLocalStorage()) {
+  if (auto *ILE = dyn_cast(Init))

Please add a comment explaining why this is specific to local variables.



Comment at: lib/Sema/SemaDecl.cpp:12053
+NTCUC_UninitAutoVar);
 }
+

Please add a comment explaining why this is specific to local variables.



Comment at: lib/Sema/SemaExpr.cpp:6097
+  // non-trivial to copy or default-initialize.
+  checkNonTrivialCUnionInInitList(ILE);
+  }

Can we extract a common function that checks the initializer expression of a 
non-trivial C union?  I think there are at least three separate places that do 
that in this patch, and it's not too hard to imagine that we might want to add 
more cases to the common analysis.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 208861.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.

Add a bit to `RecordDeclBitfields` that indicates whether the record has a 
non-trivial C union member.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclBase.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/PCH/non-trivial-c-union.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+typedef union { // expected-note 8 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 26 {{'U0' has subobjects that are non-trivial to copy}}
+  id f0; // expected-note 8 {{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 26 {{f0 has type '__strong id' that is non-trivial to copy}}
+  __weak id f1; // expected-note 8 {{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 26 {{f1 has type '__weak id' that is non-trivial to copy}}
+} U0;
+
+typedef struct {
+  U0 f0;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0;
+U0 ug1 = { .f0 = 0 };
+S0 sg0;
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+  (void)^(S0 a){ return sg0; }; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 

ahatanak wrote:
> rjmccall wrote:
> > You only want these checks to trigger on unions with non-trivial members 
> > (or structs containing them), right?  How about something like 
> > `hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more descriptive 
> > for the use sites, like `isPrimitiveCRestrictedType()`?
> > 
> > Also, it would be nice if the fast path of this could be inlined so that 
> > clients usually didn't had to make a call at all.  You can write the 
> > `getBaseElementTypeUnsafe()->getAs()` part in an `inline` 
> > implementation at the bottom this file.
> Since we don't keep track of whether a struct or union is or contains unions 
> with non-trivial members, we'll have to use the visitors to detect such 
> structs or unions or, to do it faster, add a bit to `RecordDeclBits` that 
> indicates the presence of non-trivial unions. I guess it's okay to add 
> another bit to `RecordDeclBits`?
It looks like there's plenty of space in `RecordDeclBits`, yeah.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 

rjmccall wrote:
> You only want these checks to trigger on unions with non-trivial members (or 
> structs containing them), right?  How about something like 
> `hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more descriptive 
> for the use sites, like `isPrimitiveCRestrictedType()`?
> 
> Also, it would be nice if the fast path of this could be inlined so that 
> clients usually didn't had to make a call at all.  You can write the 
> `getBaseElementTypeUnsafe()->getAs()` part in an `inline` 
> implementation at the bottom this file.
Since we don't keep track of whether a struct or union is or contains unions 
with non-trivial members, we'll have to use the visitors to detect such structs 
or unions or, to do it faster, add a bit to `RecordDeclBits` that indicates the 
presence of non-trivial unions. I guess it's okay to add another bit to 
`RecordDeclBits`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1133
+  /// Check if this is or contains a non-trivial C struct/union type.
+  bool hasNonTrivialPrimitiveCStruct() const;
 

You only want these checks to trigger on unions with non-trivial members (or 
structs containing them), right?  How about something like 
`hasNonTrivialPrimitiveCUnionMember()`?  Or maybe make it more descriptive for 
the use sites, like `isPrimitiveCRestrictedType()`?

Also, it would be nice if the fast path of this could be inlined so that 
clients usually didn't had to make a call at all.  You can write the 
`getBaseElementTypeUnsafe()->getAs()` part in an `inline` 
implementation at the bottom this file.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

ahatanak wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > ahatanak wrote:
> > > > > > rjmccall wrote:
> > > > > > > ahatanak wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > It looks like you're generally warning about this based on 
> > > > > > > > > the specific context that forced an lvalue-to-rvalue 
> > > > > > > > > conversion.  I'm not sure `volatile` is special except that 
> > > > > > > > > we actually perform the load even in unused-value contexts.  
> > > > > > > > > Is the assumption that you've exhaustively covered all the 
> > > > > > > > > other contexts of lvalue-to-rvalue conversions whose values 
> > > > > > > > > will actually be used?  That seems questionable to me.
> > > > > > > > Yes, that was my assumption. All the other contexts where 
> > > > > > > > lvalue-to-rvalue conversion is performed and the result is used 
> > > > > > > > are already covered by other calls sites of 
> > > > > > > > `checkNonTrivialCUnion`, which informs the users that the 
> > > > > > > > struct/union is being used in an invalid context.
> > > > > > > > 
> > > > > > > > Do you have a case in mind that I didn't think of where a 
> > > > > > > > lvalue-to-rvalue conversion requires a non-trivial 
> > > > > > > > initialization/destruction/copying of a union but clang fails 
> > > > > > > > to emit any diagnostics?
> > > > > > > > 
> > > > > > > > Also I realized that lvalue-to-rvalue conversion of volatile 
> > > > > > > > types doesn't always require non-trivial destruction, so I 
> > > > > > > > think `CheckDestruct` shouldn't be set in this case.
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > void foo(U0 *a, volatile U0 *b) {
> > > > > > > >   // this doesn't require destruction.
> > > > > > > >   // this is perfectly valid if U0 is non-trivial to destruct 
> > > > > > > > but trivial to copy.
> > > > > > > >   *a = *b;  
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > For the same reason, I think `CheckDestruct` shouldn't be set 
> > > > > > > > for function returns (but it should be set for function 
> > > > > > > > parameters since they are destructed by the callee).
> > > > > > > There are a *lot* of places that trigger lvalue-to-rvalue 
> > > > > > > conversion.  Many of them aren't legal with structs (in C), but 
> > > > > > > I'm worried about approving a pattern with the potential to be 
> > > > > > > wrong by default just because we didn't think about some weird 
> > > > > > > case.  As an example, casts can trigger lvalue-to-rvalue 
> > > > > > > conversion; I think the only casts allowed with structs are the 
> > > > > > > identity cast and the cast to `void`, but those are indeed 
> > > > > > > allowed.  Now, a cast to `void` means the value is ignored, so we 
> > > > > > > can elide a non-volatile load in the operand, and an identity 
> > > > > > > cast isn't terminal; if the idea is that we're checking all the 
> > > > > > > *terminal* uses of a struct r-value, then we're in much more 
> > > > > > > restricted territory (and don't need to worry about things like 
> > > > > > > commas and conditional operators that can propagate values out).  
> > > > > > > But this still worries me.
> > > > > > > 
> > > > > > > I'm not sure we need to be super-pedantic about destructibility 
> > > > > > > vs. copyability in some of this checking.  It's certain possible 
> > > > > > > in C++, but I can't imagine what sort of *primitive* type would 
> > > > > > > be trivially copyable but not trivially destructible.  (The 
> > > > > > > reverse isn't true: something like a relative pointer would be 
> > > > > > > non-trivially copyable but still trivially destructible.)
> > > > > > > 
> > > > > > > Is there any way to make this check cheaper so that we can 
> > > > > > > immediately avoid any further work for types that are obviously 
> > > > > > > copyable/destructible?  All the restricted types are (possibly 
> > > > > > > arrays of) record types, right?
> > > > > > I'm not sure I fully 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > ahatanak wrote:
> > > > > rjmccall wrote:
> > > > > > ahatanak wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > It looks like you're generally warning about this based on the 
> > > > > > > > specific context that forced an lvalue-to-rvalue conversion.  
> > > > > > > > I'm not sure `volatile` is special except that we actually 
> > > > > > > > perform the load even in unused-value contexts.  Is the 
> > > > > > > > assumption that you've exhaustively covered all the other 
> > > > > > > > contexts of lvalue-to-rvalue conversions whose values will 
> > > > > > > > actually be used?  That seems questionable to me.
> > > > > > > Yes, that was my assumption. All the other contexts where 
> > > > > > > lvalue-to-rvalue conversion is performed and the result is used 
> > > > > > > are already covered by other calls sites of 
> > > > > > > `checkNonTrivialCUnion`, which informs the users that the 
> > > > > > > struct/union is being used in an invalid context.
> > > > > > > 
> > > > > > > Do you have a case in mind that I didn't think of where a 
> > > > > > > lvalue-to-rvalue conversion requires a non-trivial 
> > > > > > > initialization/destruction/copying of a union but clang fails to 
> > > > > > > emit any diagnostics?
> > > > > > > 
> > > > > > > Also I realized that lvalue-to-rvalue conversion of volatile 
> > > > > > > types doesn't always require non-trivial destruction, so I think 
> > > > > > > `CheckDestruct` shouldn't be set in this case.
> > > > > > > 
> > > > > > > ```
> > > > > > > void foo(U0 *a, volatile U0 *b) {
> > > > > > >   // this doesn't require destruction.
> > > > > > >   // this is perfectly valid if U0 is non-trivial to destruct but 
> > > > > > > trivial to copy.
> > > > > > >   *a = *b;  
> > > > > > > }
> > > > > > > ```
> > > > > > > 
> > > > > > > For the same reason, I think `CheckDestruct` shouldn't be set for 
> > > > > > > function returns (but it should be set for function parameters 
> > > > > > > since they are destructed by the callee).
> > > > > > There are a *lot* of places that trigger lvalue-to-rvalue 
> > > > > > conversion.  Many of them aren't legal with structs (in C), but I'm 
> > > > > > worried about approving a pattern with the potential to be wrong by 
> > > > > > default just because we didn't think about some weird case.  As an 
> > > > > > example, casts can trigger lvalue-to-rvalue conversion; I think the 
> > > > > > only casts allowed with structs are the identity cast and the cast 
> > > > > > to `void`, but those are indeed allowed.  Now, a cast to `void` 
> > > > > > means the value is ignored, so we can elide a non-volatile load in 
> > > > > > the operand, and an identity cast isn't terminal; if the idea is 
> > > > > > that we're checking all the *terminal* uses of a struct r-value, 
> > > > > > then we're in much more restricted territory (and don't need to 
> > > > > > worry about things like commas and conditional operators that can 
> > > > > > propagate values out).  But this still worries me.
> > > > > > 
> > > > > > I'm not sure we need to be super-pedantic about destructibility vs. 
> > > > > > copyability in some of this checking.  It's certain possible in 
> > > > > > C++, but I can't imagine what sort of *primitive* type would be 
> > > > > > trivially copyable but not trivially destructible.  (The reverse 
> > > > > > isn't true: something like a relative pointer would be 
> > > > > > non-trivially copyable but still trivially destructible.)
> > > > > > 
> > > > > > Is there any way to make this check cheaper so that we can 
> > > > > > immediately avoid any further work for types that are obviously 
> > > > > > copyable/destructible?  All the restricted types are (possibly 
> > > > > > arrays of) record types, right?
> > > > > I'm not sure I fully understand what you are saying, but by 
> > > > > "cheaper", do you mean fewer and simpler rules for allowing or 
> > > > > disallowing non-trivial unions even when that might result in 
> > > > > rejecting unions used in contexts in which non-trivial 
> > > > > initialization/destruction/copying is not required? If so, we can 
> > > > > probably diagnose any lvalue-to-rvalue conversions regardless of 
> > > > > whether the source is volatile if the type is either non-trivial to 
> > > > > copy or destruct.
> > > > Sorry, that point was separate from the discussion of `volatile` and 
> > > > lvalue-to-rvalue conversions.  I mean that you're changing a lot of 
> > > > core paths in Sema, and it would be nice if we could very quickly 
> > > > decide based on the type that no restrictions apply instead of having 
> > > > to make a 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > ahatanak wrote:
> > > > > > rjmccall wrote:
> > > > > > > It looks like you're generally warning about this based on the 
> > > > > > > specific context that forced an lvalue-to-rvalue conversion.  I'm 
> > > > > > > not sure `volatile` is special except that we actually perform 
> > > > > > > the load even in unused-value contexts.  Is the assumption that 
> > > > > > > you've exhaustively covered all the other contexts of 
> > > > > > > lvalue-to-rvalue conversions whose values will actually be used?  
> > > > > > > That seems questionable to me.
> > > > > > Yes, that was my assumption. All the other contexts where 
> > > > > > lvalue-to-rvalue conversion is performed and the result is used are 
> > > > > > already covered by other calls sites of `checkNonTrivialCUnion`, 
> > > > > > which informs the users that the struct/union is being used in an 
> > > > > > invalid context.
> > > > > > 
> > > > > > Do you have a case in mind that I didn't think of where a 
> > > > > > lvalue-to-rvalue conversion requires a non-trivial 
> > > > > > initialization/destruction/copying of a union but clang fails to 
> > > > > > emit any diagnostics?
> > > > > > 
> > > > > > Also I realized that lvalue-to-rvalue conversion of volatile types 
> > > > > > doesn't always require non-trivial destruction, so I think 
> > > > > > `CheckDestruct` shouldn't be set in this case.
> > > > > > 
> > > > > > ```
> > > > > > void foo(U0 *a, volatile U0 *b) {
> > > > > >   // this doesn't require destruction.
> > > > > >   // this is perfectly valid if U0 is non-trivial to destruct but 
> > > > > > trivial to copy.
> > > > > >   *a = *b;  
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > For the same reason, I think `CheckDestruct` shouldn't be set for 
> > > > > > function returns (but it should be set for function parameters 
> > > > > > since they are destructed by the callee).
> > > > > There are a *lot* of places that trigger lvalue-to-rvalue conversion. 
> > > > >  Many of them aren't legal with structs (in C), but I'm worried about 
> > > > > approving a pattern with the potential to be wrong by default just 
> > > > > because we didn't think about some weird case.  As an example, casts 
> > > > > can trigger lvalue-to-rvalue conversion; I think the only casts 
> > > > > allowed with structs are the identity cast and the cast to `void`, 
> > > > > but those are indeed allowed.  Now, a cast to `void` means the value 
> > > > > is ignored, so we can elide a non-volatile load in the operand, and 
> > > > > an identity cast isn't terminal; if the idea is that we're checking 
> > > > > all the *terminal* uses of a struct r-value, then we're in much more 
> > > > > restricted territory (and don't need to worry about things like 
> > > > > commas and conditional operators that can propagate values out).  But 
> > > > > this still worries me.
> > > > > 
> > > > > I'm not sure we need to be super-pedantic about destructibility vs. 
> > > > > copyability in some of this checking.  It's certain possible in C++, 
> > > > > but I can't imagine what sort of *primitive* type would be trivially 
> > > > > copyable but not trivially destructible.  (The reverse isn't true: 
> > > > > something like a relative pointer would be non-trivially copyable but 
> > > > > still trivially destructible.)
> > > > > 
> > > > > Is there any way to make this check cheaper so that we can 
> > > > > immediately avoid any further work for types that are obviously 
> > > > > copyable/destructible?  All the restricted types are (possibly arrays 
> > > > > of) record types, right?
> > > > I'm not sure I fully understand what you are saying, but by "cheaper", 
> > > > do you mean fewer and simpler rules for allowing or disallowing 
> > > > non-trivial unions even when that might result in rejecting unions used 
> > > > in contexts in which non-trivial initialization/destruction/copying is 
> > > > not required? If so, we can probably diagnose any lvalue-to-rvalue 
> > > > conversions regardless of whether the source is volatile if the type is 
> > > > either non-trivial to copy or destruct.
> > > Sorry, that point was separate from the discussion of `volatile` and 
> > > lvalue-to-rvalue conversions.  I mean that you're changing a lot of core 
> > > paths in Sema, and it would be nice if we could very quickly decide based 
> > > on the type that no restrictions apply instead of having to make a 
> > > function call, a switch, and a bunch of other calls in order to realize 
> > > that e.g. `void*` never needs additional checking.  Currently you have a 
> > > `!CPlusPlus` check in front 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 207600.
ahatanak added a comment.

Call `hasNonTrivialPrimitiveCStruct` to check whether the type is a non-trivial 
C struct/union before calling `checkNonTrivialCUnion`. Fix comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+typedef union { // expected-note 8 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 26 {{'U0' has subobjects that are non-trivial to copy}}
+  id f0; // expected-note 8 {{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 26 {{f0 has type '__strong id' that is non-trivial to copy}}
+  __weak id f1; // expected-note 8 {{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 26 {{f1 has type '__weak id' that is non-trivial to copy}}
+} U0;
+
+typedef struct {
+  U0 f0;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0;
+U0 ug1 = { .f0 = 0 };
+S0 sg0;
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+  (void)^(S0 a){ return sg0; }; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > ahatanak wrote:
> > > > > rjmccall wrote:
> > > > > > It looks like you're generally warning about this based on the 
> > > > > > specific context that forced an lvalue-to-rvalue conversion.  I'm 
> > > > > > not sure `volatile` is special except that we actually perform the 
> > > > > > load even in unused-value contexts.  Is the assumption that you've 
> > > > > > exhaustively covered all the other contexts of lvalue-to-rvalue 
> > > > > > conversions whose values will actually be used?  That seems 
> > > > > > questionable to me.
> > > > > Yes, that was my assumption. All the other contexts where 
> > > > > lvalue-to-rvalue conversion is performed and the result is used are 
> > > > > already covered by other calls sites of `checkNonTrivialCUnion`, 
> > > > > which informs the users that the struct/union is being used in an 
> > > > > invalid context.
> > > > > 
> > > > > Do you have a case in mind that I didn't think of where a 
> > > > > lvalue-to-rvalue conversion requires a non-trivial 
> > > > > initialization/destruction/copying of a union but clang fails to emit 
> > > > > any diagnostics?
> > > > > 
> > > > > Also I realized that lvalue-to-rvalue conversion of volatile types 
> > > > > doesn't always require non-trivial destruction, so I think 
> > > > > `CheckDestruct` shouldn't be set in this case.
> > > > > 
> > > > > ```
> > > > > void foo(U0 *a, volatile U0 *b) {
> > > > >   // this doesn't require destruction.
> > > > >   // this is perfectly valid if U0 is non-trivial to destruct but 
> > > > > trivial to copy.
> > > > >   *a = *b;  
> > > > > }
> > > > > ```
> > > > > 
> > > > > For the same reason, I think `CheckDestruct` shouldn't be set for 
> > > > > function returns (but it should be set for function parameters since 
> > > > > they are destructed by the callee).
> > > > There are a *lot* of places that trigger lvalue-to-rvalue conversion.  
> > > > Many of them aren't legal with structs (in C), but I'm worried about 
> > > > approving a pattern with the potential to be wrong by default just 
> > > > because we didn't think about some weird case.  As an example, casts 
> > > > can trigger lvalue-to-rvalue conversion; I think the only casts allowed 
> > > > with structs are the identity cast and the cast to `void`, but those 
> > > > are indeed allowed.  Now, a cast to `void` means the value is ignored, 
> > > > so we can elide a non-volatile load in the operand, and an identity 
> > > > cast isn't terminal; if the idea is that we're checking all the 
> > > > *terminal* uses of a struct r-value, then we're in much more restricted 
> > > > territory (and don't need to worry about things like commas and 
> > > > conditional operators that can propagate values out).  But this still 
> > > > worries me.
> > > > 
> > > > I'm not sure we need to be super-pedantic about destructibility vs. 
> > > > copyability in some of this checking.  It's certain possible in C++, 
> > > > but I can't imagine what sort of *primitive* type would be trivially 
> > > > copyable but not trivially destructible.  (The reverse isn't true: 
> > > > something like a relative pointer would be non-trivially copyable but 
> > > > still trivially destructible.)
> > > > 
> > > > Is there any way to make this check cheaper so that we can immediately 
> > > > avoid any further work for types that are obviously 
> > > > copyable/destructible?  All the restricted types are (possibly arrays 
> > > > of) record types, right?
> > > I'm not sure I fully understand what you are saying, but by "cheaper", do 
> > > you mean fewer and simpler rules for allowing or disallowing non-trivial 
> > > unions even when that might result in rejecting unions used in contexts 
> > > in which non-trivial initialization/destruction/copying is not required? 
> > > If so, we can probably diagnose any lvalue-to-rvalue conversions 
> > > regardless of whether the source is volatile if the type is either 
> > > non-trivial to copy or destruct.
> > Sorry, that point was separate from the discussion of `volatile` and 
> > lvalue-to-rvalue conversions.  I mean that you're changing a lot of core 
> > paths in Sema, and it would be nice if we could very quickly decide based 
> > on the type that no restrictions apply instead of having to make a function 
> > call, a switch, and a bunch of other calls in order to realize that e.g. 
> > `void*` never needs additional checking.  Currently you have a `!CPlusPlus` 
> > check in front of all the `checkNonTrivialCUnion` calls; I would like 
> > something that reliably avoids doing this work for the vast majority of 
> > types that are not restricted, even in C.
> Instead of checking `!CPlusPlus`, we 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > It looks like you're generally warning about this based on the 
> > > > > specific context that forced an lvalue-to-rvalue conversion.  I'm not 
> > > > > sure `volatile` is special except that we actually perform the load 
> > > > > even in unused-value contexts.  Is the assumption that you've 
> > > > > exhaustively covered all the other contexts of lvalue-to-rvalue 
> > > > > conversions whose values will actually be used?  That seems 
> > > > > questionable to me.
> > > > Yes, that was my assumption. All the other contexts where 
> > > > lvalue-to-rvalue conversion is performed and the result is used are 
> > > > already covered by other calls sites of `checkNonTrivialCUnion`, which 
> > > > informs the users that the struct/union is being used in an invalid 
> > > > context.
> > > > 
> > > > Do you have a case in mind that I didn't think of where a 
> > > > lvalue-to-rvalue conversion requires a non-trivial 
> > > > initialization/destruction/copying of a union but clang fails to emit 
> > > > any diagnostics?
> > > > 
> > > > Also I realized that lvalue-to-rvalue conversion of volatile types 
> > > > doesn't always require non-trivial destruction, so I think 
> > > > `CheckDestruct` shouldn't be set in this case.
> > > > 
> > > > ```
> > > > void foo(U0 *a, volatile U0 *b) {
> > > >   // this doesn't require destruction.
> > > >   // this is perfectly valid if U0 is non-trivial to destruct but 
> > > > trivial to copy.
> > > >   *a = *b;  
> > > > }
> > > > ```
> > > > 
> > > > For the same reason, I think `CheckDestruct` shouldn't be set for 
> > > > function returns (but it should be set for function parameters since 
> > > > they are destructed by the callee).
> > > There are a *lot* of places that trigger lvalue-to-rvalue conversion.  
> > > Many of them aren't legal with structs (in C), but I'm worried about 
> > > approving a pattern with the potential to be wrong by default just 
> > > because we didn't think about some weird case.  As an example, casts can 
> > > trigger lvalue-to-rvalue conversion; I think the only casts allowed with 
> > > structs are the identity cast and the cast to `void`, but those are 
> > > indeed allowed.  Now, a cast to `void` means the value is ignored, so we 
> > > can elide a non-volatile load in the operand, and an identity cast isn't 
> > > terminal; if the idea is that we're checking all the *terminal* uses of a 
> > > struct r-value, then we're in much more restricted territory (and don't 
> > > need to worry about things like commas and conditional operators that can 
> > > propagate values out).  But this still worries me.
> > > 
> > > I'm not sure we need to be super-pedantic about destructibility vs. 
> > > copyability in some of this checking.  It's certain possible in C++, but 
> > > I can't imagine what sort of *primitive* type would be trivially copyable 
> > > but not trivially destructible.  (The reverse isn't true: something like 
> > > a relative pointer would be non-trivially copyable but still trivially 
> > > destructible.)
> > > 
> > > Is there any way to make this check cheaper so that we can immediately 
> > > avoid any further work for types that are obviously 
> > > copyable/destructible?  All the restricted types are (possibly arrays of) 
> > > record types, right?
> > I'm not sure I fully understand what you are saying, but by "cheaper", do 
> > you mean fewer and simpler rules for allowing or disallowing non-trivial 
> > unions even when that might result in rejecting unions used in contexts in 
> > which non-trivial initialization/destruction/copying is not required? If 
> > so, we can probably diagnose any lvalue-to-rvalue conversions regardless of 
> > whether the source is volatile if the type is either non-trivial to copy or 
> > destruct.
> Sorry, that point was separate from the discussion of `volatile` and 
> lvalue-to-rvalue conversions.  I mean that you're changing a lot of core 
> paths in Sema, and it would be nice if we could very quickly decide based on 
> the type that no restrictions apply instead of having to make a function 
> call, a switch, and a bunch of other calls in order to realize that e.g. 
> `void*` never needs additional checking.  Currently you have a `!CPlusPlus` 
> check in front of all the `checkNonTrivialCUnion` calls; I would like 
> something that reliably avoids doing this work for the vast majority of types 
> that are not restricted, even in C.
Instead of checking `!CPlusPlus`, we can call `isNonTrivialPrimitiveCType` 
(which is deleted in this patch) to do a cheaper check of whether the type 
requires any non-trivial 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > It looks like you're generally warning about this based on the specific 
> > > > context that forced an lvalue-to-rvalue conversion.  I'm not sure 
> > > > `volatile` is special except that we actually perform the load even in 
> > > > unused-value contexts.  Is the assumption that you've exhaustively 
> > > > covered all the other contexts of lvalue-to-rvalue conversions whose 
> > > > values will actually be used?  That seems questionable to me.
> > > Yes, that was my assumption. All the other contexts where 
> > > lvalue-to-rvalue conversion is performed and the result is used are 
> > > already covered by other calls sites of `checkNonTrivialCUnion`, which 
> > > informs the users that the struct/union is being used in an invalid 
> > > context.
> > > 
> > > Do you have a case in mind that I didn't think of where a 
> > > lvalue-to-rvalue conversion requires a non-trivial 
> > > initialization/destruction/copying of a union but clang fails to emit any 
> > > diagnostics?
> > > 
> > > Also I realized that lvalue-to-rvalue conversion of volatile types 
> > > doesn't always require non-trivial destruction, so I think 
> > > `CheckDestruct` shouldn't be set in this case.
> > > 
> > > ```
> > > void foo(U0 *a, volatile U0 *b) {
> > >   // this doesn't require destruction.
> > >   // this is perfectly valid if U0 is non-trivial to destruct but trivial 
> > > to copy.
> > >   *a = *b;  
> > > }
> > > ```
> > > 
> > > For the same reason, I think `CheckDestruct` shouldn't be set for 
> > > function returns (but it should be set for function parameters since they 
> > > are destructed by the callee).
> > There are a *lot* of places that trigger lvalue-to-rvalue conversion.  Many 
> > of them aren't legal with structs (in C), but I'm worried about approving a 
> > pattern with the potential to be wrong by default just because we didn't 
> > think about some weird case.  As an example, casts can trigger 
> > lvalue-to-rvalue conversion; I think the only casts allowed with structs 
> > are the identity cast and the cast to `void`, but those are indeed allowed. 
> >  Now, a cast to `void` means the value is ignored, so we can elide a 
> > non-volatile load in the operand, and an identity cast isn't terminal; if 
> > the idea is that we're checking all the *terminal* uses of a struct 
> > r-value, then we're in much more restricted territory (and don't need to 
> > worry about things like commas and conditional operators that can propagate 
> > values out).  But this still worries me.
> > 
> > I'm not sure we need to be super-pedantic about destructibility vs. 
> > copyability in some of this checking.  It's certain possible in C++, but I 
> > can't imagine what sort of *primitive* type would be trivially copyable but 
> > not trivially destructible.  (The reverse isn't true: something like a 
> > relative pointer would be non-trivially copyable but still trivially 
> > destructible.)
> > 
> > Is there any way to make this check cheaper so that we can immediately 
> > avoid any further work for types that are obviously copyable/destructible?  
> > All the restricted types are (possibly arrays of) record types, right?
> I'm not sure I fully understand what you are saying, but by "cheaper", do you 
> mean fewer and simpler rules for allowing or disallowing non-trivial unions 
> even when that might result in rejecting unions used in contexts in which 
> non-trivial initialization/destruction/copying is not required? If so, we can 
> probably diagnose any lvalue-to-rvalue conversions regardless of whether the 
> source is volatile if the type is either non-trivial to copy or destruct.
Sorry, that point was separate from the discussion of `volatile` and 
lvalue-to-rvalue conversions.  I mean that you're changing a lot of core paths 
in Sema, and it would be nice if we could very quickly decide based on the type 
that no restrictions apply instead of having to make a function call, a switch, 
and a bunch of other calls in order to realize that e.g. `void*` never needs 
additional checking.  Currently you have a `!CPlusPlus` check in front of all 
the `checkNonTrivialCUnion` calls; I would like something that reliably avoids 
doing this work for the vast majority of types that are not restricted, even in 
C.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > It looks like you're generally warning about this based on the specific 
> > > context that forced an lvalue-to-rvalue conversion.  I'm not sure 
> > > `volatile` is special except that we actually perform the load even in 
> > > unused-value contexts.  Is the assumption that you've exhaustively 
> > > covered all the other contexts of lvalue-to-rvalue conversions whose 
> > > values will actually be used?  That seems questionable to me.
> > Yes, that was my assumption. All the other contexts where lvalue-to-rvalue 
> > conversion is performed and the result is used are already covered by other 
> > calls sites of `checkNonTrivialCUnion`, which informs the users that the 
> > struct/union is being used in an invalid context.
> > 
> > Do you have a case in mind that I didn't think of where a lvalue-to-rvalue 
> > conversion requires a non-trivial initialization/destruction/copying of a 
> > union but clang fails to emit any diagnostics?
> > 
> > Also I realized that lvalue-to-rvalue conversion of volatile types doesn't 
> > always require non-trivial destruction, so I think `CheckDestruct` 
> > shouldn't be set in this case.
> > 
> > ```
> > void foo(U0 *a, volatile U0 *b) {
> >   // this doesn't require destruction.
> >   // this is perfectly valid if U0 is non-trivial to destruct but trivial 
> > to copy.
> >   *a = *b;  
> > }
> > ```
> > 
> > For the same reason, I think `CheckDestruct` shouldn't be set for function 
> > returns (but it should be set for function parameters since they are 
> > destructed by the callee).
> There are a *lot* of places that trigger lvalue-to-rvalue conversion.  Many 
> of them aren't legal with structs (in C), but I'm worried about approving a 
> pattern with the potential to be wrong by default just because we didn't 
> think about some weird case.  As an example, casts can trigger 
> lvalue-to-rvalue conversion; I think the only casts allowed with structs are 
> the identity cast and the cast to `void`, but those are indeed allowed.  Now, 
> a cast to `void` means the value is ignored, so we can elide a non-volatile 
> load in the operand, and an identity cast isn't terminal; if the idea is that 
> we're checking all the *terminal* uses of a struct r-value, then we're in 
> much more restricted territory (and don't need to worry about things like 
> commas and conditional operators that can propagate values out).  But this 
> still worries me.
> 
> I'm not sure we need to be super-pedantic about destructibility vs. 
> copyability in some of this checking.  It's certain possible in C++, but I 
> can't imagine what sort of *primitive* type would be trivially copyable but 
> not trivially destructible.  (The reverse isn't true: something like a 
> relative pointer would be non-trivially copyable but still trivially 
> destructible.)
> 
> Is there any way to make this check cheaper so that we can immediately avoid 
> any further work for types that are obviously copyable/destructible?  All the 
> restricted types are (possibly arrays of) record types, right?
I'm not sure I fully understand what you are saying, but by "cheaper", do you 
mean fewer and simpler rules for allowing or disallowing non-trivial unions 
even when that might result in rejecting unions used in contexts in which 
non-trivial initialization/destruction/copying is not required? If so, we can 
probably diagnose any lvalue-to-rvalue conversions regardless of whether the 
source is volatile if the type is either non-trivial to copy or destruct.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

ahatanak wrote:
> rjmccall wrote:
> > It looks like you're generally warning about this based on the specific 
> > context that forced an lvalue-to-rvalue conversion.  I'm not sure 
> > `volatile` is special except that we actually perform the load even in 
> > unused-value contexts.  Is the assumption that you've exhaustively covered 
> > all the other contexts of lvalue-to-rvalue conversions whose values will 
> > actually be used?  That seems questionable to me.
> Yes, that was my assumption. All the other contexts where lvalue-to-rvalue 
> conversion is performed and the result is used are already covered by other 
> calls sites of `checkNonTrivialCUnion`, which informs the users that the 
> struct/union is being used in an invalid context.
> 
> Do you have a case in mind that I didn't think of where a lvalue-to-rvalue 
> conversion requires a non-trivial initialization/destruction/copying of a 
> union but clang fails to emit any diagnostics?
> 
> Also I realized that lvalue-to-rvalue conversion of volatile types doesn't 
> always require non-trivial destruction, so I think `CheckDestruct` shouldn't 
> be set in this case.
> 
> ```
> void foo(U0 *a, volatile U0 *b) {
>   // this doesn't require destruction.
>   // this is perfectly valid if U0 is non-trivial to destruct but trivial to 
> copy.
>   *a = *b;  
> }
> ```
> 
> For the same reason, I think `CheckDestruct` shouldn't be set for function 
> returns (but it should be set for function parameters since they are 
> destructed by the callee).
There are a *lot* of places that trigger lvalue-to-rvalue conversion.  Many of 
them aren't legal with structs (in C), but I'm worried about approving a 
pattern with the potential to be wrong by default just because we didn't think 
about some weird case.  As an example, casts can trigger lvalue-to-rvalue 
conversion; I think the only casts allowed with structs are the identity cast 
and the cast to `void`, but those are indeed allowed.  Now, a cast to `void` 
means the value is ignored, so we can elide a non-volatile load in the operand, 
and an identity cast isn't terminal; if the idea is that we're checking all the 
*terminal* uses of a struct r-value, then we're in much more restricted 
territory (and don't need to worry about things like commas and conditional 
operators that can propagate values out).  But this still worries me.

I'm not sure we need to be super-pedantic about destructibility vs. copyability 
in some of this checking.  It's certain possible in C++, but I can't imagine 
what sort of *primitive* type would be trivially copyable but not trivially 
destructible.  (The reverse isn't true: something like a relative pointer would 
be non-trivially copyable but still trivially destructible.)

Is there any way to make this check cheaper so that we can immediately avoid 
any further work for types that are obviously copyable/destructible?  All the 
restricted types are (possibly arrays of) record types, right?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

rjmccall wrote:
> It looks like you're generally warning about this based on the specific 
> context that forced an lvalue-to-rvalue conversion.  I'm not sure `volatile` 
> is special except that we actually perform the load even in unused-value 
> contexts.  Is the assumption that you've exhaustively covered all the other 
> contexts of lvalue-to-rvalue conversions whose values will actually be used?  
> That seems questionable to me.
Yes, that was my assumption. All the other contexts where lvalue-to-rvalue 
conversion is performed and the result is used are already covered by other 
calls sites of `checkNonTrivialCUnion`, which informs the users that the 
struct/union is being used in an invalid context.

Do you have a case in mind that I didn't think of where a lvalue-to-rvalue 
conversion requires a non-trivial initialization/destruction/copying of a union 
but clang fails to emit any diagnostics?

Also I realized that lvalue-to-rvalue conversion of volatile types doesn't 
always require non-trivial destruction, so I think `CheckDestruct` shouldn't be 
set in this case.

```
void foo(U0 *a, volatile U0 *b) {
  // this doesn't require destruction.
  // this is perfectly valid if U0 is non-trivial to destruct but trivial to 
copy.
  *a = *b;  
}
```

For the same reason, I think `CheckDestruct` shouldn't be set for function 
returns (but it should be set for function parameters since they are destructed 
by the callee).


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Sema/Sema.h:2124
+// Implicitly initialized subobject.
+NTCUC_ImplicitInitSubObject,
+// Uninialized variable with automatic storage duration.

"default-initialized", please.



Comment at: lib/Sema/SemaExpr.cpp:16218
+checkNonTrivialCUnion(E->getType(), E->getExprLoc(),
+  Sema::NTCUC_LValueToRValueVolatile);
+

It looks like you're generally warning about this based on the specific context 
that forced an lvalue-to-rvalue conversion.  I'm not sure `volatile` is special 
except that we actually perform the load even in unused-value contexts.  Is the 
assumption that you've exhaustively covered all the other contexts of 
lvalue-to-rvalue conversions whose values will actually be used?  That seems 
questionable to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:633
+  "capture a variable of type %1}3 "
+  "since it %select{contains a union that |}2is non-trivial to "
+  "%select{default-initialize|destruct|copy}0">;

rjmccall wrote:
> Should this be "`%select{contains|is}2 a union that is non-trivial to..."?  
> None of these situations bans non-trivial types, just unions containing them 
> (and aggregates containing such unions).
Yes, that's correct. We only ban aggregates containing non-trivial unions (the 
former case) and non-trivial unions (the latter case).


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 206995.
ahatanak marked 7 inline comments as done.
ahatanak added a comment.

Address review comments. Diagnose lvalue-to-rvalue conversion of volatile 
non-trivial C types.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+typedef union { // expected-note 8 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 26 {{'U0' has subobjects that are non-trivial to copy}}
+  id f0; // expected-note 8 {{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 26 {{f0 has type '__strong id' that is non-trivial to copy}}
+  __weak id f1; // expected-note 8 {{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 26 {{f1 has type '__weak id' that is non-trivial to copy}}
+} U0;
+
+typedef struct {
+  U0 f0;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0;
+U0 ug1 = { .f0 = 0 };
+S0 sg0;
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is a union that is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is a union that is non-trivial to copy}}
+  (void)^(S0 a){ return sg0; }; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that 

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:11085
+if (isa(I))
+  continue;
+if (auto E = dyn_cast(I))

ahatanak wrote:
> rjmccall wrote:
> > Why is this okay?  Don't we need to check default-initialization for these?
> I didn't consider an `ImplicitValueInitExpr` to be a default-initialization 
> since IRGen currently emits a memcpy to initialize the field instead of 
> calling a synthesized default-initialization function as it does when a local 
> variable that's a non-trivial C struct is declared without an initializer.
> 
> ```
> typedef struct {
>   id f0, f1;
> } S0 ;
> 
> typedef struct {
>   S0 s0;
>   int f2;
> } S;
> 
> void test(void) {
>   // emits a memcpy of a global constant.
>   S s = { .f2 = 1 };
> }
> ```
> 
> It's not clear to me whether this is a default-initialization, but if it is, 
> we should check default-initialization here and IRGen should be fixed to emit 
> a call to the default-initialization function.
C does not use the terms "default-initialization" and "value-initialization", 
but the the initialization performed for static/thread storage duration objects 
generally matches what C++ would call value-initialization.  These rules are 
laid out in section 6.7.9 of the standard, which also includes the following in 
its description of the rules for list-initializations of aggregates (regardless 
of their storage duration):

> p19: ...all subobjects that are not initialized explicitly shall be 
> initialized implicitly the same as objects that have static storage duration.

So `s.s0` must be initialized as if a static object (in C++ terms, 
value-initialized), not left with an indeterminate value like an object of 
automatic storage duration without an initializer would be (in C++ terms, 
default-initialized).

I assume that the default-initialization function for a non-trivial C struct 
only initializes the fields that have non-trivial default-initialization.  
That's not good enough for value-initialization, which is required to 
recursively value-initialize *all* of the fields (and even zero-initialize any 
padding).  Note that that's the *only* difference between these two kinds of 
initialization.  Furthermore, if you think about what default-initialization 
actually means for all our leaf non-trivial C types, it's just assigning a 
predictable bit-pattern (e.g. a null pointer for ARC's `__strong` and `__weak` 
references), not doing anything that requires executing tailored code at 
runtime.  So here's what that tells us:

- We can't call the default-initialization function because it might leave 
trivial fields uninitialized (in general, although `struct S0` in your example 
doesn't have any, so effectively it does a full value-initialization).

- `memcpy` from a global constant is always a valid implementation of 
value-initialization for non-trivial C types.  It's also a valid implementation 
for default-implementation; it just might do more than it really needs to.  (In 
the case of `struct S0`, it would be *more efficient* to use `memset`, but 
`memcpy` is still *valid*.)

  Now, maybe someday we'll add a non-trivial C type that really requires 
non-trivial code to initialize, but I doubt we'd want to, because we'd have to 
forbid e.g. declaring global variables of that type, and we wouldn't have a 
simple answer for telling people how to properly initialize a 
dynamically-allocated object.  Even if we do, we can wait until then to 
generalize.

- Regardless, yes, this is a value-initialization and so it should check 
whether we can value-initialize the type.  Since value-initialization just 
means default-initialization plus a bunch of zero-initialization (which we can 
assume we can always do), that just means checking whether we can 
default-initialize the type.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
ahatanak added inline comments.



Comment at: include/clang/Sema/Sema.h:2126
+NTCUC_AutoVar,
+// Initializer expression for object with automatic storage duration.
+NTCUC_AutoObjInit,

rjmccall wrote:
> Please expand this comment to clarify that this it's only used when we (might 
> be) initializing an object by copying another.
> 
> Why is this specific to automatic storage duration?  Just because C doesn't 
> allow other initializers to be non-constant in the first place?
Yes, that was my reasoning. We want to reject copy initializations of 
non-trivial C unions with automatic storage duration since the compiler doesn't 
know how to do the copy, but I don't think variables with static duration have 
that problem since the initializers are constant, which enables them to be 
initialized according to the rules described in the C standard.



Comment at: lib/Sema/SemaDecl.cpp:11085
+if (isa(I))
+  continue;
+if (auto E = dyn_cast(I))

rjmccall wrote:
> Why is this okay?  Don't we need to check default-initialization for these?
I didn't consider an `ImplicitValueInitExpr` to be a default-initialization 
since IRGen currently emits a memcpy to initialize the field instead of calling 
a synthesized default-initialization function as it does when a local variable 
that's a non-trivial C struct is declared without an initializer.

```
typedef struct {
  id f0, f1;
} S0 ;

typedef struct {
  S0 s0;
  int f2;
} S;

void test(void) {
  // emits a memcpy of a global constant.
  S s = { .f2 = 1 };
}
```

It's not clear to me whether this is a default-initialization, but if it is, we 
should check default-initialization here and IRGen should be fixed to emit a 
call to the default-initialization function.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:633
+  "capture a variable of type %1}3 "
+  "since it %select{contains a union that |}2is non-trivial to "
+  "%select{default-initialize|destruct|copy}0">;

Should this be "`%select{contains|is}2 a union that is non-trivial to..."?  
None of these situations bans non-trivial types, just unions containing them 
(and aggregates containing such unions).



Comment at: include/clang/Sema/Sema.h:2116
 
+  // Contexts where using non-trivial C union types can be disallowed.
+  enum NonTrivialCUnionContext {

If this is parallel with the diagnostic, I'd suggest mentioning that in the 
comment.



Comment at: include/clang/Sema/Sema.h:2121
+// Function return.
+NTCUC_FuncitonReturn,
+// Uninialized variable with automatic storage duration.

Same typo in both of these.



Comment at: include/clang/Sema/Sema.h:2126
+NTCUC_AutoVar,
+// Initializer expression for object with automatic storage duration.
+NTCUC_AutoObjInit,

Please expand this comment to clarify that this it's only used when we (might 
be) initializing an object by copying another.

Why is this specific to automatic storage duration?  Just because C doesn't 
allow other initializers to be non-constant in the first place?



Comment at: lib/Sema/SemaDecl.cpp:11085
+if (isa(I))
+  continue;
+if (auto E = dyn_cast(I))

Why is this okay?  Don't we need to check default-initialization for these?



Comment at: lib/Sema/SemaDecl.cpp:11089
+else
+  checkNonTrivialCUnion(I->getType(), I->getExprLoc(), NTCUC_AutoObjInit);
+  }

Please include a comment like:

```
  // Assume all other initializers involving copying some existing object.
  // TODO: ignore any initializers where we can guarantee copy-elision
```


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak planned changes to this revision.
ahatanak added a comment.

This patch should diagnose lvalue-to-rvalue conversion of volatile non-trivial 
C unions too, but it currently doesn't.

  void test(volatile union U0 *a) {
(void)*a;
  }


Repository:
  rC Clang

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

https://reviews.llvm.org/D63753



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

[PATCH] D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: rjmccall, rsmith.
ahatanak added a project: clang.
Herald added subscribers: dexonsmith, jkorous.

This patch diagnoses uses of non-trivial C unions in the following contexts:

- function parameters
- function returns
- variables with automatic storage duration
- assignment
- compound literal with automatic storage duration
- block capture except when a `__block` variable is captured by a non-escaping 
block

Note that we already reject non-trivial C structs/unions used for variable 
function arguments. Also this patch doesn't detect non-trivial C unions passed 
to functions without function prototypes. I think that's okay since clang will 
reject the function definition's parameters, but if it isn't okay, it's 
possible to add diagnostics for that too.

See the discussion in https://reviews.llvm.org/D62988 for more background.

rdar://problem/50679094


Repository:
  rC Clang

https://reviews.llvm.org/D63753

Files:
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Type.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenObjC/Inputs/strong_in_union.h
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- /dev/null
+++ test/SemaObjC/non-trivial-c-union.m
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+
+typedef union { // expected-note 6 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 35 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 25 {{'U0' has subobjects that are non-trivial to copy}}
+  id f0; // expected-note 6 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 35 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 25 {{f0 has type '__strong id' that is non-trivial to copy}}
+  __weak id f1; // expected-note 6 {{f1 has type '__weak id' that is non-trivial to default-initialize}} expected-note 35 {{f1 has type '__weak id' that is non-trivial to destruct}} expected-note 25 {{f1 has type '__weak id' that is non-trivial to copy}}
+} U0;
+
+typedef struct {
+  U0 f0;
+  id f1;
+} S0;
+
+id g0;
+U0 ug0;
+U0 ug1 = { .f0 = 0 };
+S0 sg0;
+S0 sg1 = { .f0 = {0}, .f1 = 0 };
+
+U0 foo0(U0); // expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to copy}}
+S0 foo1(S0); // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+
+@interface C
+-(U0)m0:(U0)arg; // expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to copy}}
+-(S0)m1:(S0)arg; // expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for a function/method parameter since it contains a union that is non-trivial to copy}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to destruct}} expected-error {{cannot use type 'S0' for function/method return since it contains a union that is non-trivial to copy}}
+@end
+
+void testBlockFunction(void) {
+  (void)^(U0 a){ return ug0; }; // expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for a function/method parameter since it is non-trivial to copy}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to destruct}} expected-error {{cannot use type 'U0' for function/method return since it is non-trivial to copy}}
+  (void)^(S0 a){ return sg0; }; // expected-error {{cannot