Reland [MS][clang] Add support for vector deleting destructors #165598
Conversation
MSVC supports and extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain pointer to vector deleting destructor for classes with virtual destructors, so not having this extension not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called. This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower). This is yet another attempt to land vector deleting destructors support originally implemented by llvm#133451. This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like llvm#133950 (comment) llvm#134265 Fixes llvm#19772
llvmbot
added
clang
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesMSVC supports and extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain pointer to vector deleting destructor for classes with virtual destructors, so not having this extension not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called. This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower). This is yet another attempt to land vector deleting destructors support originally implemented by #133451. This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like Fixes #19772 Patch is 120.14 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/165598.diff 56 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index add1582344a0e..844bb3d20244a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -69,6 +69,13 @@ Potentially Breaking Changes
call the member ``operator delete`` instead of the expected global
delete operator. The old behavior is retained under ``-fclang-abi-compat=21``
flag.
+- Clang now supports MSVC vector deleting destructors when targeting Windows.
+ This means that vtables of classes with virtual destructors will contain a
+ pointer to vector deleting destructor (instead of scalar deleting destructor)
+ which in fact is a different symbol with different name and linkage. This
+ may cause runtime failures if two binaries using the same class defining a
+ virtual destructor are compiled with different versions of clang.
+
C/C++ Language Potentially Breaking Changes
-------------------------------------------
@@ -549,6 +556,8 @@ Android Support
Windows Support
^^^^^^^^^^^^^^^
+- Clang now supports MSVC vector deleting destructors (GH19772).
+
LoongArch Support
^^^^^^^^^^^^^^^^^
- Enable linker relaxation by default for loongarch64.
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 33aa2d343aa7a..9723e84f9a42c 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -370,6 +370,18 @@ class ASTContext : public RefCountedBase<ASTContext> {
mutable llvm::DenseSet<const FunctionDecl *> DestroyingOperatorDeletes;
mutable llvm::DenseSet<const FunctionDecl *> TypeAwareOperatorNewAndDeletes;
+ /// Global and array operators delete are only required for MSVC deleting
+ /// destructors support. Store them here to avoid keeping 4 pointers that are
+ /// not always used in each redeclaration of the destructor.
+ mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *>
+ OperatorDeletesForVirtualDtor;
+ mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *>
+ GlobalOperatorDeletesForVirtualDtor;
+ mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *>
+ ArrayOperatorDeletesForVirtualDtor;
+ mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *>
+ GlobalArrayOperatorDeletesForVirtualDtor;
+
/// The next string literal "version" to allocate during constant evaluation.
/// This is used to distinguish between repeated evaluations of the same
/// string literal.
@@ -3473,6 +3485,21 @@ class ASTContext : public RefCountedBase<ASTContext> {
bool IsTypeAware);
bool isTypeAwareOperatorNewOrDelete(const FunctionDecl *FD) const;
+ enum OperatorDeleteKind {
+ Regular,
+ GlobalRegular,
+ Array,
+ ArrayGlobal
+ };
+
+ void addOperatorDeleteForVDtor(const CXXDestructorDecl *Dtor,
+ FunctionDecl *OperatorDelete,
+ OperatorDeleteKind K) const;
+ FunctionDecl *getOperatorDeleteForVDtor(const CXXDestructorDecl *Dtor,
+ OperatorDeleteKind K) const;
+ bool dtorHasOperatorDelete(const CXXDestructorDecl *Dtor,
+ OperatorDeleteKind K) const;
+
/// Retrieve the context for computing mangling numbers in the given
/// DeclContext.
MangleNumberingContext &getManglingNumberContext(const DeclContext *DC);
diff --git a/clang/include/clang/AST/ASTMutationListener.h b/clang/include/clang/AST/ASTMutationListener.h
index 352af42391782..96d72b4a5c04e 100644
--- a/clang/include/clang/AST/ASTMutationListener.h
+++ b/clang/include/clang/AST/ASTMutationListener.h
@@ -90,6 +90,15 @@ class ASTMutationListener {
virtual void ResolvedOperatorGlobDelete(const CXXDestructorDecl *DD,
const FunctionDecl *GlobDelete) {}
+ /// A virtual destructor's operator array delete has been resolved.
+ virtual void ResolvedOperatorArrayDelete(const CXXDestructorDecl *DD,
+ const FunctionDecl *ArrayDelete) {}
+
+ /// A virtual destructor's operator global array delete has been resolved.
+ virtual void
+ ResolvedOperatorGlobArrayDelete(const CXXDestructorDecl *DD,
+ const FunctionDecl *GlobArrayDelete) {}
+
/// An implicit member got a definition.
virtual void CompletedImplicitDefinition(const FunctionDecl *D) {}
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index dfa3befb27dd0..5c4ad3c45da19 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -2872,8 +2872,6 @@ class CXXDestructorDecl : public CXXMethodDecl {
// FIXME: Don't allocate storage for these except in the first declaration
// of a virtual destructor.
- FunctionDecl *OperatorDelete = nullptr;
- FunctionDecl *OperatorGlobalDelete = nullptr;
Expr *OperatorDeleteThisArg = nullptr;
CXXDestructorDecl(ASTContext &C, CXXRecordDecl *RD, SourceLocation StartLoc,
@@ -2900,14 +2898,12 @@ class CXXDestructorDecl : public CXXMethodDecl {
void setOperatorDelete(FunctionDecl *OD, Expr *ThisArg);
void setOperatorGlobalDelete(FunctionDecl *OD);
-
- const FunctionDecl *getOperatorDelete() const {
- return getCanonicalDecl()->OperatorDelete;
- }
-
- const FunctionDecl *getOperatorGlobalDelete() const {
- return getCanonicalDecl()->OperatorGlobalDelete;
- }
+ void setOperatorArrayDelete(FunctionDecl *OD);
+ void setGlobalOperatorArrayDelete(FunctionDecl *OD);
+ const FunctionDecl *getOperatorDelete() const;
+ const FunctionDecl *getOperatorGlobalDelete() const;
+ const FunctionDecl *getArrayOperatorDelete() const;
+ const FunctionDecl *getGlobalArrayOperatorDelete() const;
Expr *getOperatorDeleteThisArg() const {
return getCanonicalDecl()->OperatorDeleteThisArg;
diff --git a/clang/include/clang/AST/VTableBuilder.h b/clang/include/clang/AST/VTableBuilder.h
index a5de41dbc22f1..e1efe8cddcc5e 100644
--- a/clang/include/clang/AST/VTableBuilder.h
+++ b/clang/include/clang/AST/VTableBuilder.h
@@ -150,7 +150,7 @@ class VTableComponent {
bool isRTTIKind() const { return isRTTIKind(getKind()); }
- GlobalDecl getGlobalDecl() const {
+ GlobalDecl getGlobalDecl(bool HasVectorDeletingDtors) const {
assert(isUsedFunctionPointerKind() &&
"GlobalDecl can be created only from virtual function");
@@ -161,7 +161,9 @@ class VTableComponent {
case CK_CompleteDtorPointer:
return GlobalDecl(DtorDecl, CXXDtorType::Dtor_Complete);
case CK_DeletingDtorPointer:
- return GlobalDecl(DtorDecl, CXXDtorType::Dtor_Deleting);
+ return GlobalDecl(DtorDecl, (HasVectorDeletingDtors)
+ ? CXXDtorType::Dtor_VectorDeleting
+ : CXXDtorType::Dtor_Deleting);
case CK_VCallOffset:
case CK_VBaseOffset:
case CK_OffsetToTop:
diff --git a/clang/include/clang/Basic/ABI.h b/clang/include/clang/Basic/ABI.h
index 8279529c316cf..ba1beb04a3260 100644
--- a/clang/include/clang/Basic/ABI.h
+++ b/clang/include/clang/Basic/ABI.h
@@ -37,6 +37,7 @@ enum CXXDtorType {
Dtor_Base, ///< Base object dtor
Dtor_Comdat, ///< The COMDAT used for dtors
Dtor_Unified, ///< GCC-style unified dtor
+ Dtor_VectorDeleting, ///< Vector deleting dtor
};
} // end namespace clang
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index ea73ed915bf03..14d74488244b3 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1793,6 +1793,11 @@ class TargetInfo : public TransferrableTargetInfo,
/// destructor body.
virtual bool callGlobalDeleteInDeletingDtor(const LangOptions &) const;
+ /// Controls whether to emit MSVC vector deleting destructors. The support for
+ /// vector deleting affects vtable layout and therefore is an ABI breaking
+ /// change. The support was only implemented at Clang 22 timeframe.
+ virtual bool emitVectorDeletingDtors(const LangOptions &) const;
+
/// Controls if __builtin_longjmp / __builtin_setjmp can be lowered to
/// llvm.eh.sjlj.longjmp / llvm.eh.sjlj.setjmp.
virtual bool hasSjLjLowering() const {
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 52904c72d1cfc..f932ef0417fb0 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -8565,7 +8565,8 @@ class Sema final : public SemaBase {
FunctionDecl *FindDeallocationFunctionForDestructor(SourceLocation StartLoc,
CXXRecordDecl *RD,
bool Diagnose,
- bool LookForGlobal);
+ bool LookForGlobal,
+ DeclarationName Name);
/// ActOnCXXDelete - Parsed a C++ 'delete' expression (C++ 5.3.5), as in:
/// @code ::delete ptr; @endcode
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index 28c3e55864057..d49572ce7439e 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -951,6 +951,10 @@ class ASTWriter : public ASTDeserializationListener,
Expr *ThisArg) override;
void ResolvedOperatorGlobDelete(const CXXDestructorDecl *DD,
const FunctionDecl *Delete) override;
+ void ResolvedOperatorArrayDelete(const CXXDestructorDecl *DD,
+ const FunctionDecl *Delete) override;
+ void ResolvedOperatorGlobArrayDelete(const CXXDestructorDecl *DD,
+ const FunctionDecl *Delete) override;
void CompletedImplicitDefinition(const FunctionDecl *D) override;
void InstantiationRequested(const ValueDecl *D) override;
void VariableDefinitionInstantiated(const VarDecl *D) override;
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 687cd46773f43..3c32a005a634d 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -13328,6 +13328,76 @@ bool ASTContext::isTypeAwareOperatorNewOrDelete(const FunctionDecl *FD) const {
return TypeAwareOperatorNewAndDeletes.contains(FD->getCanonicalDecl());
}
+void ASTContext::addOperatorDeleteForVDtor(const CXXDestructorDecl *Dtor,
+ FunctionDecl *OperatorDelete,
+ OperatorDeleteKind K) const {
+ switch (K) {
+ case OperatorDeleteKind::Regular:
+ OperatorDeletesForVirtualDtor[Dtor->getCanonicalDecl()] = OperatorDelete;
+ break;
+ case OperatorDeleteKind::GlobalRegular:
+ GlobalOperatorDeletesForVirtualDtor[Dtor->getCanonicalDecl()] =
+ OperatorDelete;
+ break;
+ case OperatorDeleteKind::Array:
+ ArrayOperatorDeletesForVirtualDtor[Dtor->getCanonicalDecl()] =
+ OperatorDelete;
+ break;
+ case OperatorDeleteKind::ArrayGlobal:
+ GlobalArrayOperatorDeletesForVirtualDtor[Dtor->getCanonicalDecl()] =
+ OperatorDelete;
+ break;
+ default:
+ llvm_unreachable("Unknown operator delete kind");
+ }
+}
+
+bool ASTContext::dtorHasOperatorDelete(const CXXDestructorDecl *Dtor,
+ OperatorDeleteKind K) const {
+ switch (K) {
+ case OperatorDeleteKind::Regular:
+ return OperatorDeletesForVirtualDtor.contains(Dtor->getCanonicalDecl());
+ case OperatorDeleteKind::GlobalRegular:
+ return GlobalOperatorDeletesForVirtualDtor.contains(
+ Dtor->getCanonicalDecl());
+ case OperatorDeleteKind::Array:
+ return ArrayOperatorDeletesForVirtualDtor.contains(
+ Dtor->getCanonicalDecl());
+ case OperatorDeleteKind::ArrayGlobal:
+ return GlobalArrayOperatorDeletesForVirtualDtor.contains(
+ Dtor->getCanonicalDecl());
+ default:
+ llvm_unreachable("Unknown operator delete kind");
+ }
+ return false;
+}
+
+FunctionDecl *ASTContext::getOperatorDeleteForVDtor(const CXXDestructorDecl *Dtor,
+ OperatorDeleteKind K) const {
+ const CXXDestructorDecl *Canon = Dtor->getCanonicalDecl();
+ switch (K) {
+ case OperatorDeleteKind::Regular:
+ if (OperatorDeletesForVirtualDtor.contains(Canon))
+ return OperatorDeletesForVirtualDtor[Canon];
+ return nullptr;
+ case OperatorDeleteKind::GlobalRegular:
+ if (GlobalOperatorDeletesForVirtualDtor.contains(Canon))
+ return GlobalOperatorDeletesForVirtualDtor[Canon];
+ return nullptr;
+ case OperatorDeleteKind::Array:
+ if (ArrayOperatorDeletesForVirtualDtor.contains(Canon))
+ return ArrayOperatorDeletesForVirtualDtor[Canon];
+ return nullptr;
+ case OperatorDeleteKind::ArrayGlobal:
+ if (GlobalArrayOperatorDeletesForVirtualDtor.contains(Canon))
+ return GlobalArrayOperatorDeletesForVirtualDtor[Canon];
+ return nullptr;
+ default:
+ llvm_unreachable("Unknown operator delete kind");
+ }
+ return nullptr;
+}
+
MangleNumberingContext &
ASTContext::getManglingNumberContext(const DeclContext *DC) {
assert(LangOpts.CPlusPlus); // We don't need mangling numbers for plain C.
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 24e4f189cbe4a..344ba7b9b29fb 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -3110,12 +3110,15 @@ CXXDestructorDecl *CXXDestructorDecl::Create(
}
void CXXDestructorDecl::setOperatorDelete(FunctionDecl *OD, Expr *ThisArg) {
- auto *First = cast<CXXDestructorDecl>(getFirstDecl());
- if (OD && !First->OperatorDelete) {
- First->OperatorDelete = OD;
- First->OperatorDeleteThisArg = ThisArg;
+ assert(!OD || (OD->getDeclName().getCXXOverloadedOperator() == OO_Delete));
+ if (OD && !getASTContext().dtorHasOperatorDelete(
+ this, ASTContext::OperatorDeleteKind::Regular)) {
+ getASTContext().addOperatorDeleteForVDtor(
+ this, OD, ASTContext::OperatorDeleteKind::Regular);
+ getCanonicalDecl()->OperatorDeleteThisArg = ThisArg;
if (auto *L = getASTMutationListener())
- L->ResolvedOperatorDelete(First, OD, ThisArg);
+ L->ResolvedOperatorDelete(cast<CXXDestructorDecl>(getCanonicalDecl()), OD,
+ ThisArg);
}
}
@@ -3127,14 +3130,63 @@ void CXXDestructorDecl::setOperatorGlobalDelete(FunctionDecl *OD) {
assert(!OD ||
(OD->getDeclName().getCXXOverloadedOperator() == OO_Delete &&
OD->getDeclContext()->getRedeclContext()->isTranslationUnit()));
- auto *Canonical = cast<CXXDestructorDecl>(getCanonicalDecl());
- if (!Canonical->OperatorGlobalDelete) {
- Canonical->OperatorGlobalDelete = OD;
+ if (OD && !getASTContext().dtorHasOperatorDelete(
+ this, ASTContext::OperatorDeleteKind::GlobalRegular)) {
+ getASTContext().addOperatorDeleteForVDtor(
+ this, OD, ASTContext::OperatorDeleteKind::GlobalRegular);
if (auto *L = getASTMutationListener())
- L->ResolvedOperatorGlobDelete(Canonical, OD);
+ L->ResolvedOperatorGlobDelete(cast<CXXDestructorDecl>(getCanonicalDecl()),
+ OD);
}
}
+void CXXDestructorDecl::setOperatorArrayDelete(FunctionDecl *OD) {
+ assert(!OD ||
+ (OD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete));
+ if (OD && !getASTContext().dtorHasOperatorDelete(
+ this, ASTContext::OperatorDeleteKind::Array)) {
+ getASTContext().addOperatorDeleteForVDtor(
+ this, OD, ASTContext::OperatorDeleteKind::Array);
+ if (auto *L = getASTMutationListener())
+ L->ResolvedOperatorArrayDelete(
+ cast<CXXDestructorDecl>(getCanonicalDecl()), OD);
+ }
+}
+
+void CXXDestructorDecl::setGlobalOperatorArrayDelete(FunctionDecl *OD) {
+ assert(!OD ||
+ (OD->getDeclName().getCXXOverloadedOperator() == OO_Array_Delete &&
+ OD->getDeclContext()->getRedeclContext()->isTranslationUnit()));
+ if (OD && !getASTContext().dtorHasOperatorDelete(
+ this, ASTContext::OperatorDeleteKind::ArrayGlobal)) {
+ getASTContext().addOperatorDeleteForVDtor(
+ this, OD, ASTContext::OperatorDeleteKind::ArrayGlobal);
+ if (auto *L = getASTMutationListener())
+ L->ResolvedOperatorGlobArrayDelete(
+ cast<CXXDestructorDecl>(getCanonicalDecl()), OD);
+ }
+}
+
+const FunctionDecl *CXXDestructorDecl::getOperatorDelete() const {
+ return getASTContext().getOperatorDeleteForVDtor(
+ this, ASTContext::OperatorDeleteKind::Regular);
+}
+
+const FunctionDecl *CXXDestructorDecl::getOperatorGlobalDelete() const {
+ return getASTContext().getOperatorDeleteForVDtor(
+ this, ASTContext::OperatorDeleteKind::GlobalRegular);
+}
+
+const FunctionDecl *CXXDestructorDecl::getArrayOperatorDelete() const {
+ return getASTContext().getOperatorDeleteForVDtor(
+ this, ASTContext::OperatorDeleteKind::Array);
+}
+
+const FunctionDecl *CXXDestructorDecl::getGlobalArrayOperatorDelete() const {
+ return getASTContext().getOperatorDeleteForVDtor(
+ this, ASTContext::OperatorDeleteKind::ArrayGlobal);
+}
+
bool CXXDestructorDecl::isCalledByDelete(const FunctionDecl *OpDel) const {
// C++20 [expr.delete]p6: If the value of the operand of the delete-
// expression is not a null pointer value and the selected deallocation
@@ -3146,7 +3198,8 @@ bool CXXDestructorDecl::isCalledByDelete(const FunctionDecl *OpDel) const {
// delete operator, as that destructor is never called, unless the
// destructor is virtual (see [expr.delete]p8.1) because then the
// selected operator depends on the dynamic type of the pointer.
- const FunctionDecl *SelectedOperatorDelete = OpDel ? OpDel : OperatorDelete;
+ const FunctionDecl *SelectedOperatorDelete =
+ OpDel ? OpDel : getOperatorDelete();
if (!SelectedOperatorDelete)
return true;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 340bb4b2ed6a3..1d914fa876759 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -71,6 +71,9 @@ const CXXRecordDecl *Expr::getBestDynamicClassType() const {
if (const PointerType *PTy = DerivedType->getAs<PointerType>())
DerivedType = PTy->getPointeeType();
+ while (const ArrayType *ATy = DerivedType->getAsArrayTypeUnsafe())
+ DerivedType = ATy->getElementType();
+
if (DerivedType->isDependentType())
return nullptr;
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 5572e0a7ae59c..a5bcf5c97e837 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -6040,6 +6040,8 @@ void CXXNameMangler::mangleCXXDtorType(CXXDtorType T) {
case Dtor_Comdat:
Out << "D5";
break;
+ case Dtor_VectorDeleting:
+ llvm_unreachable("Itanium ABI does not use vector deleting dtors");
}
}
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index f1baf9f49384b..551aa7bf3321c 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -1492,8 +1492,9 @@ void MicrosoftCXXNameMangler::mangleCXXDtorType(CXXDtorType T) {
// <operator-name> ::= ?_G # scalar deleting destructor
case Dtor_Deleting: Out << "?_G"; return;
// <operator-name> ::= ?_E # vector deleting destructor
- // FIXME: Add a vector deleting dtor type. It goes in the vtable, so we need
- // it.
+ case Dtor_VectorDeleting:
+ Out << "?_E";
+ return;
case Dtor_Comdat:
llvm_unreachable("not expecting a COMDAT");
case Dtor_Unified:
@@ -2913,9 +2914,12 @@ void MicrosoftCXXNameMangler::mangleFunctionType(const FunctionType *T,
// ::= @ # structors (they have no declared return type)
if (IsStructor) {
if (isa<CXXDestructorDecl>(D) && isStructorDecl(D)) {
- // The scalar deleting destructor takes an extra int argument which is not
- // reflected in the AST.
- if (StructorType == Dtor_Deleting) {
+ // The deleting destructors take an extra argument of type int that
+ // indicates whether the storage for the object should be deleted and
+ // whether a single object or an array of objects is being destroyed. This
+ // extra argument is not reflected in the AST.
+ if (StructorType == Dtor_Deleting ||
+ StructorType == Dtor_VectorDeleting) {
Out << (Pointe...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Will we generate local vftables for dllimport'd classes too? I think that is needed as part of properly supporting this feature. Example input: struct __declspec(dllexport) A {
virtual ~A();
};
struct __declspec(dllimport) B {
virtual ~B();
};
void* f() {return new A;}
void* g() {return new B;}MSVC generates: |
|
Ah, it looks like this happened way back in 2d8b200. |
|
What's the diff from the previous version of this? |
Please let me know if that is not helpful and I'll try to split old and new changes into separate commits in the branch. |
| mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *> | ||
| ArrayOperatorDeletesForVirtualDtor; | ||
| mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *> | ||
| GlobalArrayOperatorDeletesForVirtualDtor; |
There was a problem hiding this comment.
Does it make sense to have separate maps for each kind of operator delete? It seems like most destructors will either have none of these or all of them.
There was a problem hiding this comment.
Outside of Windows there will always be either none or just one of operators delete. I probably could transform 4 maps to a single map storing a tuple, but we will need to remember which is which somehow. 4 different maps seems to be easier to understand.
There was a problem hiding this comment.
I was just wondering if we could make this more efficient, but if it's common that parts aren't used, it probably isn't worth it.
| bool isRTTIKind() const { return isRTTIKind(getKind()); } | ||
|
|
||
| GlobalDecl getGlobalDecl() const { | ||
| GlobalDecl getGlobalDecl(bool HasVectorDeletingDtors) const { |
There was a problem hiding this comment.
Maybe it makes sense to store the difference between Dtor_VectorDeleting and Dtor_Deleting in the VTableComponent, instead of computing it every time we retrieve a vtable component?
There was a problem hiding this comment.
VTableComponent uses a single int64_t value to store its kind and offset. I don't think I can somehow alter it since there is already 8 kinds for 3 bits reserved for kind and I'm not sure I can touch the offset. That would probably mean adding one more member field to VTableComponent just for the sake of this check and looking at how it uses just 3 bits for storing its kind I hesitate adding anything there.
I think @rnk already thought about this in #126240 (comment) .
There was a problem hiding this comment.
Oh, I see. I guess this is okay, then; not worth increasing the size of VTableComponent.
clang/lib/AST/VTableBuilder.cpp
Outdated
| MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)] | ||
| = MI.VTableIndex - AddressPoint; | ||
| MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)] = | ||
| MI.VTableIndex - AddressPoint; |
There was a problem hiding this comment.
Please land formatting fix separately.
There was a problem hiding this comment.
Removed from this PR, thanks for the catch!
| // selected operator depends on the dynamic type of the pointer. | ||
| const FunctionDecl *SelectedOperatorDelete = OpDel ? OpDel : OperatorDelete; | ||
| const FunctionDecl *SelectedOperatorDelete = | ||
| OpDel ? OpDel : getOperatorDelete(); |
There was a problem hiding this comment.
Please land formatting fix separately.
There was a problem hiding this comment.
That is not a pure formatting change, it changes direct access of OperatorDelete field to getOperatorDelete() member call because there is no field anymore.
| return CGF.EmitScalarExpr(ThisArg); | ||
| return CGF.LoadCXXThis(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yep, added back, thanks.
|
|
||
| // 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 |
There was a problem hiding this comment.
I feel like I'm missing a step here. Why do we emit the alias if we never actually want to use it? Can we just skip emitting it in the first place?
There was a problem hiding this comment.
The problem here is that the vtable entry should always refer to the vector deleting destructor for compatibility with MSVC. Vector deleting destructor is notably more complex and bigger than scalar deleting destructor and only required for array deletions, so we try to optimize the case when no array deletions are required by only emitting scalar deleting destructors. I suppose that won't work without an alias.
We could always emit vector deleting destructor body because it can do both scalar and array deletion but I don't know if that will have negative implications for the users that do not care about this extension.
That just implements @rnk's suggestion from #19772 (comment) .
There was a problem hiding this comment.
Oh, hmm. So normally, the scalar destuctor is sufficient, but if some translation unit uses delete[], you need the vector destructor. And you handle this by emitting a weak symbol by default, but overriding it with an actual definition if you need it.
And it looks like this is what MSVC does... so that's fine, I guess. Feels a little convoluted, but I guess we didn't define it.
| bool isRTTIKind() const { return isRTTIKind(getKind()); } | ||
|
|
||
| GlobalDecl getGlobalDecl() const { | ||
| GlobalDecl getGlobalDecl(bool HasVectorDeletingDtors) const { |
There was a problem hiding this comment.
Oh, I see. I guess this is okay, then; not worth increasing the size of VTableComponent.
| mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *> | ||
| ArrayOperatorDeletesForVirtualDtor; | ||
| mutable llvm::DenseMap<const CXXDestructorDecl *, FunctionDecl *> | ||
| GlobalArrayOperatorDeletesForVirtualDtor; |
There was a problem hiding this comment.
I was just wondering if we could make this more efficient, but if it's common that parts aren't used, it probably isn't worth it.
| // selected operator depends on the dynamic type of the pointer. | ||
| const FunctionDecl *SelectedOperatorDelete = OpDel ? OpDel : OperatorDelete; | ||
| const FunctionDecl *SelectedOperatorDelete = | ||
| OpDel ? OpDel : getOperatorDelete(); |
| // so just force vector deleting destructor emission if dllexport is present. | ||
| // This matches MSVC behavior. | ||
| if (Dtor && Dtor->isVirtual() && Dtor->isDefined() && | ||
| Dtor->hasAttr<DLLExportAttr>()) |
There was a problem hiding this comment.
Does this catch the case where the class is marked dllexport?
There was a problem hiding this comment.
Yes, the test test/CodeGenCXX/dllexport.cpp confirms that.
|
|
||
| // 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 |
There was a problem hiding this comment.
Oh, hmm. So normally, the scalar destuctor is sufficient, but if some translation unit uses delete[], you need the vector destructor. And you handle this by emitting a weak symbol by default, but overriding it with an actual definition if you need it.
And it looks like this is what MSVC does... so that's fine, I guess. Feels a little convoluted, but I guess we didn't define it.
| CCE->requiresZeroInitialization()); | ||
| if (CGM.getContext().getTargetInfo().emitVectorDeletingDtors( | ||
| CGM.getContext().getLangOpts())) | ||
| CGM.requireVectorDestructorDefinition(Ctor->getParent()); |
There was a problem hiding this comment.
Probably it makes sense to add a brief comment describing the relevant MSVC ABI rule here.
|
cc @llvm/clang-vendors as it is potentially breaking change. |
|
Alright, I'm going to proceed with the merge since it has been approved and it has been out for a while. The gdb change is trivial so I won't wait for it to get approved. I know this is a big risky change, so please feel free to ping me if any problems are discovered. If that finally stays in main for reasonable amount of time, I'll make a discourse post announcing the breaking change along with c87be72 . |
This adds some minimal code to mark locations where handling is needed for Dtor_VectorDeleting type dtors, which were added in llvm#165598 This is not a comprehensive mark-up of the missing code, as some code will be needed in places where the surrounding function has larger missing pieces in CIR currently. This fixes a warning for an uncovered switch case that was causing CI builds to fail.
This adds some minimal code to mark locations where handling is needed for Dtor_VectorDeleting type dtors, which were added in #165598 This is not a comprehensive mark-up of the missing code, as some code will be needed in places where the surrounding function has larger missing pieces in CIR currently. This fixes a warning for an uncovered switch case that was causing CI builds to fail.
…g (#167969) This adds some minimal code to mark locations where handling is needed for Dtor_VectorDeleting type dtors, which were added in llvm/llvm-project#165598 This is not a comprehensive mark-up of the missing code, as some code will be needed in places where the surrounding function has larger missing pieces in CIR currently. This fixes a warning for an uncovered switch case that was causing CI builds to fail.
4 participants
MSVC supports an extension allowing to delete an array of objects via pointer whose static type doesn't match its dynamic type. This is done via generation of special destructors - vector deleting destructors. MSVC's virtual tables always contain a pointer to the vector deleting destructor for classes with virtual destructors, so not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts a pointer to a scalar deleting destructor to the vtable. As a bonus the deletion of an array of polymorphic object will work just like it does with MSVC - no memory leaks and correct destructors are called.
This patch will cause clang to emit code that is compatible with code produced by MSVC but not compatible with code produced with clang of older versions, so the new behavior can be disabled via passing -fclang-abi-compat=21 (or lower).
This is yet another attempt to land vector deleting destructors support originally implemented by #133451.
This PR contains fixes for issues reported in the original PR as well as fixes for issues related to operator delete[] search reported in several issues like
#133950 (comment) #134265
Fixes #19772