[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D41039#984009, @ahatanak wrote:

> Yes, please document this in itanium-cxx-abi. Thanks!


Looks like this hasn't happened yet.


Repository:
  rL LLVM

https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324269: Add support for attribute trivial_abi. 
(authored by ahatanak, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D41039?vs=129397=132881#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41039

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/include/clang/AST/Decl.h
  cfe/trunk/include/clang/AST/DeclCXX.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/CodeGen/CGDecl.cpp
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
  cfe/trunk/test/CodeGenCXX/trivial_abi.cpp
  cfe/trunk/test/CodeGenObjCXX/trivial_abi.mm
  cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
  cfe/trunk/test/SemaObjCXX/attr-trivial-abi.mm

Index: cfe/trunk/include/clang/AST/DeclCXX.h
===
--- cfe/trunk/include/clang/AST/DeclCXX.h
+++ cfe/trunk/include/clang/AST/DeclCXX.h
@@ -437,14 +437,25 @@
 /// which have been declared but not yet defined.
 unsigned HasTrivialSpecialMembers : 6;
 
+/// These bits keep track of the triviality of special functions for the
+/// purpose of calls. Only the bits corresponding to SMF_CopyConstructor,
+/// SMF_MoveConstructor, and SMF_Destructor are meaningful here.
+unsigned HasTrivialSpecialMembersForCall : 6;
+
 /// \brief The declared special members of this class which are known to be
 /// non-trivial.
 ///
 /// This excludes any user-declared but not user-provided special members
 /// which have been declared but not yet defined, and any implicit special
 /// members which have not yet been declared.
 unsigned DeclaredNonTrivialSpecialMembers : 6;
 
+/// These bits keep track of the declared special members that are
+/// non-trivial for the purpose of calls.
+/// Only the bits corresponding to SMF_CopyConstructor,
+/// SMF_MoveConstructor, and SMF_Destructor are meaningful here.
+unsigned DeclaredNonTrivialSpecialMembersForCall : 6;
+
 /// \brief True when this class has a destructor with no semantic effect.
 unsigned HasIrrelevantDestructor : 1;
 
@@ -1349,28 +1360,50 @@
 return data().HasTrivialSpecialMembers & SMF_CopyConstructor;
   }
 
+  bool hasTrivialCopyConstructorForCall() const {
+return data().HasTrivialSpecialMembersForCall & SMF_CopyConstructor;
+  }
+
   /// \brief Determine whether this class has a non-trivial copy constructor
   /// (C++ [class.copy]p6, C++11 [class.copy]p12)
   bool hasNonTrivialCopyConstructor() const {
 return data().DeclaredNonTrivialSpecialMembers & SMF_CopyConstructor ||
!hasTrivialCopyConstructor();
   }
 
+  bool hasNonTrivialCopyConstructorForCall() const {
+return (data().DeclaredNonTrivialSpecialMembersForCall &
+SMF_CopyConstructor) ||
+   !hasTrivialCopyConstructorForCall();
+  }
+
   /// \brief Determine whether this class has a trivial move constructor
   /// (C++11 [class.copy]p12)
   bool hasTrivialMoveConstructor() const {
 return hasMoveConstructor() &&
(data().HasTrivialSpecialMembers & SMF_MoveConstructor);
   }
 
+  bool hasTrivialMoveConstructorForCall() const {
+return hasMoveConstructor() &&
+   (data().HasTrivialSpecialMembersForCall & SMF_MoveConstructor);
+  }
+
   /// \brief Determine whether this class has a non-trivial move constructor
   /// (C++11 [class.copy]p12)
   bool hasNonTrivialMoveConstructor() const {
 return (data().DeclaredNonTrivialSpecialMembers & SMF_MoveConstructor) ||
(needsImplicitMoveConstructor() &&
 !(data().HasTrivialSpecialMembers & SMF_MoveConstructor));
   }
 
+  bool hasNonTrivialMoveConstructorForCall() const {
+return (data().DeclaredNonTrivialSpecialMembersForCall &
+SMF_MoveConstructor) ||
+   (needsImplicitMoveConstructor() &&
+!(data().HasTrivialSpecialMembersForCall & SMF_MoveConstructor));
+  }
+
   /// \brief Determine whether this class has a trivial copy assignment operator
   /// (C++ [class.copy]p11, C++11 [class.copy]p25)
   bool hasTrivialCopyAssignment() const {
@@ -1405,12 +1438,25 @@
 return data().HasTrivialSpecialMembers & SMF_Destructor;
   }
 
+ 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ahatanak marked 2 inline comments as done.
Closed by commit rC324269: Add support for attribute trivial_abi. 
(authored by ahatanak, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41039?vs=129397=132880#toc

Repository:
  rC Clang

https://reviews.llvm.org/D41039

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -2236,7 +2236,17 @@
 
   bool CheckNontrivialField(FieldDecl *FD);
   void DiagnoseNontrivial(const CXXRecordDecl *Record, CXXSpecialMember CSM);
+
+  enum TrivialABIHandling {
+/// The triviality of a method unaffected by "trivial_abi".
+TAH_IgnoreTrivialABI,
+
+/// The triviality of a method affected by "trivial_abi".
+TAH_ConsiderTrivialABI
+  };
+
   bool SpecialMemberIsTrivial(CXXMethodDecl *MD, CXXSpecialMember CSM,
+  TrivialABIHandling TAH = TAH_IgnoreTrivialABI,
   bool Diagnose = false);
   CXXSpecialMember getSpecialMember(const CXXMethodDecl *MD);
   void ActOnLastBitfield(SourceLocation DeclStart,
@@ -5796,6 +5806,11 @@
   SourceLocation BaseLoc);
 
   void CheckCompletedCXXClass(CXXRecordDecl *Record);
+
+  /// Check that the C++ class annoated with "trivial_abi" satisfies all the
+  /// conditions that are needed for the attribute to have an effect.
+  void checkIllFormedTrivialABIStruct(CXXRecordDecl );
+
   void ActOnFinishCXXMemberSpecification(Scope* S, SourceLocation RLoc,
  Decl *TagDecl,
  SourceLocation LBrac,
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2881,6 +2881,9 @@
 def err_invalid_attribute_on_virtual_function : Error<
   "%0 attribute cannot be applied to virtual functions">;
 
+def ext_cannot_use_trivial_abi : ExtWarn<
+  "'trivial_abi' cannot be applied to %0">, InGroup;
+
 // Availability attribute
 def warn_availability_unknown_platform : Warning<
   "unknown platform %0 in availability macro">, InGroup;
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2242,6 +2242,48 @@
   }];
 }
 
+def TrivialABIDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+The ``trivial_abi`` attribute can be applied to a C++ class, struct, or union.
+It instructs the compiler to pass and return the type using the C ABI for the
+underlying type when the type would otherwise be considered non-trivial for the
+purpose of calls.
+A class annotated with `trivial_abi` can have non-trivial destructors or copy/move constructors without automatically becoming non-trivial for the purposes of calls. For example:
+
+  .. code-block:: c++
+
+// A is trivial for the purposes of calls because `trivial_abi` makes the
+// user-provided special functions trivial.
+struct __attribute__((trivial_abi)) A {
+  ~A();
+  A(const A &);
+  A(A &&);
+  int x;
+};
+
+// B's destructor and copy/move constructor are considered trivial for the
+// purpose of calls because A is trivial.
+struct B {
+  A a;
+};
+
+If a type is trivial for the purposes of calls, has a non-trivial destructor,
+and is passed as an argument by value, the convention is that the callee will
+destroy the object before returning.
+
+Attribute ``trivial_abi`` has no effect in the following cases:
+
+- The class directly declares a virtual base or virtual methods.
+- The class has a base class that is non-trivial for the purposes of calls.
+- The class has a non-static data member whose type is non-trivial for the
+purposes of calls, which includes:
+ - classes that are non-trivial for the purposes of calls
+ - __weak-qualified types in Objective-C++
+ - 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.



Comment at: include/clang/Sema/Sema.h:2240-2241
+  enum TrivialityKind {
+TK_NoTrivialABI, // The triviality of a method unaffected by "trivial_abi".
+TK_TrivialABI // The triviality of a method affected by "trivial_abi".
+  };

These seem a bit opaque at call sites. How about changing this to `enum class 
TrivialABIHandling { Ignore, Consider };` or something like that?



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:863-864
+  return RAA_Default;
+// Otherwise, if the copy ctor is trivial and the object is small, pass
+// direct.
+if (CopyCtorIsTrivial &&

rsmith wrote:
> Please retain the two pre-existing "Note"s pointing out how the ABI rule here 
> is intentionally non-conforming.
This comment appears to apply to the code above it rather than the code below 
it, as does the note below.

We've still lost the *other* "non-conforming" comment, which appertains to the 
"size <= 64" case below.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Yes, please document this in itanium-cxx-abi. Thanks!


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Looks great to me!  Thanks for taking this on, it's a pretty major improvement 
for users.

Would you like to create an issue with itanium-cxx-abi to document this, or do 
you want me to handle that?


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I think I've addressed all the review comments. Any other comments from anyone?


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

In CXXRecordDecl::addedMember, set HasTrivialSpecialMembersForCall if 
Method->isTrivialForCall()  returns true. This fixes a bug where 
CXXRecordDecl::hasNonTrivialDestructorForCall would return false for the 
implicit destructor even when the parent class had attribute trivial_abi.

Run test/CodeGenObjCXX/trivial_abi.mm with  -fclang-abi-compat=4.0 to confirm 
the bug has been fixed.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+  __weak id a[2];
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+S10<__weak id> p2;
+
+template<>
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+  __weak id a;
+};
+
+template
+struct S14 {
+  T a;
+  __weak id b;
+};
+
+template
+struct __attribute__((trivial_abi)) S15 : S14 {
+};
+
+S15 s15;
+
+template
+struct __attribute__((trivial_abi)) S16 {
+  S14 a;
+};
+
+S16 s16;
+
+template
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+  __weak id a;
+};
+
+S17 s17;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenObjCXX/trivial_abi.mm

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Partially revert the changes I made to CodeGenFunction::EmitParmDecl add IRGen 
test case for exception handling.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+  __weak id a[2];
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+S10<__weak id> p2;
+
+template<>
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+  __weak id a;
+};
+
+template
+struct S14 {
+  T a;
+  __weak id b;
+};
+
+template
+struct __attribute__((trivial_abi)) S15 : S14 {
+};
+
+S15 s15;
+
+template
+struct __attribute__((trivial_abi)) S16 {
+  S14 a;
+};
+
+S16 s16;
+
+template
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+  __weak id a;
+};
+
+S17 s17;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenObjCXX/trivial_abi.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/trivial_abi.mm
@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaDecl.cpp:11700
+}
+  }
+

rsmith wrote:
> rjmccall wrote:
> > I think it's correct not to call CheckDestructorAccess and 
> > DiagnoseUseOfDecl here, since according to the standard destructor access 
> > is always supposed to be checked at the call-site, but please leave a 
> > comment explaining that.
> The corresponding code for `areArgsDestroyedLeftToRightInCallee` is in 
> `SemaChecking`. This should be done in the same place.
> 
> More generally, we have at least three different places where we check 
> `CXXABI::areArgsDestroyedLeftToRightInCallee() || 
> Type->hasTrivialABIOverride()`. It would make sense to factor that out into a 
> `isParamDestroyedInCallee` function (probably on `ASTContext`, since we don't 
> have anywhere better for ABI-specific checks at a layering level that can 
> talk about types).
I defined function ASTContext::isParamDestroyedInCallee and used it in two 
places. I didn't use it in CodeGenFunction::EmitParmDecl because the destructor 
cleanup is pushed only when the argument is passed indirectly in MSVC's case, 
whereas it is always pushed when the class is HasTrivialABIOverride.



Comment at: lib/Sema/SemaDeclCXX.cpp:7580
 
+static void checkIllFormedTrivialABIStruct(CXXRecordDecl , Sema ) {
+  auto PrintDiagAndRemoveAttr = [&]() {

rsmith wrote:
> Either "ill-formed" is the wrong word here, or this needs to produce `ext_` 
> diagnostics that `-pedantic-errors` turns into hard errors.
I made the warning derive from ExtWarn.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Add more tests for template instantiation of "trivial_abi" classes.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+  __weak id a[2];
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+S10<__weak id> p2;
+
+template<>
+struct __attribute__((trivial_abi)) S10 { // expected-warning {{'trivial_abi' cannot be applied to 'S10'}}
+  __weak id a;
+};
+
+template
+struct S14 {
+  T a;
+  __weak id b;
+};
+
+template
+struct __attribute__((trivial_abi)) S15 : S14 {
+};
+
+S15 s15;
+
+template
+struct __attribute__((trivial_abi)) S16 {
+  S14 a;
+};
+
+S16 s16;
+
+template
+struct __attribute__((trivial_abi)) S17 { // expected-warning {{'trivial_abi' cannot be applied to 'S17'}}
+  __weak id a;
+};
+
+S17 s17;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenObjCXX/trivial_abi.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/trivial_abi.mm
@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_STRONGWEAK:.*]] = type { i8*, i8* }
+// CHECK: 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I'd like to see more testing for the template instantiation case. I don't see 
any test coverage for the "attribute only affects instantiations whose members 
are trivial-for-calls" part.




Comment at: include/clang/Sema/Sema.h:2239
   bool SpecialMemberIsTrivial(CXXMethodDecl *MD, CXXSpecialMember CSM,
-  bool Diagnose = false);
+  bool ForCall = false, bool Diagnose = false);
   CXXSpecialMember getSpecialMember(const CXXMethodDecl *MD);

Please use an enum here; two bool parameters in a row is too error-prone, 
especially if both have default arguments.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:863-864
+  return RAA_Default;
+// Otherwise, if the copy ctor is trivial and the object is small, pass
+// direct.
+if (CopyCtorIsTrivial &&

Please retain the two pre-existing "Note"s pointing out how the ABI rule here 
is intentionally non-conforming.



Comment at: lib/Sema/SemaDecl.cpp:11700
+}
+  }
+

rjmccall wrote:
> I think it's correct not to call CheckDestructorAccess and DiagnoseUseOfDecl 
> here, since according to the standard destructor access is always supposed to 
> be checked at the call-site, but please leave a comment explaining that.
The corresponding code for `areArgsDestroyedLeftToRightInCallee` is in 
`SemaChecking`. This should be done in the same place.

More generally, we have at least three different places where we check 
`CXXABI::areArgsDestroyedLeftToRightInCallee() || 
Type->hasTrivialABIOverride()`. It would make sense to factor that out into a 
`isParamDestroyedInCallee` function (probably on `ASTContext`, since we don't 
have anywhere better for ABI-specific checks at a layering level that can talk 
about types).



Comment at: lib/Sema/SemaDeclCXX.cpp:7580
 
+static void checkIllFormedTrivialABIStruct(CXXRecordDecl , Sema ) {
+  auto PrintDiagAndRemoveAttr = [&]() {

Either "ill-formed" is the wrong word here, or this needs to produce `ext_` 
diagnostics that `-pedantic-errors` turns into hard errors.



Comment at: lib/Sema/SemaDeclCXX.cpp:7582-7583
+  auto PrintDiagAndRemoveAttr = [&]() {
+S.Diag(RD.getAttr()->getLocation(),
+   diag::warn_cannot_use_trivial_abi) << 
+RD.dropAttr();

Suppress the diagnostic in the case where this happens during template 
instantiation.



Comment at: lib/Sema/SemaDeclCXX.cpp:7595
+const auto *BaseClassDecl
+= cast(B.getType()->getAs()->getDecl());
+// Ill-formed if the base class is non-trivial for the purpose of calls or 
a

This can be simplified to `B.getType()->getAsCXXRecordDecl()`.



Comment at: lib/Sema/SemaDeclCXX.cpp:7598
+// virtual base.
+if (!BaseClassDecl->canPassInRegisters() || B.isVirtual()) {
+  PrintDiagAndRemoveAttr();

It isn't correct to check `canPassInRegisters` on a dependent type here; you 
need to skip this check in that case.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[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, looks good to me.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+  __weak id a[2];
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenObjCXX/trivial_abi.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/trivial_abi.mm
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_STRONGWEAK:.*]] = type { i8*, i8* }
+// CHECK: %[[STRUCT_STRONG:.*]] = type { i8* }
+
+struct __attribute__((trivial_abi)) StrongWeak {
+  id fstrong;
+  __weak id fweak;
+};
+
+struct __attribute__((trivial_abi)) Strong {
+  id fstrong;
+};
+
+// CHECK: define void @_Z19testParamStrongWeak10StrongWeak(%[[STRUCT_STRONGWEAK]]* %{{.*}})
+// CHECK-NOT: call
+// CHECK: ret void
+
+void testParamStrongWeak(StrongWeak a) {
+}
+
+// CHECK: define void @_Z18testCallStrongWeakP10StrongWeak(%[[STRUCT_STRONGWEAK]]* %[[A:.*]])
+// CHECK: %[[A_ADDR:.*]] = alloca %[[STRUCT_STRONGWEAK]]*, align 8
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_STRONGWEAK]], 

[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/Type.h:811
 
+  bool hasTrivialABIOverride() const;
+

This should get a comment, even if it's just to refer to the CXXRecordDecl 
method.



Comment at: lib/CodeGen/CGCall.cpp:3498
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
+  bool HasTrivialABIOverride = type.hasTrivialABIOverride();
 

Please sink this call to the points where you use it; it should be possible to 
avoid computing it in most cases.



Comment at: lib/CodeGen/CGCall.cpp:3505
+   CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) ||
+  HasTrivialABIOverride) {
 // If we're using inalloca, use the argument memory.  Otherwise, use a

e.g. here you can conditionalize it on HasAggregateEvalKind, which is both 
slightly faster and clearer to the reader.



Comment at: lib/CodeGen/CGCall.cpp:3518
+ CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+HasTrivialABIOverride;
 if (DestroyedInCallee)

And here you can guard it by RD && RD->hasNonTrivialDestructor(), and you can 
just call hasNonTrivialABIOverride() directly on RD.



Comment at: lib/Sema/SemaDecl.cpp:11700
+}
+  }
+

I think it's correct not to call CheckDestructorAccess and DiagnoseUseOfDecl 
here, since according to the standard destructor access is always supposed to 
be checked at the call-site, but please leave a comment explaining that.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D41039#969171, @rjmccall wrote:

> I'll trust Richard on the tricky Sema/AST bits.  The functionality of the 
> patch looks basically acceptable to me, although I'm still not thrilled about 
> the idea of actually removing the attribute from the AST rather than just 
> letting it not have effect.  But we could clean that up later if it's 
> significantly simpler to do it this way.


Sure, we can clean up this later.

> Please add a CodeGenObjCXX test case that `__weak` fields in ARC++ do 
> actually override the `trivial_abi` attribute but that `__strong` fields do 
> not.  Also, your test case does not seem to actually test arrays of `__weak` 
> references.

Done. When I was writing a test that tests `__strong` fields, I found out that 
the destructor of the struct wasn't being declared in the AST, which caused an 
assertion to fail in CodeGenFunction::destroyCXXObject when compiling a 
function that takes a trivial_abi type. I fixed it by forcing the declaration 
of the destructor in Sema::ActOnParamDeclarator.




Comment at: include/clang/Basic/Attr.td:1159
+def TrivialABI : InheritableAttr {
+  let Spellings = [Clang<"trivial_abi">];
+  let Subjects = SubjectList<[CXXRecord]>;

aaron.ballman wrote:
> Would this attribute make sense in C, or does it really only make sense in 
> C++? If it doesn't make sense in C++, then you should also set `LangOpts` to 
> be `CPlusPlus`. If it does make sense in C, then the Clang spelling should be 
> `Clang<"trivial_abi", 1>`
I don't think this attribute makes sense in C, so I've set the LangOpts to be 
CPlusPlus.



Comment at: lib/CodeGen/CGDecl.cpp:1825
+HasTrivialABIOverride =
+RD->canPassInRegisters() && RD->hasNonTrivialDestructor();
+

rjmccall wrote:
> You could make a helper function to do this computation and then declare it 
> in some internal-to-IRGen header so that you don't write it out multiple 
> times.
It turns out Sema needs this function too. I defined function 
hasTrivialABIOverride in CXXRecordDecl and moved the code there.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments.

Also, emit the declaration of the destructor of a trivial-abi override class in 
Sema::ActOnParamDeclarator and mark it as referenced. This is necessary because 
a trivial-abi type that is passed by value needs to be destructed in the callee 
and the destructor has to be declared in the AST when IRGen emits the call to 
the destructor in the callee.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/CodeGenObjCXX/trivial_abi.mm
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S12 { // expected-warning {{'trivial_abi' cannot be applied to 'S12'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S13 { // expected-warning {{'trivial_abi' cannot be applied to 'S13'}}
+  __weak id a[2];
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenObjCXX/trivial_abi.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/trivial_abi.mm
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fobjc-arc  -fobjc-weak -fobjc-runtime-has-weak -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_STRONGWEAK:.*]] = type { i8*, i8* }
+// CHECK: %[[STRUCT_STRONG:.*]] = type { i8* }
+
+struct __attribute__((trivial_abi)) StrongWeak {
+  id fstrong;
+  __weak id fweak;
+};
+
+struct __attribute__((trivial_abi)) Strong {
+  id fstrong;
+};
+
+// CHECK: define void @_Z19testParamStrongWeak10StrongWeak(%[[STRUCT_STRONGWEAK]]* %{{.*}})
+// 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:1159
+def TrivialABI : InheritableAttr {
+  let Spellings = [Clang<"trivial_abi">];
+  let Subjects = SubjectList<[CXXRecord]>;

Would this attribute make sense in C, or does it really only make sense in C++? 
If it doesn't make sense in C++, then you should also set `LangOpts` to be 
`CPlusPlus`. If it does make sense in C, then the Clang spelling should be 
`Clang<"trivial_abi", 1>`



Comment at: lib/Sema/SemaDeclCXX.cpp:7594
+  for (const auto  : RD.bases()) {
+CXXRecordDecl *BaseClassDecl
+= cast(B.getType()->getAs()->getDecl());

Can use `const auto *` here.



Comment at: lib/Sema/SemaDeclCXX.cpp:7640
+  // See if trivial_abi has to be dropped.
+  auto *RD = dyn_cast_or_null(TagDecl);
+  if (RD && RD->hasAttr())

This should use `dyn_cast` instead of `dyn_cast_or_null`.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'll trust Richard on the tricky Sema/AST bits.  The functionality of the patch 
looks basically acceptable to me, although I'm still not thrilled about the 
idea of actually removing the attribute from the AST rather than just letting 
it not have effect.  But we could clean that up later if it's significantly 
simpler to do it this way.

Please add a CodeGenObjCXX test case that `__weak` fields in ARC++ do actually 
override the `trivial_abi` attribute but that `__strong` fields do not.  Also, 
your test case does not seem to actually test arrays of `__weak` references.




Comment at: lib/CodeGen/CGDecl.cpp:1825
+HasTrivialABIOverride =
+RD->canPassInRegisters() && RD->hasNonTrivialDestructor();
+

You could make a helper function to do this computation and then declare it in 
some internal-to-IRGen header so that you don't write it out multiple times.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Rename variable to HasTrivialABIOverride.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,197 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:833
 // passed in registers, which is non-conforming.
 if (RD->hasNonTrivialDestructor() &&
 getContext().getTypeSize(RD->getTypeForDecl()) > 64)

rsmith wrote:
> Should we be checking `hasNonTrivialDestructorForCalls` here? What should 
> happen if we have a small class whose copy constructor is trivial for calls 
> but whose destructor is not? The existing machinations of this ABI only work 
> because they can make an additional trivial copy of the function parameter 
> when passing it direct, *even if* it has a non-trivial destructor. I don't 
> think that's correct in general if the copy constructor is only 
> trival-for-calls. Consider:
> 
> ```
> struct X {
>   shared_ptr sp; // shared_ptr is trivial-for-calls
>   ~X();
> };
> ```
> 
> In the absence of the destructor, X should be passed direct.
> In the presence of the destructor, though, passing direct would result in the 
> X destructor running in both caller and callee, and X being passed through 
> registers without running the copy constructor. Which results in a 
> double-delete.
> 
> So I think what we want here is this:
> 
>  * If the copy ctor and dtor are both trivial-for-calls, pass direct.
>  * Otherwise, if the copy ctor is trivial (not just trivial-for-calls) and 
> the object is small, pass direct (regardless of the triviality of the dtor -- 
> we'll make a copy, notionally using the trivial copy ctor, and destroy both 
> the original and the copy).
>  * Otherwise, pass indirect.
I'm not very familiar with microsoft's ABI, but I think that makes sense.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 128838.

https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,197 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i32*
+// CHECK: store i32* 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Check whether the argument is passed indirectly before pushing the cleanup.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,197 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:3498
   bool HasAggregateEvalKind = hasAggregateEvaluationKind(type);
+  bool TypeDestructedInCallee = false;
+  if (const auto *RD = type->getAsCXXRecordDecl())

This is a confusing name for this flag, because there are other reasons why we 
can choose to destroy in the callee (specifically the MS ABI reason). Rather, I 
think this is capturing that "this parameter's ABI was affected by a 
`trivial_abi` attribute", which implies that it should be destroyed (only) in 
the callee. Maybe calling this something like `HasTrivialABIOverride` would 
help.



Comment at: lib/CodeGen/CGDecl.cpp:1822
+  bool IsScalar = hasScalarEvaluationKind(Ty);
+  bool TypeDestructedInCallee = false;
+  if (const auto *RD = Ty->getAsCXXRecordDecl())

Same comment regarding this name.



Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:833
 // passed in registers, which is non-conforming.
 if (RD->hasNonTrivialDestructor() &&
 getContext().getTypeSize(RD->getTypeForDecl()) > 64)

Should we be checking `hasNonTrivialDestructorForCalls` here? What should 
happen if we have a small class whose copy constructor is trivial for calls but 
whose destructor is not? The existing machinations of this ABI only work 
because they can make an additional trivial copy of the function parameter when 
passing it direct, *even if* it has a non-trivial destructor. I don't think 
that's correct in general if the copy constructor is only trival-for-calls. 
Consider:

```
struct X {
  shared_ptr sp; // shared_ptr is trivial-for-calls
  ~X();
};
```

In the absence of the destructor, X should be passed direct.
In the presence of the destructor, though, passing direct would result in the X 
destructor running in both caller and callee, and X being passed through 
registers without running the copy constructor. Which results in a 
double-delete.

So I think what we want here is this:

 * If the copy ctor and dtor are both trivial-for-calls, pass direct.
 * Otherwise, if the copy ctor is trivial (not just trivial-for-calls) and the 
object is small, pass direct (regardless of the triviality of the dtor -- we'll 
make a copy, notionally using the trivial copy ctor, and destroy both the 
original and the copy).
 * Otherwise, pass indirect.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGDecl.cpp:1827
+
+  if (!IsScalar && !CurFuncIsThunk &&
+  (getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee() ||

This is not correct since it isn't checking Arg.isIndirect(). I'll fix it and 
upload a new patch.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/DeclCXX.h:1489-1491
+  bool shouldBeDestructedInCallee() const {
+return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }

rsmith wrote:
> This will return incorrect results on MSVC, where every class type should be 
> destroyed in the callee. Since this is only used  by CodeGen, and only used 
> in combination with the MSVC "destroy right-to-left in callee" ABI setting, 
> please sink it to CodeGen.
I sank the code to CGCall.cpp and CGDecl.cpp and removed the method.



Comment at: lib/CodeGen/CGCall.cpp:3518
+ CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+TypeDestructedInCallee;
 if (DestroyedInCallee)

rsmith wrote:
> This looks wrong (but the wrongness is pre-existing): in the weird MSVC 
> x86_64 case where a small object of class type is passed in registers despite 
> having a non-trivial destructor, this will set `DestroyedInCallee` to false, 
> meaning that the parameter will be destroyed twice, in both the caller and 
> callee.
> 
> And yep, that's exactly what both Clang and MSVC do: 
> https://godbolt.org/g/zjeQq6
> 
> If we fixed that, we could unconditionally `setExternallyDestructed()` here. 
> But let's leave that discussion for a separate patch :)
OK, I'll leave this for future work.



Comment at: lib/Sema/SemaDeclCXX.cpp:5923
+M->setTrivialForCall(HasTrivialABI);
+Record->finishedUserProvidedMethod(M);
+  }

rsmith wrote:
> This should have a different name if you're only going to call it for 
> user-provided copy ctors, move ctors, and dtors.
I renamed the function to setTrivialForCallFlags.



Comment at: lib/Sema/SemaDeclCXX.cpp:12108-12114
+  if (!IsTrivialForCall) {
+if (ClassDecl->needsOverloadResolutionForCopyConstructor())
+  IsTrivialForCall =
+  SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor, true);
+else
+  IsTrivialForCall = ClassDecl->hasTrivialCopyConstructorForCall();
+  }

rsmith wrote:
> Nit: can you use the same form of `?:` expression as is used a few lines 
> above to make it obvious to a reader that this computation parallels that one?
Done. I clang-formated the code above too.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 128798.
ahatanak marked an inline comment as done.

https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,197 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/DeclCXX.h:1489-1491
+  bool shouldBeDestructedInCallee() const {
+return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }

This will return incorrect results on MSVC, where every class type should be 
destroyed in the callee. Since this is only used  by CodeGen, and only used in 
combination with the MSVC "destroy right-to-left in callee" ABI setting, please 
sink it to CodeGen.



Comment at: lib/CodeGen/CGCall.cpp:3518
+ CGM.getCXXABI().getRecordArgABI(RD) != CGCXXABI::RAA_Default) ||
+TypeDestructedInCallee;
 if (DestroyedInCallee)

This looks wrong (but the wrongness is pre-existing): in the weird MSVC x86_64 
case where a small object of class type is passed in registers despite having a 
non-trivial destructor, this will set `DestroyedInCallee` to false, meaning 
that the parameter will be destroyed twice, in both the caller and callee.

And yep, that's exactly what both Clang and MSVC do: 
https://godbolt.org/g/zjeQq6

If we fixed that, we could unconditionally `setExternallyDestructed()` here. 
But let's leave that discussion for a separate patch :)



Comment at: lib/CodeGen/CGDecl.cpp:1822
+  bool IsScalar = hasScalarEvaluationKind(Ty);
+  if ((!IsScalar && !CurFuncIsThunk &&
+   getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()) ||

Should these conditions not also apply to the `shouldBeDestructedInCallee` 
case? We don't want to destroy `trivial_abi` parameters in thunks, only in the 
eventual target function.



Comment at: lib/Sema/SemaDeclCXX.cpp:5923
+M->setTrivialForCall(HasTrivialABI);
+Record->finishedUserProvidedMethod(M);
+  }

This should have a different name if you're only going to call it for 
user-provided copy ctors, move ctors, and dtors.



Comment at: lib/Sema/SemaDeclCXX.cpp:12108-12114
+  if (!IsTrivialForCall) {
+if (ClassDecl->needsOverloadResolutionForCopyConstructor())
+  IsTrivialForCall =
+  SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor, true);
+else
+  IsTrivialForCall = ClassDecl->hasTrivialCopyConstructorForCall();
+  }

Nit: can you use the same form of `?:` expression as is used a few lines above 
to make it obvious to a reader that this computation parallels that one?


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

- Serialize/deserialize the bits I added to FunctionDecl and CXXRecordDecl.
- Enable passing non-trivial structs when clang abi-compat version is 4.0 or 
lower.
- Fix a bug in Sema::CheckCompletedCXXClass where the CXXRecordDecl flags were 
not being set when the class had a defaulted special function. Fixed it by 
renaming finishedUserProvidedMethod to setTrivialForCallFlags and calling it in 
Sema::CheckCompletedCXXClass.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,197 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -fclang-abi-compat=4.0 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Sorry, the patch I've just uploaded was missing some changes I made.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 128781.

https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  lib/Serialization/ASTReaderDecl.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i32*
+// CHECK: store i32* %[[COERCE_VAL_IP]], i32** %[[COERCE_DIVE]], align 8
+// CHECK: %[[CALL:.*]] = call %[[STRUCT_SMALL]]* 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/AttrDocs.td:2261
+- The class or its subobjects have Objective-C pointer type members and ARC is
+  enabled.
+  }];

rjmccall wrote:
> I think the right list of exceptions is:
> 
>   - The class directly declares a virtual base or virtual methods.
>   - The class has a base class that is non-trivial for the purposes of calls.
>   - The class has a non-static data member whose type is non-trivial for the 
> purposes of calls, which includes:
> - classes that are non-trivial for the purposes of calls
> - __weak-qualified types in Objective-C++
> - arrays of any of the above
> 
> I don't see why __strong types would be affected.  We've talked about 
> changing the C++ ABI for structs containing __strong members as part of the 
> __strong-pointers-in-structs feature, but that's not even implicated here 
> because there's an attribute which did not previously exist, so there's no 
> established ABI.
I realized I hadn't taken care of arrays of non-trivial structs. I fixed it in 
checkIllFormedTrivialABIStruct and added a test case to 
test/SemaObjCXX/attr-trivial-abi.mm.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/DeclCXX.h:443
+/// SMF_MoveConstructor, and SMF_Destructor are meaningful here.
+unsigned HasTrivialSpecialMembersForCall : 6;
+

rjmccall wrote:
> I think you could probably get away with only three bits here (and below) if 
> you reorder those three values to be the first three.  I don't know if that's 
> important in terms of packing DefinitionData at all.  Should be okay to not 
> do for now.
Yes, only three bits are needed here. I'll fix this later.



Comment at: include/clang/AST/DeclCXX.h:478
+/// ill-formed if it is annotated with "trivial_abi".
+unsigned DropTrivialABI : 1;
+

rjmccall wrote:
> So is it actually interesting to drop the attribute from the AST?  And why is 
> this bit necessary vs., say, just checking whether the attribute is present 
> but the class is not trivial-for-calls?
I think we need to check whether a struct annotated with "trivial_abi" is 
ill-formed before ActOnFields and CheckCompletedCXXClass set the triviality 
flags in Sema::ActOnFinishCXXMemberSpecification and drop the attribute if it 
is ill-formed. If we don't want to drop the attribute, we will have to find out 
whether "trivial_abi" has no effect before calls to 
FunctionDecl::setTrivialForCall are made.

I added this bit to CXXRecordDecl in the previous patch because I felt hesitant 
to iterate over the base classes and members of the class again, but I think 
that is better than adding a bit just to check whether the class is ill-formed. 
I've removed the bit in the new patch.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

Address review comments.

I also made changes so that FunctionDecl::IsTrivialForCall is always set to 
true for special functions of "trivial_abi" classes.

There is still one microsoft IRGen test failing because I haven't implemented 
serialization of the "ForCall" bits I introduced in FunctionDecl and 
CXXRecordDecl.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi)) S11 { // expected-warning {{'trivial_abi' cannot be applied to 'S11'}}
+  S6 a[2];
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], 

[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D41039#951807, @rjmccall wrote:

> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>
> > I had a discussion with Duncan today and he pointed out that perhaps we 
> > shouldn't allow users to annotate a struct with "trivial_abi" if one of its 
> > subobjects is non-trivial and is not annotated with "trivial_abi" since 
> > that gives users too much power.
> >
> > Should we error out or drop "trivial_abi" from struct Outer when the 
> > following code is compiled?
> >
> >   struct Inner1 {
> > ~Inner1(); // non-trivial
> > int x;
> >   };
> >  
> >   struct __attribute__((trivial_abi)) Outer {
> > ~Outer();
> > Inner1 x;
> >   };
> >
> >
> > The current patch doesn't error out or drop the attribute, but the patch 
> > would probably be much simpler if we didn't allow it.
>
>
> I think it makes sense to emit an error if there is provably a 
> non-trivial-ABI component.  However, for class temploids I think that 
> diagnostic should only fire on the definition, not on instantiations; for 
> example:
>
>   template  struct __attribute__((trivial_abi)) holder {
>  T value;
>  ~holder() {}
>   };
>   holder hs; // this instantiation should be legal despite the 
> fact that holder cannot be trivial-ABI.
>   
>
> But we should still be able to emit the diagnostic in template definitions, 
> e.g.:
>
>   template  struct __attribute__((trivial_abi)) named_holder {
>  std::string name; // there are no instantiations of this template that 
> could ever be trivial-ABI
>  T value;
>  ~named_holder() {}
>   };
>   
>
> The wording should be something akin to the standard template rule that a 
> template is ill-formed if it has no valid instantiations, no diagnostic 
> required.
>
> I would definitely like to open the conversation about the name of the 
> attribute.  I don't think we've used "abi" in an existing attribute name; 
> usually it's more descriptive.  And "trivial" is a weighty word in the 
> standard.  I'm not sure I have a great counter-proposal off the top of my 
> head, though.







Comment at: include/clang/AST/DeclCXX.h:443
+/// SMF_MoveConstructor, and SMF_Destructor are meaningful here.
+unsigned HasTrivialSpecialMembersForCall : 6;
+

I think you could probably get away with only three bits here (and below) if 
you reorder those three values to be the first three.  I don't know if that's 
important in terms of packing DefinitionData at all.  Should be okay to not do 
for now.



Comment at: include/clang/AST/DeclCXX.h:478
+/// ill-formed if it is annotated with "trivial_abi".
+unsigned DropTrivialABI : 1;
+

So is it actually interesting to drop the attribute from the AST?  And why is 
this bit necessary vs., say, just checking whether the attribute is present but 
the class is not trivial-for-calls?



Comment at: include/clang/AST/DeclCXX.h:1380
+return data().DeclaredNonTrivialSpecialMembersForCall &
+   SMF_CopyConstructor || !hasTrivialCopyConstructorForCall();
+  }

Please parenthesize, even if it isn't strictly required.



Comment at: include/clang/AST/DeclCXX.h:1502
+return data().CanPassInRegisters && hasNonTrivialDestructor();
+  }
+

These method names read as if they're actions, not predicates.  Please use 
"is..." or "can..." or "should..." or something that makes it clear that 
they're predicates.



Comment at: include/clang/Basic/AttrDocs.td:2234
+when the type is considered non-trivial for the purpose of calls according to
+the C++ ABI. Non-trivial destructors or copy/move constructors of classes
+annotated with ``trivial_abi`` do not cause the corresponding special functions

You should say "when the type would otherwise be considered..." here, because 
this attribute actually changes the definition of non-trivial for the purposes 
of calls.

I would suggest rephrasing the bit about non-trivial special members like so: 
"A class annotated with ``trivial_abi`` can have non-trivial destructors or 
copy/move constructors without automatically becoming non-trivial for the 
purposes of calls."



Comment at: include/clang/Basic/AttrDocs.td:2251
+  A a;
+};
+

You should state explicitly in this example that A is itself trivial for the 
purposes of calls.



Comment at: include/clang/Basic/AttrDocs.td:2255
+callee is responsible for destroying the object regardless of how it is passed
+under the C ABI.
+

"If a type is trivial for the purposes of calls, has a non-trivial destructor, 
and is passed as an argument by value, the convention is that the callee will 
destroy the object before returning."



Comment at: include/clang/Basic/AttrDocs.td:2257
+
+Attribute ``trivial_abi`` will be dropped from a class in 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

No worries. Upload the patch again.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+  NonTrivial();
+  ~NonTrivial();
+  int a;
+};
+
+struct HasTrivial {
+  Small s;
+  Trivial m;
+};
+
+struct HasNonTrivial {
+  Small s;
+  NonTrivial m;
+};
+
+// CHECK: define void @_Z14testParamSmall5Small(i64 %[[A_COERCE:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_SMALL]], align 8
+// CHECK: %[[COERCE_DIVE:.*]] = getelementptr inbounds %[[STRUCT_SMALL]], %[[STRUCT_SMALL]]* %[[A]], i32 0, i32 0
+// CHECK: %[[COERCE_VAL_IP:.*]] = inttoptr i64 %[[A_COERCE]] to i32*
+// CHECK: store i32* %[[COERCE_VAL_IP]], i32** %[[COERCE_DIVE]], align 8
+// CHECK: %[[CALL:.*]] = call %[[STRUCT_SMALL]]* @_ZN5SmallD1Ev(%[[STRUCT_SMALL]]* %[[A]])
+// CHECK: ret void
+// CHECK: }
+
+void testParamSmall(Small a) {
+}
+
+// CHECK: define i64 @_Z15testReturnSmallv()
+// CHECK: 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Unfortunately it looks like I do not have permission to re-upload Akira's last 
patch to this review.


Repository:
  rL LLVM

https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Adrian Prantl via Phabricator via cfe-commits]

aprantl reopened this revision.
aprantl added a comment.

I'm sorry, I made a copy error in a the commit message for 
https://reviews.llvm.org/D41743 and accidentally associated that commit with 
this review. I suppose I should learn to use arcanist.
Reopening, and my apologies for the noise!


Repository:
  rL LLVM

https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Phabricator via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL321845: Debug Info: Support DW_AT_calling_convention on 
composite types. (authored by adrian, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41039?vs=128642=128691#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41039

Files:
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp


Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -2803,9 +2803,18 @@
 
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
 
+  // Explicitly record the calling convention for C++ records.
+  auto Flags = llvm::DINode::FlagZero;
+  if (auto CXXRD = dyn_cast(RD)) {
+if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
+  Flags |= llvm::DINode::FlagTypePassByReference;
+else
+  Flags |= llvm::DINode::FlagTypePassByValue;
+  }
+
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
-  llvm::DINode::FlagZero, FullName);
+  Flags, FullName);
 
   // Elements of composite types usually have back to the type, creating
   // uniquing cycles.  Distinct nodes are more efficient.
Index: cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// Not trivially copyable because of the explicit destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefDtor",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefDtor {
+  int i;
+  ~RefDtor() {}
+} refDtor;
+
+// Not trivially copyable because of the explicit copy constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefCopy",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefCopy {
+  int i;
+  RefCopy() = default;
+  RefCopy(RefCopy ) {}
+} refCopy;
+
+// Not trivially copyable because of the explicit move constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefMove",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefMove {
+  int i;
+  RefMove() = default;
+  RefMove(RefMove &) {}
+} refMove;
+
+// POD-like type even though it defines a destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Podlike", {{.*}}flags: 
DIFlagTypePassByValue
+struct Podlike {
+  int i;
+  Podlike() = default;
+  Podlike(Podlike &) = default;
+  ~Podlike() = default;
+} podlike;
+
+
+// This is a POD type.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Pod",{{.*}}flags: 
DIFlagTypePassByValue
+struct Pod {
+  int i;
+} pod;
+
+// This is definitely not a POD type.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Complex",{{.*}}flags: 
DIFlagTypePassByReference
+struct Complex {
+  Complex() {}
+  Complex(Complex ) : i(Copy.i) {};
+  int i;
+} complex;


Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -2803,9 +2803,18 @@
 
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
 
+  // Explicitly record the calling convention for C++ records.
+  auto Flags = llvm::DINode::FlagZero;
+  if (auto CXXRD = dyn_cast(RD)) {
+if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
+  Flags |= llvm::DINode::FlagTypePassByReference;
+else
+  Flags |= llvm::DINode::FlagTypePassByValue;
+  }
+
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
-  llvm::DINode::FlagZero, FullName);
+  Flags, FullName);
 
   // Elements of composite types usually have back to the type, creating
   // uniquing cycles.  Distinct nodes are more efficient.
Index: cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-composite-cc.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// Not trivially copyable because of the explicit destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefDtor",{{.*}}flags: DIFlagTypePassByReference
+struct RefDtor {
+  int i;
+  ~RefDtor() {}
+} refDtor;
+
+// Not trivially copyable because of the explicit copy constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefCopy",{{.*}}flags: 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Phabricator via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC321845: Debug Info: Support DW_AT_calling_convention on 
composite types. (authored by adrian, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41039?vs=128642=128690#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41039

Files:
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGenCXX/debug-info-composite-cc.cpp


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2803,9 +2803,18 @@
 
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
 
+  // Explicitly record the calling convention for C++ records.
+  auto Flags = llvm::DINode::FlagZero;
+  if (auto CXXRD = dyn_cast(RD)) {
+if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
+  Flags |= llvm::DINode::FlagTypePassByReference;
+else
+  Flags |= llvm::DINode::FlagTypePassByValue;
+  }
+
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
-  llvm::DINode::FlagZero, FullName);
+  Flags, FullName);
 
   // Elements of composite types usually have back to the type, creating
   // uniquing cycles.  Distinct nodes are more efficient.
Index: test/CodeGenCXX/debug-info-composite-cc.cpp
===
--- test/CodeGenCXX/debug-info-composite-cc.cpp
+++ test/CodeGenCXX/debug-info-composite-cc.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple 
%itanium_abi_triple %s -o - | FileCheck %s
+
+// Not trivially copyable because of the explicit destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefDtor",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefDtor {
+  int i;
+  ~RefDtor() {}
+} refDtor;
+
+// Not trivially copyable because of the explicit copy constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefCopy",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefCopy {
+  int i;
+  RefCopy() = default;
+  RefCopy(RefCopy ) {}
+} refCopy;
+
+// Not trivially copyable because of the explicit move constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefMove",{{.*}}flags: 
DIFlagTypePassByReference
+struct RefMove {
+  int i;
+  RefMove() = default;
+  RefMove(RefMove &) {}
+} refMove;
+
+// POD-like type even though it defines a destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Podlike", {{.*}}flags: 
DIFlagTypePassByValue
+struct Podlike {
+  int i;
+  Podlike() = default;
+  Podlike(Podlike &) = default;
+  ~Podlike() = default;
+} podlike;
+
+
+// This is a POD type.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Pod",{{.*}}flags: 
DIFlagTypePassByValue
+struct Pod {
+  int i;
+} pod;
+
+// This is definitely not a POD type.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "Complex",{{.*}}flags: 
DIFlagTypePassByReference
+struct Complex {
+  Complex() {}
+  Complex(Complex ) : i(Copy.i) {};
+  int i;
+} complex;


Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2803,9 +2803,18 @@
 
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
 
+  // Explicitly record the calling convention for C++ records.
+  auto Flags = llvm::DINode::FlagZero;
+  if (auto CXXRD = dyn_cast(RD)) {
+if (CGM.getCXXABI().getRecordArgABI(CXXRD) == CGCXXABI::RAA_Indirect)
+  Flags |= llvm::DINode::FlagTypePassByReference;
+else
+  Flags |= llvm::DINode::FlagTypePassByValue;
+  }
+
   llvm::DICompositeType *RealDecl = DBuilder.createReplaceableCompositeType(
   getTagForRecord(RD), RDName, RDContext, DefUnit, Line, 0, Size, Align,
-  llvm::DINode::FlagZero, FullName);
+  Flags, FullName);
 
   // Elements of composite types usually have back to the type, creating
   // uniquing cycles.  Distinct nodes are more efficient.
Index: test/CodeGenCXX/debug-info-composite-cc.cpp
===
--- test/CodeGenCXX/debug-info-composite-cc.cpp
+++ test/CodeGenCXX/debug-info-composite-cc.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+// Not trivially copyable because of the explicit destructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefDtor",{{.*}}flags: DIFlagTypePassByReference
+struct RefDtor {
+  int i;
+  ~RefDtor() {}
+} refDtor;
+
+// Not trivially copyable because of the explicit copy constructor.
+// CHECK-DAG: !DICompositeType({{.*}}, name: "RefCopy",{{.*}}flags: DIFlagTypePassByReference
+struct RefCopy {
+  int i;
+  RefCopy() = default;
+  RefCopy(RefCopy ) {}
+} refCopy;
+
+// Not trivially copyable 

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

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

I've only fixed the places where the bits to track the triviality of special 
functions are set or reset, so this is still a WIP. I'll update the patch again 
later today, but let me know if anyone has any feedback in the meantime.

A couple of comments and questions about this patch:

- CXXRecordDecl and FunctionDecl still have the flags that keep track of the 
triviality of special functions for calls. I don't think we can avoid using 
them even under the new simpler rules? I also had to add 
"DeclaredNonTrivialSpecialMembersForCall" since 
ItaniumCXXABI::passClassIndirect needs to know whether a struct has a 
non-trivial destructor or copy constructor (I plan to make changes to  
passClassIndirect later so that hasNonTrivial*ForCalls methods are called 
there).

- If a struct annotated with "trivial_abi" turns out to be ill-formed (because 
it has virtual bases, virtual functions, or __weak pointers), the attribute is 
dropped after all the members explicitly declared in the struct are seen. The 
triviality bits for user-provided special functions are set or reset only after 
we know whether "trivial_abi" has to be dropped or not. Currently, a diagnostic 
is printed if the struct becomes ill-formed because of the attribute, but it is 
possible to make changes to suppress them or make them more user-friendly.


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via cfe-commits]

> On Jan 3, 2018, at 6:39 PM, John McCall  wrote:
> 
> On Wed, Jan 3, 2018 at 2:07 PM, Akira Hatanaka  > wrote:
>> On Jan 3, 2018, at 10:25 AM, John McCall > > wrote:
>> 
>> On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka > > wrote:
>>> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits 
>>> > wrote:
>>> 
>>> 
>>> 
>>> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka >> > wrote:
>>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall >> > wrote:
>>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie >> > wrote:
>>> On Mon, Dec 11, 2017 at 5:38 PM John McCall >> > wrote:
>>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie >> > wrote:
>>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator 
>>> > wrote:
>>> rjmccall added a comment.
>>> 
>>> In https://reviews.llvm.org/D41039#951648 
>>> , @ahatanak wrote:
>>> 
>>> > I had a discussion with Duncan today and he pointed out that perhaps we 
>>> > shouldn't allow users to annotate a struct with "trivial_abi" if one of 
>>> > its subobjects is non-trivial and is not annotated with "trivial_abi" 
>>> > since that gives users too much power.
>>> >
>>> > Should we error out or drop "trivial_abi" from struct Outer when the 
>>> > following code is compiled?
>>> >
>>> >   struct Inner1 {
>>> > ~Inner1(); // non-trivial
>>> > int x;
>>> >   };
>>> >
>>> >   struct __attribute__((trivial_abi)) Outer {
>>> > ~Outer();
>>> > Inner1 x;
>>> >   };
>>> >
>>> >
>>> > The current patch doesn't error out or drop the attribute, but the patch 
>>> > would probably be much simpler if we didn't allow it.
>>> 
>>> 
>>> I think it makes sense to emit an error if there is provably a 
>>> non-trivial-ABI component.  However, for class temploids I think that 
>>> diagnostic should only fire on the definition, not on instantiations; for 
>>> example:
>>> 
>>>   template  struct __attribute__((trivial_abi)) holder {
>>>  T value;
>>>  ~holder() {}
>>>   };
>>>   holder hs; // this instantiation should be legal despite the 
>>> fact that holder cannot be trivial-ABI.
>>> 
>>> But we should still be able to emit the diagnostic in template definitions, 
>>> e.g.:
>>> 
>>>   template  struct __attribute__((trivial_abi)) named_holder {
>>>  std::string name; // there are no instantiations of this template that 
>>> could ever be trivial-ABI
>>>  T value;
>>>  ~named_holder() {}
>>>   };
>>> 
>>> The wording should be something akin to the standard template rule that a 
>>> template is ill-formed if it has no valid instantiations, no diagnostic 
>>> required.
>>> 
>>> I would definitely like to open the conversation about the name of the 
>>> attribute.  I don't think we've used "abi" in an existing attribute name; 
>>> usually it's more descriptive.  And "trivial" is a weighty word in the 
>>> standard.  I'm not sure I have a great counter-proposal off the top of my 
>>> head, though.
>>> 
>>> Agreed on both counts (would love a better name, don't have any stand-out 
>>> candidates off the top of my head).
>>> 
>>> I feel like a more descriptive term about the property of the object would 
>>> make me happier - something like "address_independent_identity" 
>>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>>> 
>>> Incidentally, your comments are not showing up on Phabricator for some 
>>> reason.
>>> 
>>> Yeah, Phab doesn't do a great job looking on the mailing list for 
>>> interesting replies - I forget, maybe it only catches bottom post or top 
>>> post but not infix replies or something... 
>>> 
>>> Well, fortunately the mailing list is also archived. :)
>>> 
>>> 
>>> The term "trivially movable" suggests itself, with two caveats:
>>>   - What we're talking about is trivial *destructive* movability, i.e. that 
>>> the combination of moving the value to a new object and not destroying the 
>>> old object can be done trivially, which is not quite the same as trivial 
>>> movability in the normal C++ sense, which I guess could be a property that 
>>> someone theoretically might care about (if the type is trivially 
>>> destructed, but it isn't copyable for semantic reasons?).
>>>   - Trivial destructive movability is a really common property, and it's 
>>> something that a compiler would really like to optimize based on even in 
>>> cases where trivial_abi can't be adopted for binary-compatibility reasons.  
>>> Stealing the term for the stronger property 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via cfe-commits]

On Wed, Jan 3, 2018 at 2:07 PM, Akira Hatanaka  wrote:

> On Jan 3, 2018, at 10:25 AM, John McCall  wrote:
>
> On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka 
> wrote:
>
>> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>
>>
>> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka 
>> wrote:
>>
>>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall  wr
>>> ote:
>>>
 On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie  w
 rote:

> On Mon, Dec 11, 2017 at 5:38 PM John McCall 
> wrote:
>
>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie  w
>> rote:
>>
>>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 rjmccall added a comment.

 In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:

 > I had a discussion with Duncan today and he pointed out that
 perhaps we shouldn't allow users to annotate a struct with 
 "trivial_abi" if
 one of its subobjects is non-trivial and is not annotated with
 "trivial_abi" since that gives users too much power.
 >
 > Should we error out or drop "trivial_abi" from struct Outer when
 the following code is compiled?
 >
 >   struct Inner1 {
 > ~Inner1(); // non-trivial
 > int x;
 >   };
 >
 >   struct __attribute__((trivial_abi)) Outer {
 > ~Outer();
 > Inner1 x;
 >   };
 >
 >
 > The current patch doesn't error out or drop the attribute, but
 the patch would probably be much simpler if we didn't allow it.


 I think it makes sense to emit an error if there is provably a
 non-trivial-ABI component.  However, for class temploids I think that
 diagnostic should only fire on the definition, not on instantiations; 
 for
 example:

   template  struct __attribute__((trivial_abi)) holder {
  T value;
  ~holder() {}
   };
   holder hs; // this instantiation should be legal
 despite the fact that holder cannot be trivial-ABI.

 But we should still be able to emit the diagnostic in template
 definitions, e.g.:

   template  struct __attribute__((trivial_abi))
 named_holder {
  std::string name; // there are no instantiations of this
 template that could ever be trivial-ABI
  T value;
  ~named_holder() {}
   };

 The wording should be something akin to the standard template rule
 that a template is ill-formed if it has no valid instantiations, no
 diagnostic required.

 I would definitely like to open the conversation about the name of
 the attribute.  I don't think we've used "abi" in an existing attribute
 name; usually it's more descriptive.  And "trivial" is a weighty word 
 in
 the standard.  I'm not sure I have a great counter-proposal off the 
 top of
 my head, though.

>>>
>>> Agreed on both counts (would love a better name, don't have any
>>> stand-out candidates off the top of my head).
>>>
>>> I feel like a more descriptive term about the property of the object
>>> would make me happier - something like "address_independent_identity"
>>> (s/identity/value/?) though, yeah, that's not spectacular by any 
>>> stretch.
>>>
>>
>> Incidentally, your comments are not showing up on Phabricator for
>> some reason.
>>
>
> Yeah, Phab doesn't do a great job looking on the mailing list for
> interesting replies - I forget, maybe it only catches bottom post or top
> post but not infix replies or something...
>

 Well, fortunately the mailing list is also archived. :)


> The term "trivially movable" suggests itself, with two caveats:
>>   - What we're talking about is trivial *destructive* movability,
>> i.e. that the combination of moving the value to a new object and not
>> destroying the old object can be done trivially, which is not quite the
>> same as trivial movability in the normal C++ sense, which I guess could 
>> be
>> a property that someone theoretically might care about (if the type is
>> trivially destructed, but it isn't copyable for semantic reasons?).
>>   - Trivial destructive movability is a really common property, and
>> it's something that a compiler would really like to optimize based on 
>> even
>> in cases where trivial_abi can't be adopted for binary-compatibility
>> reasons.  Stealing the 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via cfe-commits]

> On Jan 3, 2018, at 10:25 AM, John McCall  wrote:
> 
> On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka  > wrote:
>> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits 
>> > wrote:
>> 
>> 
>> 
>> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka > > wrote:
>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall > > wrote:
>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie > > wrote:
>> On Mon, Dec 11, 2017 at 5:38 PM John McCall > > wrote:
>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie > > wrote:
>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator 
>> > wrote:
>> rjmccall added a comment.
>> 
>> In https://reviews.llvm.org/D41039#951648 
>> , @ahatanak wrote:
>> 
>> > I had a discussion with Duncan today and he pointed out that perhaps we 
>> > shouldn't allow users to annotate a struct with "trivial_abi" if one of 
>> > its subobjects is non-trivial and is not annotated with "trivial_abi" 
>> > since that gives users too much power.
>> >
>> > Should we error out or drop "trivial_abi" from struct Outer when the 
>> > following code is compiled?
>> >
>> >   struct Inner1 {
>> > ~Inner1(); // non-trivial
>> > int x;
>> >   };
>> >
>> >   struct __attribute__((trivial_abi)) Outer {
>> > ~Outer();
>> > Inner1 x;
>> >   };
>> >
>> >
>> > The current patch doesn't error out or drop the attribute, but the patch 
>> > would probably be much simpler if we didn't allow it.
>> 
>> 
>> I think it makes sense to emit an error if there is provably a 
>> non-trivial-ABI component.  However, for class temploids I think that 
>> diagnostic should only fire on the definition, not on instantiations; for 
>> example:
>> 
>>   template  struct __attribute__((trivial_abi)) holder {
>>  T value;
>>  ~holder() {}
>>   };
>>   holder hs; // this instantiation should be legal despite the 
>> fact that holder cannot be trivial-ABI.
>> 
>> But we should still be able to emit the diagnostic in template definitions, 
>> e.g.:
>> 
>>   template  struct __attribute__((trivial_abi)) named_holder {
>>  std::string name; // there are no instantiations of this template that 
>> could ever be trivial-ABI
>>  T value;
>>  ~named_holder() {}
>>   };
>> 
>> The wording should be something akin to the standard template rule that a 
>> template is ill-formed if it has no valid instantiations, no diagnostic 
>> required.
>> 
>> I would definitely like to open the conversation about the name of the 
>> attribute.  I don't think we've used "abi" in an existing attribute name; 
>> usually it's more descriptive.  And "trivial" is a weighty word in the 
>> standard.  I'm not sure I have a great counter-proposal off the top of my 
>> head, though.
>> 
>> Agreed on both counts (would love a better name, don't have any stand-out 
>> candidates off the top of my head).
>> 
>> I feel like a more descriptive term about the property of the object would 
>> make me happier - something like "address_independent_identity" 
>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>> 
>> Incidentally, your comments are not showing up on Phabricator for some 
>> reason.
>> 
>> Yeah, Phab doesn't do a great job looking on the mailing list for 
>> interesting replies - I forget, maybe it only catches bottom post or top 
>> post but not infix replies or something... 
>> 
>> Well, fortunately the mailing list is also archived. :)
>> 
>> 
>> The term "trivially movable" suggests itself, with two caveats:
>>   - What we're talking about is trivial *destructive* movability, i.e. that 
>> the combination of moving the value to a new object and not destroying the 
>> old object can be done trivially, which is not quite the same as trivial 
>> movability in the normal C++ sense, which I guess could be a property that 
>> someone theoretically might care about (if the type is trivially destructed, 
>> but it isn't copyable for semantic reasons?).
>>   - Trivial destructive movability is a really common property, and it's 
>> something that a compiler would really like to optimize based on even in 
>> cases where trivial_abi can't be adopted for binary-compatibility reasons.  
>> Stealing the term for the stronger property that the type is trivially 
>> destructively movable *and its ABI should reflect that in a specific way* 
>> would be unfortunate.
>> 
>> Fair point.
>>  
>> "trivially_movable" is a long attribute anyway, and 
>> "trivially_destructively_movable" is even worse.
>> 
>> Maybe that second point is telling us that this isn't 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via cfe-commits]

On Wed, Jan 3, 2018 at 12:24 PM, Akira Hatanaka  wrote:

> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>
>
> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka  wrote:
>
>> On Tue, Dec 12, 2017 at 12:12 PM, John McCall  wrote:
>>
>>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie  w
>>> rote:
>>>
 On Mon, Dec 11, 2017 at 5:38 PM John McCall  wrote:

> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie  w
> rote:
>
>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> rjmccall added a comment.
>>>
>>> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>>>
>>> > I had a discussion with Duncan today and he pointed out that
>>> perhaps we shouldn't allow users to annotate a struct with 
>>> "trivial_abi" if
>>> one of its subobjects is non-trivial and is not annotated with
>>> "trivial_abi" since that gives users too much power.
>>> >
>>> > Should we error out or drop "trivial_abi" from struct Outer when
>>> the following code is compiled?
>>> >
>>> >   struct Inner1 {
>>> > ~Inner1(); // non-trivial
>>> > int x;
>>> >   };
>>> >
>>> >   struct __attribute__((trivial_abi)) Outer {
>>> > ~Outer();
>>> > Inner1 x;
>>> >   };
>>> >
>>> >
>>> > The current patch doesn't error out or drop the attribute, but the
>>> patch would probably be much simpler if we didn't allow it.
>>>
>>>
>>> I think it makes sense to emit an error if there is provably a
>>> non-trivial-ABI component.  However, for class temploids I think that
>>> diagnostic should only fire on the definition, not on instantiations; 
>>> for
>>> example:
>>>
>>>   template  struct __attribute__((trivial_abi)) holder {
>>>  T value;
>>>  ~holder() {}
>>>   };
>>>   holder hs; // this instantiation should be legal
>>> despite the fact that holder cannot be trivial-ABI.
>>>
>>> But we should still be able to emit the diagnostic in template
>>> definitions, e.g.:
>>>
>>>   template  struct __attribute__((trivial_abi))
>>> named_holder {
>>>  std::string name; // there are no instantiations of this
>>> template that could ever be trivial-ABI
>>>  T value;
>>>  ~named_holder() {}
>>>   };
>>>
>>> The wording should be something akin to the standard template rule
>>> that a template is ill-formed if it has no valid instantiations, no
>>> diagnostic required.
>>>
>>> I would definitely like to open the conversation about the name of
>>> the attribute.  I don't think we've used "abi" in an existing attribute
>>> name; usually it's more descriptive.  And "trivial" is a weighty word in
>>> the standard.  I'm not sure I have a great counter-proposal off the top 
>>> of
>>> my head, though.
>>>
>>
>> Agreed on both counts (would love a better name, don't have any
>> stand-out candidates off the top of my head).
>>
>> I feel like a more descriptive term about the property of the object
>> would make me happier - something like "address_independent_identity"
>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>>
>
> Incidentally, your comments are not showing up on Phabricator for some
> reason.
>

 Yeah, Phab doesn't do a great job looking on the mailing list for
 interesting replies - I forget, maybe it only catches bottom post or top
 post but not infix replies or something...

>>>
>>> Well, fortunately the mailing list is also archived. :)
>>>
>>>
 The term "trivially movable" suggests itself, with two caveats:
>   - What we're talking about is trivial *destructive* movability,
> i.e. that the combination of moving the value to a new object and not
> destroying the old object can be done trivially, which is not quite the
> same as trivial movability in the normal C++ sense, which I guess could be
> a property that someone theoretically might care about (if the type is
> trivially destructed, but it isn't copyable for semantic reasons?).
>   - Trivial destructive movability is a really common property, and
> it's something that a compiler would really like to optimize based on even
> in cases where trivial_abi can't be adopted for binary-compatibility
> reasons.  Stealing the term for the stronger property that the type is
> trivially destructively movable *and its ABI should reflect that in a
> specific way* would be unfortunate.
>

 Fair point.


> "trivially_movable" is a long attribute anyway, and
> "trivially_destructively_movable" is even worse.

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via cfe-commits]

> On Jan 2, 2018, at 9:42 AM, David Blaikie via cfe-commits 
>  wrote:
> 
> 
> 
> On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka  > wrote:
> On Tue, Dec 12, 2017 at 12:12 PM, John McCall  > wrote:
> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie  > wrote:
> On Mon, Dec 11, 2017 at 5:38 PM John McCall  > wrote:
> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie  > wrote:
> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator 
> > wrote:
> rjmccall added a comment.
> 
> In https://reviews.llvm.org/D41039#951648 
> , @ahatanak wrote:
> 
> > I had a discussion with Duncan today and he pointed out that perhaps we 
> > shouldn't allow users to annotate a struct with "trivial_abi" if one of its 
> > subobjects is non-trivial and is not annotated with "trivial_abi" since 
> > that gives users too much power.
> >
> > Should we error out or drop "trivial_abi" from struct Outer when the 
> > following code is compiled?
> >
> >   struct Inner1 {
> > ~Inner1(); // non-trivial
> > int x;
> >   };
> >
> >   struct __attribute__((trivial_abi)) Outer {
> > ~Outer();
> > Inner1 x;
> >   };
> >
> >
> > The current patch doesn't error out or drop the attribute, but the patch 
> > would probably be much simpler if we didn't allow it.
> 
> 
> I think it makes sense to emit an error if there is provably a 
> non-trivial-ABI component.  However, for class temploids I think that 
> diagnostic should only fire on the definition, not on instantiations; for 
> example:
> 
>   template  struct __attribute__((trivial_abi)) holder {
>  T value;
>  ~holder() {}
>   };
>   holder hs; // this instantiation should be legal despite the 
> fact that holder cannot be trivial-ABI.
> 
> But we should still be able to emit the diagnostic in template definitions, 
> e.g.:
> 
>   template  struct __attribute__((trivial_abi)) named_holder {
>  std::string name; // there are no instantiations of this template that 
> could ever be trivial-ABI
>  T value;
>  ~named_holder() {}
>   };
> 
> The wording should be something akin to the standard template rule that a 
> template is ill-formed if it has no valid instantiations, no diagnostic 
> required.
> 
> I would definitely like to open the conversation about the name of the 
> attribute.  I don't think we've used "abi" in an existing attribute name; 
> usually it's more descriptive.  And "trivial" is a weighty word in the 
> standard.  I'm not sure I have a great counter-proposal off the top of my 
> head, though.
> 
> Agreed on both counts (would love a better name, don't have any stand-out 
> candidates off the top of my head).
> 
> I feel like a more descriptive term about the property of the object would 
> make me happier - something like "address_independent_identity" 
> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
> 
> Incidentally, your comments are not showing up on Phabricator for some reason.
> 
> Yeah, Phab doesn't do a great job looking on the mailing list for interesting 
> replies - I forget, maybe it only catches bottom post or top post but not 
> infix replies or something... 
> 
> Well, fortunately the mailing list is also archived. :)
> 
> 
> The term "trivially movable" suggests itself, with two caveats:
>   - What we're talking about is trivial *destructive* movability, i.e. that 
> the combination of moving the value to a new object and not destroying the 
> old object can be done trivially, which is not quite the same as trivial 
> movability in the normal C++ sense, which I guess could be a property that 
> someone theoretically might care about (if the type is trivially destructed, 
> but it isn't copyable for semantic reasons?).
>   - Trivial destructive movability is a really common property, and it's 
> something that a compiler would really like to optimize based on even in 
> cases where trivial_abi can't be adopted for binary-compatibility reasons.  
> Stealing the term for the stronger property that the type is trivially 
> destructively movable *and its ABI should reflect that in a specific way* 
> would be unfortunate.
> 
> Fair point.
>  
> "trivially_movable" is a long attribute anyway, and 
> "trivially_destructively_movable" is even worse.
> 
> Maybe that second point is telling us that this isn't purely descriptive — 
> it's inherently talking about the ABI, not just the semantics of the type.  I 
> might be talking myself into accepting trivial_abi if we don't end up with a 
> better suggestion.
> 
> *nod* I think if you want to slice this that way (ensuring there's space to 
> support describing a similar property for use only 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[David Blaikie via cfe-commits]

On Tue, Dec 19, 2017 at 9:43 PM Akira Hatanaka  wrote:

> On Tue, Dec 12, 2017 at 12:12 PM, John McCall  wrote:
>
>> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie 
>> wrote:
>>
>>> On Mon, Dec 11, 2017 at 5:38 PM John McCall  wrote:
>>>
 On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie 
 wrote:

> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> rjmccall added a comment.
>>
>> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>>
>> > I had a discussion with Duncan today and he pointed out that
>> perhaps we shouldn't allow users to annotate a struct with "trivial_abi" 
>> if
>> one of its subobjects is non-trivial and is not annotated with
>> "trivial_abi" since that gives users too much power.
>> >
>> > Should we error out or drop "trivial_abi" from struct Outer when
>> the following code is compiled?
>> >
>> >   struct Inner1 {
>> > ~Inner1(); // non-trivial
>> > int x;
>> >   };
>> >
>> >   struct __attribute__((trivial_abi)) Outer {
>> > ~Outer();
>> > Inner1 x;
>> >   };
>> >
>> >
>> > The current patch doesn't error out or drop the attribute, but the
>> patch would probably be much simpler if we didn't allow it.
>>
>>
>> I think it makes sense to emit an error if there is provably a
>> non-trivial-ABI component.  However, for class temploids I think that
>> diagnostic should only fire on the definition, not on instantiations; for
>> example:
>>
>>   template  struct __attribute__((trivial_abi)) holder {
>>  T value;
>>  ~holder() {}
>>   };
>>   holder hs; // this instantiation should be legal
>> despite the fact that holder cannot be trivial-ABI.
>>
>> But we should still be able to emit the diagnostic in template
>> definitions, e.g.:
>>
>>   template  struct __attribute__((trivial_abi)) named_holder
>> {
>>  std::string name; // there are no instantiations of this
>> template that could ever be trivial-ABI
>>  T value;
>>  ~named_holder() {}
>>   };
>>
>> The wording should be something akin to the standard template rule
>> that a template is ill-formed if it has no valid instantiations, no
>> diagnostic required.
>>
>> I would definitely like to open the conversation about the name of
>> the attribute.  I don't think we've used "abi" in an existing attribute
>> name; usually it's more descriptive.  And "trivial" is a weighty word in
>> the standard.  I'm not sure I have a great counter-proposal off the top 
>> of
>> my head, though.
>>
>
> Agreed on both counts (would love a better name, don't have any
> stand-out candidates off the top of my head).
>
> I feel like a more descriptive term about the property of the object
> would make me happier - something like "address_independent_identity"
> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>

 Incidentally, your comments are not showing up on Phabricator for some
 reason.

>>>
>>> Yeah, Phab doesn't do a great job looking on the mailing list for
>>> interesting replies - I forget, maybe it only catches bottom post or top
>>> post but not infix replies or something...
>>>
>>
>> Well, fortunately the mailing list is also archived. :)
>>
>>
>>> The term "trivially movable" suggests itself, with two caveats:
   - What we're talking about is trivial *destructive* movability, i.e.
 that the combination of moving the value to a new object and not destroying
 the old object can be done trivially, which is not quite the same as
 trivial movability in the normal C++ sense, which I guess could be a
 property that someone theoretically might care about (if the type is
 trivially destructed, but it isn't copyable for semantic reasons?).
   - Trivial destructive movability is a really common property, and
 it's something that a compiler would really like to optimize based on even
 in cases where trivial_abi can't be adopted for binary-compatibility
 reasons.  Stealing the term for the stronger property that the type is
 trivially destructively movable *and its ABI should reflect that in a
 specific way* would be unfortunate.

>>>
>>> Fair point.
>>>
>>>
 "trivially_movable" is a long attribute anyway, and
 "trivially_destructively_movable" is even worse.

 Maybe that second point is telling us that this isn't purely
 descriptive — it's inherently talking about the ABI, not just the semantics
 of the type.  I might be talking myself into accepting trivial_abi if we
 don't end up with a better suggestion.

>>>
>>> *nod* I think if you want to 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via cfe-commits]

On Tue, Dec 12, 2017 at 12:12 PM, John McCall  wrote:

> On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie  wrote:
>
>> On Mon, Dec 11, 2017 at 5:38 PM John McCall  wrote:
>>
>>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie 
>>> wrote:
>>>
 On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
 revi...@reviews.llvm.org> wrote:

> rjmccall added a comment.
>
> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>
> > I had a discussion with Duncan today and he pointed out that perhaps
> we shouldn't allow users to annotate a struct with "trivial_abi" if one of
> its subobjects is non-trivial and is not annotated with "trivial_abi" 
> since
> that gives users too much power.
> >
> > Should we error out or drop "trivial_abi" from struct Outer when the
> following code is compiled?
> >
> >   struct Inner1 {
> > ~Inner1(); // non-trivial
> > int x;
> >   };
> >
> >   struct __attribute__((trivial_abi)) Outer {
> > ~Outer();
> > Inner1 x;
> >   };
> >
> >
> > The current patch doesn't error out or drop the attribute, but the
> patch would probably be much simpler if we didn't allow it.
>
>
> I think it makes sense to emit an error if there is provably a
> non-trivial-ABI component.  However, for class temploids I think that
> diagnostic should only fire on the definition, not on instantiations; for
> example:
>
>   template  struct __attribute__((trivial_abi)) holder {
>  T value;
>  ~holder() {}
>   };
>   holder hs; // this instantiation should be legal
> despite the fact that holder cannot be trivial-ABI.
>
> But we should still be able to emit the diagnostic in template
> definitions, e.g.:
>
>   template  struct __attribute__((trivial_abi)) named_holder {
>  std::string name; // there are no instantiations of this template
> that could ever be trivial-ABI
>  T value;
>  ~named_holder() {}
>   };
>
> The wording should be something akin to the standard template rule
> that a template is ill-formed if it has no valid instantiations, no
> diagnostic required.
>
> I would definitely like to open the conversation about the name of the
> attribute.  I don't think we've used "abi" in an existing attribute name;
> usually it's more descriptive.  And "trivial" is a weighty word in the
> standard.  I'm not sure I have a great counter-proposal off the top of my
> head, though.
>

 Agreed on both counts (would love a better name, don't have any
 stand-out candidates off the top of my head).

 I feel like a more descriptive term about the property of the object
 would make me happier - something like "address_independent_identity"
 (s/identity/value/?) though, yeah, that's not spectacular by any stretch.

>>>
>>> Incidentally, your comments are not showing up on Phabricator for some
>>> reason.
>>>
>>
>> Yeah, Phab doesn't do a great job looking on the mailing list for
>> interesting replies - I forget, maybe it only catches bottom post or top
>> post but not infix replies or something...
>>
>
> Well, fortunately the mailing list is also archived. :)
>
>
>> The term "trivially movable" suggests itself, with two caveats:
>>>   - What we're talking about is trivial *destructive* movability, i.e.
>>> that the combination of moving the value to a new object and not destroying
>>> the old object can be done trivially, which is not quite the same as
>>> trivial movability in the normal C++ sense, which I guess could be a
>>> property that someone theoretically might care about (if the type is
>>> trivially destructed, but it isn't copyable for semantic reasons?).
>>>   - Trivial destructive movability is a really common property, and it's
>>> something that a compiler would really like to optimize based on even in
>>> cases where trivial_abi can't be adopted for binary-compatibility reasons.
>>> Stealing the term for the stronger property that the type is trivially
>>> destructively movable *and its ABI should reflect that in a specific way*
>>> would be unfortunate.
>>>
>>
>> Fair point.
>>
>>
>>> "trivially_movable" is a long attribute anyway, and
>>> "trivially_destructively_movable" is even worse.
>>>
>>> Maybe that second point is telling us that this isn't purely descriptive
>>> — it's inherently talking about the ABI, not just the semantics of the
>>> type.  I might be talking myself into accepting trivial_abi if we don't end
>>> up with a better suggestion.
>>>
>>
>> *nod* I think if you want to slice this that way (ensuring there's space
>> to support describing a similar property for use only in non-ABI-breaking
>> contexts as well) it seems like ABI is the term to use somewhere in the
>> name.
>>
>
> Yeah.  

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via cfe-commits]

On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie  wrote:

> On Mon, Dec 11, 2017 at 5:38 PM John McCall  wrote:
>
>> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie 
>> wrote:
>>
>>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 rjmccall added a comment.

 In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:

 > I had a discussion with Duncan today and he pointed out that perhaps
 we shouldn't allow users to annotate a struct with "trivial_abi" if one of
 its subobjects is non-trivial and is not annotated with "trivial_abi" since
 that gives users too much power.
 >
 > Should we error out or drop "trivial_abi" from struct Outer when the
 following code is compiled?
 >
 >   struct Inner1 {
 > ~Inner1(); // non-trivial
 > int x;
 >   };
 >
 >   struct __attribute__((trivial_abi)) Outer {
 > ~Outer();
 > Inner1 x;
 >   };
 >
 >
 > The current patch doesn't error out or drop the attribute, but the
 patch would probably be much simpler if we didn't allow it.


 I think it makes sense to emit an error if there is provably a
 non-trivial-ABI component.  However, for class temploids I think that
 diagnostic should only fire on the definition, not on instantiations; for
 example:

   template  struct __attribute__((trivial_abi)) holder {
  T value;
  ~holder() {}
   };
   holder hs; // this instantiation should be legal despite
 the fact that holder cannot be trivial-ABI.

 But we should still be able to emit the diagnostic in template
 definitions, e.g.:

   template  struct __attribute__((trivial_abi)) named_holder {
  std::string name; // there are no instantiations of this template
 that could ever be trivial-ABI
  T value;
  ~named_holder() {}
   };

 The wording should be something akin to the standard template rule that
 a template is ill-formed if it has no valid instantiations, no diagnostic
 required.

 I would definitely like to open the conversation about the name of the
 attribute.  I don't think we've used "abi" in an existing attribute name;
 usually it's more descriptive.  And "trivial" is a weighty word in the
 standard.  I'm not sure I have a great counter-proposal off the top of my
 head, though.

>>>
>>> Agreed on both counts (would love a better name, don't have any
>>> stand-out candidates off the top of my head).
>>>
>>> I feel like a more descriptive term about the property of the object
>>> would make me happier - something like "address_independent_identity"
>>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>>>
>>
>> Incidentally, your comments are not showing up on Phabricator for some
>> reason.
>>
>
> Yeah, Phab doesn't do a great job looking on the mailing list for
> interesting replies - I forget, maybe it only catches bottom post or top
> post but not infix replies or something...
>

Well, fortunately the mailing list is also archived. :)


> The term "trivially movable" suggests itself, with two caveats:
>>   - What we're talking about is trivial *destructive* movability, i.e.
>> that the combination of moving the value to a new object and not destroying
>> the old object can be done trivially, which is not quite the same as
>> trivial movability in the normal C++ sense, which I guess could be a
>> property that someone theoretically might care about (if the type is
>> trivially destructed, but it isn't copyable for semantic reasons?).
>>   - Trivial destructive movability is a really common property, and it's
>> something that a compiler would really like to optimize based on even in
>> cases where trivial_abi can't be adopted for binary-compatibility reasons.
>> Stealing the term for the stronger property that the type is trivially
>> destructively movable *and its ABI should reflect that in a specific way*
>> would be unfortunate.
>>
>
> Fair point.
>
>
>> "trivially_movable" is a long attribute anyway, and
>> "trivially_destructively_movable" is even worse.
>>
>> Maybe that second point is telling us that this isn't purely descriptive
>> — it's inherently talking about the ABI, not just the semantics of the
>> type.  I might be talking myself into accepting trivial_abi if we don't end
>> up with a better suggestion.
>>
>
> *nod* I think if you want to slice this that way (ensuring there's space
> to support describing a similar property for use only in non-ABI-breaking
> contexts as well) it seems like ABI is the term to use somewhere in the
> name.
>

Yeah.  We just had a little internal conversation about it, and the idea of
"address_invariant_abi" came up, but I'm not sure it has enough to
recommend it over "trivial_abi" to justify the longer name.


> Random 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[David Blaikie via cfe-commits]

On Mon, Dec 11, 2017 at 5:38 PM John McCall  wrote:

> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie  wrote:
>
>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> rjmccall added a comment.
>>>
>>> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>>>
>>> > I had a discussion with Duncan today and he pointed out that perhaps
>>> we shouldn't allow users to annotate a struct with "trivial_abi" if one of
>>> its subobjects is non-trivial and is not annotated with "trivial_abi" since
>>> that gives users too much power.
>>> >
>>> > Should we error out or drop "trivial_abi" from struct Outer when the
>>> following code is compiled?
>>> >
>>> >   struct Inner1 {
>>> > ~Inner1(); // non-trivial
>>> > int x;
>>> >   };
>>> >
>>> >   struct __attribute__((trivial_abi)) Outer {
>>> > ~Outer();
>>> > Inner1 x;
>>> >   };
>>> >
>>> >
>>> > The current patch doesn't error out or drop the attribute, but the
>>> patch would probably be much simpler if we didn't allow it.
>>>
>>>
>>> I think it makes sense to emit an error if there is provably a
>>> non-trivial-ABI component.  However, for class temploids I think that
>>> diagnostic should only fire on the definition, not on instantiations; for
>>> example:
>>>
>>>   template  struct __attribute__((trivial_abi)) holder {
>>>  T value;
>>>  ~holder() {}
>>>   };
>>>   holder hs; // this instantiation should be legal despite
>>> the fact that holder cannot be trivial-ABI.
>>>
>>> But we should still be able to emit the diagnostic in template
>>> definitions, e.g.:
>>>
>>>   template  struct __attribute__((trivial_abi)) named_holder {
>>>  std::string name; // there are no instantiations of this template
>>> that could ever be trivial-ABI
>>>  T value;
>>>  ~named_holder() {}
>>>   };
>>>
>>> The wording should be something akin to the standard template rule that
>>> a template is ill-formed if it has no valid instantiations, no diagnostic
>>> required.
>>>
>>> I would definitely like to open the conversation about the name of the
>>> attribute.  I don't think we've used "abi" in an existing attribute name;
>>> usually it's more descriptive.  And "trivial" is a weighty word in the
>>> standard.  I'm not sure I have a great counter-proposal off the top of my
>>> head, though.
>>>
>>
>> Agreed on both counts (would love a better name, don't have any stand-out
>> candidates off the top of my head).
>>
>> I feel like a more descriptive term about the property of the object
>> would make me happier - something like "address_independent_identity"
>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>>
>
> Incidentally, your comments are not showing up on Phabricator for some
> reason.
>

Yeah, Phab doesn't do a great job looking on the mailing list for
interesting replies - I forget, maybe it only catches bottom post or top
post but not infix replies or something...


>
> The term "trivially movable" suggests itself, with two caveats:
>   - What we're talking about is trivial *destructive* movability, i.e.
> that the combination of moving the value to a new object and not destroying
> the old object can be done trivially, which is not quite the same as
> trivial movability in the normal C++ sense, which I guess could be a
> property that someone theoretically might care about (if the type is
> trivially destructed, but it isn't copyable for semantic reasons?).
>   - Trivial destructive movability is a really common property, and it's
> something that a compiler would really like to optimize based on even in
> cases where trivial_abi can't be adopted for binary-compatibility reasons.
> Stealing the term for the stronger property that the type is trivially
> destructively movable *and its ABI should reflect that in a specific way*
> would be unfortunate.
>

Fair point.


> "trivially_movable" is a long attribute anyway, and
> "trivially_destructively_movable" is even worse.
>
> Maybe that second point is telling us that this isn't purely descriptive —
> it's inherently talking about the ABI, not just the semantics of the type.
> I might be talking myself into accepting trivial_abi if we don't end up
> with a better suggestion.
>

*nod* I think if you want to slice this that way (ensuring there's space to
support describing a similar property for use only in non-ABI-breaking
contexts as well) it seems like ABI is the term to use somewhere in the
name.


> Random thing that occurred to me: is it actually reasonable to enforce
> trivial_abi correctness in a non-template context?  Templates aren't the
> only case where a user could reasonably want to add trivial_abi and just
> have it be suppressed if it's wrong.  Imagine if some stdlib made
> std::string trivial_abi; someone might reasonably want to make my
> named_holder example above trivial_abi as well, with the expectation that
> it would only have an effect on 

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via cfe-commits]

On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie  wrote:

> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> rjmccall added a comment.
>>
>> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>>
>> > I had a discussion with Duncan today and he pointed out that perhaps we
>> shouldn't allow users to annotate a struct with "trivial_abi" if one of its
>> subobjects is non-trivial and is not annotated with "trivial_abi" since
>> that gives users too much power.
>> >
>> > Should we error out or drop "trivial_abi" from struct Outer when the
>> following code is compiled?
>> >
>> >   struct Inner1 {
>> > ~Inner1(); // non-trivial
>> > int x;
>> >   };
>> >
>> >   struct __attribute__((trivial_abi)) Outer {
>> > ~Outer();
>> > Inner1 x;
>> >   };
>> >
>> >
>> > The current patch doesn't error out or drop the attribute, but the
>> patch would probably be much simpler if we didn't allow it.
>>
>>
>> I think it makes sense to emit an error if there is provably a
>> non-trivial-ABI component.  However, for class temploids I think that
>> diagnostic should only fire on the definition, not on instantiations; for
>> example:
>>
>>   template  struct __attribute__((trivial_abi)) holder {
>>  T value;
>>  ~holder() {}
>>   };
>>   holder hs; // this instantiation should be legal despite
>> the fact that holder cannot be trivial-ABI.
>>
>> But we should still be able to emit the diagnostic in template
>> definitions, e.g.:
>>
>>   template  struct __attribute__((trivial_abi)) named_holder {
>>  std::string name; // there are no instantiations of this template
>> that could ever be trivial-ABI
>>  T value;
>>  ~named_holder() {}
>>   };
>>
>> The wording should be something akin to the standard template rule that a
>> template is ill-formed if it has no valid instantiations, no diagnostic
>> required.
>>
>> I would definitely like to open the conversation about the name of the
>> attribute.  I don't think we've used "abi" in an existing attribute name;
>> usually it's more descriptive.  And "trivial" is a weighty word in the
>> standard.  I'm not sure I have a great counter-proposal off the top of my
>> head, though.
>>
>
> Agreed on both counts (would love a better name, don't have any stand-out
> candidates off the top of my head).
>
> I feel like a more descriptive term about the property of the object would
> make me happier - something like "address_independent_identity"
> (s/identity/value/?) though, yeah, that's not spectacular by any stretch.
>

Incidentally, your comments are not showing up on Phabricator for some
reason.

The term "trivially movable" suggests itself, with two caveats:
  - What we're talking about is trivial *destructive* movability, i.e. that
the combination of moving the value to a new object and not destroying the
old object can be done trivially, which is not quite the same as trivial
movability in the normal C++ sense, which I guess could be a property that
someone theoretically might care about (if the type is trivially
destructed, but it isn't copyable for semantic reasons?).
  - Trivial destructive movability is a really common property, and it's
something that a compiler would really like to optimize based on even in
cases where trivial_abi can't be adopted for binary-compatibility reasons.
Stealing the term for the stronger property that the type is trivially
destructively movable *and its ABI should reflect that in a specific way*
would be unfortunate.

"trivially_movable" is a long attribute anyway, and
"trivially_destructively_movable" is even worse.

Maybe that second point is telling us that this isn't purely descriptive —
it's inherently talking about the ABI, not just the semantics of the type.
I might be talking myself into accepting trivial_abi if we don't end up
with a better suggestion.

Random thing that occurred to me: is it actually reasonable to enforce
trivial_abi correctness in a non-template context?  Templates aren't the
only case where a user could reasonably want to add trivial_abi and just
have it be suppressed if it's wrong.  Imagine if some stdlib made
std::string trivial_abi; someone might reasonably want to make my
named_holder example above trivial_abi as well, with the expectation that
it would only have an effect on targets where std::string was trivial_abi.
At the very least, I'm concerned that we might be opening ourselves up to a
need to add supporting features, like a way to be conditionally trivial_abi
based on context.

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

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[David Blaikie via cfe-commits]

On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> rjmccall added a comment.
>
> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:
>
> > I had a discussion with Duncan today and he pointed out that perhaps we
> shouldn't allow users to annotate a struct with "trivial_abi" if one of its
> subobjects is non-trivial and is not annotated with "trivial_abi" since
> that gives users too much power.
> >
> > Should we error out or drop "trivial_abi" from struct Outer when the
> following code is compiled?
> >
> >   struct Inner1 {
> > ~Inner1(); // non-trivial
> > int x;
> >   };
> >
> >   struct __attribute__((trivial_abi)) Outer {
> > ~Outer();
> > Inner1 x;
> >   };
> >
> >
> > The current patch doesn't error out or drop the attribute, but the patch
> would probably be much simpler if we didn't allow it.
>
>
> I think it makes sense to emit an error if there is provably a
> non-trivial-ABI component.  However, for class temploids I think that
> diagnostic should only fire on the definition, not on instantiations; for
> example:
>
>   template  struct __attribute__((trivial_abi)) holder {
>  T value;
>  ~holder() {}
>   };
>   holder hs; // this instantiation should be legal despite
> the fact that holder cannot be trivial-ABI.
>
> But we should still be able to emit the diagnostic in template
> definitions, e.g.:
>
>   template  struct __attribute__((trivial_abi)) named_holder {
>  std::string name; // there are no instantiations of this template
> that could ever be trivial-ABI
>  T value;
>  ~named_holder() {}
>   };
>
> The wording should be something akin to the standard template rule that a
> template is ill-formed if it has no valid instantiations, no diagnostic
> required.
>
> I would definitely like to open the conversation about the name of the
> attribute.  I don't think we've used "abi" in an existing attribute name;
> usually it's more descriptive.  And "trivial" is a weighty word in the
> standard.  I'm not sure I have a great counter-proposal off the top of my
> head, though.
>

Agreed on both counts (would love a better name, don't have any stand-out
candidates off the top of my head).

I feel like a more descriptive term about the property of the object would
make me happier - something like "address_independent_identity"
(s/identity/value/?) though, yeah, that's not spectacular by any stretch.


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

[PATCH] D41039: Add support for attribute "trivial_abi"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D41039#951648, @ahatanak wrote:

> I had a discussion with Duncan today and he pointed out that perhaps we 
> shouldn't allow users to annotate a struct with "trivial_abi" if one of its 
> subobjects is non-trivial and is not annotated with "trivial_abi" since that 
> gives users too much power.
>
> Should we error out or drop "trivial_abi" from struct Outer when the 
> following code is compiled?
>
>   struct Inner1 {
> ~Inner1(); // non-trivial
> int x;
>   };
>  
>   struct __attribute__((trivial_abi)) Outer {
> ~Outer();
> Inner1 x;
>   };
>
>
> The current patch doesn't error out or drop the attribute, but the patch 
> would probably be much simpler if we didn't allow it.


I think it makes sense to emit an error if there is provably a non-trivial-ABI 
component.  However, for class temploids I think that diagnostic should only 
fire on the definition, not on instantiations; for example:

  template  struct __attribute__((trivial_abi)) holder {
 T value;
 ~holder() {}
  };
  holder hs; // this instantiation should be legal despite the 
fact that holder cannot be trivial-ABI.

But we should still be able to emit the diagnostic in template definitions, 
e.g.:

  template  struct __attribute__((trivial_abi)) named_holder {
 std::string name; // there are no instantiations of this template that 
could ever be trivial-ABI
 T value;
 ~named_holder() {}
  };

The wording should be something akin to the standard template rule that a 
template is ill-formed if it has no valid instantiations, no diagnostic 
required.

I would definitely like to open the conversation about the name of the 
attribute.  I don't think we've used "abi" in an existing attribute name; 
usually it's more descriptive.  And "trivial" is a weighty word in the 
standard.  I'm not sure I have a great counter-proposal off the top of my head, 
though.


https://reviews.llvm.org/D41039



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

Re: [PATCH] D41039: Add support for attribute "trivial_abi"

[David Blaikie via cfe-commits]

My bet would be: warn and ignore it, but probably Richard's & John might
have stronger thoughts/justifications/etc.

On Mon, Dec 11, 2017 at 1:38 PM Akira Hatanaka via Phabricator <
revi...@reviews.llvm.org> wrote:

> ahatanak added a comment.
>
> I had a discussion with Duncan today and he pointed out that perhaps we
> shouldn't allow users to annotate a struct with "trivial_abi" if one of its
> subobjects is non-trivial and is not annotated with "trivial_abi" since
> that gives users too much power.
>
> Should we error out or drop "trivial_abi" from struct Outer when the
> following code is compiled?
>
>   struct Inner1 {
> ~Inner1(); // non-trivial
> int x;
>   };
>
>   struct __attribute__((trivial_abi)) Outer {
> ~Outer();
> Inner1 x;
>   };
>
> The current patch doesn't error out or drop the attribute, but the patch
> would probably be much simpler if we didn't allow it.
>
>
> https://reviews.llvm.org/D41039
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I had a discussion with Duncan today and he pointed out that perhaps we 
shouldn't allow users to annotate a struct with "trivial_abi" if one of its 
subobjects is non-trivial and is not annotated with "trivial_abi" since that 
gives users too much power.

Should we error out or drop "trivial_abi" from struct Outer when the following 
code is compiled?

  struct Inner1 {
~Inner1(); // non-trivial
int x;
  };
  
  struct __attribute__((trivial_abi)) Outer {
~Outer();
Inner1 x;
  };

The current patch doesn't error out or drop the attribute, but the patch would 
probably be much simpler if we didn't allow it.


https://reviews.llvm.org/D41039



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

[PATCH] D41039: Add support for attribute "trivial_abi"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
Herald added a subscriber: javed.absar.

This patch adds support for a new attribute "trivial_abi", which will be used 
to instruct clang to pass and return non-trivial C++ structs directly when it's 
possible to do so. The original RFC I sent to cfe-dev last month is here:

http://lists.llvm.org/pipermail/cfe-dev/2017-November/055955.html

A couple of questions:

1. The attribute is tentatively named "trivial_abi". Is there a better name 
that conveys what the attribute is supposed to do? How about c_abi or 
pass_by_value?

2. Propagating the property of a "trivial_abi" class to the containing classes 
turned out to be more complicated than I initially expected. Do we really need 
or want clang to do that rather than asking users to annotate the containing 
classes with "trivial_abi"? If we do, is there a simpler way to accomplish that 
than what I did in this patch?


https://reviews.llvm.org/D41039

Files:
  include/clang/AST/Decl.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/Type.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CodeGenFunction.cpp
  lib/Sema/SemaDeclAttr.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenCXX/trivial_abi.cpp
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaObjCXX/attr-trivial-abi.mm

Index: test/SemaObjCXX/attr-trivial-abi.mm
===
--- /dev/null
+++ test/SemaObjCXX/attr-trivial-abi.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fobjc-runtime-has-weak -fobjc-weak -fobjc-arc -fsyntax-only -verify %s
+
+void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
+
+struct [[clang::trivial_abi]] S0 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S1 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S2 { // expected-warning {{'trivial_abi' cannot be applied to 'S2'}}
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S3 { // expected-warning {{'trivial_abi' cannot be applied to 'S3'}}
+  virtual void m();
+};
+
+struct S4 {
+  int a;
+};
+
+struct __attribute__((trivial_abi)) S5 : public virtual S4 { // expected-warning {{'trivial_abi' cannot be applied to 'S5'}}
+};
+
+struct __attribute__((trivial_abi)) S9 : public S4 {
+};
+
+struct S6 {
+  __weak id a;
+};
+
+struct __attribute__((trivial_abi)) S7 { // expected-warning {{'trivial_abi' cannot be applied to 'S7'}}
+  S6 a;
+};
+
+struct __attribute__((trivial_abi(1))) S8 { // expected-error {{'trivial_abi' attribute takes no arguments}}
+  int a;
+};
+
+template
+struct __attribute__((trivial_abi)) S10 {
+  T p;
+};
+
+S10 p1;
+
+// Do not warn when 'trivial_abi' is used to annotate a template class.
+S10<__weak id> p2;
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -2,7 +2,7 @@
 
 // The number of supported attributes should never go down!
 
-// CHECK: #pragma clang attribute supports 66 attributes:
+// CHECK: #pragma clang attribute supports 67 attributes:
 // CHECK-NEXT: AMDGPUFlatWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumSGPR (SubjectMatchRule_function)
 // CHECK-NEXT: AMDGPUNumVGPR (SubjectMatchRule_function)
@@ -66,6 +66,7 @@
 // CHECK-NEXT: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
+// CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: WarnUnusedResult (SubjectMatchRule_objc_method, SubjectMatchRule_enum, SubjectMatchRule_record, SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: XRayInstrument (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: XRayLogArgs (SubjectMatchRule_function, SubjectMatchRule_objc_method)
Index: test/CodeGenCXX/trivial_abi.cpp
===
--- /dev/null
+++ test/CodeGenCXX/trivial_abi.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios11 -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_SMALL:.*]] = type { i32* }
+// CHECK: %[[STRUCT_LARGE:.*]] = type { i32*, [128 x i32] }
+// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { i32 }
+// CHECK: %[[STRUCT_NONTRIVIAL:.*]] = type { i32 }
+
+struct __attribute__((trivial_abi)) Small {
+  int *p;
+  Small();
+  ~Small();
+  Small(const Small &);
+  Small =(const Small &);
+};
+
+struct __attribute__((trivial_abi)) Large {
+  int *p;
+  int a[128];
+  Large();
+  ~Large();
+  Large(const Large &);
+  Large =(const Large &);
+};
+
+struct Trivial {
+  int a;
+};
+
+struct NonTrivial {
+