[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360446: [Sema] Mark array element destructors referenced 
during initialization (authored by epilk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61165?vs=198944=199038#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61165

Files:
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp
  cfe/trunk/test/CodeGenCXX/no_destroy.cpp
  cfe/trunk/test/SemaCXX/no_destroy.cpp

Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -3880,6 +3880,39 @@
 storage duration shouldn't have its exit-time destructor run. Annotating every
 static and thread duration variable with this attribute is equivalent to
 invoking clang with -fno-c++-static-destructors.
+
+If a variable is declared with this attribute, clang doesn't access check or
+generate the type's destructor. If you have a type that you only want to be
+annotated with ``no_destroy``, you can therefore declare the destructor private:
+
+.. code-block:: c++
+
+  struct only_no_destroy {
+only_no_destroy();
+  private:
+~only_no_destroy();
+  };
+
+  [[clang::no_destroy]] only_no_destroy global; // fine!
+
+Note that destructors are still required for subobjects of aggregates annotated
+with this attribute. This is because previously constructed subobjects need to
+be destroyed if an exception gets thrown before the initialization of the
+complete object is complete. For instance:
+
+.. code-block::c++
+
+  void f() {
+try {
+  [[clang::no_destroy]]
+  static only_no_destroy array[10]; // error, only_no_destroy has a private destructor.
+} catch (...) {
+  // Handle the error
+}
+  }
+
+Here, if the construction of `array[9]` fails with an exception, `array[0..8]`
+will be destroyed, so the element's destructor needs to be accessible.
   }];
 }
 
Index: cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp
===
--- cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp
+++ cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp
@@ -10,7 +10,7 @@
 
 void test() {
   new ctor[0]; // expected-error{{calling a private constructor of class 'ctor'}}
-  new dtor[0]; // expected-error{{calling a private destructor of class 'dtor'}}
-  new dtor[3]; // expected-error{{calling a private destructor of class 'dtor'}}
-  new dtor[3][3]; // expected-error{{calling a private destructor of class 'dtor'}}
+  new dtor[0]; // expected-error{{temporary of type 'dtor' has private destructor}}
+  new dtor[3]; // expected-error{{temporary of type 'dtor' has private destructor}}
+  new dtor[3][3]; // expected-error{{temporary of type 'dtor' has private destructor}}
 }
Index: cfe/trunk/test/CodeGenCXX/no_destroy.cpp
===
--- cfe/trunk/test/CodeGenCXX/no_destroy.cpp
+++ cfe/trunk/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ -25,7 +31,51 @@
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define void @_Z1gv
+void g() {
+  // CHECK-NOT: __cxa_atexit
+  [[clang::no_destroy]] static NonTrivial2 nt21;
+  // CHECK-NOT: _tlv_atexit
+  [[clang::no_destroy]] thread_local NonTrivial2 nt22;
+}
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial 

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

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

LGTM.




Comment at: clang/test/SemaCXX/aggregate-initialization.cpp:199
   void test0() {
-auto *y = new Y {}; // expected-error {{temporary of type 
'ElementDestructor::X' has private destructor}}
+auto *y = new Y {}; // expected-error {{calling a private destructor of 
class 'ElementDestructor::X}}
   }

Feel free to address this in a follow-up, but this seems like a regression.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

rjmccall wrote:
> erik.pilkington wrote:
> > rjmccall wrote:
> > > erik.pilkington wrote:
> > > > rjmccall wrote:
> > > > > rsmith wrote:
> > > > > > Hmm, perhaps it would be cleaner if the destructor for array 
> > > > > > elements were required by the initialization of the array, instead 
> > > > > > of here. That's how this works for struct aggregate initialization 
> > > > > > (see `InitListChecker::CheckStructUnionTypes`), and (indirectly) 
> > > > > > how it works for initialization by constructor, and so on. And it 
> > > > > > reflects the fact that it's the initialization process that needs 
> > > > > > the array element destructor, not the destruction of the variable 
> > > > > > (which never happens).
> > > > > > 
> > > > > > For the case of aggregate initialization of an array, we appear to 
> > > > > > fail to implement [dcl.init.aggr]/8:
> > > > > > 
> > > > > > """
> > > > > > The destructor for each element of class type is potentially 
> > > > > > invoked (11.3.6) from the context where the aggregate 
> > > > > > initialization occurs. [Note: This provision ensures that 
> > > > > > destructors can be called for fully-constructed subobjects in case 
> > > > > > an exception is thrown (14.2). — end note]
> > > > > > """
> > > > > > 
> > > > > > (But there doesn't appear to be any corresponding wording for 
> > > > > > default / value initialization of arrays. See also the special case 
> > > > > > at the end of `BuildCXXNewExpr`.)
> > > > > That makes sense to me.  The default/value initialization case seems 
> > > > > like an obvious oversight in the standard, but I think it might be a 
> > > > > distinction without a difference: the special cases for 
> > > > > base-or-member initializers and new-expressions might cover literally 
> > > > > every situation where initialization doesn't come with an adjacent 
> > > > > need for non-exceptional destruction.
> > > > Sure, done. Moving this to initialization time seems like a nice 
> > > > cleanup.
> > > Hmm.  What I just mentioned for the documentation is relevant here, 
> > > right?  We only need to check access to subobject destructors if 
> > > exceptions are enabled, so this attempt to avoid duplicate diagnostics 
> > > might leave us not flagging the use if exceptions are disabled.
> > > 
> > > Well, maybe the check isn't actually restricted that way, hmm.  Shouldn't 
> > > it be?
> > I thought we tried in general to keep `-fno-exceptions` as similar to 
> > normal C++ as possible, rather than invent new language rules for it (even 
> > when they make sense).
> There's at least one other place where `-fno-exceptions` changes the language 
> rules: we don't look for `operator delete` in a new-expression.  It does look 
> like that's all I was remembering, though, and that we don't generally apply 
> that same principle to destructors, so feel free to ignore my comment.
> 
> Still, could you make sure there's a test case that tests that we check 
> access to the destructor of an array variable even when exceptions are 
> disabled, since that's the interesting corner case created by this new 
> condition?
Sure, thats included in test/SemaCXX/no_destroy.cpp. Pretty much every test is 
running in -fno-exceptions mode, since thats the default in `-cc1` for some 
reason. All the more reason to keep the two modes as similar as possible I 
guess :)


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 198944.
erik.pilkington marked an inline comment as done.
erik.pilkington added a comment.

Rename `hasAccessibleDestructor`.


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

https://reviews.llvm.org/D61165

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGenCXX/no_destroy.cpp
  clang/test/SemaCXX/aggregate-initialization.cpp
  clang/test/SemaCXX/no_destroy.cpp

Index: clang/test/SemaCXX/no_destroy.cpp
===
--- clang/test/SemaCXX/no_destroy.cpp
+++ clang/test/SemaCXX/no_destroy.cpp
@@ -1,11 +1,13 @@
-// RUN: %clang_cc1 -DNO_DTORS -fno-c++-static-destructors -verify %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -DNO_EXCEPTIONS -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -DNO_EXCEPTIONS -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -fexceptions -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -fexceptions -verify %s
 
 struct SecretDestructor {
 #ifndef NO_DTORS
   // expected-note@+2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
@@ -44,3 +46,30 @@
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+// expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+SecretDestructor arr[10];
+
+void local_arrays() {
+  // expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+  static SecretDestructor arr2[10];
+  // expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+  thread_local SecretDestructor arr3[10];
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+void dontcrash() {
+  [[clang::no_destroy]] static Derived2 d2[] = {0, 0};
+}
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};
Index: clang/test/SemaCXX/aggregate-initialization.cpp
===
--- clang/test/SemaCXX/aggregate-initialization.cpp
+++ clang/test/SemaCXX/aggregate-initialization.cpp
@@ -196,7 +196,7 @@
   struct Y { X x; };
 
   void test0() {
-auto *y = new Y {}; // expected-error {{temporary of type 'ElementDestructor::X' has private destructor}}
+auto *y = new Y {}; // expected-error {{calling a private destructor of class 'ElementDestructor::X}}
   }
 
   struct S0 { int f; ~S0() = delete; }; // expected-note 3 {{'~S0' has been explicitly marked deleted here}}
Index: clang/test/CodeGenCXX/no_destroy.cpp
===
--- clang/test/CodeGenCXX/no_destroy.cpp
+++ clang/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ -25,7 +31,51 @@
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define void @_Z1gv
+void g() {
+  // CHECK-NOT: __cxa_atexit
+  [[clang::no_destroy]] static NonTrivial2 nt21;
+  // CHECK-NOT: _tlv_atexit
+  [[clang::no_destroy]] thread_local NonTrivial2 nt22;
+}
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial nt3;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] thread_local NonTrivial nt4;
+
+
+struct NonTrivial3 {
+  NonTrivial3();
+  ~NonTrivial3();
+};
+
+[[clang::no_destroy]] NonTrivial3 arr[10];
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

erik.pilkington wrote:
> rjmccall wrote:
> > erik.pilkington wrote:
> > > rjmccall wrote:
> > > > rsmith wrote:
> > > > > Hmm, perhaps it would be cleaner if the destructor for array elements 
> > > > > were required by the initialization of the array, instead of here. 
> > > > > That's how this works for struct aggregate initialization (see 
> > > > > `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it 
> > > > > works for initialization by constructor, and so on. And it reflects 
> > > > > the fact that it's the initialization process that needs the array 
> > > > > element destructor, not the destruction of the variable (which never 
> > > > > happens).
> > > > > 
> > > > > For the case of aggregate initialization of an array, we appear to 
> > > > > fail to implement [dcl.init.aggr]/8:
> > > > > 
> > > > > """
> > > > > The destructor for each element of class type is potentially invoked 
> > > > > (11.3.6) from the context where the aggregate initialization occurs. 
> > > > > [Note: This provision ensures that destructors can be called for 
> > > > > fully-constructed subobjects in case an exception is thrown (14.2). — 
> > > > > end note]
> > > > > """
> > > > > 
> > > > > (But there doesn't appear to be any corresponding wording for default 
> > > > > / value initialization of arrays. See also the special case at the 
> > > > > end of `BuildCXXNewExpr`.)
> > > > That makes sense to me.  The default/value initialization case seems 
> > > > like an obvious oversight in the standard, but I think it might be a 
> > > > distinction without a difference: the special cases for base-or-member 
> > > > initializers and new-expressions might cover literally every situation 
> > > > where initialization doesn't come with an adjacent need for 
> > > > non-exceptional destruction.
> > > Sure, done. Moving this to initialization time seems like a nice cleanup.
> > Hmm.  What I just mentioned for the documentation is relevant here, right?  
> > We only need to check access to subobject destructors if exceptions are 
> > enabled, so this attempt to avoid duplicate diagnostics might leave us not 
> > flagging the use if exceptions are disabled.
> > 
> > Well, maybe the check isn't actually restricted that way, hmm.  Shouldn't 
> > it be?
> I thought we tried in general to keep `-fno-exceptions` as similar to normal 
> C++ as possible, rather than invent new language rules for it (even when they 
> make sense).
There's at least one other place where `-fno-exceptions` changes the language 
rules: we don't look for `operator delete` in a new-expression.  It does look 
like that's all I was remembering, though, and that we don't generally apply 
that same principle to destructors, so feel free to ignore my comment.

Still, could you make sure there's a test case that tests that we check access 
to the destructor of an array variable even when exceptions are disabled, since 
that's the interesting corner case created by this new condition?


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington marked 2 inline comments as done.
erik.pilkington added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

rjmccall wrote:
> erik.pilkington wrote:
> > rjmccall wrote:
> > > rsmith wrote:
> > > > Hmm, perhaps it would be cleaner if the destructor for array elements 
> > > > were required by the initialization of the array, instead of here. 
> > > > That's how this works for struct aggregate initialization (see 
> > > > `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it 
> > > > works for initialization by constructor, and so on. And it reflects the 
> > > > fact that it's the initialization process that needs the array element 
> > > > destructor, not the destruction of the variable (which never happens).
> > > > 
> > > > For the case of aggregate initialization of an array, we appear to fail 
> > > > to implement [dcl.init.aggr]/8:
> > > > 
> > > > """
> > > > The destructor for each element of class type is potentially invoked 
> > > > (11.3.6) from the context where the aggregate initialization occurs. 
> > > > [Note: This provision ensures that destructors can be called for 
> > > > fully-constructed subobjects in case an exception is thrown (14.2). — 
> > > > end note]
> > > > """
> > > > 
> > > > (But there doesn't appear to be any corresponding wording for default / 
> > > > value initialization of arrays. See also the special case at the end of 
> > > > `BuildCXXNewExpr`.)
> > > That makes sense to me.  The default/value initialization case seems like 
> > > an obvious oversight in the standard, but I think it might be a 
> > > distinction without a difference: the special cases for base-or-member 
> > > initializers and new-expressions might cover literally every situation 
> > > where initialization doesn't come with an adjacent need for 
> > > non-exceptional destruction.
> > Sure, done. Moving this to initialization time seems like a nice cleanup.
> Hmm.  What I just mentioned for the documentation is relevant here, right?  
> We only need to check access to subobject destructors if exceptions are 
> enabled, so this attempt to avoid duplicate diagnostics might leave us not 
> flagging the use if exceptions are disabled.
> 
> Well, maybe the check isn't actually restricted that way, hmm.  Shouldn't it 
> be?
I thought we tried in general to keep `-fno-exceptions` as similar to normal 
C++ as possible, rather than invent new language rules for it (even when they 
make sense).



Comment at: clang/lib/Sema/SemaInit.cpp:6328
+if (hasAccessibleDestructor(S.Context.getBaseElementType(AT), Loc, S))
+  return ExprError();
+

rjmccall wrote:
> There's no way that the right semantics are to return failure if the type has 
> an accessible destructor. :)  Looks like the function is misnamed.  Also, it 
> should also be named something that makes it clear that it's more than a 
> predicate, it's actually checking access to / marking the use of the 
> destructor.  I know this is an existing function, but could you take care of 
> that?
Yeah, the name doesn't make much sense. I'll update it...


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:3915
+Here, if the construction of `array[9]` fails with an exception, `array[0..8]`
+will be destroyed, so the element's destructor needs to be accessible.
   }];

Probably worth adding somewhere in here that is only required if exceptions are 
enabled, since disabling exceptions is a pretty common configuration.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

erik.pilkington wrote:
> rjmccall wrote:
> > rsmith wrote:
> > > Hmm, perhaps it would be cleaner if the destructor for array elements 
> > > were required by the initialization of the array, instead of here. That's 
> > > how this works for struct aggregate initialization (see 
> > > `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it works 
> > > for initialization by constructor, and so on. And it reflects the fact 
> > > that it's the initialization process that needs the array element 
> > > destructor, not the destruction of the variable (which never happens).
> > > 
> > > For the case of aggregate initialization of an array, we appear to fail 
> > > to implement [dcl.init.aggr]/8:
> > > 
> > > """
> > > The destructor for each element of class type is potentially invoked 
> > > (11.3.6) from the context where the aggregate initialization occurs. 
> > > [Note: This provision ensures that destructors can be called for 
> > > fully-constructed subobjects in case an exception is thrown (14.2). — end 
> > > note]
> > > """
> > > 
> > > (But there doesn't appear to be any corresponding wording for default / 
> > > value initialization of arrays. See also the special case at the end of 
> > > `BuildCXXNewExpr`.)
> > That makes sense to me.  The default/value initialization case seems like 
> > an obvious oversight in the standard, but I think it might be a distinction 
> > without a difference: the special cases for base-or-member initializers and 
> > new-expressions might cover literally every situation where initialization 
> > doesn't come with an adjacent need for non-exceptional destruction.
> Sure, done. Moving this to initialization time seems like a nice cleanup.
Hmm.  What I just mentioned for the documentation is relevant here, right?  We 
only need to check access to subobject destructors if exceptions are enabled, 
so this attempt to avoid duplicate diagnostics might leave us not flagging the 
use if exceptions are disabled.

Well, maybe the check isn't actually restricted that way, hmm.  Shouldn't it be?



Comment at: clang/lib/Sema/SemaInit.cpp:6328
+if (hasAccessibleDestructor(S.Context.getBaseElementType(AT), Loc, S))
+  return ExprError();
+

There's no way that the right semantics are to return failure if the type has 
an accessible destructor. :)  Looks like the function is misnamed.  Also, it 
should also be named something that makes it clear that it's more than a 
predicate, it's actually checking access to / marking the use of the 
destructor.  I know this is an existing function, but could you take care of 
that?


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 198916.
erik.pilkington marked 6 inline comments as done.
erik.pilkington added a comment.

Address review comments. Also remove the special case where we wouldn't check a 
destructor for an array in `-fno-exceptions` mode. This seems inconsistent with 
both how we're treating `-fno-exceptions` in general, and inconsistent with 
other cases of aggregate initialization (i.e. we still check struct members 
here).

In D61165#1494250 , @rjmccall wrote:

> All of the IRGen changes in this patch are unnecessary according to the model 
> we've worked out, right?  The fix is just to mark the destructor as still 
> used when we're constructing an array.


Yeah, the IRGen changes were to stop clang from referencing a non-existant dtor 
for a global no_destroy array, but if we're using it regardless then its not 
necessary. New patch removes it.


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

https://reviews.llvm.org/D61165

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGenCXX/no_destroy.cpp
  clang/test/SemaCXX/aggregate-initialization.cpp
  clang/test/SemaCXX/no_destroy.cpp

Index: clang/test/SemaCXX/no_destroy.cpp
===
--- clang/test/SemaCXX/no_destroy.cpp
+++ clang/test/SemaCXX/no_destroy.cpp
@@ -1,11 +1,13 @@
-// RUN: %clang_cc1 -DNO_DTORS -fno-c++-static-destructors -verify %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -DNO_EXCEPTIONS -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -DNO_EXCEPTIONS -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -fexceptions -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -fexceptions -verify %s
 
 struct SecretDestructor {
 #ifndef NO_DTORS
   // expected-note@+2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
@@ -44,3 +46,30 @@
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+// expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+SecretDestructor arr[10];
+
+void local_arrays() {
+  // expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+  static SecretDestructor arr2[10];
+  // expected-error@+1 {{calling a private destructor of class 'SecretDestructor'}}
+  thread_local SecretDestructor arr3[10];
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+void dontcrash() {
+  [[clang::no_destroy]] static Derived2 d2[] = {0, 0};
+}
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};
Index: clang/test/SemaCXX/aggregate-initialization.cpp
===
--- clang/test/SemaCXX/aggregate-initialization.cpp
+++ clang/test/SemaCXX/aggregate-initialization.cpp
@@ -196,7 +196,7 @@
   struct Y { X x; };
 
   void test0() {
-auto *y = new Y {}; // expected-error {{temporary of type 'ElementDestructor::X' has private destructor}}
+auto *y = new Y {}; // expected-error {{calling a private destructor of class 'ElementDestructor::X}}
   }
 
   struct S0 { int f; ~S0() = delete; }; // expected-note 3 {{'~S0' has been explicitly marked deleted here}}
Index: clang/test/CodeGenCXX/no_destroy.cpp
===
--- clang/test/CodeGenCXX/no_destroy.cpp
+++ clang/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ 

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:3897-3900
+This works in almost all cases, but if ``no_destroy`` is applied to a 
``static``
+or ``thread_local`` local builtin array variable and exceptions are enabled, 
the
+destructor is still needed to perform cleanup if the construction of an element
+of the array throws. For instance:

rsmith wrote:
> I think this is the wrong way to think about and describe this. 
> `[[no_destroy]]` removes the need for the type of the variable to have a 
> usable destructor. But all immediate subobjects still need usable 
> destructors, in case an exception is thrown during the object's construction. 
> This is then identical to the constraints on `new T` -- subobjects of `T` 
> (including array elements) still need to be destructible, but `T` itself does 
> not.
Sure, that makes sense. The new patch rephrases this.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119
+  // variable is static local array and exceptions are enabled, since then we
+  // need to clean up the elements.
+  if (VD->isNoDestroy(getASTContext()) &&

rjmccall wrote:
> This isn't "emitting" the destructor, it's "using" it.  Also, the comment 
> should make it clear that this is about aggregate initialization in general, 
> and it should contain a FIXME if there's part of that rule we're not 
> implementing correctly.
Are you alluding to the CodeGen bug? That seems unrelated to this function. 



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

rjmccall wrote:
> rsmith wrote:
> > Hmm, perhaps it would be cleaner if the destructor for array elements were 
> > required by the initialization of the array, instead of here. That's how 
> > this works for struct aggregate initialization (see 
> > `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it works 
> > for initialization by constructor, and so on. And it reflects the fact that 
> > it's the initialization process that needs the array element destructor, 
> > not the destruction of the variable (which never happens).
> > 
> > For the case of aggregate initialization of an array, we appear to fail to 
> > implement [dcl.init.aggr]/8:
> > 
> > """
> > The destructor for each element of class type is potentially invoked 
> > (11.3.6) from the context where the aggregate initialization occurs. [Note: 
> > This provision ensures that destructors can be called for fully-constructed 
> > subobjects in case an exception is thrown (14.2). — end note]
> > """
> > 
> > (But there doesn't appear to be any corresponding wording for default / 
> > value initialization of arrays. See also the special case at the end of 
> > `BuildCXXNewExpr`.)
> That makes sense to me.  The default/value initialization case seems like an 
> obvious oversight in the standard, but I think it might be a distinction 
> without a difference: the special cases for base-or-member initializers and 
> new-expressions might cover literally every situation where initialization 
> doesn't come with an adjacent need for non-exceptional destruction.
Sure, done. Moving this to initialization time seems like a nice cleanup.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

rsmith wrote:
> Hmm, perhaps it would be cleaner if the destructor for array elements were 
> required by the initialization of the array, instead of here. That's how this 
> works for struct aggregate initialization (see 
> `InitListChecker::CheckStructUnionTypes`), and (indirectly) how it works for 
> initialization by constructor, and so on. And it reflects the fact that it's 
> the initialization process that needs the array element destructor, not the 
> destruction of the variable (which never happens).
> 
> For the case of aggregate initialization of an array, we appear to fail to 
> implement [dcl.init.aggr]/8:
> 
> """
> The destructor for each element of class type is potentially invoked (11.3.6) 
> from the context where the aggregate initialization occurs. [Note: This 
> provision ensures that destructors can be called for fully-constructed 
> subobjects in case an exception is thrown (14.2). — end note]
> """
> 
> (But there doesn't appear to be any corresponding wording for default / value 
> initialization of arrays. See also the special case at the end of 
> `BuildCXXNewExpr`.)
That makes sense to me.  The default/value initialization case seems like an 
obvious oversight in the standard, but I think it might be a distinction 
without a difference: the special cases for base-or-member initializers and 
new-expressions might cover literally every situation where initialization 
doesn't come with an adjacent need for non-exceptional destruction.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122
+  if (VD->isNoDestroy(getASTContext()) &&
+  !(VD->getType()->isArrayType() && getLangOpts().Exceptions &&
+VD->isStaticLocal()))
 return;

Hmm, perhaps it would be cleaner if the destructor for array elements were 
required by the initialization of the array, instead of here. That's how this 
works for struct aggregate initialization (see 
`InitListChecker::CheckStructUnionTypes`), and (indirectly) how it works for 
initialization by constructor, and so on. And it reflects the fact that it's 
the initialization process that needs the array element destructor, not the 
destruction of the variable (which never happens).

For the case of aggregate initialization of an array, we appear to fail to 
implement [dcl.init.aggr]/8:

"""
The destructor for each element of class type is potentially invoked (11.3.6) 
from the context where the aggregate initialization occurs. [Note: This 
provision ensures that destructors can be called for fully-constructed 
subobjects in case an exception is thrown (14.2). — end note]
"""

(But there doesn't appear to be any corresponding wording for default / value 
initialization of arrays. See also the special case at the end of 
`BuildCXXNewExpr`.)


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

All of the IRGen changes in this patch are unnecessary according to the model 
we've worked out, right?  The fix is just to mark the destructor as still used 
when we're constructing an array.




Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13119
+  // variable is static local array and exceptions are enabled, since then we
+  // need to clean up the elements.
+  if (VD->isNoDestroy(getASTContext()) &&

This isn't "emitting" the destructor, it's "using" it.  Also, the comment 
should make it clear that this is about aggregate initialization in general, 
and it should contain a FIXME if there's part of that rule we're not 
implementing correctly.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1494001 , @rsmith wrote:

> In D61165#1492830 , @rjmccall wrote:
>
> > In D61165#1492781 , @rsmith wrote:
> >
> > > For the purposes of this patch, I think that means we never need a 
> > > destructor for the type of a `[[no_destroy]]` variable.
> >
> >
> > Arrays and other subobjects of an aggregate initializaton, unless applying 
> > the standard "literally" implies the obviously perverse result that we 
> > don't destroy subobjects in such cases.
>
>
> Yes, but you need the destructors for those subobjects as a side-condition of 
> the initialization, irrespective of what kind of object that initialization 
> is initializing. I don't think that's got anything to do with 
> `[[no_destroy]]`. I think it remains the case that you never need a 
> destructor for the type of a `[[no_destroy]]` variable.


Okay, so the rule should be that `[[no_destroy]]` never requires the destructor 
for the exact type of the variable, but it might require the destructors of 
sub-object types of a `static` local that uses aggregate initialization when 
exceptions are enabled, which necessarily includes the element type of an array.

> So far the opinion of folks on the core reflector has unanimously been that 
> (1) is the right model. And I think that makes sense: it would be somewhat 
> strange for the initialization of a complete object to be considered complete 
> after temporaries are destroyed, but the initializations of its subobjects to 
> be considered complete before the temporaries are destroyed.

Okay.  I'm not sure that's the decision I would make, but I can accept it, and 
it certainly simplifies the model in some ways.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

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

rsmith added a comment.

In D61165#1492830 , @rjmccall wrote:

> In D61165#1492781 , @rsmith wrote:
>
> > (Those destructor calls are separate full-expressions that happen 
> > afterwards; see [intro.execution]/5.)
>
>
> Huh?  The destruction of temporaries at the end of a full-expression is 
> definitely still part of the full-expression.  [class.temporary]p4: Temporary 
> objects are destroyed as the last step in evaluating the full-expression that 
> (lexically) contains the point where they were created.  [intro.execution] 
> clarifies that all cases of individual initializers are full-expressions.


Sorry, I confused myself. You're right, of course. But I don't think that makes 
a material difference to anything else.

>> For the purposes of this patch, I think that means we never need a 
>> destructor for the type of a `[[no_destroy]]` variable.
> 
> Arrays and other subobjects of an aggregate initializaton, unless applying 
> the standard "literally" implies the obviously perverse result that we don't 
> destroy subobjects in such cases.

Yes, but you need the destructors for those subobjects as a side-condition of 
the initialization, irrespective of what kind of object that initialization is 
initializing. I don't think that's got anything to do with `[[no_destroy]]`. I 
think it remains the case that you never need a destructor for the type of a 
`[[no_destroy]]` variable.

So far the opinion of folks on the core reflector has unanimously been that (1) 
is the right model. And I think that makes sense: it would be somewhat strange 
for the initialization of a complete object to be considered complete after 
temporaries are destroyed, but the initializations of its subobjects to be 
considered complete before the temporaries are destroyed.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1492781 , @rsmith wrote:

> In D61165#1490417 , @erik.pilkington 
> wrote:
>
> > In D61165#1490168 , @rjmccall 
> > wrote:
> >
> > > I think the intuitive rule is that initialization is complete when the 
> > > full-expression performing the initialization is complete because that's 
> > > the normal unit of sequencing.  Note that the standard does use both 
> > > "constructed" and "initialized" in different contexts, although this 
> > > might just be an editorial choice.
> >
> >
> > I think it would be quite subtle if the standard was deliberately splitting 
> > this particular hair by implicitly creating a "constructed, but not 
> > initialized" state of an object, without calling that out anywhere :)
>
>
> If by "constructed" you mean "a constructor has finished", then we need to 
> split this hair. Consider:
>
>   struct B { ~B(); };
>   struct A {
> A() {}
> A(B &&) : A() { throw 0; }
>   };
>   void f() {
> static A a = B();
>   }
>
>
> At the point when the exception is thrown, a constructor for `a` has 
> completed, but its initialization is not complete.
>
> [except.ctor]/3 and /4 lay out the rules:
>
> """
>  If the initialization or destruction of an object other than by delegating 
> constructor is terminated by an
>  exception, the destructor is invoked for each of the object’s direct 
> subobjects and, for a complete object,
>  virtual base class subobjects, whose initialization has completed (9.3) and 
> whose destructor has not yet begun
>  execution, except that in the case of destruction, the variant members of a 
> union-like class are not destroyed.
>  [Note: If such an object has a reference member that extends the lifetime of 
> a temporary object, this ends
>  the lifetime of the reference member, so the lifetime of the temporary 
> object is effectively not extended.
>  — end note] The subobjects are destroyed in the reverse order of the 
> completion of their construction. Such
>  destruction is sequenced before entering a handler of the function-try-block 
> of the constructor or destructor,
>  if any.
>
> If the compound-statement of the function-body of a delegating constructor 
> for an object exits via an
>  exception, the object’s destructor is invoked. Such destruction is sequenced 
> before entering a handler of the
>  function-try-block of a delegating constructor for that object, if any.
>  """
>
> The above wording seems to suggest that the initialization of an object of 
> class type completes when its outermost constructor finishes (at least for 
> the case of initialization by constructor). And indeed all the other wording 
> I can find that has some bearing on when an object is deemed initialized 
> suggests that option 1 is correct, and in general that the initialization of 
> a variable is complete when the initialization full-expression ends, which is 
> before the destructors for any temporaries run. (Those destructor calls are 
> separate full-expressions that happen afterwards; see [intro.execution]/5.)


Huh?  The destruction of temporaries at the end of a full-expression is 
definitely still part of the full-expression.  [class.temporary]p4: Temporary 
objects are destroyed as the last step in evaluating the full-expression that 
(lexically) contains the point where they were created.  [intro.execution] 
clarifies that all cases of individual initializers are full-expressions.

> For the purposes of this patch, I think that means we never need a destructor 
> for the type of a `[[no_destroy]]` variable.

Arrays and other subobjects of an aggregate initializaton, unless applying the 
standard "literally" implies the obviously perverse result that we don't 
destroy subobjects in such cases.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1492780 , @dexonsmith wrote:

> In D61165#1492724 , @rjmccall wrote:
>
> > In D61165#1492582 , 
> > @erik.pilkington wrote:
> >
> > > Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which 
> > > appears to claim that initialization ends with the execution of the 
> > > constructor, excluding temporary destructors.
> > >
> > > > (13) In a non-delegating constructor, initialization proceeds in the 
> > > > following order:
> > > >  /* list of steps... */
> > > >  (13.4) **Finally, the compound-statement of the constructor body is 
> > > > executed.**
> > >
> > > John: do you agree with this analysis?
> >
> >
> > That's talking about constructor bodies.
>
>
> Sure, but that's all that dcl.int suggests happens during initialization.  
> Skipping over paragraphs that don't apply:
>
> > http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as 
> > follows.
> > 
> > - http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a 
> > (possibly cv-qualified) class type:
> >   - http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is 
> > direct-initialization, or if it is copy-initialization where the 
> > cv-unqualified version of the source type is the same class as, or a 
> > derived class of, the class of the destination, constructors are 
> > considered. The applicable constructors are enumerated ([over.match.ctor]), 
> > and the best one is chosen through overload resolution ([over.match]). 
> > **Then:**
> > - http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is 
> > successful, **the selected constructor is called to initialize the 
> > object**, with the initializer expression or expression-list as its 
> > argument(s).


We've been talking about two things in this thread:

1. We've talked about aggregate initialization, which is a kind of 
list-initialization, for which this paragraph does not apply.
2. We've talked about temporaries in the initializer, which are not part of the 
constructor body and are definitely not ordered before the destruction of 
subobjects within the constructor when an exception is thrown.

That's why I don't think that section is determinative.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

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

rsmith added a comment.

In D61165#1490417 , @erik.pilkington 
wrote:

> In D61165#1490168 , @rjmccall wrote:
>
> > I think the intuitive rule is that initialization is complete when the 
> > full-expression performing the initialization is complete because that's 
> > the normal unit of sequencing.  Note that the standard does use both 
> > "constructed" and "initialized" in different contexts, although this might 
> > just be an editorial choice.
>
>
> I think it would be quite subtle if the standard was deliberately splitting 
> this particular hair by implicitly creating a "constructed, but not 
> initialized" state of an object, without calling that out anywhere :)


If by "constructed" you mean "a constructor has finished", then we need to 
split this hair. Consider:

  struct B { ~B(); };
  struct A {
A() {}
A(B &&) : A() { throw 0; }
  };
  void f() {
static A a = B();
  }

At the point when the exception is thrown, a constructor for `a` has completed, 
but its initialization is not complete.

[except.ctor]/3 and /4 lay out the rules:

"""
If the initialization or destruction of an object other than by delegating 
constructor is terminated by an
exception, the destructor is invoked for each of the object’s direct subobjects 
and, for a complete object,
virtual base class subobjects, whose initialization has completed (9.3) and 
whose destructor has not yet begun
execution, except that in the case of destruction, the variant members of a 
union-like class are not destroyed.
[Note: If such an object has a reference member that extends the lifetime of a 
temporary object, this ends
the lifetime of the reference member, so the lifetime of the temporary object 
is effectively not extended.
— end note] The subobjects are destroyed in the reverse order of the completion 
of their construction. Such
destruction is sequenced before entering a handler of the function-try-block of 
the constructor or destructor,
if any.

If the compound-statement of the function-body of a delegating constructor for 
an object exits via an
exception, the object’s destructor is invoked. Such destruction is sequenced 
before entering a handler of the
function-try-block of a delegating constructor for that object, if any.
"""

The above wording seems to suggest that the initialization of an object of 
class type completes when its outermost constructor finishes (at least for the 
case of initialization by constructor). And indeed all the other wording I can 
find that has some bearing on when an object is deemed initialized suggests 
that option 1 is correct, and in general that the initialization of a variable 
is complete when the initialization full-expression ends, which is before the 
destructors for any temporaries run. (Those destructor calls are separate 
full-expressions that happen afterwards; see [intro.execution]/5.)

Following the standard literally (and in particular the rule that destruction 
of temporaries is a separate full-expression and so -- presumably -- not part 
of the initialization of the variable), I think we must conclude that a 
temporary whose destructor throws does *not* destroy the static-storage 
duration variable (its initialization already completed). For the purposes of 
this patch, I think that means we never need a destructor for the type of a 
`[[no_destroy]]` variable.

I've mailed the core reflector to ask for clarification, but my reading is that 
(1) is the intended model.




Comment at: clang/include/clang/Basic/AttrDocs.td:3897-3900
+This works in almost all cases, but if ``no_destroy`` is applied to a 
``static``
+or ``thread_local`` local builtin array variable and exceptions are enabled, 
the
+destructor is still needed to perform cleanup if the construction of an element
+of the array throws. For instance:

I think this is the wrong way to think about and describe this. 
`[[no_destroy]]` removes the need for the type of the variable to have a usable 
destructor. But all immediate subobjects still need usable destructors, in case 
an exception is thrown during the object's construction. This is then identical 
to the constraints on `new T` -- subobjects of `T` (including array elements) 
still need to be destructible, but `T` itself does not.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D61165#1492724 , @rjmccall wrote:

> In D61165#1492582 , @erik.pilkington 
> wrote:
>
> > Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which 
> > appears to claim that initialization ends with the execution of the 
> > constructor, excluding temporary destructors.
> >
> > > (13) In a non-delegating constructor, initialization proceeds in the 
> > > following order:
> > >  /* list of steps... */
> > >  (13.4) **Finally, the compound-statement of the constructor body is 
> > > executed.**
> >
> > John: do you agree with this analysis?
>
>
> That's talking about constructor bodies.


Sure, but that's all that dcl.int suggests happens during initialization.  
Skipping over paragraphs that don't apply:

> http://eel.is/c++draft/dcl.init#17: The semantics of initializers are as 
> follows.
> 
> - http://eel.is/c++draft/dcl.init#17.6: [I]f the destination type is a 
> (possibly cv-qualified) class type:
>   - http://eel.is/c++draft/dcl.init#17.6.2: [I]f the initialization is 
> direct-initialization, or if it is copy-initialization where the 
> cv-unqualified version of the source type is the same class as, or a derived 
> class of, the class of the destination, constructors are considered. The 
> applicable constructors are enumerated ([over.match.ctor]), and the best one 
> is chosen through overload resolution ([over.match]). **Then:**
> - http://eel.is/c++draft/dcl.init#17.6.2.1: If overload resolution is 
> successful, **the selected constructor is called to initialize the object**, 
> with the initializer expression or expression-list as its argument(s).

Which I read as saying, "the selected constructor is the thing that initializes 
the object".  But, IIUC, you're saying the object isn't necessarily initialized 
when the selected constructor returns.  I can't find any text that would 
support your interpretation.

Let me argue from the other direction as well:

> http://eel.is/c++draft/dcl.init#21: An object whose initialization has 
> completed is deemed to be constructed, [...]

IIUC, you're suggesting that there could be a "constructed, but not yet 
initialized" state, but the definition of "constructed" seems to refute that.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1492582 , @erik.pilkington 
wrote:

> Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which 
> appears to claim that initialization ends with the execution of the 
> constructor, excluding temporary destructors.
>
> > (13) In a non-delegating constructor, initialization proceeds in the 
> > following order:
> >  /* list of steps... */
> >  (13.4) **Finally, the compound-statement of the constructor body is 
> > executed.**
>
> John: do you agree with this analysis?


That's talking about constructor bodies.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears 
to claim that initialization ends with the execution of the constructor, 
excluding temporary destructors.

> (13) In a non-delegating constructor, initialization proceeds in the 
> following order:
>  /* list of steps... */
>  (13.4) **Finally, the compound-statement of the constructor body is 
> executed.**

John: do you agree with this analysis?


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D61165#1490608 , @rjmccall wrote:

> The flip side of that argument is that (1) there aren't very many users right 
> now and (2) it's much easier to start conservative and then weaken the rule 
> than it will be to strengthen it later.  It really isn't acceptable to just 
> turn off access/use-checking for the destructor, so if we get trapped by the 
> choice we make here, we'll end up having to either leak or call 
> `std::terminate`.


I agree that'd we'd be much better off if we had to change our minds and relax 
the requirement here. Though we haven't been pushing on this, I would disagree 
with the point that there aren't many users, this was included in an open 
source release, an Xcode release, and there was a wg21 paper about it. That 
paper is currently the first result on google for the search "disabling static 
destructors". Hedging our bets here is an option, but I'd really like to avoid 
it if we can.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The flip side of that argument is that (1) there aren't very many users right 
now and (2) it's much easier to start conservative and then weaken the rule 
than it will be to strengthen it later.  It really isn't acceptable to just 
turn off access/use-checking for the destructor, so if we get trapped by the 
choice we make here, we'll end up having to either leak or call 
`std::terminate`.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I've only been lurking but FWIW (1) above makes the most sense to me, unless 
the Standard clearly draws a distinction between *constructed* and 
*initialized* in the way that was described, in which case (3) is the right 
approach. However, I would wait for at least a CWG issue to be filed to clarify 
the intent of the standard before adopting (3), otherwise it seems like we're 
adopting a slightly surprising behavior (and also one that's different from 
GCC) on a presumption of intent.

So for now I'd personally go with (1) and consider it a bugfix if the Standard 
decides to clarify intent in a way that (3) is the right thing to do -- we'll 
already have to change stuff anyway if that happens.

Also, I would personally be keen on potentially breaking source compatibility 
by doing access checking, as it's not clear to me at all that this is going to 
cause any actual breakage in the real world given the age and narrowness of the 
attribute.

Just my .02


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1490417 , @erik.pilkington 
wrote:

> In D61165#1490168 , @rjmccall wrote:
>
> > I think the intuitive rule is that initialization is complete when the 
> > full-expression performing the initialization is complete because that's 
> > the normal unit of sequencing.  Note that the standard does use both 
> > "constructed" and "initialized" in different contexts, although this might 
> > just be an editorial choice.
>
>
> I think it would be quite subtle if the standard was deliberately splitting 
> this particular hair by implicitly creating a "constructed, but not 
> initialized" state of an object, without calling that out anywhere :)


Well, but you're assuming that the standard is just using two different words 
for the same concept, often in close proximity.  That's probably against some 
canon of interpretation.

> As far as I see it, we have three options here:
> 
> 1. Claim that the object is formally initialized when the constructor returns
> 
>   This appears to be what GCC implements.

Outside of aggregate initialization, yes.  For aggregate initialization, GCC 
appears to treat the object as fully initialized as soon as its last member is 
constructed; until that point, no destructors are run for fully-constructed 
previous members, which is obviously a bug at some level.

I guess my constructed-vs-initialized rule would naturally suggest that 
exceptions thrown by full-expression destructors for the last member's 
initializer cause all the members to be individually destroyed without invoking 
the aggregate's destructor.  (Somewhat oddly, aggregates can have user-defined 
destructors, so this is a detectable difference.)  This is a little weird.

> 2. Claim that the object is formally initialized when the full-expression 
> ends, and if a temporary throws don't call the destructor because the object 
> isn't initialized.
> 
>   This is what Clang implements today, but seems wrong.

Yes, I acknowledged way upthread that this is a bug.

> 3. Claim that the object is formally initialized when the full-expression 
> ends, but if a temporary throws call the destructor because the constructor 
> ran.
> 
>   This seems weird to me. If destroying temporaries is an indivisible part of 
> the initialization of an object, then we shouldn't be able to call the 
> destructor, because the initialization of the object didn't succeed. (I mean, 
> assuming there isn't a distinction between constructed and initialized)

I think drawing that distinction is a necessary precursor to applying my rule.

>> there really can't be *that* many uses of this feature yet, especially in 
>> exceptions-enabled code. We can fix semantic mistakes.
> 
> Yeah, if we assert rule 3 then we I guess we should just do this, rather than 
> try to determine whether we need the dtor for the static local in Sema.
> 
> Anyways, I think I've layed out my thinking here as clearly as I can. If you 
> still think that 3 is right here, then I'm happy to defer to you (although it 
> would be quite nice if @rsmith chimed in too). I'm happy to implement 
> whatever the right thing for CodeGen to do here is too.

I would also be interested in getting Richard's opinion on this.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D61165#1490168 , @rjmccall wrote:

> I think the intuitive rule is that initialization is complete when the 
> full-expression performing the initialization is complete because that's the 
> normal unit of sequencing.  Note that the standard does use both 
> "constructed" and "initialized" in different contexts, although this might 
> just be an editorial choice.


I think it would be quite subtle if the standard was deliberately splitting 
this particular hair by implicitly creating a "constructed, but not 
initialized" state of an object, without calling that out anywhere :)

As far as I see it, we have three options here:

1. Claim that the object is formally initialized when the constructor returns

This appears to be what GCC implements.

2. Claim that the object is formally initialized when the full-expression ends, 
and if a temporary throws don't call the destructor because the object isn't 
initialized.

This is what Clang implements today, but seems wrong.

3. Claim that the object is formally initialized when the full-expression ends, 
but if a temporary throws call the destructor because the constructor ran.

This seems weird to me. If destroying temporaries is an indivisible part of the 
initialization of an object, then we shouldn't be able to call the destructor, 
because the initialization of the object didn't succeed. (I mean, assuming 
there isn't a distinction between constructed and initialized)

> I see it as inconsistent that a destructor for a temporary can abort the 
> initialization of an automatic object but not a static one. I have seen 
> programs that build idioms reliant on this kind of destructor ordering, e.g. 
> as a way to commit / abort a "transaction".

If we assert rule 1, then there isn't any inconsistency. The destructor of the 
temporary would never abort the initialization of an object when the object was 
initialized by a constructor, regardless of whether the object was static or 
automatic. The object would be considered initialized and destructed whenever 
appropriate for it's storage duration. I guess it would theoretically break 
that "transaction" pattern, buts its not like we or any other compiler actually 
supported that on static locals to begin with. We don't even do the "right 
thing" for automatics there, since we don't `invoke` the temporary destructor 
(see the IR I posted above).

> there really can't be *that* many uses of this feature yet, especially in 
> exceptions-enabled code. We can fix semantic mistakes.

Yeah, if we assert rule 3 then we I guess we should just do this, rather than 
try to determine whether we need the dtor for the static local in Sema.

Anyways, I think I've layed out my thinking here as clearly as I can. If you 
still think that 3 is right here, then I'm happy to defer to you (although it 
would be quite nice if @rsmith chimed in too). I'm happy to implement whatever 
the right thing for CodeGen to do here is too.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

>>> That's only true for subobjects of an enclosing aggregate before that 
>>> aggregate's initialization is complete though, right? So it doesn't seem 
>>> like that much of an inconsistency, just mimicking what we would be doing 
>>> if an exception was thrown in, say, the body of the ctor before the 
>>> object's initialization is completed.
>> 
>> Conceptually yes, but formally no.  The standard *could* write this rule as 
>> "all currently-initialized subobjects must be separately destroyed when an 
>> exception aborts initialization of the containing aggregate, including 
>> constructor bodies and aggregate initialization", but it doesn't actually do 
>> so; instead it has specific rules covering the behavior when an exception is 
>> thrown out of the body of a constructor, and those rules simply do not apply 
>> to aggregate initialization.
> 
> Right, I was just trying to draw an analogy. Can you be more specific about 
> the inconsistency you mentioned above? What objects with static storage 
> duration have to be destroyed when an exception is thrown? Just subobjects of 
> static aggregates that had their initialization aborted by an exception? If 
> so, that obviously doesn't seem inconsistent.

I see it as inconsistent that a destructor for a temporary can abort the 
initialization of an automatic object but not a static one.  I have seen 
programs that build idioms reliant on this kind of destructor ordering, e.g. as 
a way to commit / abort a "transaction".

>> Even if it did, though, that wouldn't tell us how to resolve this because 
>> this is fundamentally about when exactly the initialization of an object is 
>> "complete", which doesn't seem to be clearly defined in the standard.  
>> There's a rule for when a *constructor* is complete, but among other things, 
>> not all initializations involve constructors.
> 
> Yeah, I agree that this is the fundamental problem here, and that the 
> standard isn't much help. I'm trying to understand why you think this choice 
> of semantics is better (or at least, good enough to make us hedge our bets 
> here at the cost of keeping this feature simple and useful). I'm not seeing 
> the aggregate initialization inconsistency you mentioned above. If the 
> standard wanted us to call the destructor before the object's initialization 
> was formally complete, then that seems like something they would mention.  
> Other implementations (well, GCC) seem to have made the opposite decision, 
> which I think makes more intuitive sense.

I think the intuitive rule is that initialization is complete when the 
full-expression performing the initialization is complete because that's the 
normal unit of sequencing.  Note that the standard does use both "constructed" 
and "initialized" in different contexts, although this might just be an 
editorial choice.

>>> So what should that path forward be here? I'd really like to get this crash 
>>> fixed soon. If we want to consider a static local no_destroy dtor 
>>> potentially-invoked in Sema if the initializer has a temporary with a 
>>> throwing dtor, then we could do that, but it'd be unfortunate for 98/03 
>>> where I believe a dtor isn't noexcept by default, so we'd have to assume 
>>> the worst. I guess it'd be easier to change our minds in the future if we 
>>> treat the dtor as potentially-invoked, but I'm not really seeing the 
>>> argument that we shouldn't just use this rule.
>> 
>> I think the simplest rule would be to say that the destructor must still be 
>> accessible for static or thread-local locals and that it'll be used in 
>> certain cases when initialization is aborted.
> 
> If we did this the source compat problem would be a lot less theoretical.

That's a good point, but there really can't be *that* many uses of this feature 
yet, especially in exceptions-enabled code.  We can fix semantic mistakes.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D61165#1488657 , @rjmccall wrote:

> In D61165#1488553 , @erik.pilkington 
> wrote:
>
> > In D61165#1487328 , @rjmccall 
> > wrote:
> >
> > > In D61165#1487100 , 
> > > @erik.pilkington wrote:
> > >
> > > > It seems like the most common sense interpretation here is to just 
> > > > treat the initialization of `G` as completed at the point when the 
> > > > constructor finishes (this appears to be what GCC implements: 
> > > > https://wandbox.org/permlink/R3C9pPhoT4efAdL1).
> > >
> > >
> > > Your example doesn't actually demonstrate that that's what GCC implements 
> > > because it doesn't try to execute the declaration twice.  But you're 
> > > right, GCC does appear to consider the initialization complete as soon as 
> > > the static object's constructor returns normally.  On the other hand, GCC 
> > > gets the array case here wrong: if a static local has array type, and a 
> > > destructor for a temporary required by the first element initializer 
> > > throws, then it does not destroy the element but also (correctly) does 
> > > not mark the variable as fully initialized, and so a second attempt to 
> > > run the initializer will simply construct a new object on top of an 
> > > already-constructed one.  This is arguably correct under the standard — 
> > > the first array element is not a previously-constructed object of 
> > > automatic duration — but I hope it's obvious that that's a defect.
> > >
> > > > So if it was static it would just get destroyed at exit-time, and 
> > > > therefore should be disable-able with `no_destroy`. If the standard 
> > > > implies that we should be doing something else, then we should do that, 
> > > > but I can't seem to find any reference to the rule you're describing.
> > >
> > > Like I said, this is a poorly-specified part of the standard, because at 
> > > least *some* objects with static storage duration have to be destroyed 
> > > when an exception is thrown (because of aggregate initialization), but 
> > > the standard wants to pretend that this isn't true.  I think that 
> > > allowing temporary destructors to cancel initialization uniformly across 
> > > all kinds of objects is the most consistent rule,
> >
> >
> > That's only true for subobjects of an enclosing aggregate before that 
> > aggregate's initialization is complete though, right? So it doesn't seem 
> > like that much of an inconsistency, just mimicking what we would be doing 
> > if an exception was thrown in, say, the body of the ctor before the 
> > object's initialization is completed.
>
>
> Conceptually yes, but formally no.  The standard *could* write this rule as 
> "all currently-initialized subobjects must be separately destroyed when an 
> exception aborts initialization of the containing aggregate, including 
> constructor bodies and aggregate initialization", but it doesn't actually do 
> so; instead it has specific rules covering the behavior when an exception is 
> thrown out of the body of a constructor, and those rules simply do not apply 
> to aggregate initialization.


Right, I was just trying to draw an analogy. Can you be more specific about the 
inconsistency you mentioned above? What objects with static storage duration 
have to be destroyed when an exception is thrown? Just subobjects of static 
aggregates that had their initialization aborted by an exception? If so, that 
obviously doesn't seem inconsistent.

> Even if it did, though, that wouldn't tell us how to resolve this because 
> this is fundamentally about when exactly the initialization of an object is 
> "complete", which doesn't seem to be clearly defined in the standard.  
> There's a rule for when a *constructor* is complete, but among other things, 
> not all initializations involve constructors.

Yeah, I agree that this is the fundamental problem here, and that the standard 
isn't much help. I'm trying to understand why you think this choice of 
semantics is better (or at least, good enough to make us hedge our bets here at 
the cost of keeping this feature simple and useful). I'm not seeing the 
aggregate initialization inconsistency you mentioned above. If the standard 
wanted us to call the destructor before the object's initialization was 
formally complete, then that seems like something they would mention. Other 
implementations (well, GCC) seem to have made the opposite decision, which I 
think makes more intuitive sense.

>>> but if the standard wants to use a rule that non-aggregate initialization 
>>> of static and thread-local locals is complete as soon as the constructor 
>>> (or assignment) completes, as opposed to the end of the full-expression, 
>>> that's also implementable.
>> 
>> So what should that path forward be here? I'd really like to get this crash 
>> fixed soon. If we want to consider 

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1488553 , @erik.pilkington 
wrote:

> In D61165#1487328 , @rjmccall wrote:
>
> > In D61165#1487100 , 
> > @erik.pilkington wrote:
> >
> > > It seems like the most common sense interpretation here is to just treat 
> > > the initialization of `G` as completed at the point when the constructor 
> > > finishes (this appears to be what GCC implements: 
> > > https://wandbox.org/permlink/R3C9pPhoT4efAdL1).
> >
> >
> > Your example doesn't actually demonstrate that that's what GCC implements 
> > because it doesn't try to execute the declaration twice.  But you're right, 
> > GCC does appear to consider the initialization complete as soon as the 
> > static object's constructor returns normally.  On the other hand, GCC gets 
> > the array case here wrong: if a static local has array type, and a 
> > destructor for a temporary required by the first element initializer 
> > throws, then it does not destroy the element but also (correctly) does not 
> > mark the variable as fully initialized, and so a second attempt to run the 
> > initializer will simply construct a new object on top of an 
> > already-constructed one.  This is arguably correct under the standard — the 
> > first array element is not a previously-constructed object of automatic 
> > duration — but I hope it's obvious that that's a defect.
> >
> > > So if it was static it would just get destroyed at exit-time, and 
> > > therefore should be disable-able with `no_destroy`. If the standard 
> > > implies that we should be doing something else, then we should do that, 
> > > but I can't seem to find any reference to the rule you're describing.
> >
> > Like I said, this is a poorly-specified part of the standard, because at 
> > least *some* objects with static storage duration have to be destroyed when 
> > an exception is thrown (because of aggregate initialization), but the 
> > standard wants to pretend that this isn't true.  I think that allowing 
> > temporary destructors to cancel initialization uniformly across all kinds 
> > of objects is the most consistent rule,
>
>
> That's only true for subobjects of an enclosing aggregate before that 
> aggregate's initialization is complete though, right? So it doesn't seem like 
> that much of an inconsistency, just mimicking what we would be doing if an 
> exception was thrown in, say, the body of the ctor before the object's 
> initialization is completed.


Conceptually yes, but formally no.  The standard *could* write this rule as 
"all currently-initialized subobjects must be separately destroyed when an 
exception aborts initialization of the containing aggregate, including 
constructor bodies and aggregate initialization", but it doesn't actually do 
so; instead it has specific rules covering the behavior when an exception is 
thrown out of the body of a constructor, and those rules simply do not apply to 
aggregate initialization.

Even if it did, though, that wouldn't tell us how to resolve this because this 
is fundamentally about when exactly the initialization of an object is 
"complete", which doesn't seem to be clearly defined in the standard.  There's 
a rule for when a *constructor* is complete, but among other things, not all 
initializations involve constructors.

>> but if the standard wants to use a rule that non-aggregate initialization of 
>> static and thread-local locals is complete as soon as the constructor (or 
>> assignment) completes, as opposed to the end of the full-expression, that's 
>> also implementable.
> 
> So what should that path forward be here? I'd really like to get this crash 
> fixed soon. If we want to consider a static local no_destroy dtor 
> potentially-invoked in Sema if the initializer has a temporary with a 
> throwing dtor, then we could do that, but it'd be unfortunate for 98/03 where 
> I believe a dtor isn't noexcept by default, so we'd have to assume the worst. 
> I guess it'd be easier to change our minds in the future if we treat the dtor 
> as potentially-invoked, but I'm not really seeing the argument that we 
> shouldn't just use this rule.

I think the simplest rule would be to say that the destructor must still be 
accessible for static or thread-local locals and that it'll be used in certain 
cases when initialization is aborted.

>> I guess it would also technically be feasible for repeated attempts at 
>> initialization to just pick up after the last subobject to be successfully 
>> constructed, although that would be really obnoxious to actually implement 
>> (and would require an ABI change for static local aggregates with vague 
>> linkage).
> 
> Yeah, that seems quite strange, especially if initialization got picked up in 
> another thread :)

Actually, cross-thread isn't a problem because the C++ runtime already 
serializes all initialization attempts.


CHANGES SINCE LAST 

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D61165#1487328 , @rjmccall wrote:

> In D61165#1487100 , @erik.pilkington 
> wrote:
>
> > It seems like the most common sense interpretation here is to just treat 
> > the initialization of `G` as completed at the point when the constructor 
> > finishes (this appears to be what GCC implements: 
> > https://wandbox.org/permlink/R3C9pPhoT4efAdL1).
>
>
> Your example doesn't actually demonstrate that that's what GCC implements 
> because it doesn't try to execute the declaration twice.  But you're right, 
> GCC does appear to consider the initialization complete as soon as the static 
> object's constructor returns normally.  On the other hand, GCC gets the array 
> case here wrong: if a static local has array type, and a destructor for a 
> temporary required by the first element initializer throws, then it does not 
> destroy the element but also (correctly) does not mark the variable as fully 
> initialized, and so a second attempt to run the initializer will simply 
> construct a new object on top of an already-constructed one.  This is 
> arguably correct under the standard — the first array element is not a 
> previously-constructed object of automatic duration — but I hope it's obvious 
> that that's a defect.
>
> > So if it was static it would just get destroyed at exit-time, and therefore 
> > should be disable-able with `no_destroy`. If the standard implies that we 
> > should be doing something else, then we should do that, but I can't seem to 
> > find any reference to the rule you're describing.
>
> Like I said, this is a poorly-specified part of the standard, because at 
> least *some* objects with static storage duration have to be destroyed when 
> an exception is thrown (because of aggregate initialization), but the 
> standard wants to pretend that this isn't true.  I think that allowing 
> temporary destructors to cancel initialization uniformly across all kinds of 
> objects is the most consistent rule,


That's only true for subobjects of an enclosing aggregate before that 
aggregate's initialization is complete though, right? So it doesn't seem like 
that much of an inconsistency, just mimicking what we would be doing if an 
exception was thrown in, say, the body of the ctor before the object's 
initialization is completed. Maybe I'm misunderstanding?

> but if the standard wants to use a rule that non-aggregate initialization of 
> static and thread-local locals is complete as soon as the constructor (or 
> assignment) completes, as opposed to the end of the full-expression, that's 
> also implementable.

So what should that path forward be here? I'd really like to get this crash 
fixed soon. If we want to consider a static local no_destroy dtor 
potentially-invoked in Sema if the initializer has a temporary with a throwing 
dtor, then we could do that, but it'd be unfortunate for 98/03 where I believe 
a dtor isn't noexcept by default, so we'd have to assume the worst. I guess 
it'd be easier to change our minds in the future if we treat the dtor as 
potentially-invoked, but I'm not really seeing the argument that we shouldn't 
just use this rule.

> I guess it would also technically be feasible for repeated attempts at 
> initialization to just pick up after the last subobject to be successfully 
> constructed, although that would be really obnoxious to actually implement 
> (and would require an ABI change for static local aggregates with vague 
> linkage).

Yeah, that seems quite strange, especially if initialization got picked up in 
another thread :)


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D61165#1487100 , @erik.pilkington 
wrote:

> It seems like the most common sense interpretation here is to just treat the 
> initialization of `G` as completed at the point when the constructor finishes 
> (this appears to be what GCC implements: 
> https://wandbox.org/permlink/R3C9pPhoT4efAdL1).


Your example doesn't actually demonstrate that that's what GCC implements 
because it doesn't try to execute the declaration twice.  But you're right, GCC 
does appear to consider the initialization complete as soon as the static 
object's constructor returns normally.  On the other hand, GCC gets the array 
case here wrong: if a static local has array type, and a destructor for a 
temporary required by the first element initializer throws, then it does not 
destroy the element but also (correctly) does not mark the variable as fully 
initialized, and so a second attempt to run the initializer will simply 
construct a new object on top of an already-constructed one.  This is arguably 
correct under the standard — the first array element is not a 
previously-constructed object of automatic duration — but I hope it's obvious 
that that's a defect.

> So if it was static it would just get destroyed at exit-time, and therefore 
> should be disable-able with `no_destroy`. If the standard implies that we 
> should be doing something else, then we should do that, but I can't seem to 
> find any reference to the rule you're describing.

Like I said, this is a poorly-specified part of the standard, because at least 
*some* objects with static storage duration have to be destroyed when an 
exception is thrown (because of aggregate initialization), but the standard 
wants to pretend that this isn't true.  I think that allowing temporary 
destructors to cancel initialization uniformly across all kinds of objects is 
the most consistent rule, but if the standard wants to use a rule that 
non-aggregate initialization of static and thread-local locals is complete as 
soon as the constructor (or assignment) completes, as opposed to the end of the 
full-expression, that's also implementable.  I guess it would also technically 
be feasible for repeated attempts at initialization to just pick up after the 
last subobject to be successfully constructed, although that would be really 
obnoxious to actually implement (and would require an ABI change for static 
local aggregates with vague linkage).


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D61165#1485514 , @rjmccall wrote:

> Hmm.  You know, there's another case where the destructor can be called even 
> for a non-array: if constructing the object requires a temporary, I believe 
> an exception thrown from that temporary's destructor is supposed to go back 
> and destroy the variable.  (This is, sadly, not entirely clear under the 
> standard since the object is not automatic.)  Now, Clang does not actually 
> get this correct — we abort the construction, but we don't destroy the 
> variable — but (1) we should fix that someday and (2) in the meantime, we 
> shouldn't implement something which will be a problem when we go to fix that.


Just to be clear, you're thinking of something like this:

  struct S {
S();
~S() noexcept(false) { throw 0; }
  };
  
  struct G {
G(const S &) noexcept;
~G();
  };
  
  int main() { G g = S(); }

Which clang just generates straight-line code for, even in `-fexceptions` mode. 
This seems wrong.

  call void @_ZN1SC1Ev(%struct.S* %2)
  call void @_ZN1GC1ERK1S(%struct.G* %1, %struct.S* dereferenceable(1) %2) #4
  call void @_ZN1SD1Ev(%struct.S* %2)
  call void @_ZN1GD1Ev(%struct.G* %1) #4

It seems like the most common sense interpretation here is to just treat the 
initialization of `G` as completed at the point when the constructor finishes 
(this appears to be what GCC implements: 
https://wandbox.org/permlink/R3C9pPhoT4efAdL1). So if it was static it would 
just get destroyed at exit-time, and therefore should be disable-able with 
`no_destroy`. If the standard implies that we should be doing something else, 
then we should do that, but I can't seem to find any reference to the rule 
you're describing.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Hmm.  You know, there's another case where the destructor can be called even 
for a non-array: if constructing the object requires a temporary, I believe an 
exception thrown from that temporary's destructor is supposed to go back and 
destroy the variable.  (This is, sadly, not entirely clear under the standard 
since the object is not automatic.)  Now, Clang does not actually get this 
correct — we abort the construction, but we don't destroy the variable — but 
(1) we should fix that someday and (2) in the meantime, we shouldn't implement 
something which will be a problem when we go to fix that.

This doesn't affect non-locals because there the exception will call 
`std::terminate()` as specified in [except.terminate]p1.




Comment at: clang/include/clang/Basic/AttrDocs.td:3893
+~only_no_destroy();
+  }
+

Missing semicolon.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 197477.
erik.pilkington added a comment.

Add a try/catch block to the docs.


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

https://reviews.llvm.org/D61165

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGValue.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CodeGenCXX/no_destroy.cpp
  clang/test/SemaCXX/no_destroy.cpp

Index: clang/test/SemaCXX/no_destroy.cpp
===
--- clang/test/SemaCXX/no_destroy.cpp
+++ clang/test/SemaCXX/no_destroy.cpp
@@ -1,11 +1,13 @@
-// RUN: %clang_cc1 -DNO_DTORS -fno-c++-static-destructors -verify %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -DNO_EXCEPTIONS -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -DNO_EXCEPTIONS -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -fexceptions -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -fexceptions -verify %s
 
 struct SecretDestructor {
 #ifndef NO_DTORS
   // expected-note@+2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
@@ -44,3 +46,36 @@
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+SecretDestructor arr[10];
+#ifndef NO_DTORS
+// expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+
+void local_arrays() {
+  static SecretDestructor arr2[10];
+#if !defined(NO_DTORS) || !defined(NO_EXCEPTIONS)
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+  thread_local SecretDestructor arr3[10];
+#if !defined(NO_DTORS) || !defined(NO_EXCEPTIONS)
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+void dontcrash() {
+  [[clang::no_destroy]] static Derived2 d2[] = {0, 0};
+}
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};
Index: clang/test/CodeGenCXX/no_destroy.cpp
===
--- clang/test/CodeGenCXX/no_destroy.cpp
+++ clang/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ -25,7 +31,50 @@
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define void @_Z1gv
+void g() {
+  // CHECK-NOT: __cxa_atexit
+  [[clang::no_destroy]] static NonTrivial2 nt21;
+  // CHECK-NOT: _tlv_atexit
+  [[clang::no_destroy]] thread_local NonTrivial2 nt22;
+}
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial nt3;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] thread_local NonTrivial nt4;
+
+
+struct NonTrivial3 {
+  NonTrivial3();
+  ~NonTrivial3();
+};
+
+[[clang::no_destroy]] NonTrivial3 arr[10];
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// CHECK-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void h() {
+  [[clang::no_destroy]] static NonTrivial3 slarr[10];
+}
+
+// CHECK-LABEL: define void @_Z1hv
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void i() {
+  [[clang::no_destroy]] thread_local NonTrivial3 tlarr[10];
+}

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D61165#1479937 , @rjmccall wrote:

> Are you sure these are the right semantics for `nodestroy`?  I think there's 
> a reasonable argument that we should not destroy previous elements of a 
> `nodestroy` array just because an element constructor throws.  It's basically 
> academic for true globals because the exception will terminate the process 
> anyway, and even for `thread_local`s and `static` locals (where I believe the 
> exception is theoretically recoverable) it's at least arguable that we should 
> either decline to destroy (possibly causing leaks) or simply call 
> `std::terminate`.


I think `std::terminate` makes the most sense. Getting teardown correctly is 
always tricky, and I'm willing to bet that teardown caused by an exception in 
construction of an array is even harder and done wrong.




Comment at: clang/include/clang/Basic/AttrDocs.td:3906
+[[clang::no_destroy]]
+static only_no_destroy array[10]; // error, only_no_destroy has a private 
destructor.
+

You probably want a `try`/`catch` here to illustrate why exceptions can matter.


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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 197453.
erik.pilkington added a comment.

Address review comments.


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

https://reviews.llvm.org/D61165

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGValue.h
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CodeGenCXX/no_destroy.cpp
  clang/test/SemaCXX/no_destroy.cpp

Index: clang/test/SemaCXX/no_destroy.cpp
===
--- clang/test/SemaCXX/no_destroy.cpp
+++ clang/test/SemaCXX/no_destroy.cpp
@@ -1,11 +1,13 @@
-// RUN: %clang_cc1 -DNO_DTORS -fno-c++-static-destructors -verify %s
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -DNO_EXCEPTIONS -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -DNO_EXCEPTIONS -verify %s
+// RUN: %clang_cc1 -DNO_DTORS -fexceptions -fno-c++-static-destructors -verify %s
+// RUN: %clang_cc1 -fexceptions -verify %s
 
 struct SecretDestructor {
 #ifndef NO_DTORS
   // expected-note@+2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
@@ -44,3 +46,36 @@
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+SecretDestructor arr[10];
+#ifndef NO_DTORS
+// expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+
+void local_arrays() {
+  static SecretDestructor arr2[10];
+#if !defined(NO_DTORS) || !defined(NO_EXCEPTIONS)
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+  thread_local SecretDestructor arr3[10];
+#if !defined(NO_DTORS) || !defined(NO_EXCEPTIONS)
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+void dontcrash() {
+  [[clang::no_destroy]] static Derived2 d2[] = {0, 0};
+}
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};
Index: clang/test/CodeGenCXX/no_destroy.cpp
===
--- clang/test/CodeGenCXX/no_destroy.cpp
+++ clang/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
-// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,NO_EXCEPTIONS
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ -25,7 +31,50 @@
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define void @_Z1gv
+void g() {
+  // CHECK-NOT: __cxa_atexit
+  [[clang::no_destroy]] static NonTrivial2 nt21;
+  // CHECK-NOT: _tlv_atexit
+  [[clang::no_destroy]] thread_local NonTrivial2 nt22;
+}
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial nt3;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] thread_local NonTrivial nt4;
+
+
+struct NonTrivial3 {
+  NonTrivial3();
+  ~NonTrivial3();
+};
+
+[[clang::no_destroy]] NonTrivial3 arr[10];
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// CHECK-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void h() {
+  [[clang::no_destroy]] static NonTrivial3 slarr[10];
+}
+
+// CHECK-LABEL: define void @_Z1hv
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
+
+void i() {
+  [[clang::no_destroy]] thread_local NonTrivial3 tlarr[10];
+}
+
+// 

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Okay, SGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington planned changes to this revision.
erik.pilkington added a comment.

In D61165#1480976 , @rjmccall wrote:

> I believe at least one of the goals of `nodestroy` is to allow the type to 
> potentially not provide a destructor at all, so if we're going to implicitly 
> require the destructor anyway in certain situations, we should clearly 
> document that, and we should be aware that we may be making the attribute 
> less useful.
>
> Since I believe the dominant use-case here is a true global, does only 
> requiring the destructor for arrays in the static-local case when exceptions 
> are enabled at least make it acceptable to do proper access checking, or is 
> that still a source-compatibility problem for existing clients?


It's hard to say exactly how bad the source compatibility problem is, we 
haven't been pushing on this much internally (yet), so I doubt we'll run into 
any issues. There was included in an open-source release though. If we only 
checked access when we needed it, the sequence of steps to actually break 
anything is pretty far: you'd have to have a type with a private dtor, used in 
a static local array, with the attribute, and be compiling with exceptions. I'd 
actually be quite surprised if we ran into any issues, so I guess we should do 
the right thing with access checking.

I'll update the patch and the docs.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I believe at least one of the goals of `nodestroy` is to allow the type to 
potentially not provide a destructor at all, so if we're going to implicitly 
require the destructor anyway in certain situations, we should clearly document 
that, and we should be aware that we may be making the attribute less useful.

Since I believe the dominant use-case here is a true global, does only 
requiring the destructor for arrays in the static-local case when exceptions 
are enabled at least make it acceptable to do proper access checking, or is 
that still a source-compatibility problem for existing clients?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

By using `no_destroy`, you're saying that exit-time destructor doesn't matter 
because the OS will either reclaim the resources automatically, or its just 
doesn't matter since the process is going down. I don't think that implies that 
we can safely ignore the destructor in the middle of the program's execution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

JF, Michael, and I were talking about this offline, and we think that the right 
choice of semantics for the static local case is to call the destructors.

  struct HoldsResource {
HoldsResource() { tryToAcquireItMayThrow(); }
~HoldsResource() { releaseIt(); }
  };
  
  void doSomeThings() {
try { 
  [[clang::no_destroy]] static HoldsResource MyResources[10];
} catch (...) {
  /* recover gracefully somehow */
}
  }

Here, its possible to call `doSomeThings` many times, until it actually manages 
to construct `MyResources`. Just not calling the dtor doesn't seem right since 
we'd be leaking resources. Calling `terminate` doesn't make sense either, since 
its possible to recover from this and try again or continue. `no_destroy` 
doesn't mean don't destroy (lol), it means don't register exit-time dtors, 
that's why it only applies to static/thread local declarations. @rjmccall: 
WDYT? This is obviously a pretty narrow edge-case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D61165#1479937 , @rjmccall wrote:

> Are you sure these are the right semantics for `nodestroy`?  I think there's 
> a reasonable argument that we should not destroy previous elements of a 
> `nodestroy` array just because an element constructor throws.  It's basically 
> academic for true globals because the exception will terminate the process 
> anyway, and even for `thread_local`s and `static` locals (where I believe the 
> exception is theoretically recoverable) it's at least arguable that we should 
> either decline to destroy (possibly causing leaks) or simply call 
> `std::terminate`.


I think `std::terminate` makes the most sense. Getting teardown correctly is 
always tricky, and I'm willing to bet that teardown caused by an exception in 
construction of an array is even harder and done wrong.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Are you sure these are the right semantics for `nodestroy`?  I think there's a 
reasonable argument that we should not destroy previous elements of a 
`nodestroy` array just because an element constructor throws.  It's basically 
academic for true globals because the exception will terminate the process 
anyway, and even for `thread_local`s and `static` locals (where I believe the 
exception is theoretically recoverable) it's at least arguable that we should 
either decline to destroy (possibly causing leaks) or simply call 
`std::terminate`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61165



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

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

[Erik Pilkington via Phabricator via cfe-commits]

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

Previously, we didn't mark an array's element type's destructor referenced when 
it was annotated with no_destroy. This is not correct: we still need the 
destructor if we need to do any cleanup if an element's constructor throws. 
This was leading to crashes and linker errors. The fix is just to mark the 
destructor referenced in the array case.

This leads to an inconsistency with access control: If the array element type's 
destructor is used, then we really ought check its access. However, that would 
be a source breaking change. This patch ignores access checking, which is a bit 
unfortunate, but I think its the best we can do if we're not willing to break 
source compatibility.

Fixes rdar://48462498

Thanks!
Erik


Repository:
  rC Clang

https://reviews.llvm.org/D61165

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CodeGenCXX/no_destroy.cpp
  clang/test/SemaCXX/no_destroy.cpp

Index: clang/test/SemaCXX/no_destroy.cpp
===
--- clang/test/SemaCXX/no_destroy.cpp
+++ clang/test/SemaCXX/no_destroy.cpp
@@ -6,6 +6,9 @@
   // expected-note@+2 4 {{private}}
 #endif
 private: ~SecretDestructor(); // expected-note 2 {{private}}
+#ifndef NO_DTORS
+  // expected-note@-2 3 {{private}}
+#endif
 };
 
 SecretDestructor sd1;
@@ -44,3 +47,32 @@
 
 [[clang::no_destroy(0)]] int no_args; // expected-error{{'no_destroy' attribute takes no arguments}}
 [[clang::always_destroy(0)]] int no_args2; // expected-error{{'always_destroy' attribute takes no arguments}}
+
+SecretDestructor arr[10];
+#ifndef NO_DTORS
+// expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+
+void local_arrays() {
+  static SecretDestructor arr2[10];
+#ifndef NO_DTORS
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+  thread_local SecretDestructor arr3[10];
+#ifndef NO_DTORS
+  // expected-error@-2 {{variable of type 'SecretDestructor [10]' has private destructor}}
+#endif
+}
+
+struct Base {
+  ~Base();
+};
+struct Derived1 {
+  Derived1(int);
+  Base b;
+};
+struct Derived2 {
+  Derived1 b;
+};
+
+[[clang::no_destroy]] Derived2 d2[] = {0, 0};
Index: clang/test/CodeGenCXX/no_destroy.cpp
===
--- clang/test/CodeGenCXX/no_destroy.cpp
+++ clang/test/CodeGenCXX/no_destroy.cpp
@@ -1,11 +1,14 @@
 // RUN: %clang_cc1 %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s
+// RUN: %clang_cc1 -fexceptions %s -emit-llvm -triple x86_64-apple-macosx10.13.0 -o - | FileCheck %s --check-prefixes=CHECK,EXCEPTIONS
 
 struct NonTrivial {
   ~NonTrivial();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] NonTrivial nt1;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK-NOT: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::no_destroy]] thread_local NonTrivial nt2;
 
@@ -13,11 +16,14 @@
   ~NonTrivial2();
 };
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
 NonTrivial2 nt21;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN11NonTrivial2D1Ev
 thread_local NonTrivial2 nt22;
 
+// CHECK-LABEL: define void @_Z1fv
 void f() {
   // CHECK: __cxa_atexit{{.*}}_ZN11NonTrivial2D1Ev
   static NonTrivial2 nt21;
@@ -25,7 +31,21 @@
   thread_local NonTrivial2 nt22;
 }
 
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: __cxa_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] NonTrivial nt3;
+// CHECK-LABEL: define internal void @__cxx_global_var_init
 // CHECK: _tlv_atexit{{.*}}_ZN10NonTrivialD1Ev
 [[clang::always_destroy]] thread_local NonTrivial nt4;
+
+
+struct NonTrivial3 {
+  NonTrivial3();
+  ~NonTrivial3();
+};
+[[clang::no_destroy]] NonTrivial3 arr[10];
+
+// CHECK-LABEL: define internal void @__cxx_global_var_init
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: call i32 @__cxa_atexit
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -13108,11 +13108,16 @@
   if (ClassDecl->hasIrrelevantDestructor()) return;
   if (ClassDecl->isDependentContext()) return;
 
-  if (VD->isNoDestroy(getASTContext()))
+  bool IsNoDestroy = VD->isNoDestroy(getASTContext());
+  if (IsNoDestroy && !VD->getType()->isArrayType())
 return;
 
   CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl);
   MarkFunctionReferenced(VD->getLocation(), Destructor);
+
+  if (IsNoDestroy)
+return;
+
   CheckDestructorAccess(VD->getLocation(),