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 <https://reviews.llvm.org/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;
@@ -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
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-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];
+}
+
+// CHECK-LABEL: define void @_Z1iv
+// CHECK: {{invoke|call}} void @_ZN11NonTrivial3C1Ev
+// EXCEPTIONS: call void @_ZN11NonTrivial3D1Ev
+// NO_EXCEPTIONS-NOT: call void @_ZN11NonTrivial3D1Ev
+// CHECK-NOT: _tlv_atexit
Index: clang/lib/Sema/SemaInit.cpp
===================================================================
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -1707,6 +1707,30 @@
}
}
+/// Check if the type of a class element has an accessible destructor.
+///
+/// Aggregate initialization requires a class element's destructor be
+/// accessible per 11.6.1 [dcl.init.aggr]:
+///
+/// The destructor for each element of class type is potentially invoked
+/// (15.4 [class.dtor]) from the context where the aggregate initialization
+/// occurs.
+static bool hasAccessibleDestructor(QualType ElementType, SourceLocation Loc,
+ Sema &SemaRef) {
+ auto *CXXRD = ElementType->getAsCXXRecordDecl();
+ if (!CXXRD)
+ return false;
+
+ CXXDestructorDecl *Destructor = SemaRef.LookupDestructor(CXXRD);
+ SemaRef.CheckDestructorAccess(Loc, Destructor,
+ SemaRef.PDiag(diag::err_access_dtor)
+ << ElementType);
+ SemaRef.MarkFunctionReferenced(Loc, Destructor);
+ if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc))
+ return true;
+ return false;
+}
+
void InitListChecker::CheckArrayType(const InitializedEntity &Entity,
InitListExpr *IList, QualType &DeclType,
llvm::APSInt elementIndex,
@@ -1716,6 +1740,14 @@
unsigned &StructuredIndex) {
const ArrayType *arrayType = SemaRef.Context.getAsArrayType(DeclType);
+ if (!VerifyOnly) {
+ if (hasAccessibleDestructor(arrayType->getElementType(),
+ IList->getEndLoc(), SemaRef)) {
+ hadError = true;
+ return;
+ }
+ }
+
// Check for the special-case of initializing an array with a string.
if (Index < IList->getNumInits()) {
if (IsStringInit(IList->getInit(Index), arrayType, SemaRef.Context) ==
@@ -1877,30 +1909,6 @@
return FlexArrayDiag != diag::ext_flexible_array_init;
}
-/// Check if the type of a class element has an accessible destructor.
-///
-/// Aggregate initialization requires a class element's destructor be
-/// accessible per 11.6.1 [dcl.init.aggr]:
-///
-/// The destructor for each element of class type is potentially invoked
-/// (15.4 [class.dtor]) from the context where the aggregate initialization
-/// occurs.
-static bool hasAccessibleDestructor(QualType ElementType, SourceLocation Loc,
- Sema &SemaRef) {
- auto *CXXRD = ElementType->getAsCXXRecordDecl();
- if (!CXXRD)
- return false;
-
- CXXDestructorDecl *Destructor = SemaRef.LookupDestructor(CXXRD);
- SemaRef.CheckDestructorAccess(Loc, Destructor,
- SemaRef.PDiag(diag::err_access_dtor_temp)
- << ElementType);
- SemaRef.MarkFunctionReferenced(Loc, Destructor);
- if (SemaRef.DiagnoseUseOfDecl(Destructor, Loc))
- return true;
- return false;
-}
-
void InitListChecker::CheckStructUnionTypes(
const InitializedEntity &Entity, InitListExpr *IList, QualType DeclType,
CXXRecordDecl::base_class_range Bases, RecordDecl::field_iterator Field,
@@ -6315,6 +6323,10 @@
if (S.DiagnoseUseOfDecl(Step.Function.FoundDecl, Loc))
return ExprError();
+ if (const ArrayType *AT = S.Context.getAsArrayType(Entity.getType()))
+ if (hasAccessibleDestructor(S.Context.getBaseElementType(AT), Loc, S))
+ return ExprError();
+
if (shouldBindAsTemporary(Entity))
CurInit = S.MaybeBindToTemporary(CurInit.get());
Index: clang/lib/Sema/SemaExprCXX.cpp
===================================================================
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -2166,24 +2166,6 @@
MarkFunctionReferenced(StartLoc, OperatorDelete);
}
- // C++0x [expr.new]p17:
- // If the new expression creates an array of objects of class type,
- // access and ambiguity control are done for the destructor.
- QualType BaseAllocType = Context.getBaseElementType(AllocType);
- if (ArraySize && !BaseAllocType->isDependentType()) {
- if (const RecordType *BaseRecordType = BaseAllocType->getAs<RecordType>()) {
- if (CXXDestructorDecl *dtor = LookupDestructor(
- cast<CXXRecordDecl>(BaseRecordType->getDecl()))) {
- MarkFunctionReferenced(StartLoc, dtor);
- CheckDestructorAccess(StartLoc, dtor,
- PDiag(diag::err_access_dtor)
- << BaseAllocType);
- if (DiagnoseUseOfDecl(dtor, StartLoc))
- return ExprError();
- }
- }
- }
-
return CXXNewExpr::Create(Context, UseGlobal, OperatorNew, OperatorDelete,
PassAlignment, UsualArrayDeleteWantsSize,
PlacementArgs, TypeIdParens, ArraySize, initStyle,
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -13117,12 +13117,17 @@
return;
CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl);
- MarkFunctionReferenced(VD->getLocation(), Destructor);
- CheckDestructorAccess(VD->getLocation(), Destructor,
- PDiag(diag::err_access_dtor_var)
- << VD->getDeclName()
- << VD->getType());
- DiagnoseUseOfDecl(Destructor, VD->getLocation());
+
+ // If this is an array, we'll require the destructor during initialization, so
+ // we can skip over this. We still want to emit exit-time destructor warnings
+ // though.
+ if (!VD->getType()->isArrayType()) {
+ MarkFunctionReferenced(VD->getLocation(), Destructor);
+ CheckDestructorAccess(VD->getLocation(), Destructor,
+ PDiag(diag::err_access_dtor_var)
+ << VD->getDeclName() << VD->getType());
+ DiagnoseUseOfDecl(Destructor, VD->getLocation());
+ }
if (Destructor->isTrivial()) return;
if (!VD->hasGlobalStorage()) return;
Index: clang/include/clang/Basic/AttrDocs.td
===================================================================
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/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.
}];
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
