[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

It looks like this caused a clang-cl regression 
https://bugs.llvm.org/show_bug.cgi?id=37146 "clang-cl emits special functions 
for non-trivial C-structs ('__destructor_8') introduced for Objective-C".


Repository:
  rL LLVM

https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326307: [ObjC] Allow declaring __strong pointer fields in 
structs in Objective-C (authored by ahatanak, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D41228?vs=135703=136232#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41228

Files:
  cfe/trunk/docs/LanguageExtensions.rst
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/AST/Decl.cpp
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/CodeGen/CGBlocks.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
  cfe/trunk/lib/CodeGen/CGExprAgg.cpp
  cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
  cfe/trunk/lib/CodeGen/CMakeLists.txt
  cfe/trunk/lib/CodeGen/CodeGenFunction.h
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/lib/Sema/JumpDiagnostics.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/ARCMT/checking.m
  cfe/trunk/test/CodeGenObjC/nontrivial-c-struct-exception.m
  cfe/trunk/test/CodeGenObjC/nontrivial-c-struct-func-name-collision.m
  cfe/trunk/test/CodeGenObjC/strong-in-c-struct.m
  cfe/trunk/test/Lexer/has_feature_objc_arc.m
  cfe/trunk/test/SemaObjC/arc-decls.m
  cfe/trunk/test/SemaObjC/arc-system-header.m
  cfe/trunk/test/SemaObjC/strong-in-c-struct.m

Index: cfe/trunk/test/ARCMT/checking.m
===
--- cfe/trunk/test/ARCMT/checking.m
+++ cfe/trunk/test/ARCMT/checking.m
@@ -116,7 +116,7 @@
 }
 
 struct S {
-  A* a; // expected-error {{ARC forbids Objective-C objects in struct}}
+  A* a;
 };
 
 @interface B
Index: cfe/trunk/test/SemaObjC/arc-system-header.m
===
--- cfe/trunk/test/SemaObjC/arc-system-header.m
+++ cfe/trunk/test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: cfe/trunk/test/SemaObjC/arc-decls.m
===
--- cfe/trunk/test/SemaObjC/arc-decls.m
+++ cfe/trunk/test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: cfe/trunk/test/SemaObjC/strong-in-c-struct.m
===
--- cfe/trunk/test/SemaObjC/strong-in-c-struct.m
+++ cfe/trunk/test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[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!  This looks ready; thank you for your patience in working this out.




Comment at: include/clang/AST/Type.h:1108
+PCK_ARCStrong,  // objc strong pointer.
+PCK_Struct   // non-trivial C struct.
+  };

ahatanak wrote:
> rjmccall wrote:
> > These should all be /// documentation comments, and they mostly shouldn't 
> > talk about fields since this is a query on QualType, not FieldDecl.  I 
> > would suggest something like:
> > 
> > for Trivial - The type does not fall into any of the following categories.  
> > Note that this case is zero-valued so that values of this enum can be used 
> > as a boolean condition for non-triviality.
> > 
> > for VolatileTrivial - The type would be trivial except that it is 
> > volatile-qualified.  Types that fall into one of the other non-trivial 
> > cases may additionally be volatile-qualified.
> > 
> > for ARCStrong - The type is an Objective-C retainable pointer type that is 
> > qualified with the ARC __strong qualifier.
> > 
> > for Struct - The type is a struct containing a field whose type is not 
> > PCK_Trivial.  Note that a C++ struct type does not necessarily match this; 
> > C++ copying semantics are too complex to express here, in part because they 
> > depend on the exact constructor or assignment operator that is chosen by 
> > overload resolution to do the copy.
> Thanks, I copied your comments verbatim except the comment on PCK_Struct: 
> types that are PCK_Struct have a field that is neither PCK_Trivial nor 
> PCK_VolatileTrivial. We can use the original comment once we start 
> distinguishing PCK_Trivial structs that have volatile fields from those that 
> don't. 
Yes, of course, that makes sense.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1108
+PCK_ARCStrong,  // objc strong pointer.
+PCK_Struct   // non-trivial C struct.
+  };

rjmccall wrote:
> These should all be /// documentation comments, and they mostly shouldn't 
> talk about fields since this is a query on QualType, not FieldDecl.  I would 
> suggest something like:
> 
> for Trivial - The type does not fall into any of the following categories.  
> Note that this case is zero-valued so that values of this enum can be used as 
> a boolean condition for non-triviality.
> 
> for VolatileTrivial - The type would be trivial except that it is 
> volatile-qualified.  Types that fall into one of the other non-trivial cases 
> may additionally be volatile-qualified.
> 
> for ARCStrong - The type is an Objective-C retainable pointer type that is 
> qualified with the ARC __strong qualifier.
> 
> for Struct - The type is a struct containing a field whose type is not 
> PCK_Trivial.  Note that a C++ struct type does not necessarily match this; 
> C++ copying semantics are too complex to express here, in part because they 
> depend on the exact constructor or assignment operator that is chosen by 
> overload resolution to do the copy.
Thanks, I copied your comments verbatim except the comment on PCK_Struct: types 
that are PCK_Struct have a field that is neither PCK_Trivial nor 
PCK_VolatileTrivial. We can use the original comment once we start 
distinguishing PCK_Trivial structs that have volatile fields from those that 
don't. 



Comment at: lib/CodeGen/CGBlocks.cpp:1779
+break;
   }
 

rjmccall wrote:
> This entire switch can be within an `#ifndef NDEBUG` if you really feel it's 
> important to assert.
It's probably not important to assert here, so I removed the switch.



Comment at: lib/CodeGen/CGBlocks.cpp:1792
+return std::make_pair(BlockCaptureEntityKind::BlockObject,
+  getBlockFieldFlagsForObjCObjectPointer(CI, T));
 

rjmccall wrote:
> I think you should leave the byref case of that function here, but you can 
> make it local to this if-clause.
I also simplified the QualType::DK_none case below.



Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

rjmccall wrote:
> rjmccall wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > This can only be getARCCleanupKind() if it's non-trivial to destroy 
> > > > > solely because of __strong members.  Since the setup here is 
> > > > > significantly more general than ARC, I think you need to default to 
> > > > > the more-correct behavior; if you want to add a special-case check 
> > > > > for only __strong members, you ought to do that explicitly.
> > > > I added a DestructionKind (DK_c_struct_strong_field) that is used just 
> > > > for structs that have strong fields (but doesn't have weak fields).
> > > Sure, that's a simple enough way to do it, and I think it's fairly 
> > > warranted.
> > Okay, since you don't have a DK_c_struct_strong_field anymore, you either 
> > need to stop using getARCCleanupKind() or do the test for just __strong 
> > fields.
> Ping about this again.
Sorry I missed this one.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 135703.
ahatanak marked 9 inline comments as done.
ahatanak added a comment.

Address review comments.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/nontrivial-c-struct-exception.m
  test/CodeGenObjC/nontrivial-c-struct-func-name-collision.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void has_objc_arc_feature();
 // CHECK-ARCLITE: void 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1121
+  /// after it is moved, as opposed to a truely destructive move in which the
+  /// source object is placed in an uninitialized state.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

ahatanak wrote:
> rjmccall wrote:
> > "truly"
> > 
> > Hmm.  Now that I'm thinking more about it, I'm not sure there's any point 
> > in tracking non-triviality of a C++-style destructive move separately from 
> > the non-triviality of a copy.  It's hard to imagine that there would ever 
> > be a non-C++ type that primitively has non-trivial copies but trivial 
> > C++-style moves or vice-versa.  Type-based destructors imply that the type 
> > represents some kind of resource, and a C++-style move will always be 
> > non-trivial for resource types because ownership of the resource needs to 
> > be given up by the old location.  Otherwise, a type might be non-trivial to 
> > copy but not destroy because there's something special about how it's 
> > stored (like volatility), but it's hard to imagine what could possibly 
> > cause it to be non-trivial to destroy but not copy.
> > 
> > If we were tracking non-triviality of an *unsafe* destructive move, one 
> > that leaves the source in an uninitialized state, that's quite different.
> > 
> > I think there are three reasonable options here:
> > 
> > - Ignore the argument I just made about the types that we're *likely* to 
> > care about modeling and generalize your tracking to also distinguish 
> > construction from assignment.  In such an environment, I think you can 
> > absolutely make an argument that it's still interesting to track C++-style 
> > moves separately from copies.
> > 
> > - Drop the tracking of destructive moves completely.  If you want to keep 
> > the method around, find, but it can just call 
> > `isNonTrivialToPrimitiveCopy()`.
> > 
> > - Change the tracking of *destructive* moves to instead track 
> > *deinitializing* moves.  The implementation would stop considering 
> > `__strong` types to be non-trivial to move.
> > 
> > But as things stand today, I do not see any point in separately tracking 
> > triviality of C++-style destructive moves.
> The second option seems most reasonable to me. We can always make changes if 
> someone comes up with a type that requires tracking destructive moves 
> separately.
Well, we already have a type that would have a trivial deinitializing move: ARC 
`__strong` pointers.  What we don't have is anything in IRGen that currently 
would take advantage of that fact.  Anyway, I agree that we can wait to start 
tracking this until we add such code to the compiler.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+TrivialFieldIsVolatile |= FT.isVolatileQualified();
+if (Start == End)

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > I feel like maybe volatile fields should be individually copied instead 
> > > > of being aggregated into a multi-field memcpy.  This is a more natural 
> > > > interpretation of the C volatile rules than we currently do.  In fact, 
> > > > arguably we should really add a PrimitiveCopyKind enumerator for 
> > > > volatile fields (that are otherwise trivially-copyable) and force all 
> > > > copies of structs with volatile fields into this path precisely so that 
> > > > we can make a point of copying the volatile fields this way.  
> > > > (Obviously that part is not something that's your responsibility to do.)
> > > > 
> > > > To get that right with bit-fields, you'll need to propagate the actual 
> > > > FieldDecl down.  On the plus side, that should let you use 
> > > > EmitLValueForField to do the field projection in the common case.
> > > I added method visitVolatileTrivial that copies volatile fields 
> > > individually. Please see test case test_copy_constructor_Bitfield1 in 
> > > test/CodeGenObjC/strong-in-c-struct.m.
> > Okay, great!  I like the name.
> > 
> > Does this mean we're now copying all structs that contain volatile fields 
> > with one of these helper functions?  If so, please add a C test case 
> > testing just that.  Also, you should retitle this review and stress that 
> > we're changing how *all* non-trivial types are copied, and that that 
> > includes both volatile and ARC-qualified fields.
> No, the current patch doesn't copy volatile fields of a struct individually 
> unless the struct is a non-trivial type (which means its primitive copy kind 
> is PCK_Struct). I'll look into today how I can force structs with volatile 
> fields that are not non-trivial to be copied using the helper functions.
> 
> It seems like we would need a boolean flag in RecordDecl that tracks the 
> presence of volatile fields in the struct or one of its subobjects. I assume 
> we want to copy volatile fields individually in C++ too, in which case the 
> flag needs to be set in both C 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1121
+  /// after it is moved, as opposed to a truely destructive move in which the
+  /// source object is placed in an uninitialized state.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

rjmccall wrote:
> "truly"
> 
> Hmm.  Now that I'm thinking more about it, I'm not sure there's any point in 
> tracking non-triviality of a C++-style destructive move separately from the 
> non-triviality of a copy.  It's hard to imagine that there would ever be a 
> non-C++ type that primitively has non-trivial copies but trivial C++-style 
> moves or vice-versa.  Type-based destructors imply that the type represents 
> some kind of resource, and a C++-style move will always be non-trivial for 
> resource types because ownership of the resource needs to be given up by the 
> old location.  Otherwise, a type might be non-trivial to copy but not destroy 
> because there's something special about how it's stored (like volatility), 
> but it's hard to imagine what could possibly cause it to be non-trivial to 
> destroy but not copy.
> 
> If we were tracking non-triviality of an *unsafe* destructive move, one that 
> leaves the source in an uninitialized state, that's quite different.
> 
> I think there are three reasonable options here:
> 
> - Ignore the argument I just made about the types that we're *likely* to care 
> about modeling and generalize your tracking to also distinguish construction 
> from assignment.  In such an environment, I think you can absolutely make an 
> argument that it's still interesting to track C++-style moves separately from 
> copies.
> 
> - Drop the tracking of destructive moves completely.  If you want to keep the 
> method around, find, but it can just call `isNonTrivialToPrimitiveCopy()`.
> 
> - Change the tracking of *destructive* moves to instead track 
> *deinitializing* moves.  The implementation would stop considering `__strong` 
> types to be non-trivial to move.
> 
> But as things stand today, I do not see any point in separately tracking 
> triviality of C++-style destructive moves.
The second option seems most reasonable to me. We can always make changes if 
someone comes up with a type that requires tracking destructive moves 
separately.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+TrivialFieldIsVolatile |= FT.isVolatileQualified();
+if (Start == End)

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > I feel like maybe volatile fields should be individually copied instead 
> > > of being aggregated into a multi-field memcpy.  This is a more natural 
> > > interpretation of the C volatile rules than we currently do.  In fact, 
> > > arguably we should really add a PrimitiveCopyKind enumerator for volatile 
> > > fields (that are otherwise trivially-copyable) and force all copies of 
> > > structs with volatile fields into this path precisely so that we can make 
> > > a point of copying the volatile fields this way.  (Obviously that part is 
> > > not something that's your responsibility to do.)
> > > 
> > > To get that right with bit-fields, you'll need to propagate the actual 
> > > FieldDecl down.  On the plus side, that should let you use 
> > > EmitLValueForField to do the field projection in the common case.
> > I added method visitVolatileTrivial that copies volatile fields 
> > individually. Please see test case test_copy_constructor_Bitfield1 in 
> > test/CodeGenObjC/strong-in-c-struct.m.
> Okay, great!  I like the name.
> 
> Does this mean we're now copying all structs that contain volatile fields 
> with one of these helper functions?  If so, please add a C test case testing 
> just that.  Also, you should retitle this review and stress that we're 
> changing how *all* non-trivial types are copied, and that that includes both 
> volatile and ARC-qualified fields.
No, the current patch doesn't copy volatile fields of a struct individually 
unless the struct is a non-trivial type (which means its primitive copy kind is 
PCK_Struct). I'll look into today how I can force structs with volatile fields 
that are not non-trivial to be copied using the helper functions.

It seems like we would need a boolean flag in RecordDecl that tracks the 
presence of volatile fields in the struct or one of its subobjects. I assume we 
want to copy volatile fields individually in C++ too, in which case the flag 
needs to be set in both C and C++ mode. Is that right?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1108
+PCK_ARCStrong,  // objc strong pointer.
+PCK_Struct   // non-trivial C struct.
+  };

These should all be /// documentation comments, and they mostly shouldn't talk 
about fields since this is a query on QualType, not FieldDecl.  I would suggest 
something like:

for Trivial - The type does not fall into any of the following categories.  
Note that this case is zero-valued so that values of this enum can be used as a 
boolean condition for non-triviality.

for VolatileTrivial - The type would be trivial except that it is 
volatile-qualified.  Types that fall into one of the other non-trivial cases 
may additionally be volatile-qualified.

for ARCStrong - The type is an Objective-C retainable pointer type that is 
qualified with the ARC __strong qualifier.

for Struct - The type is a struct containing a field whose type is not 
PCK_Trivial.  Note that a C++ struct type does not necessarily match this; C++ 
copying semantics are too complex to express here, in part because they depend 
on the exact constructor or assignment operator that is chosen by overload 
resolution to do the copy.



Comment at: include/clang/AST/Type.h:1121
+  /// after it is moved, as opposed to a truely destructive move in which the
+  /// source object is placed in an uninitialized state.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

"truly"

Hmm.  Now that I'm thinking more about it, I'm not sure there's any point in 
tracking non-triviality of a C++-style destructive move separately from the 
non-triviality of a copy.  It's hard to imagine that there would ever be a 
non-C++ type that primitively has non-trivial copies but trivial C++-style 
moves or vice-versa.  Type-based destructors imply that the type represents 
some kind of resource, and a C++-style move will always be non-trivial for 
resource types because ownership of the resource needs to be given up by the 
old location.  Otherwise, a type might be non-trivial to copy but not destroy 
because there's something special about how it's stored (like volatility), but 
it's hard to imagine what could possibly cause it to be non-trivial to destroy 
but not copy.

If we were tracking non-triviality of an *unsafe* destructive move, one that 
leaves the source in an uninitialized state, that's quite different.

I think there are three reasonable options here:

- Ignore the argument I just made about the types that we're *likely* to care 
about modeling and generalize your tracking to also distinguish construction 
from assignment.  In such an environment, I think you can absolutely make an 
argument that it's still interesting to track C++-style moves separately from 
copies.

- Drop the tracking of destructive moves completely.  If you want to keep the 
method around, find, but it can just call `isNonTrivialToPrimitiveCopy()`.

- Change the tracking of *destructive* moves to instead track *deinitializing* 
moves.  The implementation would stop considering `__strong` types to be 
non-trivial to move.

But as things stand today, I do not see any point in separately tracking 
triviality of C++-style destructive moves.



Comment at: lib/AST/Type.cpp:2231
+
+  Qualifiers::ObjCLifetime Lifetime = getQualifiers().getObjCLifetime();
+  if (Lifetime == Qualifiers::OCL_Strong)

You can re-use this call to getQualifiers() in the check for `volatile` below.



Comment at: lib/AST/Type.cpp:3945
+if (RT->getDecl()->isNonTrivialToPrimitiveDestroy())
+  return DK_nontrivial_c_struct;
+

Please combine this check with the getAsCXXRecordDecl() check below.  That is, 
(1) check whether it's a record type once, and if so, (2) it's a CXXRecordDecl, 
ask about its destructor, and (3) if it's not a CXXRecordDecl, ask whether it's 
non-trivial to primitive destroy.



Comment at: lib/CodeGen/CGBlocks.cpp:1779
+break;
   }
 

This entire switch can be within an `#ifndef NDEBUG` if you really feel it's 
important to assert.



Comment at: lib/CodeGen/CGBlocks.cpp:1792
+return std::make_pair(BlockCaptureEntityKind::BlockObject,
+  getBlockFieldFlagsForObjCObjectPointer(CI, T));
 

I think you should leave the byref case of that function here, but you can make 
it local to this if-clause.



Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

rjmccall wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > This can only be getARCCleanupKind() if it's non-trivial to destroy 
> > > > solely because of __strong members.  Since the setup here is 
> > > > significantly more general than ARC, I think you need to default to 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 135309.
ahatanak marked 14 inline comments as done.
ahatanak added a comment.

Address review comments.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/nontrivial-c-struct-exception.m
  test/CodeGenObjC/nontrivial-c-struct-func-name-collision.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void has_objc_arc_feature();
 // CHECK-ARCLITE: void 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1423
+  if (capturedByInit)
+drillIntoBlockVariable(*this, lvalue, cast(D));
+

rjmccall wrote:
> The "delay" is that we generally handle `__block` captures within 
> initializers by delaying the drill-into until we're ready to actually store 
> into the variable.  Not delaying has the advantage of allowing us to just 
> project out the stack storage instead of actually chasing the forwarding 
> pointer, but  this isn't safe if the forwarding pointer might no longer point 
> to the stack storage, which is what happens when a block capturing a 
> `__block` variable is copied to the heap.
> 
> Delaying the drill-into is easy when the stores can be completely separated 
> from the initializer, as is true for scalar and complex types.  It's not easy 
> for aggregates because we need to emit the initializer in place, but it's 
> possible that a block capturing the `__block` variable will be copied during 
> the emission of that initializer (e.g. during a later element of a 
> braced-init-list or within a C++ constructor call), meaning that we might be 
> copying an object that's only partly initialized.  This is particularly 
> dangerous if there are destructors involved.
> 
> Probably the best solution is to forbid capturing `__block` variables of 
> aggregate type within their own initializer.  It's possible that that might 
> break existing code that's not doing the specific dangerous things, though.
> 
> The more complex solution is to make IRGen force the `__block` variable to 
> the heap before actually initializing it.  (It can do this by directly 
> calling `_Block_object_assign`.) We would then initialize the heap copy 
> in-place, safely knowing that there is no possibility that it will move 
> again.  Effectively, the stack copy of the variable would never exist as an 
> object; only its header would matter, and only enough to allow the runtime to 
> "copy" it to the heap.  We'd have to hack the __block variable's copy helper 
> to not try to call the move-constructor, and we'd need to suppress the 
> cleanup that destroys the stack copy of the variable.  We already push a 
> cleanup to release the `__block` variable when it goes out of scope, and that 
> would stay.
Ah I see. I'll see if I can implement the first solution you suggested in a 
separate patch. If it turns out that doing so breaks too much code, we can 
consider implementing the second solution.

Is this something users do commonly?




Comment at: lib/CodeGen/CGExprAgg.cpp:255
+  Ty.isDestructedType() == QualType::DK_nontrivial_c_struct)
+CGF.pushDestroy(Ty.isDestructedType(), src.getAggregateAddress(), Ty);
+

rjmccall wrote:
> Hrm.  Okay, I guess.  This is specific to ignored results because otherwise 
> we assume that the caller is going to manage the lifetime of the object?
Yes, this is specific to ignored results. This is needed because otherwise 
function return values that are ignored won't get destructed by the caller.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:193
+
+TrivialFieldIsVolatile |= FT.isVolatileQualified();
+if (Start == End)

rjmccall wrote:
> I feel like maybe volatile fields should be individually copied instead of 
> being aggregated into a multi-field memcpy.  This is a more natural 
> interpretation of the C volatile rules than we currently do.  In fact, 
> arguably we should really add a PrimitiveCopyKind enumerator for volatile 
> fields (that are otherwise trivially-copyable) and force all copies of 
> structs with volatile fields into this path precisely so that we can make a 
> point of copying the volatile fields this way.  (Obviously that part is not 
> something that's your responsibility to do.)
> 
> To get that right with bit-fields, you'll need to propagate the actual 
> FieldDecl down.  On the plus side, that should let you use EmitLValueForField 
> to do the field projection in the common case.
I added method visitVolatileTrivial that copies volatile fields individually. 
Please see test case test_copy_constructor_Bitfield1 in 
test/CodeGenObjC/strong-in-c-struct.m.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Decl.h:3619
+return NonTrivialToPrimitiveDestructiveMove;
+  }
+

Please document that this means a C++-style destructive move that leaves the 
source object in a validly-initialized state.



Comment at: include/clang/AST/Type.h:1094
+PCK_Trivial, // a field of a trivial type.
+PCK_Strong,  // objc strong pointer.
+PCK_Struct   // non-trivial C struct.

Please call this ARCStrong, it'll be much clearer (and easier to visually 
distinguish from "struct').



Comment at: include/clang/AST/Type.h:1096
+PCK_Struct   // non-trivial C struct.
+  };
+

Nit-pick: please declare this with the methods that use it.



Comment at: lib/AST/ASTContext.cpp:5795
+  // These cases should have been taken care of when checking the type's
+  // non-triviality
   case Qualifiers::OCL_Weak:

Nit-pick: period.



Comment at: lib/CodeGen/CGBlocks.cpp:1770
   if (T->isBlockPointerType())
 Flags = BLOCK_FIELD_IS_BLOCK;
 

Should we just have a helper function like 
`getBlockFieldFlagsForObjCObjectPointer(QualType);`?  You could just call it 
from all the object-specific places instead of having this Flags variable 
that's not valid in half the cases.



Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > This can only be getARCCleanupKind() if it's non-trivial to destroy 
> > > solely because of __strong members.  Since the setup here is 
> > > significantly more general than ARC, I think you need to default to the 
> > > more-correct behavior; if you want to add a special-case check for only 
> > > __strong members, you ought to do that explicitly.
> > I added a DestructionKind (DK_c_struct_strong_field) that is used just for 
> > structs that have strong fields (but doesn't have weak fields).
> Sure, that's a simple enough way to do it, and I think it's fairly warranted.
Okay, since you don't have a DK_c_struct_strong_field anymore, you either need 
to stop using getARCCleanupKind() or do the test for just __strong fields.



Comment at: lib/CodeGen/CGDecl.cpp:1423
+  if (capturedByInit)
+drillIntoBlockVariable(*this, lvalue, cast(D));
+

The "delay" is that we generally handle `__block` captures within initializers 
by delaying the drill-into until we're ready to actually store into the 
variable.  Not delaying has the advantage of allowing us to just project out 
the stack storage instead of actually chasing the forwarding pointer, but  this 
isn't safe if the forwarding pointer might no longer point to the stack 
storage, which is what happens when a block capturing a `__block` variable is 
copied to the heap.

Delaying the drill-into is easy when the stores can be completely separated 
from the initializer, as is true for scalar and complex types.  It's not easy 
for aggregates because we need to emit the initializer in place, but it's 
possible that a block capturing the `__block` variable will be copied during 
the emission of that initializer (e.g. during a later element of a 
braced-init-list or within a C++ constructor call), meaning that we might be 
copying an object that's only partly initialized.  This is particularly 
dangerous if there are destructors involved.

Probably the best solution is to forbid capturing `__block` variables of 
aggregate type within their own initializer.  It's possible that that might 
break existing code that's not doing the specific dangerous things, though.

The more complex solution is to make IRGen force the `__block` variable to the 
heap before actually initializing it.  (It can do this by directly calling 
`_Block_object_assign`.) We would then initialize the heap copy in-place, 
safely knowing that there is no possibility that it will move again.  
Effectively, the stack copy of the variable would never exist as an object; 
only its header would matter, and only enough to allow the runtime to "copy" it 
to the heap.  We'd have to hack the __block variable's copy helper to not try 
to call the move-constructor, and we'd need to suppress the cleanup that 
destroys the stack copy of the variable.  We already push a cleanup to release 
the `__block` variable when it goes out of scope, and that would stay.



Comment at: lib/CodeGen/CGExprAgg.cpp:82
+  void EmitFinalDestCopy(QualType type, const LValue ,
+ bool SrcIsRValue = false);
   void EmitFinalDestCopy(QualType type, RValue src);

Please use a boolean enum for this instead of a raw bool.



Comment at: lib/CodeGen/CGExprAgg.cpp:255
+  Ty.isDestructedType() == 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1137
+return isNonTrivialToPrimitiveDestroy() == PCK_Struct;
+  }
+

rjmccall wrote:
> Are these four queries really important enough to provide API for them on 
> QualType?
I removed these functions.



Comment at: lib/CodeGen/CGDecl.cpp:1299
+return;
+  }
+

rjmccall wrote:
> Again, it would be nice to restructure this code to use the result of 
> isNonTrivialToPrimitiveDefaultInitialize() exhaustively.
It looks like this code needs restructuring, but I didn't understand how I can 
use isNonTrivialToPrimitiveDefaultInitialize() here. The only change I made 
here is that drillIntoBlockVariable is called if the variable is byref. This 
fixes a bug where a non-trivial struct variable declared as __block wasn't 
initialized correctly.

Also, in the process, I found that when a c++ struct declared as a __block is 
captured by its initializer, an assertion fails (see test case in 
test/CodeGenObjCXX/arc-blocks.mm). I fixed the bug in 
CodeGenFunction::EmitExprAsInit, but I'm not sure that's the right way to fix 
it. I'm not sure what the comment "how can we delay here if D is captured by 
its initializer" means.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:38
+  template 
+  static void visit(Visitor , QualType FT, bool IsVolatile, unsigned Offset,
+Ts... Args) {

rjmccall wrote:
> These visitors would be re-usable components outside of IRGen if they just 
> took a QualType and then forwarded an arbitrary set of other arguments.  I 
> would suggest structuring them like this:
> 
>   template  struct DestructedTypeVisitor {
> Derived () { return static_cast(*this); }
> 
> template 
> RetTy visit(QualType Ty, Ts &&...Args) {
>   return asDerived().visit(Ty.isDestructedType(), Ty, 
> std::forward(Args)...);
> }
> 
> template 
> RetTy visit(QualType::DestructionKind DK, QualType Ty, Ts &&...Args) {
>   switch (DK) {
>   case QualType::DK_none: return asDerived().visitTrivial(Ty, 
> std::forward(Args)...);
>   case QualType::DK_cxx_destructor: return 
> asDerived().visitCXXDestructor(Ty, std::forward(Args)...);
>   ...
>   }
> }
>   };
> 
> You can then pretty easily add the special behavior for IsVolatile and arrays 
> (when DK is not DK_none) in an IRGen-specific subclass.  visitArray would 
> take a DK and then recurse by calling back into visit with that same DK.
I created three visitor classes that can be used outside of IRGen,



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:55
+  break;
+default:
+  break;

rjmccall wrote:
> Better not to have a default case here.  You can add an assert for the C++ 
> case that it should never get here.
I added llvm_unreachable in StructVisitor's methods visitWeak and 
visitCXXDestructor.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 134430.
ahatanak marked 14 inline comments as done.
ahatanak added a comment.

Address review comments.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/nontrivial-c-struct-exception.m
  test/CodeGenObjC/nontrivial-c-struct-func-name-collision.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/CodeGenObjCXX/arc-blocks.mm
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1102
+PDK_Struct   // non-trivial C struct.
+  };
+

This is unused.



Comment at: lib/CodeGen/CGBlocks.cpp:1565
 // For all other types, the memcpy is fine.
 return std::make_pair(BlockCaptureEntityKind::None, Flags);
 

Please restructure this code to use the result of 
isNonTrivialToPrimitiveCopy(), preferably in a way that involves an exhaustive 
switch.  That should be as easy as breaking out in the PCK_Strong case and in 
the PCK_None case when !isObjCRetainableType().



Comment at: lib/CodeGen/CGBlocks.cpp:1773
+return std::make_pair(BlockCaptureEntityKind::NonTrivialCStruct,
+  BlockFieldFlags());
+

Same as above with using the result of isDestructedType().



Comment at: lib/CodeGen/CGBlocks.cpp:2070
+  bool needsDispose() const override {
+return VarType.isDestructedType() == QualType::DK_nontrivial_c_struct;
+  }

Just isDestructedType() should be fine.



Comment at: lib/CodeGen/CGDecl.cpp:1299
+return;
+  }
+

Again, it would be nice to restructure this code to use the result of 
isNonTrivialToPrimitiveDefaultInitialize() exhaustively.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:38
+  template 
+  static void visit(Visitor , QualType FT, bool IsVolatile, unsigned Offset,
+Ts... Args) {

These visitors would be re-usable components outside of IRGen if they just took 
a QualType and then forwarded an arbitrary set of other arguments.  I would 
suggest structuring them like this:

  template  struct DestructedTypeVisitor {
Derived () { return static_cast(*this); }

template 
RetTy visit(QualType Ty, Ts &&...Args) {
  return asDerived().visit(Ty.isDestructedType(), Ty, 
std::forward(Args)...);
}

template 
RetTy visit(QualType::DestructionKind DK, QualType Ty, Ts &&...Args) {
  switch (DK) {
  case QualType::DK_none: return asDerived().visitTrivial(Ty, 
std::forward(Args)...);
  case QualType::DK_cxx_destructor: return 
asDerived().visitCXXDestructor(Ty, std::forward(Args)...);
  ...
  }
}
  };

You can then pretty easily add the special behavior for IsVolatile and arrays 
(when DK is not DK_none) in an IRGen-specific subclass.  visitArray would take 
a DK and then recurse by calling back into visit with that same DK.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:55
+  break;
+default:
+  break;

Better not to have a default case here.  You can add an assert for the C++ case 
that it should never get here.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:92
+  }
+};
+

This entire template is dead.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:110
+  unsigned FOffset =
+  Offset + RL.getFieldOffset(FieldNo++) / Ctx.getCharWidth();
+  FieldVisitor::visit(getDerived(), FT,

Please pass around offsets as CharUnits until you actually need to interact 
with LLVM.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:131
+
+template  struct BinaryFuncStructVisitor {
+  typedef BinaryFieldVisitor FieldVisitorTy;

I think you should just call this CopyStructVisitor.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:145
+if (PCK)
+  getDerived().flushTrivialFields(Args...);
+

This would also be a good place to check for arrays so you don't have to do it 
in every non-trivial case below.

There's a couple other places in this file where I would suggest the same thing.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:289
+/// Return an address with the specified offset from the passed address.
+static Address getAddrWithOffset(Address Addr, unsigned Offset,
+ CodeGenFunction ) {

Offset should definitely be a CharUnits here.



Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:432
+// If the special function already exists in the module, return it.
+if (llvm::Function *F = CGM.getModule().getFunction(FuncName))
+  return F;

You should at least make sure this has the right function type; it's possible, 
even if not really allowed, to declare a C function that collides with one of 
these names.



Comment at: lib/CodeGen/CodeGenFunction.h:3407
+ QualType QT, bool IsVolatile);
+  void callCStructDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+

I think we're moving towards generally taking LValues instead of Addresses 
unless you're talking about a very low-level routine.


https://reviews.llvm.org/D41228

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 133994.
ahatanak marked 8 inline comments as done.
ahatanak added a comment.

Address review comments and feedback I got from John offline.

The main changes are in CGNonTrivialStruct.cpp. I cleaned up the class 
hierarchy and used variadic template functions in CGNonTrivialStruct.cpp to 
enable sharing code between the class creating the function name and the class 
emitting IR for the special functions. I also made changes so that memset is 
used instead of a loop to default-initialize a large array (see 
test_constructor_destructor_IDArray in test/CodeGenObjC/strong-in-c-struct.m).


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/nontrivial-c-struct-exception.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1148
+DK_objc_weak_lifetime,
+DK_c_struct_strong_field
   };

ahatanak wrote:
> rjmccall wrote:
> > I don't think you want to refer to the fact that the C struct specifically 
> > contains a __strong field here.  As we add more reasons, would we create a 
> > new enumerator for each?  What if a struct is non-trivial for multiple 
> > reasons?  Just say that it's a non-trivial C struct.
> I added an enumerator for DK_c_struct_strong_field since 
> CodeGenFunction::needsEHCleanup distinguishes between `__weak` and `__strong` 
> types. Is it not necessary to distinguish between a struct that has a 
> `__weak` field and a struct that has a `__strong` field but not a `__weak` 
> field? 
Oh, that's right.

I... hmm.  The problem is that that's really a special-case behavior, and we're 
designing a more general feature.  If we try to handle the special case in the 
first patch, we'll end up trivializing the general case, and that will permeate 
the design in unfortunate ways.

So I recommend that we *not* treat them separately right now; just emit 
cleanups for them unconditionally.  You're still planning to add support for 
`__weak` properties in a follow-up patch, right?  Once that's in, it will make 
more sense to carve out the `__strong`-only behavior as a special case.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1148
+DK_objc_weak_lifetime,
+DK_c_struct_strong_field
   };

rjmccall wrote:
> I don't think you want to refer to the fact that the C struct specifically 
> contains a __strong field here.  As we add more reasons, would we create a 
> new enumerator for each?  What if a struct is non-trivial for multiple 
> reasons?  Just say that it's a non-trivial C struct.
I added an enumerator for DK_c_struct_strong_field since 
CodeGenFunction::needsEHCleanup distinguishes between `__weak` and `__strong` 
types. Is it not necessary to distinguish between a struct that has a `__weak` 
field and a struct that has a `__strong` field but not a `__weak` field? 


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Decl.h:3529
+  /// Basic properties of non-trivial C structs.
+  bool HasStrongObjCPointer : 1;
+

Is it interesting to track all the individual reasons that a struct is 
non-trivial at the struct level, as opposed to (like we do with 
CXXDefinitionData) just tracking the four aggregate properties you've described 
below?



Comment at: include/clang/AST/Type.h:1098
+  /// and return the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDefaultInitialize() const;
+

I think this one should probably get its own enum.  It's not too hard to 
imagine types (maybe relative references, or something else that's 
address-sensitive?) that require non-trivial copy semantics but isn't 
non-trivial to default-initialize.



Comment at: include/clang/AST/Type.h:1108
+  /// move and return the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;
+

You need to define what you mean by "destructive move":

- A C++-style move leaves the source in a still-initialized state, just 
potentially with a different value (such as nil for a __strong reference).  
This is what we would do when e.g. loading from an r-value reference in C++.  
It's also what we have to do when moving a __block variable to the heap, since 
there may still be pointers to the stack copy of the variable, and since the 
compiler will unconditionally attempt to destroy that stack copy when it goes 
out of scope.  Since the source still needs to be destroyed, a 
non-trivially-copyable type will probably always have to do non-trivial work to 
do this.  Because of that, a function for this query could reasonably just 
return PrimitiveCopyKind.

- A truly primitive destructive move is something that actually leaves the 
source uninitialized.  This is generally only possible in narrow circumstances 
where the compiler can guarantee that the previous value is never again going 
to be referenced, including to destroy it.  I don't know if you have a reason 
to track this at all (i.e. whether there are copies you want to perform with a 
destructive move in IRGen), but if you do, you should use a different return 
type, because many types that are non-trivial to "C++-style move" are trivial 
to "destructively move": for example, __strong references are trivial to 
destructively move.



Comment at: include/clang/AST/Type.h:1113
+  /// the kind.
+  PrimitiveCopyKind isNonTrivialToPrimitiveDestroy() const;
+

Is there a reason we need this in addition to isDestructedType()?  It seems to 
me that it's exactly the same except ruling out the possibility of a C++ 
destructor.



Comment at: include/clang/AST/Type.h:1137
+return isNonTrivialToPrimitiveDestroy() == PCK_Struct;
+  }
+

Are these four queries really important enough to provide API for them on 
QualType?



Comment at: include/clang/AST/Type.h:1148
+DK_objc_weak_lifetime,
+DK_c_struct_strong_field
   };

I don't think you want to refer to the fact that the C struct specifically 
contains a __strong field here.  As we add more reasons, would we create a new 
enumerator for each?  What if a struct is non-trivial for multiple reasons?  
Just say that it's a non-trivial C struct.



Comment at: lib/AST/ASTContext.cpp:5778
+  if (Ty.isNonTrivialToPrimitiveDestructiveMoveStruct() ||
+  Ty.isNonTrivialToPrimitiveDestroyStruct())
+return true;

I think you should use the non-struct-specific checks here.  In addition to 
being cleaner, it would also let you completely short-circuit the rest of this 
function in ARC mode.  In non-ARC mode, you do still have to check lifetime 
(only to see if it's __unsafe_unretained; the __strong and __weak cases would 
be unreachable), and you need the type-specific special cases.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

In EmitCallArg and EmitParmDecl, use the existing code for Microsoft C++ ABI to 
determine whether the argument should be destructed in the callee. Also, add a 
test case that checks the destructor for a non-trivial C struct is called on 
the EH path.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/ASTContext.h
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/nontrivial-c-struct-exception.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[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:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > ahatanak wrote:
> > > > rjmccall wrote:
> > > > > We don't actually distinguish arrays in DestructionKind.  Is it 
> > > > > important here?  You can't actually do anything with that information.
> > > > > 
> > > > > Regarding your naming question, I wonder if it would be useful to 
> > > > > just invent a new term here, something meaning "non-trivial" but 
> > > > > without the C++ baggage.  Maybe just "PrimitiveCopyKind", with the 
> > > > > understanding that C++ can never do a "primitive" copy?  And for 
> > > > > consistency with DestructionKind, maybe you should lowercase the 
> > > > > second words.
> > > > > 
> > > > > I'm not sure how isNonTrivialToCopy is supposed to be used.  Where 
> > > > > does the function come from?  And why is a context required when it 
> > > > > isn't required for isDestructedType?
> > > > Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to 
> > > > distinguish the different types of fields in a C struct. Currently, 
> > > > there are four types that are handled by visitField functions in 
> > > > CGNonTrivialStruct.cpp:
> > > > 
> > > > - struct field
> > > > - array field
> > > > - __strong field
> > > > - trivial field that can be copied using memcpy
> > > > 
> > > > I thought using enums would make the code easier to read, but of course 
> > > > it's possible to remove them and instead just check the field types 
> > > > calling functions like getAsArrayType or getAs. ASTContext 
> > > > is passed to isNonTrivialToCopy so that it can call 
> > > > ASTContext::getAsArrayType to determine whether the type is an array. 
> > > > Maybe there is another way to detect an array that doesn't require 
> > > > ASTContext?
> > > > 
> > > > As for the naming, PrimitiveCopyKind seems find if we want to 
> > > > distinguish it from C++ non-trivial classes (I don't have a better 
> > > > name). If we are going to call non-trivial C structs primitiveCopyKind, 
> > > > what should we call the C structs that are trivial (those that can be 
> > > > copied using memcpy)?
> > > I think "trivial" is a reasonable kind of primitive-copy semantics.  
> > > Basically, we're saying that all complete object types other than 
> > > non-trivial C++ types have some sort of primitive-copy semantics, and 
> > > this enum tells us what those semantics are.
> > > 
> > > DestructionKind has to deal with arrays as well, but it does so by just 
> > > reporting the appropriate value for the underlying element type without 
> > > mentioning that it's specifically an *array* of the type.  I think that's 
> > > a reasonable design, since most places do not need to distinguish arrays 
> > > from non-arrays.  IRGen does, but only when you're actually finally 
> > > emitting code; prior to that (when e.g. deciding that a field is 
> > > non-trivial to copy) you can just use the enum value.
> > > 
> > > You can do without getAsArrayType the same way that isDestructedType() 
> > > does: the more complex operation is only necessary in order to preserve 
> > > qualifiers on the element type, but that's unnecessary for this operation 
> > > because the array type itself is considered to have the same qualifiers 
> > > as its element type.  That is, you can ask the original type whether it's 
> > > __strong/__weak-qualified without having to specifically handle array 
> > > types, and once you've done all of those queries, you can use the 
> > > "unsafe" operations to drill down to the element type because you no 
> > > longer care about qualification.
> > So QualType::hasNonTrivialToDefaultInitializeStruct should be renamed to 
> > QualType::hasPrimitiveDefaultInitializeStruct and 
> > RecordDecl::isNonTrivialToPrimitiveDefaultInitialize to 
> > RecordDecl::isPrimitiveDefaultInitialize, for example?
> > 
> > Also, I realized that, after I made the changes that removed the NonTrivial 
> > flags from RecordDecl, CGNonTrivialStruct.cpp is the only user of enum 
> > NonTrivialCopyKind and the following methods of QualType 
> > (Sema::isValidVarArgType calls nonTrivialToDestroy, but it should call 
> > methods like hasNonTrivialToDestroyStruct that detect non-trivial C structs 
> > instead):
> > 
> > nonTrivialToDefaultInitialize
> > nonTrivialToCopy
> > nonTrivialToDestructiveMove
> > nonTrivialToDestroy
> > 
> > Maybe we should make these enums and functions local to 
> > CGNonTrivialStruct.cpp to avoid adding something that is not used outside 
> > CGNonTrivialStruct.cpp?
> I think the right name would be isNonTrivialToPrimitiveDefaultInitialize(), 
> and like isDestructedType() it would return an enum that happens to have a 
> zero value (and is thus false) for the trivial case.
> 
> I 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

Rename functions. Check whether a non-trivial type passed to 
Sema::isValidVarArgType is a struct.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void has_objc_arc_feature();
 // CHECK-ARCLITE: void no_objc_arc_weak_feature();
+// CHECK-ARCLITE: void has_objc_arc_fields();
Index: 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > We don't actually distinguish arrays in DestructionKind.  Is it 
> > > > important here?  You can't actually do anything with that information.
> > > > 
> > > > Regarding your naming question, I wonder if it would be useful to just 
> > > > invent a new term here, something meaning "non-trivial" but without the 
> > > > C++ baggage.  Maybe just "PrimitiveCopyKind", with the understanding 
> > > > that C++ can never do a "primitive" copy?  And for consistency with 
> > > > DestructionKind, maybe you should lowercase the second words.
> > > > 
> > > > I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does 
> > > > the function come from?  And why is a context required when it isn't 
> > > > required for isDestructedType?
> > > Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to 
> > > distinguish the different types of fields in a C struct. Currently, there 
> > > are four types that are handled by visitField functions in 
> > > CGNonTrivialStruct.cpp:
> > > 
> > > - struct field
> > > - array field
> > > - __strong field
> > > - trivial field that can be copied using memcpy
> > > 
> > > I thought using enums would make the code easier to read, but of course 
> > > it's possible to remove them and instead just check the field types 
> > > calling functions like getAsArrayType or getAs. ASTContext is 
> > > passed to isNonTrivialToCopy so that it can call 
> > > ASTContext::getAsArrayType to determine whether the type is an array. 
> > > Maybe there is another way to detect an array that doesn't require 
> > > ASTContext?
> > > 
> > > As for the naming, PrimitiveCopyKind seems find if we want to distinguish 
> > > it from C++ non-trivial classes (I don't have a better name). If we are 
> > > going to call non-trivial C structs primitiveCopyKind, what should we 
> > > call the C structs that are trivial (those that can be copied using 
> > > memcpy)?
> > I think "trivial" is a reasonable kind of primitive-copy semantics.  
> > Basically, we're saying that all complete object types other than 
> > non-trivial C++ types have some sort of primitive-copy semantics, and this 
> > enum tells us what those semantics are.
> > 
> > DestructionKind has to deal with arrays as well, but it does so by just 
> > reporting the appropriate value for the underlying element type without 
> > mentioning that it's specifically an *array* of the type.  I think that's a 
> > reasonable design, since most places do not need to distinguish arrays from 
> > non-arrays.  IRGen does, but only when you're actually finally emitting 
> > code; prior to that (when e.g. deciding that a field is non-trivial to 
> > copy) you can just use the enum value.
> > 
> > You can do without getAsArrayType the same way that isDestructedType() 
> > does: the more complex operation is only necessary in order to preserve 
> > qualifiers on the element type, but that's unnecessary for this operation 
> > because the array type itself is considered to have the same qualifiers as 
> > its element type.  That is, you can ask the original type whether it's 
> > __strong/__weak-qualified without having to specifically handle array 
> > types, and once you've done all of those queries, you can use the "unsafe" 
> > operations to drill down to the element type because you no longer care 
> > about qualification.
> So QualType::hasNonTrivialToDefaultInitializeStruct should be renamed to 
> QualType::hasPrimitiveDefaultInitializeStruct and 
> RecordDecl::isNonTrivialToPrimitiveDefaultInitialize to 
> RecordDecl::isPrimitiveDefaultInitialize, for example?
> 
> Also, I realized that, after I made the changes that removed the NonTrivial 
> flags from RecordDecl, CGNonTrivialStruct.cpp is the only user of enum 
> NonTrivialCopyKind and the following methods of QualType 
> (Sema::isValidVarArgType calls nonTrivialToDestroy, but it should call 
> methods like hasNonTrivialToDestroyStruct that detect non-trivial C structs 
> instead):
> 
> nonTrivialToDefaultInitialize
> nonTrivialToCopy
> nonTrivialToDestructiveMove
> nonTrivialToDestroy
> 
> Maybe we should make these enums and functions local to 
> CGNonTrivialStruct.cpp to avoid adding something that is not used outside 
> CGNonTrivialStruct.cpp?
I think the right name would be isNonTrivialToPrimitiveDefaultInitialize(), and 
like isDestructedType() it would return an enum that happens to have a zero 
value (and is thus false) for the trivial case.

I think we are likely to add more places that use these methods and enums.  For 
example, if we want to add a diagnostic that warns about memcpy'ing non-trivial 
struct types, that diagnostic should probably include a note about which field 
is 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > We don't actually distinguish arrays in DestructionKind.  Is it important 
> > > here?  You can't actually do anything with that information.
> > > 
> > > Regarding your naming question, I wonder if it would be useful to just 
> > > invent a new term here, something meaning "non-trivial" but without the 
> > > C++ baggage.  Maybe just "PrimitiveCopyKind", with the understanding that 
> > > C++ can never do a "primitive" copy?  And for consistency with 
> > > DestructionKind, maybe you should lowercase the second words.
> > > 
> > > I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does 
> > > the function come from?  And why is a context required when it isn't 
> > > required for isDestructedType?
> > Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to 
> > distinguish the different types of fields in a C struct. Currently, there 
> > are four types that are handled by visitField functions in 
> > CGNonTrivialStruct.cpp:
> > 
> > - struct field
> > - array field
> > - __strong field
> > - trivial field that can be copied using memcpy
> > 
> > I thought using enums would make the code easier to read, but of course 
> > it's possible to remove them and instead just check the field types calling 
> > functions like getAsArrayType or getAs. ASTContext is passed to 
> > isNonTrivialToCopy so that it can call ASTContext::getAsArrayType to 
> > determine whether the type is an array. Maybe there is another way to 
> > detect an array that doesn't require ASTContext?
> > 
> > As for the naming, PrimitiveCopyKind seems find if we want to distinguish 
> > it from C++ non-trivial classes (I don't have a better name). If we are 
> > going to call non-trivial C structs primitiveCopyKind, what should we call 
> > the C structs that are trivial (those that can be copied using memcpy)?
> I think "trivial" is a reasonable kind of primitive-copy semantics.  
> Basically, we're saying that all complete object types other than non-trivial 
> C++ types have some sort of primitive-copy semantics, and this enum tells us 
> what those semantics are.
> 
> DestructionKind has to deal with arrays as well, but it does so by just 
> reporting the appropriate value for the underlying element type without 
> mentioning that it's specifically an *array* of the type.  I think that's a 
> reasonable design, since most places do not need to distinguish arrays from 
> non-arrays.  IRGen does, but only when you're actually finally emitting code; 
> prior to that (when e.g. deciding that a field is non-trivial to copy) you 
> can just use the enum value.
> 
> You can do without getAsArrayType the same way that isDestructedType() does: 
> the more complex operation is only necessary in order to preserve qualifiers 
> on the element type, but that's unnecessary for this operation because the 
> array type itself is considered to have the same qualifiers as its element 
> type.  That is, you can ask the original type whether it's 
> __strong/__weak-qualified without having to specifically handle array types, 
> and once you've done all of those queries, you can use the "unsafe" 
> operations to drill down to the element type because you no longer care about 
> qualification.
So QualType::hasNonTrivialToDefaultInitializeStruct should be renamed to 
QualType::hasPrimitiveDefaultInitializeStruct and 
RecordDecl::isNonTrivialToPrimitiveDefaultInitialize to 
RecordDecl::isPrimitiveDefaultInitialize, for example?

Also, I realized that, after I made the changes that removed the NonTrivial 
flags from RecordDecl, CGNonTrivialStruct.cpp is the only user of enum 
NonTrivialCopyKind and the following methods of QualType 
(Sema::isValidVarArgType calls nonTrivialToDestroy, but it should call methods 
like hasNonTrivialToDestroyStruct that detect non-trivial C structs instead):

nonTrivialToDefaultInitialize
nonTrivialToCopy
nonTrivialToDestructiveMove
nonTrivialToDestroy

Maybe we should make these enums and functions local to CGNonTrivialStruct.cpp 
to avoid adding something that is not used outside CGNonTrivialStruct.cpp?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

ahatanak wrote:
> rjmccall wrote:
> > We don't actually distinguish arrays in DestructionKind.  Is it important 
> > here?  You can't actually do anything with that information.
> > 
> > Regarding your naming question, I wonder if it would be useful to just 
> > invent a new term here, something meaning "non-trivial" but without the C++ 
> > baggage.  Maybe just "PrimitiveCopyKind", with the understanding that C++ 
> > can never do a "primitive" copy?  And for consistency with DestructionKind, 
> > maybe you should lowercase the second words.
> > 
> > I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does the 
> > function come from?  And why is a context required when it isn't required 
> > for isDestructedType?
> Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to distinguish 
> the different types of fields in a C struct. Currently, there are four types 
> that are handled by visitField functions in CGNonTrivialStruct.cpp:
> 
> - struct field
> - array field
> - __strong field
> - trivial field that can be copied using memcpy
> 
> I thought using enums would make the code easier to read, but of course it's 
> possible to remove them and instead just check the field types calling 
> functions like getAsArrayType or getAs. ASTContext is passed to 
> isNonTrivialToCopy so that it can call ASTContext::getAsArrayType to 
> determine whether the type is an array. Maybe there is another way to detect 
> an array that doesn't require ASTContext?
> 
> As for the naming, PrimitiveCopyKind seems find if we want to distinguish it 
> from C++ non-trivial classes (I don't have a better name). If we are going to 
> call non-trivial C structs primitiveCopyKind, what should we call the C 
> structs that are trivial (those that can be copied using memcpy)?
I think "trivial" is a reasonable kind of primitive-copy semantics.  Basically, 
we're saying that all complete object types other than non-trivial C++ types 
have some sort of primitive-copy semantics, and this enum tells us what those 
semantics are.

DestructionKind has to deal with arrays as well, but it does so by just 
reporting the appropriate value for the underlying element type without 
mentioning that it's specifically an *array* of the type.  I think that's a 
reasonable design, since most places do not need to distinguish arrays from 
non-arrays.  IRGen does, but only when you're actually finally emitting code; 
prior to that (when e.g. deciding that a field is non-trivial to copy) you can 
just use the enum value.

You can do without getAsArrayType the same way that isDestructedType() does: 
the more complex operation is only necessary in order to preserve qualifiers on 
the element type, but that's unnecessary for this operation because the array 
type itself is considered to have the same qualifiers as its element type.  
That is, you can ask the original type whether it's __strong/__weak-qualified 
without having to specifically handle array types, and once you've done all of 
those queries, you can use the "unsafe" operations to drill down to the element 
type because you no longer care about qualification.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/Type.h:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

rjmccall wrote:
> We don't actually distinguish arrays in DestructionKind.  Is it important 
> here?  You can't actually do anything with that information.
> 
> Regarding your naming question, I wonder if it would be useful to just invent 
> a new term here, something meaning "non-trivial" but without the C++ baggage. 
>  Maybe just "PrimitiveCopyKind", with the understanding that C++ can never do 
> a "primitive" copy?  And for consistency with DestructionKind, maybe you 
> should lowercase the second words.
> 
> I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does the 
> function come from?  And why is a context required when it isn't required for 
> isDestructedType?
Enum NonTrivialCopyKind and isNonTrivialToCopy() are used just to distinguish 
the different types of fields in a C struct. Currently, there are four types 
that are handled by visitField functions in CGNonTrivialStruct.cpp:

- struct field
- array field
- __strong field
- trivial field that can be copied using memcpy

I thought using enums would make the code easier to read, but of course it's 
possible to remove them and instead just check the field types calling 
functions like getAsArrayType or getAs. ASTContext is passed to 
isNonTrivialToCopy so that it can call ASTContext::getAsArrayType to determine 
whether the type is an array. Maybe there is another way to detect an array 
that doesn't require ASTContext?

As for the naming, PrimitiveCopyKind seems find if we want to distinguish it 
from C++ non-trivial classes (I don't have a better name). If we are going to 
call non-trivial C structs primitiveCopyKind, what should we call the C structs 
that are trivial (those that can be copied using memcpy)?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:1152
+NTFK_Struct,  // non-trivial C struct.
+NTFK_Array// array that has non-trivial elements.
+  };

We don't actually distinguish arrays in DestructionKind.  Is it important here? 
 You can't actually do anything with that information.

Regarding your naming question, I wonder if it would be useful to just invent a 
new term here, something meaning "non-trivial" but without the C++ baggage.  
Maybe just "PrimitiveCopyKind", with the understanding that C++ can never do a 
"primitive" copy?  And for consistency with DestructionKind, maybe you should 
lowercase the second words.

I'm not sure how isNonTrivialToCopy is supposed to be used.  Where does the 
function come from?  And why is a context required when it isn't required for 
isDestructedType?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

- Improved IRGen for copying trivial fields in a non-trivial C struct. IRGen in 
CGNonTrivialStruct.cpp now calls a single memcpy if there are consecutive 
trivial fields in a struct (it does something similar to what FieldMemcpyizer 
does)

- IRGen for structs containing bitfields was not correct, so I fixed that too.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGExprAgg.cpp:315
+}
+  }
 }

ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Do these functions have a memcpy as a precondition?  I would expect 
> > > > them to do the full copy (for code size if nothing else).
> > > Yes, there should always be a call to memcpy before the copy/move special 
> > > functions are called.  I don't think we want to fold the call to memcpy 
> > > into the body of the special functions since another special function can 
> > > be called from the body if the non-trivial C struct has a field whose 
> > > type is a non-trivial C struct too (in which case, there will be a 
> > > redundant memcpy to copy the C struct field).
> > > 
> > > For example, in the following code, there shouldn't be a call to memcpy 
> > > to copy field "f0" of StrongOuter if there is already a memcpy that 
> > > copies struct StrongOuter:
> > > 
> > > ```
> > > typedef struct {
> > >   int f0;
> > >   id f1;
> > > } Strong;
> > > 
> > > typedef struct {
> > >   Strong f0;
> > >   id f1;
> > > } StrongOuter;
> > > ```
> > > 
> > Well, I guess I was imagining something more C++-ish where you don't 
> > necessarily have a struct-wide memcpy, and instead you just memcpy the 
> > parts where that's profitable and otherwise do something type-specific, 
> > which would mean recursing for a struct.  Your approach is reasonable if 
> > the non-trivial copying is relatively sparse and the structure is large; on 
> > the other hand, if the non-trivial copying is dense, the memcpy itself 
> > might be mostly redundant.  And it does mean a bigger code-size hit in the 
> > original place that kicks off the copy.
> > 
> > IIRC we do already have some code in copy-constructor emission that's 
> > capable of emitting sequences of field copies with a memcpy, if you wanted 
> > to try the C++ style, you could probably take advantage of that pretty 
> > easily.  But I won't hold up the patch if you think the memcpy precondition 
> > is the right way to go.
> I made changes so that either a load/store pair or a call to memcpy is 
> emitted in the copy/move special functions to copy fields that are not 
> non-trivial.
I haven't implemented the optimization you suggested which merges a sequence of 
field copies into a single memcpy. I'll look into it later.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGExprAgg.cpp:315
+}
+  }
 }

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Do these functions have a memcpy as a precondition?  I would expect them 
> > > to do the full copy (for code size if nothing else).
> > Yes, there should always be a call to memcpy before the copy/move special 
> > functions are called.  I don't think we want to fold the call to memcpy 
> > into the body of the special functions since another special function can 
> > be called from the body if the non-trivial C struct has a field whose type 
> > is a non-trivial C struct too (in which case, there will be a redundant 
> > memcpy to copy the C struct field).
> > 
> > For example, in the following code, there shouldn't be a call to memcpy to 
> > copy field "f0" of StrongOuter if there is already a memcpy that copies 
> > struct StrongOuter:
> > 
> > ```
> > typedef struct {
> >   int f0;
> >   id f1;
> > } Strong;
> > 
> > typedef struct {
> >   Strong f0;
> >   id f1;
> > } StrongOuter;
> > ```
> > 
> Well, I guess I was imagining something more C++-ish where you don't 
> necessarily have a struct-wide memcpy, and instead you just memcpy the parts 
> where that's profitable and otherwise do something type-specific, which would 
> mean recursing for a struct.  Your approach is reasonable if the non-trivial 
> copying is relatively sparse and the structure is large; on the other hand, 
> if the non-trivial copying is dense, the memcpy itself might be mostly 
> redundant.  And it does mean a bigger code-size hit in the original place 
> that kicks off the copy.
> 
> IIRC we do already have some code in copy-constructor emission that's capable 
> of emitting sequences of field copies with a memcpy, if you wanted to try the 
> C++ style, you could probably take advantage of that pretty easily.  But I 
> won't hold up the patch if you think the memcpy precondition is the right way 
> to go.
I made changes so that either a load/store pair or a call to memcpy is emitted 
in the copy/move special functions to copy fields that are not non-trivial.



Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > There's no need to put this in an anonymous namespace.
> > > 
> > > This enum makes me wonder if it would make more sense to create 
> > > equivalents to QualType::DestructionKind for each of these operations and 
> > > all of the different primitive types.  That would let you e.g. implement 
> > > your only-strong-members check above much more easily, and it would also 
> > > make it simpler to guarantee that all the places that need to 
> > > exhaustively enumerate the reasons why something might need special 
> > > initialization/copying logic don't miss an important new case.
> > I'm not sure if I understood your point here. Do you mean there should be 
> > DestructionKinds for arrays or structs too or merge these enums into 
> > DestructionKind?
> I was suggesting there should be equivalents to DestructionKind for some of 
> the other operations, like a NonTrivialCopyKind, and queries on QualType 
> analogous to isDestructedType(), e.g. isNonTrivialToCopy().  That way you can 
> directly put this stuff in the AST, and then here you can just walk the 
> struct fields and switch on the enum, at which point I think you don't need 
> FieldKind at all.  Remember that some types are non-trivial in only a subset 
> of these dimensions.
I added enum NonTrivialCopyKind and function isNonTrivialToCopy to QualType and 
removed FieldKind from CGNonTrivialStruct.cpp. Since there are special 
functions other than copy constructor/assignment operator, should the enum's 
name be NonTrivialStructKind or something instead of NonTrivialCopyKind?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments. Changed the mangling of special functions to make it a 
bit easier to read.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void has_objc_arc_feature();
 // CHECK-ARCLITE: void no_objc_arc_weak_feature();
+// CHECK-ARCLITE: void has_objc_arc_fields();
Index: 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

ahatanak wrote:
> rjmccall wrote:
> > This can only be getARCCleanupKind() if it's non-trivial to destroy solely 
> > because of __strong members.  Since the setup here is significantly more 
> > general than ARC, I think you need to default to the more-correct behavior; 
> > if you want to add a special-case check for only __strong members, you 
> > ought to do that explicitly.
> I added a DestructionKind (DK_c_struct_strong_field) that is used just for 
> structs that have strong fields (but doesn't have weak fields).
Sure, that's a simple enough way to do it, and I think it's fairly warranted.



Comment at: lib/CodeGen/CGExprAgg.cpp:315
+}
+  }
 }

ahatanak wrote:
> rjmccall wrote:
> > Do these functions have a memcpy as a precondition?  I would expect them to 
> > do the full copy (for code size if nothing else).
> Yes, there should always be a call to memcpy before the copy/move special 
> functions are called.  I don't think we want to fold the call to memcpy into 
> the body of the special functions since another special function can be 
> called from the body if the non-trivial C struct has a field whose type is a 
> non-trivial C struct too (in which case, there will be a redundant memcpy to 
> copy the C struct field).
> 
> For example, in the following code, there shouldn't be a call to memcpy to 
> copy field "f0" of StrongOuter if there is already a memcpy that copies 
> struct StrongOuter:
> 
> ```
> typedef struct {
>   int f0;
>   id f1;
> } Strong;
> 
> typedef struct {
>   Strong f0;
>   id f1;
> } StrongOuter;
> ```
> 
Well, I guess I was imagining something more C++-ish where you don't 
necessarily have a struct-wide memcpy, and instead you just memcpy the parts 
where that's profitable and otherwise do something type-specific, which would 
mean recursing for a struct.  Your approach is reasonable if the non-trivial 
copying is relatively sparse and the structure is large; on the other hand, if 
the non-trivial copying is dense, the memcpy itself might be mostly redundant.  
And it does mean a bigger code-size hit in the original place that kicks off 
the copy.

IIRC we do already have some code in copy-constructor emission that's capable 
of emitting sequences of field copies with a memcpy, if you wanted to try the 
C++ style, you could probably take advantage of that pretty easily.  But I 
won't hold up the patch if you think the memcpy precondition is the right way 
to go.



Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}

ahatanak wrote:
> rjmccall wrote:
> > There's no need to put this in an anonymous namespace.
> > 
> > This enum makes me wonder if it would make more sense to create equivalents 
> > to QualType::DestructionKind for each of these operations and all of the 
> > different primitive types.  That would let you e.g. implement your 
> > only-strong-members check above much more easily, and it would also make it 
> > simpler to guarantee that all the places that need to exhaustively 
> > enumerate the reasons why something might need special 
> > initialization/copying logic don't miss an important new case.
> I'm not sure if I understood your point here. Do you mean there should be 
> DestructionKinds for arrays or structs too or merge these enums into 
> DestructionKind?
I was suggesting there should be equivalents to DestructionKind for some of the 
other operations, like a NonTrivialCopyKind, and queries on QualType analogous 
to isDestructedType(), e.g. isNonTrivialToCopy().  That way you can directly 
put this stuff in the AST, and then here you can just walk the struct fields 
and switch on the enum, at which point I think you don't need FieldKind at all. 
 Remember that some types are non-trivial in only a subset of these dimensions.



Comment at: lib/CodeGen/CodeGenFunction.cpp:1144
+// indirectly, the callee is responsible for destructing the argument.
+if (Ty.hasNonTrivialToDestroyStruct()) {
+  AutoVarEmission Emission(*VD);

ahatanak wrote:
> rjmccall wrote:
> > You're not actually checking the "is not passed indirectly" part of this, 
> > but I think that's okay, because I don't think we actually want the 
> > ownership aspects of the ABI to vary based on how the argument is passed.  
> > So you should just strike that part from the comment.
> > 
> > Also, this should really be done in EmitParmDecl.
> To make ARC and ARC++ compatible, I think we'll have to change the ABI of C++ 
> functions that are passed structs containing weak fields if we want to 
> destruct non-trivial C arguments destructed in the callee. I guess that's OK 
> since we have to break the ABI for 

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 127456.
ahatanak marked 5 inline comments as done.
ahatanak added a comment.

Update patch.


https://reviews.llvm.org/D41228

Files:
  docs/LanguageExtensions.rst
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenFunction.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/Lexer/has_feature_objc_arc.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/Lexer/has_feature_objc_arc.m
===
--- test/Lexer/has_feature_objc_arc.m
+++ test/Lexer/has_feature_objc_arc.m
@@ -13,8 +13,16 @@
 void no_objc_arc_weak_feature();
 #endif
 
+#if __has_feature(objc_arc_fields)
+void has_objc_arc_fields();
+#else
+void no_objc_arc_fields();
+#endif
+
 // CHECK-ARC: void has_objc_arc_feature();
 // CHECK-ARC: void has_objc_arc_weak_feature();
+// CHECK-ARC: void has_objc_arc_fields();
 
 // CHECK-ARCLITE: void has_objc_arc_feature();
 // CHECK-ARCLITE: void no_objc_arc_weak_feature();
+// CHECK-ARCLITE: void has_objc_arc_fields();
Index: test/CodeGenObjC/strong-in-c-struct.m

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 9 inline comments as done.
ahatanak added a comment.

Address review comments.




Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

rjmccall wrote:
> This can only be getARCCleanupKind() if it's non-trivial to destroy solely 
> because of __strong members.  Since the setup here is significantly more 
> general than ARC, I think you need to default to the more-correct behavior; 
> if you want to add a special-case check for only __strong members, you ought 
> to do that explicitly.
I added a DestructionKind (DK_c_struct_strong_field) that is used just for 
structs that have strong fields (but doesn't have weak fields).



Comment at: lib/CodeGen/CGExprAgg.cpp:315
+}
+  }
 }

rjmccall wrote:
> Do these functions have a memcpy as a precondition?  I would expect them to 
> do the full copy (for code size if nothing else).
Yes, there should always be a call to memcpy before the copy/move special 
functions are called.  I don't think we want to fold the call to memcpy into 
the body of the special functions since another special function can be called 
from the body if the non-trivial C struct has a field whose type is a 
non-trivial C struct too (in which case, there will be a redundant memcpy to 
copy the C struct field).

For example, in the following code, there shouldn't be a call to memcpy to copy 
field "f0" of StrongOuter if there is already a memcpy that copies struct 
StrongOuter:

```
typedef struct {
  int f0;
  id f1;
} Strong;

typedef struct {
  Strong f0;
  id f1;
} StrongOuter;
```




Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}

rjmccall wrote:
> There's no need to put this in an anonymous namespace.
> 
> This enum makes me wonder if it would make more sense to create equivalents 
> to QualType::DestructionKind for each of these operations and all of the 
> different primitive types.  That would let you e.g. implement your 
> only-strong-members check above much more easily, and it would also make it 
> simpler to guarantee that all the places that need to exhaustively enumerate 
> the reasons why something might need special initialization/copying logic 
> don't miss an important new case.
I'm not sure if I understood your point here. Do you mean there should be 
DestructionKinds for arrays or structs too or merge these enums into 
DestructionKind?



Comment at: lib/CodeGen/CodeGenFunction.cpp:1144
+// indirectly, the callee is responsible for destructing the argument.
+if (Ty.hasNonTrivialToDestroyStruct()) {
+  AutoVarEmission Emission(*VD);

rjmccall wrote:
> You're not actually checking the "is not passed indirectly" part of this, but 
> I think that's okay, because I don't think we actually want the ownership 
> aspects of the ABI to vary based on how the argument is passed.  So you 
> should just strike that part from the comment.
> 
> Also, this should really be done in EmitParmDecl.
To make ARC and ARC++ compatible, I think we'll have to change the ABI of C++ 
functions that are passed structs containing weak fields if we want to destruct 
non-trivial C arguments destructed in the callee. I guess that's OK since we 
have to break the ABI for structs containing strong fields too.



Comment at: lib/CodeGen/CodeGenFunction.h:3347
+  QualType QT, bool IsVolatile);
+  void callDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+

rjmccall wrote:
> These methods should *definitely* be somehow namespaced for your new feature.
I renamed these methods to make it clear that they are calling special 
functions for C structs. Is that what you mean by "namespaced"?


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I accidentally hit 'send' too early on my review, so here's part two.  I still 
haven't fully reviewed the new IRGen file.




Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+  FK_Array   // array that has non-trivial elements.
+};
+}

There's no need to put this in an anonymous namespace.

This enum makes me wonder if it would make more sense to create equivalents to 
QualType::DestructionKind for each of these operations and all of the different 
primitive types.  That would let you e.g. implement your only-strong-members 
check above much more easily, and it would also make it simpler to guarantee 
that all the places that need to exhaustively enumerate the reasons why 
something might need special initialization/copying logic don't miss an 
important new case.



Comment at: lib/CodeGen/CodeGenFunction.cpp:1144
+// indirectly, the callee is responsible for destructing the argument.
+if (Ty.hasNonTrivialToDestroyStruct()) {
+  AutoVarEmission Emission(*VD);

You're not actually checking the "is not passed indirectly" part of this, but I 
think that's okay, because I don't think we actually want the ownership aspects 
of the ABI to vary based on how the argument is passed.  So you should just 
strike that part from the comment.

Also, this should really be done in EmitParmDecl.



Comment at: lib/CodeGen/CodeGenFunction.h:3347
+  QualType QT, bool IsVolatile);
+  void callDestructor(Address DstPtr, QualType QT, bool IsVolatile);
+

These methods should *definitely* be somehow namespaced for your new feature.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

You should add a __has_feature check for this 
(__has_feature(objc_arc_fields)?), as well as documentation.  The documentation 
would go in the ARC spec.




Comment at: include/clang/AST/Decl.h:3575
+  /// Functions to query basic properties of non-trivial C structs.
+  bool nonTrivialToDefaultInitialize() const {
+return NonTrivialToDefaultInitialize;

Naming style would be "isNonTrivialToDefualtInitialize" and so on for all of 
these.

If these aren't guaranteed to be consistent with the C++ definitions (and the 
copying one almost certainly isn't, since there isn't a single definition of 
copying in C++), I think you probably need to name these in a way that makes it 
clear that it's the C bit that's being queried/changed.  Perhaps adding 
"Primitive" to each one, e.g. isNonTrivialToPrimitiveCopy?  Richard is probably 
opinionated about this.



Comment at: lib/AST/ASTContext.cpp:5780
+  // Return true if Ty is a non-trivial C struct type that is non-trivial to
+  // destructly move or destroy.
+  if (Ty.hasNonTrivialToDestructiveMoveStruct() ||

"Return true" is just a description of the code that follows.  I would suggest 
something like "The block needs copy/destroy helpers if...".



Comment at: lib/CodeGen/CGBlocks.cpp:2244
+CGM, byrefInfo, NonTrivialCStructByrefHelpers(valueAlignment, type));
+
   // Otherwise, if we don't have a retainable type, there's nothing to do.

You've updated the code for __block captures, but I think you missed 
computeBlockInfo for direct captures.



Comment at: lib/CodeGen/CGDecl.cpp:1421
+destroyer = CodeGenFunction::destroyNonTrivialCStruct;
+cleanupKind = getARCCleanupKind();
+break;

This can only be getARCCleanupKind() if it's non-trivial to destroy solely 
because of __strong members.  Since the setup here is significantly more 
general than ARC, I think you need to default to the more-correct behavior; if 
you want to add a special-case check for only __strong members, you ought to do 
that explicitly.



Comment at: lib/CodeGen/CGExprAgg.cpp:315
+}
+  }
 }

Do these functions have a memcpy as a precondition?  I would expect them to do 
the full copy (for code size if nothing else).



Comment at: lib/CodeGen/CMakeLists.txt:73
   CodeGenAction.cpp
+  CodeGenCStruct.cpp
   CodeGenFunction.cpp

I think CGNonTrivialStruct.cpp would be a better name.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

Insert an underscore before src-alignment in the BNF rule for alignment-info.

 ::=  ["_" ]


https://reviews.llvm.org/D41228

Files:
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenCStruct.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/CodeGenObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/CodeGenObjC/strong-in-c-struct.m
@@ -0,0 +1,495 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -emit-llvm -o - %s | FileCheck %s
+
+typedef void (^BlockTy)(void);
+
+typedef struct {
+  int f0;
+  id f1;
+} Strong;
+
+typedef struct {
+  Strong f0;
+  id f1;
+} StrongOuter;
+
+typedef struct {
+  int f0;
+  volatile id f1;
+} StrongVolatile;
+
+typedef struct {
+  BlockTy f0;
+} StrongBlock;
+
+typedef struct {
+  int i;
+  id f0[2][2];
+} IDArray;
+
+typedef struct {
+  double d;
+  Strong f0[2][2];
+} StructArray;
+
+Strong getStrong(void);

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

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

Fixed a bug where the code gets stuck in an infinite loop when a struct with an 
array field is passed to FieldInfoToString. Encoded the offset of an array in a 
struct into the function name. Also, added comments that describe the structure 
of the mangled function name.


https://reviews.llvm.org/D41228

Files:
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenCStruct.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/CodeGenObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/CodeGenObjC/strong-in-c-struct.m
@@ -0,0 +1,495 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -emit-llvm -o - %s | FileCheck %s
+
+typedef void (^BlockTy)(void);
+
+typedef struct {
+  int f0;
+  id f1;
+} Strong;
+
+typedef struct {
+  Strong f0;
+  id f1;
+} StrongOuter;
+
+typedef struct {
+  int f0;
+  volatile id f1;
+} StrongVolatile;
+
+typedef struct {
+  

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I just found that the code that creates the mangled name for a special function 
is not correct. Two structs with different record layouts can end up having 
functions that have the same name.

I'll fix the bug and update the patch today.


https://reviews.llvm.org/D41228



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

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: rjmccall, doug.gregor, rsmith.
Herald added a subscriber: mgorny.

ObjectiveC ARC in C mode currently disallows having __strong and __weak 
pointers in structs. This patch takes the first step towards lifting that 
restriction. In order to properly manage the lifetimes of the objects owned by 
the structs, this patch modifies IRGen to synthesize special functions for 
structs with __strong pointers and call them when those structs have to be 
initialized, copied, and destructed similarly to what C++ special member 
functions of non-trivial classes do.

I plan to send a patch that allows __weak pointers in structs after this patch 
is committed.


https://reviews.llvm.org/D41228

Files:
  include/clang/AST/Decl.h
  include/clang/AST/Type.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/Decl.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExprAgg.cpp
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenCStruct.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Sema/JumpDiagnostics.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/ARCMT/checking.m
  test/CodeGenObjC/strong-in-c-struct.m
  test/SemaObjC/arc-decls.m
  test/SemaObjC/arc-system-header.m
  test/SemaObjC/strong-in-c-struct.m

Index: test/SemaObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/SemaObjC/strong-in-c-struct.m
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -fobjc-arc -fblocks  -fobjc-runtime=ios-11.0 -fsyntax-only -verify %s
+
+typedef struct {
+  id a;
+} Strong;
+
+void callee_variadic(const char *, ...);
+
+void test_variadic(void) {
+  Strong t;
+  callee_variadic("s", t); // expected-error {{cannot pass non-trivial C object of type 'Strong' by value to variadic function}}
+}
+
+void test_jump0(int cond) {
+  switch (cond) {
+  case 0:
+;
+Strong x; // expected-note {{jump bypasses initialization of variable of non-trivial C struct type}}
+break;
+  case 1: // expected-error {{cannot jump from switch statement to this case label}}
+x.a = 0;
+break;
+  }
+}
+
+void test_jump1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+typedef void (^BlockTy)(void);
+void func(BlockTy);
+void func2(Strong);
+
+void test_block_scope0(int cond) {
+  Strong x; // expected-note {{jump enters lifetime of block which captures a C struct that is non-trivial to destroy}}
+  switch (cond) {
+  case 0:
+func(^{ func2(x); });
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_block_scope1(void) {
+  static void *ips[] = { & };
+L0:  // expected-note {{possible target of indirect goto}}
+  ;
+  Strong x; // expected-note {{jump exits scope of variable with non-trivial destructor}} expected-note {{jump exits lifetime of block which captures a C struct that is non-trivial to destroy}}
+  func(^{ func2(x); });
+  goto *ips; // expected-error {{cannot jump}}
+}
Index: test/SemaObjC/arc-system-header.m
===
--- test/SemaObjC/arc-system-header.m
+++ test/SemaObjC/arc-system-header.m
@@ -23,8 +23,7 @@
 }
 
 void test5(struct Test5 *p) {
-  p->field = 0; // expected-error {{'field' is unavailable in ARC}}
-// expected-note@arc-system-header.h:25 {{field has non-trivial ownership qualification}}
+  p->field = 0;
 }
 
 id test6() {
@@ -49,8 +48,7 @@
 
 extern void doSomething(Test9 arg);
 void test9() {
-Test9 foo2 = {0, 0}; // expected-error {{'field' is unavailable in ARC}}
- // expected-note@arc-system-header.h:56 {{field has non-trivial ownership qualification}}
+Test9 foo2 = {0, 0};
 doSomething(foo2);
 }
 #endif
Index: test/SemaObjC/arc-decls.m
===
--- test/SemaObjC/arc-decls.m
+++ test/SemaObjC/arc-decls.m
@@ -3,7 +3,7 @@
 // rdar://8843524
 
 struct A {
-id x; // expected-error {{ARC forbids Objective-C objects in struct}}
+id x;
 };
 
 union u {
@@ -13,7 +13,7 @@
 @interface I {
struct A a; 
struct B {
-id y[10][20]; // expected-error {{ARC forbids Objective-C objects in struct}}
+id y[10][20];
 id z;
} b;
 
@@ -23,7 +23,7 @@
 
 // rdar://10260525
 struct r10260525 {
-  id (^block) (); // expected-error {{ARC forbids blocks in struct}}
+  id (^block) ();
 };
 
 struct S { 
Index: test/CodeGenObjC/strong-in-c-struct.m
===
--- /dev/null
+++ test/CodeGenObjC/strong-in-c-struct.m
@@ -0,0 +1,483 @@