diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 05302c30d18d1..8a3d202871cf8 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -389,8 +389,9 @@ class ASTContext : public RefCountedBase { mutable llvm::DenseMap GlobalArrayOperatorDeletesForVirtualDtor; - /// To remember which types did require a vector deleting dtor. - llvm::DenseSet RequireVectorDeletingDtor; + /// To remember for which types we met new[] call, these potentially require a + /// vector deleting dtor. + llvm::DenseSet MaybeRequireVectorDeletingDtor; /// The next string literal "version" to allocate during constant evaluation. /// This is used to distinguish between repeated evaluations of the same @@ -3561,8 +3562,8 @@ class ASTContext : public RefCountedBase { OperatorDeleteKind K) const; bool dtorHasOperatorDelete(const CXXDestructorDecl *Dtor, OperatorDeleteKind K) const; - void setClassNeedsVectorDeletingDestructor(const CXXRecordDecl *RD); - bool classNeedsVectorDeletingDestructor(const CXXRecordDecl *RD); + void setClassMaybeNeedsVectorDeletingDestructor(const CXXRecordDecl *RD); + bool classMaybeNeedsVectorDeletingDestructor(const CXXRecordDecl *RD); /// Retrieve the context for computing mangling numbers in the given /// DeclContext. diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index 4c672a03eb855..19e5609160a99 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -13639,24 +13639,20 @@ ASTContext::getOperatorDeleteForVDtor(const CXXDestructorDecl *Dtor, return nullptr; } -bool ASTContext::classNeedsVectorDeletingDestructor(const CXXRecordDecl *RD) { +bool ASTContext::classMaybeNeedsVectorDeletingDestructor( + const CXXRecordDecl *RD) { if (!getTargetInfo().emitVectorDeletingDtors(getLangOpts())) return false; - CXXDestructorDecl *Dtor = RD->getDestructor(); - // The compiler can't know if new[]/delete[] will be used outside of the DLL, - // so just force vector deleting destructor emission if dllexport is present. - // This matches MSVC behavior. - if (Dtor && Dtor->isVirtual() && Dtor->hasAttr()) - return true; - return RequireVectorDeletingDtor.count(RD); + return MaybeRequireVectorDeletingDtor.count(RD); } -void ASTContext::setClassNeedsVectorDeletingDestructor( +void ASTContext::setClassMaybeNeedsVectorDeletingDestructor( const CXXRecordDecl *RD) { if (!getTargetInfo().emitVectorDeletingDtors(getLangOpts())) return; - RequireVectorDeletingDtor.insert(RD); + + MaybeRequireVectorDeletingDtor.insert(RD); } MangleNumberingContext & diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index 93ac0305df38f..82300c3ede183 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -1201,6 +1201,12 @@ void CodeGenFunction::EmitNewArrayInitializer( EmitCXXAggrConstructorCall(Ctor, NumElements, CurPtr, CCE, /*NewPointerIsChecked*/ true, CCE->requiresZeroInitialization()); + if (getContext().getTargetInfo().emitVectorDeletingDtors( + getContext().getLangOpts())) { + CXXDestructorDecl *Dtor = Ctor->getParent()->getDestructor(); + if (Dtor && Dtor->isVirtual()) + CGM.requireVectorDestructorDefinition(Ctor->getParent()); + } return; } diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index d08fe0feb692e..82a389fe04e29 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -8538,3 +8538,55 @@ std::string CodeGenModule::getPFPFieldName(const FieldDecl *FD) { Out << "." << FD->getName(); return OutName; } + +bool CodeGenModule::classNeedsVectorDestructor(const CXXRecordDecl *RD) { + if (!Context.getTargetInfo().emitVectorDeletingDtors(Context.getLangOpts())) + return false; + CXXDestructorDecl *Dtor = RD->getDestructor(); + // The compiler can't know if new[]/delete[] will be used outside of the DLL, + // so just force vector deleting destructor emission if dllexport is present. + // This matches MSVC behavior. + if (Dtor && Dtor->isVirtual() && Dtor->hasAttr()) + return true; + + return RequireVectorDeletingDtor.count(RD); +} + +void CodeGenModule::requireVectorDestructorDefinition(const CXXRecordDecl *RD) { + if (!Context.getTargetInfo().emitVectorDeletingDtors(Context.getLangOpts())) + return; + RequireVectorDeletingDtor.insert(RD); + + // To reduce code size in general case we lazily emit scalar deleting + // destructor definition and an alias from vector deleting destructor to + // scalar deleting destructor. It may happen that we first emitted the scalar + // deleting destructor definition and the alias and then discovered that the + // definition of the vector deleting destructor is required. Then we need to + // remove the alias and the scalar deleting destructor and queue vector + // deleting destructor body for emission. Check if that is the case. + CXXDestructorDecl *DtorD = RD->getDestructor(); + GlobalDecl ScalarDtorGD(DtorD, Dtor_Deleting); + StringRef MangledName = getMangledName(ScalarDtorGD); + llvm::GlobalValue *Entry = GetGlobalValue(MangledName); + GlobalDecl VectorDtorGD(DtorD, Dtor_VectorDeleting); + if (Entry && !Entry->isDeclaration()) { + StringRef VDName = getMangledName(VectorDtorGD); + llvm::GlobalValue *VDEntry = GetGlobalValue(VDName); + // It exists and it should be an alias. + assert(VDEntry && isa(VDEntry)); + auto *NewFn = llvm::Function::Create( + cast(VDEntry->getValueType()), + llvm::Function::ExternalLinkage, VDName, &getModule()); + SetFunctionAttributes(VectorDtorGD, NewFn, /*IsIncompleteFunction*/ false, + /*IsThunk*/ false); + NewFn->takeName(VDEntry); + VDEntry->replaceAllUsesWith(NewFn); + VDEntry->eraseFromParent(); + Entry->replaceAllUsesWith(NewFn); + Entry->eraseFromParent(); + } + // Always add a deferred decl to emit once we confirmed that vector deleting + // destructor definition is required. That helps to enforse its generation + // even if destructor is only declared. + addDeferredDeclToEmit(VectorDtorGD); +} diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h index 0081bf5c4cf5f..0a697c84b66a7 100644 --- a/clang/lib/CodeGen/CodeGenModule.h +++ b/clang/lib/CodeGen/CodeGenModule.h @@ -529,6 +529,11 @@ class CodeGenModule : public CodeGenTypeCache { /// that we don't re-emit the initializer. llvm::DenseMap DelayedCXXInitPosition; + /// To remember which types did require a vector deleting destructor body. + /// This set basically contains classes that have virtual destructor and new[] + /// was emitted for the class. + llvm::SmallPtrSet RequireVectorDeletingDtor; + typedef std::pair GlobalInitData; @@ -1578,6 +1583,13 @@ class CodeGenModule : public CodeGenTypeCache { /// are emitted lazily. void EmitGlobal(GlobalDecl D); + /// Record that new[] was called for the class, transform vector deleting + /// destructor definition in a form of alias to the actual definition. + void requireVectorDestructorDefinition(const CXXRecordDecl *RD); + + /// Check that class need vector deleting destructor body. + bool classNeedsVectorDestructor(const CXXRecordDecl *RD); + bool TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D); void EmitDefinitionAsAlias(GlobalDecl Alias, GlobalDecl Target); diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp index 06fce6171eb28..d959b89f860e4 100644 --- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp @@ -4107,7 +4107,7 @@ void MicrosoftCXXABI::emitCXXStructor(GlobalDecl GD) { return; if (GD.getDtorType() == Dtor_VectorDeleting && - !getContext().classNeedsVectorDeletingDestructor(dtor->getParent())) { + !CGM.classNeedsVectorDestructor(dtor->getParent())) { // Create GlobalDecl object with the correct type for the scalar // deleting destructor. GlobalDecl ScalarDtorGD(dtor, Dtor_Deleting); diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 56f315e005320..f0c45033efea1 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -11274,6 +11274,7 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) { if (Context.getTargetInfo().emitVectorDeletingDtors( Context.getLangOpts())) { + bool DestructorIsExported = Destructor->hasAttr(); // Lookup delete[] too in case we have to emit a vector deleting dtor. DeclarationName VDeleteName = Context.DeclarationNames.getCXXOperatorName(OO_Array_Delete); @@ -11287,7 +11288,8 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) { VDeleteName); Destructor->setGlobalOperatorArrayDelete(GlobalArrOperatorDelete); if (GlobalArrOperatorDelete && - Context.classNeedsVectorDeletingDestructor(RD)) + (Context.classMaybeNeedsVectorDeletingDestructor(RD) || + DestructorIsExported)) MarkFunctionReferenced(Loc, GlobalArrOperatorDelete); } else if (!ArrOperatorDelete) { ArrOperatorDelete = FindDeallocationFunctionForDestructor( @@ -11295,7 +11297,9 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) { /*LookForGlobal*/ true, VDeleteName); } Destructor->setOperatorArrayDelete(ArrOperatorDelete); - if (ArrOperatorDelete && Context.classNeedsVectorDeletingDestructor(RD)) + if (ArrOperatorDelete && + (Context.classMaybeNeedsVectorDeletingDestructor(RD) || + DestructorIsExported)) MarkFunctionReferenced(Loc, ArrOperatorDelete); } } @@ -19134,6 +19138,8 @@ void Sema::MarkVTableUsed(SourceLocation Loc, CXXRecordDecl *Class, // delete(). ContextRAII SavedContext(*this, DD); CheckDestructor(DD); + if (!DD->getOperatorDelete()) + DD->setInvalidDecl(); } else { MarkFunctionReferenced(Loc, Class->getDestructor()); } diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 5a5bbf4d900dc..5de4a1e7475f2 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -2636,17 +2636,31 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal, MarkFunctionReferenced(StartLoc, OperatorDelete); } - // For MSVC vector deleting destructors support we record that for the class - // new[] was called. We try to optimize the code size and only emit vector - // deleting destructors when they are required. Vector deleting destructors - // are required for delete[] call but MSVC triggers emission of them - // whenever new[] is called for an object of the class and we do the same - // for compatibility. - if (const CXXConstructExpr *CCE = - dyn_cast_or_null(Initializer); - CCE && ArraySize) { - Context.setClassNeedsVectorDeletingDestructor( - CCE->getConstructor()->getParent()); + // new[] will trigger vector deleting destructor emission if the class has + // virtual destructor for MSVC compatibility. Perform necessary checks. + if (Context.getTargetInfo().emitVectorDeletingDtors(Context.getLangOpts())) { + if (const CXXConstructExpr *CCE = + dyn_cast_or_null(Initializer); + CCE && ArraySize) { + CXXRecordDecl *ClassDecl = CCE->getConstructor()->getParent(); + // We probably already did this for another new[] with this class so don't + // do it twice. + if (!Context.classMaybeNeedsVectorDeletingDestructor(ClassDecl)) { + auto *Dtor = ClassDecl->getDestructor(); + if (Dtor && Dtor->isVirtual() && !Dtor->isDeleted()) { + Context.setClassMaybeNeedsVectorDeletingDestructor(ClassDecl); + if (!Dtor->isDefined() && !Dtor->isInvalidDecl()) { + // Call CheckDestructor if destructor is not defined. This is + // needed to find operators delete and delete[] for vector deleting + // destructor body because new[] will trigger emission of vector + // deleting destructor body even if destructor is defined in another + // translation unit. + ContextRAII SavedContext(*this, Dtor); + CheckDestructor(Dtor); + } + } + } + } } return CXXNewExpr::Create(Context, UseGlobal, OperatorNew, OperatorDelete, diff --git a/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors-new-array.cpp b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors-new-array.cpp new file mode 100644 index 0000000000000..b8b6e44b6b2f8 --- /dev/null +++ b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors-new-array.cpp @@ -0,0 +1,122 @@ +// RUN: %clang_cc1 -emit-llvm -fms-extensions %s -triple=x86_64-pc-windows-msvc -o - | FileCheck %s + +// Test that vector deleting destructors are emitted when new[] is used, +// even when the destructor definition is in another translation unit. + +struct ForwardDeclared { + ForwardDeclared(); + virtual ~ForwardDeclared(); +}; + +struct DefinedInTU { + virtual ~DefinedInTU(); +}; + +struct NonVirtualDtor { + ~NonVirtualDtor(); +}; + +struct NoDtor { + virtual void foo(); + int x; +}; + +struct DeclDerived : ForwardDeclared { + ~DeclDerived() override; +}; + +struct InlineDefaulted { + virtual ~InlineDefaulted() = default; +}; + +struct OutOfLineDefaulted { + virtual ~OutOfLineDefaulted(); +}; + +OutOfLineDefaulted::~OutOfLineDefaulted() = default; + +template +struct Container { + T data; + virtual ~Container(); +}; + +extern template class Container; +Container *arr = new Container[5]; + +struct ImplicitVDtorDerived : ForwardDeclared{ + int data; +}; + +struct __declspec(dllimport) DllImported { + virtual ~DllImported(); +}; + +struct VirtualDerived : virtual ForwardDeclared { + ~VirtualDerived() override; +}; + +struct DeclaredCtorDefinedDtor { + DeclaredCtorDefinedDtor(); + virtual ~DeclaredCtorDefinedDtor() {} +}; + +struct TemplateNotAllocated { + TemplateNotAllocated(); + virtual ~TemplateNotAllocated(); +}; + +struct TemplateAllocated { + TemplateAllocated(); + virtual ~TemplateAllocated(); +}; + +template +void allocate() { + TemplateNotAllocated *arr = new TemplateNotAllocated[T]; +} + +template +void actuallyAllocate() { + T *arr = new T[10]; + delete[] arr; +} + +void cases() { + ForwardDeclared *arr = new ForwardDeclared[5]; + DefinedInTU *arr1 = new DefinedInTU[5]; + NonVirtualDtor *arr2 = new NonVirtualDtor[5]; + NoDtor *arr3 = new NoDtor[5]; + ForwardDeclared *arr4 = new DeclDerived[5]; + InlineDefaulted *arr5 = new InlineDefaulted[5]; + OutOfLineDefaulted *arr6 = new OutOfLineDefaulted[5]; + ImplicitVDtorDerived *arr7 = new ImplicitVDtorDerived[5]; + DllImported *arr8 = new DllImported[5]; + VirtualDerived *arr9 = new VirtualDerived[3]; + DeclaredCtorDefinedDtor *arr10 = new DeclaredCtorDefinedDtor[5]; + actuallyAllocate(); +} + + +// CHECK-DAG: declare dso_local void @"??1ForwardDeclared@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EForwardDeclared@@UEAAPEAXI@Z"( +// CHECK-DAG: define dso_local void @"??1DefinedInTU@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EDefinedInTU@@UEAAPEAXI@Z"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EDeclDerived@@UEAAPEAXI@Z"( +// CHECK-DAG: declare dso_local void @"??1DeclDerived@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EInlineDefaulted@@UEAAPEAXI@Z"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EOutOfLineDefaulted@@UEAAPEAXI@Z"( +// CHECK-DAG: declare dso_local void @"??1?$Container@H@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_E?$Container@H@@UEAAPEAXI@Z"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EImplicitVDtorDerived@@UEAAPEAXI@Z"( +// CHECK-DAG: declare dllimport void @"??1DllImported@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EDllImported@@UEAAPEAXI@Z"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EVirtualDerived@@UEAAPEAXI@Z"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_EDeclaredCtorDefinedDtor@@UEAAPEAXI@Z"( +// CHECK-DAG: declare dso_local void @"??1TemplateAllocated@@UEAA@XZ"( +// CHECK-DAG: define weak dso_local noundef ptr @"??_ETemplateAllocated@@UEAAPEAXI@Z"( +// CHECK-NOT: @"??_ETemplateNotAllocated@@ +// CHECK-NOT: @"??_ENonVirtualDtor@@ +// CHECK-NOT: @"??_ENoDtor@@ + +DefinedInTU::~DefinedInTU() {} diff --git a/clang/test/SemaCXX/gh134265.cpp b/clang/test/SemaCXX/gh134265.cpp index 790165411c938..421197cf3d7f7 100644 --- a/clang/test/SemaCXX/gh134265.cpp +++ b/clang/test/SemaCXX/gh134265.cpp @@ -1,5 +1,5 @@ -// RUN: %clang_cc1 %s -verify=expected -fsyntax-only -triple=x86_64-unknown-linux-gnu -// RUN: %clang_cc1 %s -verify=expected -fsyntax-only -triple=x86_64-unknown-linux-gnu -std=c++20 +// RUN: %clang_cc1 %s -verify=expected,noms -fsyntax-only -triple=x86_64-unknown-linux-gnu +// RUN: %clang_cc1 %s -verify=expected,noms -fsyntax-only -triple=x86_64-unknown-linux-gnu -std=c++20 // RUN: %clang_cc1 %s -verify=expected,ms -fms-extensions -fms-compatibility -triple=x86_64-pc-windows-msvc -DMS // Verify that clang doesn't emit additional errors when searching for @@ -56,7 +56,34 @@ struct Final1 : BaseDelete1, BaseDelete2, BaseDestructor { }; #endif // MS +// Make sure there is no double diagnosing for declared-only destructors and +// new[]. +struct DeclaredOnly { + virtual ~DeclaredOnly(); // ms-error {{attempt to use a deleted function}} + static void operator delete(void* ptr) = delete; // ms-note {{explicitly marked deleted here}} +}; + +struct DeclaredOnlyArr { + virtual ~DeclaredOnlyArr(); + static void operator delete[](void* ptr) = delete; +}; + void foo() { Final* a = new Final[10](); FinalExplicit* b = new FinalExplicit[10](); + DeclaredOnly *d = new DeclaredOnly[5](); + DeclaredOnlyArr *e = new DeclaredOnlyArr[5](); } + +// Make sure there is no double diagnosing for forward declared destructors +// and new[]. +namespace std { struct destroying_delete_t {}; } +struct A { + void operator delete( + A*, //expected-error {{cannot cast 'D' to its private base class 'A'}} + std::destroying_delete_t); +}; +struct B : private A { using A::operator delete; }; //expected-note {{declared private here}} +struct D : B { virtual ~D(); }; //ms-note {{while checking implicit 'delete this' for virtual destructor}} +void f() { new D[5]; } +D::~D() {} // noms-note {{while checking implicit 'delete this' for virtual destructor}}