[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.
Herald added a subscriber: ributzka.

Okay, now that I understand the source-compatibility issues a little better, I 
think this approach is probably okay.  If it causes trouble, we can consider 
special-casing these headers to treat the member as `__unsafe_unretained` 
implicitly — the special case isn't great, but it's better than the potential 
unsoundness.




Comment at: include/clang/AST/ASTContext.h:2060
+  /// attr::ObjCOwnership implies an ownership qualifier was explicitly
+  /// specified rather than being added implicitly by the compiler.
+  bool isObjCOwnershipAttributedType(QualType Ty) const;

How about something like: "Return true if the type has been explicitly 
qualified with ObjC ownership.  A type may be implicitly qualified with 
ownership under ObjC ARC, and in some cases the compiler treats these 
differently".

Could you look for other places where we look for an explicit qualifier?  I'm 
pretty sure I remember that happening once or twice.



Comment at: lib/Sema/SemaDecl.cpp:11176
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);

Can we make these exceptions only apply to the attributes we synthesize rather 
than arbitrary uses of `__attribute__((unavailable))`?  These aren't really 
good semantics in general.

You can do the check in a well-named function like 
`isSuppressedNonTrivialMember`, which would be a good place for a comment 
explaining what's going on here and why this seemed like the most reasonable 
solution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

r365985 stopped marking those fields as unavailable, which caused the union's 
NonTrivialToPrimitive* bits to be set to true. This patch restores the behavior 
prior to r365985.


Repository:
  rC Clang

https://reviews.llvm.org/D65256

Files:
  lib/Sema/SemaDecl.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m


Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak 
-verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak 
-I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial 
to default-initialize}} expected-note 36 {{'U0' has subobjects that are 
non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are 
non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial 
to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is 
non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is 
non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where 
it causes an lvalue-to-rvalue conversion since it is a union that is 
non-trivial to destruct}} // expected-error {{cannot use volatile type 
'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a 
union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use 
type 'U1_SystemHeader' for a function/method parameter since it is a union that 
is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' 
for a function/method parameter since it is a union that is non-trivial to 
copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,13 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as 
unavailable
+// unless the qualification is explicit.
+
+#pragma clang system_header
+
+typedef union {
+  id f0;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are 
non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that 
are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is 
non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is 
non-trivial to copy}}
+} U1_SystemHeader;
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16372,6 +16372,17 @@
 << FixItHint::CreateInsertion(FD->getLocation(), "*");
   QualType T = Context.getObjCObjectPointerType(FD->getType());
   FD->setType(T);
+} else if (Record && Record->isUnion() &&
+   FD->getType().hasNonTrivialObjCLifetime() &&
+   getSourceManager().isInSystemHeader(FD->getLocation()) &&
+   !getLangOpts().CPlusPlus && !FD->hasAttr() &&
+   !FD->getType()->getAs()) {
+  // For backward compatibility, fields of C unions declared in system
+  // headers that have non-trivial ObjC ownership qualifications are marked
+  // as unavailable unless the qualification is explicit.
+  FD->addAttr(UnavailableAttr::CreateImplicit(
+  Context, "", UnavailableAttr::IR_ARCFieldWithOwnership,
+  FD->getLocation()));
 } else if (getLangOpts().ObjC &&
getLangOpts().getGC() != LangOptions::NonGC &&
Record && !Record->hasObjectMember()) {


Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id'

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:16379
+   !getLangOpts().CPlusPlus && !FD->hasAttr() &&
+   !FD->getType()->getAs()) {
+  // For backward compatibility, fields of C unions declared in system

This check seems a little worrisome since we could have some other kind of 
attributed type. Is there an easy way to check specifically for a lifetime 
qualifier?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaDecl.cpp:16379
+   !getLangOpts().CPlusPlus && !FD->hasAttr() &&
+   !FD->getType()->getAs()) {
+  // For backward compatibility, fields of C unions declared in system

jordan_rose wrote:
> This check seems a little worrisome since we could have some other kind of 
> attributed type. Is there an easy way to check specifically for a lifetime 
> qualifier?
I think I can make `IsObjCOwnershipAttributedType` in SemaType.cpp a method of 
`ASTContext` or `Sema` and call it here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Check that the attributed type is an ObjC qualified type.  Do not diagnose 
fields that are unavailable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,15 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualification is explicit.
+
+#pragma clang system_header
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15456,27 +15456,11 @@
 
   // Warn about implicitly autoreleasing indirect parameters captured by blocks.
   if (const auto *PT = CaptureType->getAs()) {
-// This function finds out whether there is an AttributedType of kind
-// attr::ObjCOwnership in Ty. The existence of AttributedType of kind
-// attr::ObjCOwnership implies __autoreleasing was explicitly specified
-// rather than being added implicitly by the compiler.
-auto IsObjCOwnershipAttributedType = [](QualType Ty) {
-  while (const auto *AttrTy = Ty->getAs()) {
-if (AttrTy->getAttrKind() == attr::ObjCOwnership)
-  return true;
-
-// Peel off AttributedTypes that are not of kind ObjCOwnership.
-Ty = AttrTy->getModifiedType();
-  }
-
-  return false;
-};
-
 QualType PointeeTy = PT->getPointeeType();
 
 if (!Invalid && PointeeTy->getAs() &&
 PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing &&
-!IsObjCOwnershipAttributedType(PointeeTy)) {
+!S.Context.isObjCOwnershipAttributedType(PointeeTy)) {
   if (BuildAndDiagnose) {
 SourceLocation VarLoc = Var->getLocation();
 S.Diag(Loc, diag::warn_block_capture_autoreleasing);
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11173,7 +11173,8 @@
   << 0 << 0 << QT.getUnqualifiedType() << "";
 
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
   }
 
   void visitTrivial(QualType QT, const 

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Mark fields that don't have an explicit `__strong` qualifier as unavailable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,16 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15456,27 +15456,11 @@
 
   // Warn about implicitly autoreleasing indirect parameters captured by blocks.
   if (const auto *PT = CaptureType->getAs()) {
-// This function finds out whether there is an AttributedType of kind
-// attr::ObjCOwnership in Ty. The existence of AttributedType of kind
-// attr::ObjCOwnership implies __autoreleasing was explicitly specified
-// rather than being added implicitly by the compiler.
-auto IsObjCOwnershipAttributedType = [](QualType Ty) {
-  while (const auto *AttrTy = Ty->getAs()) {
-if (AttrTy->getAttrKind() == attr::ObjCOwnership)
-  return true;
-
-// Peel off AttributedTypes that are not of kind ObjCOwnership.
-Ty = AttrTy->getModifiedType();
-  }
-
-  return false;
-};
-
 QualType PointeeTy = PT->getPointeeType();
 
 if (!Invalid && PointeeTy->getAs() &&
 PointeeTy.getObjCLifetime() == Qualifiers::OCL_Autoreleasing &&
-!IsObjCOwnershipAttributedType(PointeeTy)) {
+!S.Context.isObjCOwnershipAttributedType(PointeeTy)) {
   if (BuildAndDiagnose) {
 SourceLocation VarLoc = Var->getLocation();
 S.Diag(Loc, diag::warn_block_capture_autoreleasing);
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11173,7 +11173,8 @@
   << 0 << 0 << QT.getUnqualifiedType() << "";
 
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
   }
 
   void visitTrivial(QualType QT, const FieldDecl *FD, bool InNonTrivialUnion) {}
@@ 

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: test/SemaObjC/Inputs/non-trivial-c-union.h:10
+  _Nonnull id f1;
+  __weak id f2;
+} U0_SystemHeader;

Rather than making an exception for explicit `__strong` fields, should we have 
an attribute that can be used to instruct the compiler not to mark the field as 
unavailable?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

These were unavailable in system headers before because otherwise we would've 
had to make them invalid.  Since these unions are no longer otherwise invalid, 
there shouldn't be a problem with allowing them in system headers, and in fact 
making the semantics vary that way seems quite problematic.  Now, specific 
*uses* in system headers might still appear to be invalid — e.g. an ObjC ivar 
of type `union { __strong id x; }` — and the right behavior is definitely that 
those use sites should be marked as invalid instead of refusing to compile the 
system header.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

In D65256#1601410 , @rjmccall wrote:

> These were unavailable in system headers before because otherwise we would've 
> had to make them invalid.  Since these unions are no longer otherwise 
> invalid, there shouldn't be a problem with allowing them in system headers, 
> and in fact making the semantics vary that way seems quite problematic.  Now, 
> specific *uses* in system headers might still appear to be invalid — e.g. an 
> ObjC ivar of type `union { __strong id x; }` — and the right behavior is 
> definitely that those use sites should be marked as invalid instead of 
> refusing to compile the system header.


That's the right answer on paper, but it's source-breaking in practice, for 
both Swift and Objective-C. On the Objective-C side, someone could have been 
using, say, `glob_t` in a copyable way in their ARC code, never touching the 
block field, and it would have been working fine. On the Swift side, we won't 
be able to import such a union at all when previously we would have.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

I'm personally still of the opinion that allowing non-trivial fields in unions 
was a mistake, but it's too late to change that as well.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D65256#1601415 , @jordan_rose wrote:

> In D65256#1601410 , @rjmccall wrote:
>
> > These were unavailable in system headers before because otherwise we 
> > would've had to make them invalid.  Since these unions are no longer 
> > otherwise invalid, there shouldn't be a problem with allowing them in 
> > system headers, and in fact making the semantics vary that way seems quite 
> > problematic.  Now, specific *uses* in system headers might still appear to 
> > be invalid — e.g. an ObjC ivar of type `union { __strong id x; }` — and the 
> > right behavior is definitely that those use sites should be marked as 
> > invalid instead of refusing to compile the system header.
>
>
> That's the right answer on paper, but it's source-breaking in practice, for 
> both Swift and Objective-C. On the Objective-C side, someone could have been 
> using, say, `glob_t` in a copyable way in their ARC code, never touching the 
> block field, and it would have been working fine. On the Swift side, we won't 
> be able to import such a union at all when previously we would have.


Sorry, am I missing something?  Such a union would've been either ill-formed or 
unavailable in ARC (depending on where it was declared) before this recent work.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added a comment.

In D65256#1601509 , @rjmccall wrote:

> Sorry, am I missing something?  Such a union would've been either ill-formed 
> or unavailable in ARC (depending on where it was declared) before this recent 
> work.


Apparently that was not the case if it was in a system header. Instead, Clang 
marked the //member// unavailable rather than the entire union.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D65256#1601510 , @jordan_rose wrote:

> In D65256#1601509 , @rjmccall wrote:
>
> > Sorry, am I missing something?  Such a union would've been either 
> > ill-formed or unavailable in ARC (depending on where it was declared) 
> > before this recent work.
>
>
> Apparently that was not the case if it was in a system header. Instead, Clang 
> marked the //member// unavailable rather than the entire union.


Ah, that's unfortunate.  It also just seems like a bug.

I guess my questions are whether we're fixing a specific source-compatibility 
problem here and, if so, whether this is the only reasonable approach for 
solving it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: include/clang/AST/ASTContext.h:2060
+  /// attr::ObjCOwnership implies an ownership qualifier was explicitly
+  /// specified rather than being added implicitly by the compiler.
+  bool isObjCOwnershipAttributedType(QualType Ty) const;

rjmccall wrote:
> How about something like: "Return true if the type has been explicitly 
> qualified with ObjC ownership.  A type may be implicitly qualified with 
> ownership under ObjC ARC, and in some cases the compiler treats these 
> differently".
> 
> Could you look for other places where we look for an explicit qualifier?  I'm 
> pretty sure I remember that happening once or twice.
I found that there is a function named `hasDirectOwnershipQualifier` in 
SemaType.cpp which also detects explicit ownership qualifiers, so I'm using 
that instead of `isObjCOwnershipAttributedType`. The difference between 
`isObjCOwnershipAttributedType` is that it detects paren types like 
`I*(__strong x)` and doesn't look through typedefs (see the test case in 
non-trivial-c-union.h).



Comment at: lib/Sema/SemaDecl.cpp:11176
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);

rjmccall wrote:
> Can we make these exceptions only apply to the attributes we synthesize 
> rather than arbitrary uses of `__attribute__((unavailable))`?  These aren't 
> really good semantics in general.
> 
> You can do the check in a well-named function like 
> `isSuppressedNonTrivialMember`, which would be a good place for a comment 
> explaining what's going on here and why this seemed like the most reasonable 
> solution.
We are ignoring unavailable fields here since they don't make the containing 
union non-trivial and we don't want the user to think that the unavailable 
field has to be a trivial field in order to fix a compile error. For example, 
without the check for `UnavailableAttr`, clang will diagnose the unavailable 
field `f0` when the following code is compiled:

```
union U3 {
  id f0 __attribute__((unavailable)); // expected-note {{f0 has type '__strong 
id' that is non-trivial to default-initialize}}
  __weak id f1; // expected-note {{f1 has type '__weak id' that is non-trivial 
to default-initialize}}
};

void test(void) {
  union U3 b; // expected-error {{cannot default-initialize an object of type 
'union U3' since it is a union that is non-trivial to default-initialize}}
}
```

In that case, does it matter whether the field is explicitly annotated 
unavailable in the source code or implicitly by the compiler?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,19 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -6027,36 +6027,6 @@
   }
 }
 
-/// Does this type have a "direct" ownership qualifier?  That is,
-/// is it written like "__strong id", as opposed to something like
-/// "typeof(foo)", where that happens to be strong?
-static bool hasDirectOwnershipQualifier(QualType type) {
-  // Fast path: no qualifier at all.
-  assert(type.getQualifiers().hasObjCLifetime());
-
-  while (true) {
-// __strong id
-if (const AttributedType *attr = dyn_cast(type)) {
-  if (attr->getAttrKind() == attr::ObjCOwnership)
-return true;
-
-  type = attr->getModifiedType();
-
-// X *__strong (...)
-} else if (const ParenType *paren = dyn_cast(type)) {
-  type = paren->getInnerType();
-
-// That's it for things we want to complain about.  In particular,
-// we do not want to look through typedefs, typeof(expr),
-// typeof(type), or any other way that the type is somehow
-// abstracted.
-} else {
-
-  return false;
-}
-  }
-}
-
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
@@ -6132,7 +6102,7 @@
   if (Qualifiers::ObjCLifetime previousLifetime
 = type.getQualifiers().getObjCLifetime()) {
 // If it's written directly, that's an error.
-if (hasDirectOwnershipQualifier(type)) {
+if (S.Context.hasDirectOwnershipQualifier(type)) {
   S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
 << type;
   return true;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15700,27 +15700,11 @@
 
   // Warn about implicitly autoreleasing indi

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/ASTContext.h:2060
+  /// attr::ObjCOwnership implies an ownership qualifier was explicitly
+  /// specified rather than being added implicitly by the compiler.
+  bool isObjCOwnershipAttributedType(QualType Ty) const;

ahatanak wrote:
> rjmccall wrote:
> > How about something like: "Return true if the type has been explicitly 
> > qualified with ObjC ownership.  A type may be implicitly qualified with 
> > ownership under ObjC ARC, and in some cases the compiler treats these 
> > differently".
> > 
> > Could you look for other places where we look for an explicit qualifier?  
> > I'm pretty sure I remember that happening once or twice.
> I found that there is a function named `hasDirectOwnershipQualifier` in 
> SemaType.cpp which also detects explicit ownership qualifiers, so I'm using 
> that instead of `isObjCOwnershipAttributedType`. The difference between 
> `isObjCOwnershipAttributedType` is that it detects paren types like 
> `I*(__strong x)` and doesn't look through typedefs (see the test case in 
> non-trivial-c-union.h).
Okay.  I don't know what the right rule here is, but I agree we should be using 
the same rule in both places.



Comment at: lib/Sema/SemaDecl.cpp:11176
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);

ahatanak wrote:
> rjmccall wrote:
> > Can we make these exceptions only apply to the attributes we synthesize 
> > rather than arbitrary uses of `__attribute__((unavailable))`?  These aren't 
> > really good semantics in general.
> > 
> > You can do the check in a well-named function like 
> > `isSuppressedNonTrivialMember`, which would be a good place for a comment 
> > explaining what's going on here and why this seemed like the most 
> > reasonable solution.
> We are ignoring unavailable fields here since they don't make the containing 
> union non-trivial and we don't want the user to think that the unavailable 
> field has to be a trivial field in order to fix a compile error. For example, 
> without the check for `UnavailableAttr`, clang will diagnose the unavailable 
> field `f0` when the following code is compiled:
> 
> ```
> union U3 {
>   id f0 __attribute__((unavailable)); // expected-note {{f0 has type 
> '__strong id' that is non-trivial to default-initialize}}
>   __weak id f1; // expected-note {{f1 has type '__weak id' that is 
> non-trivial to default-initialize}}
> };
> 
> void test(void) {
>   union U3 b; // expected-error {{cannot default-initialize an object of type 
> 'union U3' since it is a union that is non-trivial to default-initialize}}
> }
> ```
> 
> In that case, does it matter whether the field is explicitly annotated 
> unavailable in the source code or implicitly by the compiler?
Oh, I didn't realize we were already honoring the ordinary attribute in the 
ABI, but from the code it looks we do.  In that case, yeah, it makes sense to 
ignore it here.  Should we extract a function to ask whether a `FieldDecl` 
should be ignored for triviality computations so that it's more 
self-documenting that there's a consistent logic here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,19 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -6027,36 +6027,6 @@
   }
 }
 
-/// Does this type have a "direct" ownership qualifier?  That is,
-/// is it written like "__strong id", as opposed to something like
-/// "typeof(foo)", where that happens to be strong?
-static bool hasDirectOwnershipQualifier(QualType type) {
-  // Fast path: no qualifier at all.
-  assert(type.getQualifiers().hasObjCLifetime());
-
-  while (true) {
-// __strong id
-if (const AttributedType *attr = dyn_cast(type)) {
-  if (attr->getAttrKind() == attr::ObjCOwnership)
-return true;
-
-  type = attr->getModifiedType();
-
-// X *__strong (...)
-} else if (const ParenType *paren = dyn_cast(type)) {
-  type = paren->getInnerType();
-
-// That's it for things we want to complain about.  In particular,
-// we do not want to look through typedefs, typeof(expr),
-// typeof(type), or any other way that the type is somehow
-// abstracted.
-} else {
-
-  return false;
-}
-  }
-}
-
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
@@ -6132,7 +6102,7 @@
   if (Qualifiers::ObjCLifetime previousLifetime
 = type.getQualifiers().getObjCLifetime()) {
 // If it's written directly, that's an error.
-if (hasDirectOwnershipQualifier(type)) {
+if (S.Context.hasDirectOwnershipQualifier(type)) {
   S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
 << type;
   return true;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15700,27 +15700,11 @@
 
   // Warn about implicitly autoreleasing indi

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:11176
 for (const FieldDecl *FD : RD->fields())
-  asDerived().visit(FD->getType(), FD, InNonTrivialUnion);
+  if (!FD->hasAttr())
+asDerived().visit(FD->getType(), FD, InNonTrivialUnion);

rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Can we make these exceptions only apply to the attributes we synthesize 
> > > rather than arbitrary uses of `__attribute__((unavailable))`?  These 
> > > aren't really good semantics in general.
> > > 
> > > You can do the check in a well-named function like 
> > > `isSuppressedNonTrivialMember`, which would be a good place for a comment 
> > > explaining what's going on here and why this seemed like the most 
> > > reasonable solution.
> > We are ignoring unavailable fields here since they don't make the 
> > containing union non-trivial and we don't want the user to think that the 
> > unavailable field has to be a trivial field in order to fix a compile 
> > error. For example, without the check for `UnavailableAttr`, clang will 
> > diagnose the unavailable field `f0` when the following code is compiled:
> > 
> > ```
> > union U3 {
> >   id f0 __attribute__((unavailable)); // expected-note {{f0 has type 
> > '__strong id' that is non-trivial to default-initialize}}
> >   __weak id f1; // expected-note {{f1 has type '__weak id' that is 
> > non-trivial to default-initialize}}
> > };
> > 
> > void test(void) {
> >   union U3 b; // expected-error {{cannot default-initialize an object of 
> > type 'union U3' since it is a union that is non-trivial to 
> > default-initialize}}
> > }
> > ```
> > 
> > In that case, does it matter whether the field is explicitly annotated 
> > unavailable in the source code or implicitly by the compiler?
> Oh, I didn't realize we were already honoring the ordinary attribute in the 
> ABI, but from the code it looks we do.  In that case, yeah, it makes sense to 
> ignore it here.  Should we extract a function to ask whether a `FieldDecl` 
> should be ignored for triviality computations so that it's more 
> self-documenting that there's a consistent logic here?
I added function `shouldDiagnoseNonTrivialField`, which returns false if the 
field is unavailable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Could you give it a slightly more general name and then use it in the main 
semantic check in ActOnFields?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Rename function to `ignoreForTrivialityComputation`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,19 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -6027,36 +6027,6 @@
   }
 }
 
-/// Does this type have a "direct" ownership qualifier?  That is,
-/// is it written like "__strong id", as opposed to something like
-/// "typeof(foo)", where that happens to be strong?
-static bool hasDirectOwnershipQualifier(QualType type) {
-  // Fast path: no qualifier at all.
-  assert(type.getQualifiers().hasObjCLifetime());
-
-  while (true) {
-// __strong id
-if (const AttributedType *attr = dyn_cast(type)) {
-  if (attr->getAttrKind() == attr::ObjCOwnership)
-return true;
-
-  type = attr->getModifiedType();
-
-// X *__strong (...)
-} else if (const ParenType *paren = dyn_cast(type)) {
-  type = paren->getInnerType();
-
-// That's it for things we want to complain about.  In particular,
-// we do not want to look through typedefs, typeof(expr),
-// typeof(type), or any other way that the type is somehow
-// abstracted.
-} else {
-
-  return false;
-}
-  }
-}
-
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
@@ -6132,7 +6102,7 @@
   if (Qualifiers::ObjCLifetime previousLifetime
 = type.getQualifiers().getObjCLifetime()) {
 // If it's written directly, that's an error.
-if (hasDirectOwnershipQualifier(type)) {
+if (S.Context.hasDirectOwnershipQualifier(type)) {
   S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
 << type;
   return true;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15700,27 +15700,11 @@
 
   // Warn about implicitly autoreleasing indirect parameters

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.
Herald added a subscriber: ychen.



Comment at: lib/Sema/SemaDecl.cpp:11142
 
+bool ignoreForTrivialityComputation(const FieldDecl *FD) {
+  // Ignore unavailable fields since they don't affect the triviality of the

`shouldIgnoreForRecordTriviality` or something like that, please.



Comment at: lib/Sema/SemaDecl.cpp:11144
+  // Ignore unavailable fields since they don't affect the triviality of the
+  // containing struct/union.
+  return FD->hasAttr();

The "since" clause here is circular: this function *defines* what affects the 
triviality.  The comment should talk about the motivations (e.g. fields that 
aren't available in particular language modes) and why this might be okay or is 
the best-available option (despite e.g. the potential for ABI incompatibility 
across language modes),


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

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

Rename function and fix comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/SemaObjC/Inputs/non-trivial-c-union.h
  test/SemaObjC/non-trivial-c-union.m

Index: test/SemaObjC/non-trivial-c-union.m
===
--- test/SemaObjC/non-trivial-c-union.m
+++ test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- /dev/null
+++ test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,19 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -6027,36 +6027,6 @@
   }
 }
 
-/// Does this type have a "direct" ownership qualifier?  That is,
-/// is it written like "__strong id", as opposed to something like
-/// "typeof(foo)", where that happens to be strong?
-static bool hasDirectOwnershipQualifier(QualType type) {
-  // Fast path: no qualifier at all.
-  assert(type.getQualifiers().hasObjCLifetime());
-
-  while (true) {
-// __strong id
-if (const AttributedType *attr = dyn_cast(type)) {
-  if (attr->getAttrKind() == attr::ObjCOwnership)
-return true;
-
-  type = attr->getModifiedType();
-
-// X *__strong (...)
-} else if (const ParenType *paren = dyn_cast(type)) {
-  type = paren->getInnerType();
-
-// That's it for things we want to complain about.  In particular,
-// we do not want to look through typedefs, typeof(expr),
-// typeof(type), or any other way that the type is somehow
-// abstracted.
-} else {
-
-  return false;
-}
-  }
-}
-
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
@@ -6132,7 +6102,7 @@
   if (Qualifiers::ObjCLifetime previousLifetime
 = type.getQualifiers().getObjCLifetime()) {
 // If it's written directly, that's an error.
-if (hasDirectOwnershipQualifier(type)) {
+if (S.Context.hasDirectOwnershipQualifier(type)) {
   S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
 << type;
   return true;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -15700,27 +15700,11 @@
 
   // Warn about implicitly autorelea

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Sema/SemaDecl.cpp:11144
+  // Ignore unavailable fields since they don't affect the triviality of the
+  // containing struct/union.
+  return FD->hasAttr();

rjmccall wrote:
> The "since" clause here is circular: this function *defines* what affects the 
> triviality.  The comment should talk about the motivations (e.g. fields that 
> aren't available in particular language modes) and why this might be okay or 
> is the best-available option (despite e.g. the potential for ABI 
> incompatibility across language modes),
I commented on the ABI incompatibility issue in `ActOnFields` where the 
unavailable attribute is added to the field.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[John McCall via Phabricator via cfe-commits]

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

Okay, thanks.  LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65256



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

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371276: [Sema][ObjC] Mark C union fields that have 
non-trivial ObjC ownership (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65256?vs=219191&id=219206#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65256

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/SemaObjC/Inputs/non-trivial-c-union.h
  cfe/trunk/test/SemaObjC/non-trivial-c-union.m

Index: cfe/trunk/test/SemaObjC/non-trivial-c-union.m
===
--- cfe/trunk/test/SemaObjC/non-trivial-c-union.m
+++ cfe/trunk/test/SemaObjC/non-trivial-c-union.m
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fobjc-arc -fobjc-runtime-has-weak -I %S/Inputs -verify %s
+
+#include "non-trivial-c-union.h"
 
 typedef union { // expected-note 12 {{'U0' has subobjects that are non-trivial to default-initialize}} expected-note 36 {{'U0' has subobjects that are non-trivial to destruct}} expected-note 28 {{'U0' has subobjects that are non-trivial to copy}}
   id f0; // expected-note 12 {{f0 has type '__strong id' that is non-trivial to default-initialize}} expected-note 36 {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note 28 {{f0 has type '__strong id' that is non-trivial to copy}}
@@ -80,3 +82,7 @@
 void testVolatileLValueToRValue(volatile U0 *a) {
   (void)*a; // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to destruct}} // expected-error {{cannot use volatile type 'volatile U0' where it causes an lvalue-to-rvalue conversion since it is a union that is non-trivial to copy}}
 }
+
+void unionInSystemHeader0(U0_SystemHeader);
+
+void unionInSystemHeader1(U1_SystemHeader); // expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to destruct}} expected-error {{cannot use type 'U1_SystemHeader' for a function/method parameter since it is a union that is non-trivial to copy}}
Index: cfe/trunk/test/SemaObjC/Inputs/non-trivial-c-union.h
===
--- cfe/trunk/test/SemaObjC/Inputs/non-trivial-c-union.h
+++ cfe/trunk/test/SemaObjC/Inputs/non-trivial-c-union.h
@@ -0,0 +1,19 @@
+// For backward compatibility, fields of C unions declared in system headers
+// that have non-trivial ObjC ownership qualifications are marked as unavailable
+// unless the qualifier is explicit and __strong.
+
+#pragma clang system_header
+
+typedef __strong id StrongID;
+
+typedef union {
+  id f0;
+  _Nonnull id f1;
+  __weak id f2;
+  StrongID f3;
+} U0_SystemHeader;
+
+typedef union { // expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to destruct}} expected-note {{'U1_SystemHeader' has subobjects that are non-trivial to copy}}
+  __strong id f0; // expected-note {{f0 has type '__strong id' that is non-trivial to destruct}} expected-note {{f0 has type '__strong id' that is non-trivial to copy}}
+  _Nonnull id f1;
+} U1_SystemHeader;
Index: cfe/trunk/lib/AST/ASTContext.cpp
===
--- cfe/trunk/lib/AST/ASTContext.cpp
+++ cfe/trunk/lib/AST/ASTContext.cpp
@@ -7949,6 +7949,28 @@
   return false;
 }
 
+bool ASTContext::hasDirectOwnershipQualifier(QualType Ty) const {
+  while (true) {
+// __strong id
+if (const AttributedType *Attr = dyn_cast(Ty)) {
+  if (Attr->getAttrKind() == attr::ObjCOwnership)
+return true;
+
+  Ty = Attr->getModifiedType();
+
+// X *__strong (...)
+} else if (const ParenType *Paren = dyn_cast(Ty)) {
+  Ty = Paren->getInnerType();
+
+// We do not want to look through typedefs, typeof(expr),
+// typeof(type), or any other way that the type is somehow
+// abstracted.
+} else {
+  return false;
+}
+  }
+}
+
 //===--===//
 // ObjCQualifiedIdTypesAreCompatible - Compatibility testing for qualified id's.
 //===--===//
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -11255,6 +11255,15 @@
 
 namespace {
 
+bool shouldIgnoreForRecordTriviality(const FieldDecl *FD) {
+  // Ignore unavailable fields. A field can be marked as unavailable explicitly
+  // in the source