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]; +} + +// 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/SemaDeclCXX.cpp =================================================================== --- clang/lib/Sema/SemaDeclCXX.cpp +++ clang/lib/Sema/SemaDeclCXX.cpp @@ -13113,7 +13113,13 @@ if (ClassDecl->hasIrrelevantDestructor()) return; if (ClassDecl->isDependentContext()) return; - if (VD->isNoDestroy(getASTContext())) + // If a variable is no_destroy, either due to an attribute or flag, don't emit + // the destructor. As a corner case, we still need the destructor if the + // variable is static local array and exceptions are enabled, since then we + // need to clean up the elements. + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && + VD->isStaticLocal())) return; CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl); Index: clang/lib/CodeGen/CGValue.h =================================================================== --- clang/lib/CodeGen/CGValue.h +++ clang/lib/CodeGen/CGValue.h @@ -485,6 +485,12 @@ /// them. bool SanitizerCheckedFlag : 1; + /// If set to true, this slot corresponds to a static or thread local global + /// variable that was annotated with the no_destroy attribute. These have + /// slightly different semantics from no_destroy static locals, where + /// exceptions are recoverable. + bool NoDestroyGlobalFlag : 1; + public: enum IsAliased_t { IsNotAliased, IsAliased }; enum IsDestructed_t { IsNotDestructed, IsDestructed }; @@ -492,6 +498,7 @@ enum Overlap_t { DoesNotOverlap, MayOverlap }; enum NeedsGCBarriers_t { DoesNotNeedGCBarriers, NeedsGCBarriers }; enum IsSanitizerChecked_t { IsNotSanitizerChecked, IsSanitizerChecked }; + enum IsNoDestroyGlobal_t { IsNotNoDestroyGlobal, IsNoDestroyGlobal }; /// ignored - Returns an aggregate value slot indicating that the /// aggregate value is being ignored. @@ -510,14 +517,12 @@ /// for calling destructors on this object /// \param needsGC - true if the slot is potentially located /// somewhere that ObjC GC calls should be emitted for - static AggValueSlot forAddr(Address addr, - Qualifiers quals, - IsDestructed_t isDestructed, - NeedsGCBarriers_t needsGC, - IsAliased_t isAliased, - Overlap_t mayOverlap, - IsZeroed_t isZeroed = IsNotZeroed, - IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) { + static AggValueSlot + forAddr(Address addr, Qualifiers quals, IsDestructed_t isDestructed, + NeedsGCBarriers_t needsGC, IsAliased_t isAliased, + Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed, + IsSanitizerChecked_t isChecked = IsNotSanitizerChecked, + IsNoDestroyGlobal_t isNoDestroyGlobal = IsNotNoDestroyGlobal) { AggValueSlot AV; if (addr.isValid()) { AV.Addr = addr.getPointer(); @@ -533,18 +538,19 @@ AV.AliasedFlag = isAliased; AV.OverlapFlag = mayOverlap; AV.SanitizerCheckedFlag = isChecked; + AV.NoDestroyGlobalFlag = isNoDestroyGlobal; return AV; } - static AggValueSlot forLValue(const LValue &LV, - IsDestructed_t isDestructed, - NeedsGCBarriers_t needsGC, - IsAliased_t isAliased, - Overlap_t mayOverlap, - IsZeroed_t isZeroed = IsNotZeroed, - IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) { + static AggValueSlot + forLValue(const LValue &LV, IsDestructed_t isDestructed, + NeedsGCBarriers_t needsGC, IsAliased_t isAliased, + Overlap_t mayOverlap, IsZeroed_t isZeroed = IsNotZeroed, + IsSanitizerChecked_t isChecked = IsNotSanitizerChecked, + IsNoDestroyGlobal_t isNoDestroyGlobal = IsNotNoDestroyGlobal) { return forAddr(LV.getAddress(), LV.getQuals(), isDestructed, needsGC, - isAliased, mayOverlap, isZeroed, isChecked); + isAliased, mayOverlap, isZeroed, isChecked, + isNoDestroyGlobal); } IsDestructed_t isExternallyDestructed() const { @@ -603,6 +609,8 @@ return SanitizerCheckedFlag; } + bool isNoDestroyGlobal() const { return NoDestroyGlobalFlag; } + RValue asRValue() const { if (isIgnored()) { return RValue::getIgnored(); Index: clang/lib/CodeGen/CGExprAgg.cpp =================================================================== --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -513,7 +513,7 @@ Address endOfInit = Address::invalid(); EHScopeStack::stable_iterator cleanup; llvm::Instruction *cleanupDominator = nullptr; - if (CGF.needsEHCleanup(dtorKind)) { + if (CGF.needsEHCleanup(dtorKind) && !Dest.isNoDestroyGlobal()) { // In principle we could tell the cleanup where we are more // directly, but the control flow can get so varied here that it // would actually be quite complex. Therefore we go through an Index: clang/lib/CodeGen/CGDeclCXX.cpp =================================================================== --- clang/lib/CodeGen/CGDeclCXX.cpp +++ clang/lib/CodeGen/CGDeclCXX.cpp @@ -52,13 +52,19 @@ case TEK_Complex: CGF.EmitComplexExprIntoLValue(Init, lv, /*isInit*/ true); return; - case TEK_Aggregate: - CGF.EmitAggExpr(Init, AggValueSlot::forLValue(lv,AggValueSlot::IsDestructed, - AggValueSlot::DoesNotNeedGCBarriers, - AggValueSlot::IsNotAliased, - AggValueSlot::DoesNotOverlap)); + case TEK_Aggregate: { + auto IsNoDestroy = !D.isStaticLocal() && D.isNoDestroy(CGF.getContext()) + ? AggValueSlot::IsNoDestroyGlobal + : AggValueSlot::IsNotNoDestroyGlobal; + auto Slot = AggValueSlot::forLValue( + lv, AggValueSlot::IsDestructed, AggValueSlot::DoesNotNeedGCBarriers, + AggValueSlot::IsNotAliased, AggValueSlot::DoesNotOverlap, + AggValueSlot::IsNotZeroed, AggValueSlot::IsNotSanitizerChecked, + IsNoDestroy); + CGF.EmitAggExpr(Init, Slot); return; } + } llvm_unreachable("bad evaluation kind"); } Index: clang/include/clang/Basic/AttrDocs.td =================================================================== --- clang/include/clang/Basic/AttrDocs.td +++ clang/include/clang/Basic/AttrDocs.td @@ -3880,6 +3880,32 @@ 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 { + private: + ~only_no_destroy(); + } + + [[clang::no_destroy]] only_no_destroy global; // fine! + +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: + +.. code-block::c++ + + void f() { + [[clang::no_destroy]] + static only_no_destroy array[10]; // error, only_no_destroy has a private destructor. + + }]; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits