r360446 - [Sema] Mark array element destructors referenced during initialization

Erik Pilkington via cfe-commits cfe-commits at lists.llvm.org
Fri May 10 10:52:27 PDT 2019


Author: epilk
Date: Fri May 10 10:52:26 2019
New Revision: 360446

URL: http://llvm.org/viewvc/llvm-project?rev=360446&view=rev
Log:
[Sema] Mark array element destructors referenced during initialization

This fixes a crash where we would neglect to mark a destructor referenced for an
__attribute__((no_destory)) array. The destructor is needed though, since if an
exception is thrown we need to cleanup the elements.

rdar://48462498

Differential revision: https://reviews.llvm.org/D61165

Modified:
    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

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Fri May 10 10:52:26 2019
@@ -3880,6 +3880,39 @@ The ``no_destroy`` attribute specifies t
 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.
   }];
 }
 

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Fri May 10 10:52:26 2019
@@ -13098,12 +13098,17 @@ void Sema::FinalizeVarWithDestructor(Var
     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;

Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Fri May 10 10:52:26 2019
@@ -2200,24 +2200,6 @@ Sema::BuildCXXNew(SourceRange Range, boo
     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,

Modified: cfe/trunk/lib/Sema/SemaInit.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)
+++ cfe/trunk/lib/Sema/SemaInit.cpp Fri May 10 10:52:26 2019
@@ -1707,6 +1707,30 @@ void InitListChecker::CheckVectorType(co
   }
 }
 
+/// Check if the type of a class element has an accessible destructor, and marks
+/// it referenced. Returns true if we shouldn't form a reference to the
+/// 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 checkDestructorReference(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);
+  return SemaRef.DiagnoseUseOfDecl(Destructor, Loc);
+}
+
 void InitListChecker::CheckArrayType(const InitializedEntity &Entity,
                                      InitListExpr *IList, QualType &DeclType,
                                      llvm::APSInt elementIndex,
@@ -1716,6 +1740,14 @@ void InitListChecker::CheckArrayType(con
                                      unsigned &StructuredIndex) {
   const ArrayType *arrayType = SemaRef.Context.getAsArrayType(DeclType);
 
+  if (!VerifyOnly) {
+    if (checkDestructorReference(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 @@ bool InitListChecker::CheckFlexibleArray
   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,
@@ -1924,7 +1932,7 @@ void InitListChecker::CheckStructUnionTy
     if (!VerifyOnly)
       for (FieldDecl *FD : RD->fields()) {
         QualType ET = SemaRef.Context.getBaseElementType(FD->getType());
-        if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+        if (checkDestructorReference(ET, IList->getEndLoc(), SemaRef)) {
           hadError = true;
           return;
         }
@@ -1984,7 +1992,7 @@ void InitListChecker::CheckStructUnionTy
     }
 
     if (!VerifyOnly)
-      if (hasAccessibleDestructor(Base.getType(), InitLoc, SemaRef)) {
+      if (checkDestructorReference(Base.getType(), InitLoc, SemaRef)) {
         hadError = true;
         return;
       }
@@ -2026,7 +2034,7 @@ void InitListChecker::CheckStructUnionTy
         while (std::next(F) != Field)
           ++F;
         QualType ET = SemaRef.Context.getBaseElementType(F->getType());
-        if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+        if (checkDestructorReference(ET, InitLoc, SemaRef)) {
           hadError = true;
           return;
         }
@@ -2075,7 +2083,7 @@ void InitListChecker::CheckStructUnionTy
 
     if (!VerifyOnly) {
       QualType ET = SemaRef.Context.getBaseElementType(Field->getType());
-      if (hasAccessibleDestructor(ET, InitLoc, SemaRef)) {
+      if (checkDestructorReference(ET, InitLoc, SemaRef)) {
         hadError = true;
         return;
       }
@@ -2131,7 +2139,7 @@ void InitListChecker::CheckStructUnionTy
                                                      : Field;
     for (RecordDecl::field_iterator E = RD->field_end(); I != E; ++I) {
       QualType ET = SemaRef.Context.getBaseElementType(I->getType());
-      if (hasAccessibleDestructor(ET, IList->getEndLoc(), SemaRef)) {
+      if (checkDestructorReference(ET, IList->getEndLoc(), SemaRef)) {
         hadError = true;
         return;
       }
@@ -6325,6 +6333,10 @@ PerformConstructorInitialization(Sema &S
   if (S.DiagnoseUseOfDecl(Step.Function.FoundDecl, Loc))
     return ExprError();
 
+  if (const ArrayType *AT = S.Context.getAsArrayType(Entity.getType()))
+    if (checkDestructorReference(S.Context.getBaseElementType(AT), Loc, S))
+      return ExprError();
+
   if (shouldBindAsTemporary(Entity))
     CurInit = S.MaybeBindToTemporary(CurInit.get());
 

Modified: cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp (original)
+++ cfe/trunk/test/CXX/expr/expr.unary/expr.new/p17.cpp Fri May 10 10:52:26 2019
@@ -10,7 +10,7 @@ class dtor {
 
 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}}
 }

Modified: cfe/trunk/test/CodeGenCXX/no_destroy.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/no_destroy.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/no_destroy.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/no_destroy.cpp Fri May 10 10:52:26 2019
@@ -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 @@ struct NonTrivial2 {
   ~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 @@ void f() {
   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

Modified: cfe/trunk/test/SemaCXX/no_destroy.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/no_destroy.cpp?rev=360446&r1=360445&r2=360446&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/no_destroy.cpp (original)
+++ cfe/trunk/test/SemaCXX/no_destroy.cpp Fri May 10 10:52:26 2019
@@ -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 at +2 4 {{private}}
 #endif
-private: ~SecretDestructor(); // expected-note 2 {{private}}
+private: ~SecretDestructor(); // expected-note + {{private}}
 };
 
 SecretDestructor sd1;
@@ -44,3 +46,30 @@ struct [[clang::no_destroy]] DoesntApply
 
 [[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 at +1 {{temporary of type 'SecretDestructor' has private destructor}}
+SecretDestructor arr[10];
+
+void local_arrays() {
+  // expected-error at +1 {{temporary of type 'SecretDestructor' has private destructor}}
+  static SecretDestructor arr2[10];
+  // expected-error at +1 {{temporary of type 'SecretDestructor' has private destructor}}
+  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};




More information about the cfe-commits mailing list