[MS][clang] Add support for vector deleting destructors #126240
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. Fixes llvm#19772
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-codegen Author: Mariya Podchishchaeva (Fznamznon) ChangesWhereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Fixes #19772 Patch is 78.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126240.diff 33 Files Affected:
diff --git a/clang/include/clang/AST/VTableBuilder.h b/clang/include/clang/AST/VTableBuilder.h
index a5de41dbc22f14b..e1efe8cddcc5e58 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 231bad799a42cbd..48969e4f295c364 100644
--- a/clang/include/clang/Basic/ABI.h
+++ b/clang/include/clang/Basic/ABI.h
@@ -31,10 +31,11 @@ enum CXXCtorType {
/// C++ destructor types.
enum CXXDtorType {
- Dtor_Deleting, ///< Deleting dtor
- Dtor_Complete, ///< Complete object dtor
- Dtor_Base, ///< Base object dtor
- Dtor_Comdat ///< The COMDAT used for dtors
+ Dtor_Deleting, ///< Deleting dtor
+ Dtor_Complete, ///< Complete object dtor
+ Dtor_Base, ///< Base object dtor
+ Dtor_Comdat, ///< The COMDAT used for dtors
+ Dtor_VectorDeleting ///< Vector deleting dtor
};
} // end namespace clang
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index e5eb22eae7dd139..3209c1777fc7f4b 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -6001,6 +6001,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 fe34251688a9888..d5dc7833a7fcc43 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -1484,8 +1484,7 @@ 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");
}
@@ -2886,9 +2885,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
+ // 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) {
+ if (StructorType == Dtor_Deleting ||
+ StructorType == Dtor_VectorDeleting) {
Out << (PointersAre64Bit ? "PEAXI@Z" : "PAXI@Z");
return;
}
@@ -3861,10 +3863,10 @@ void MicrosoftMangleContextImpl::mangleCXXDtorThunk(const CXXDestructorDecl *DD,
const ThunkInfo &Thunk,
bool /*ElideOverrideInfo*/,
raw_ostream &Out) {
- // FIXME: Actually, the dtor thunk should be emitted for vector deleting
- // dtors rather than scalar deleting dtors. Just use the vector deleting dtor
- // mangling manually until we support both deleting dtor types.
- assert(Type == Dtor_Deleting);
+ // The dtor thunk should use vector deleting dtor mangling, however as an
+ // optimization we may end up emitting only scalar deleting dtor body, so just
+ // use the vector deleting dtor mangling manually.
+ assert(Type == Dtor_Deleting || Type == Dtor_VectorDeleting);
msvc_hashing_ostream MHO(Out);
MicrosoftCXXNameMangler Mangler(*this, MHO, DD, Type);
Mangler.getStream() << "??_E";
diff --git a/clang/lib/AST/VTableBuilder.cpp b/clang/lib/AST/VTableBuilder.cpp
index fa3055dd1206f41..9b7f72e2390f445 100644
--- a/clang/lib/AST/VTableBuilder.cpp
+++ b/clang/lib/AST/VTableBuilder.cpp
@@ -1733,8 +1733,8 @@ void ItaniumVTableBuilder::LayoutPrimaryAndSecondaryVTables(
const CXXMethodDecl *MD = I.first;
const MethodInfo &MI = I.second;
if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
- MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)]
- = MI.VTableIndex - AddressPoint;
+ MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)] =
+ MI.VTableIndex - AddressPoint;
MethodVTableIndices[GlobalDecl(DD, Dtor_Deleting)]
= MI.VTableIndex + 1 - AddressPoint;
} else {
@@ -2655,7 +2655,10 @@ class VFTableBuilder {
MethodVFTableLocation Loc(MI.VBTableIndex, WhichVFPtr.getVBaseWithVPtr(),
WhichVFPtr.NonVirtualOffset, MI.VFTableIndex);
if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
- MethodVFTableLocations[GlobalDecl(DD, Dtor_Deleting)] = Loc;
+ if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+ MethodVFTableLocations[GlobalDecl(DD, Dtor_Deleting)] = Loc;
+ else
+ MethodVFTableLocations[GlobalDecl(DD, Dtor_VectorDeleting)] = Loc;
} else {
MethodVFTableLocations[MD] = Loc;
}
@@ -3285,7 +3288,10 @@ void VFTableBuilder::dumpLayout(raw_ostream &Out) {
const CXXDestructorDecl *DD = Component.getDestructorDecl();
DD->printQualifiedName(Out);
- Out << "() [scalar deleting]";
+ if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+ Out << "() [vector deleting]";
+ else
+ Out << "() [scalar deleting]";
if (DD->isPureVirtual())
Out << " [pure]";
@@ -3756,7 +3762,7 @@ void MicrosoftVTableContext::dumpMethodLocations(
PredefinedIdentKind::PrettyFunctionNoVirtual, MD);
if (isa<CXXDestructorDecl>(MD)) {
- IndicesMap[I.second] = MethodName + " [scalar deleting]";
+ IndicesMap[I.second] = MethodName + " [vector deleting]";
} else {
IndicesMap[I.second] = MethodName;
}
@@ -3873,7 +3879,7 @@ MicrosoftVTableContext::getMethodVFTableLocation(GlobalDecl GD) {
assert(hasVtableSlot(cast<CXXMethodDecl>(GD.getDecl())) &&
"Only use this method for virtual methods or dtors");
if (isa<CXXDestructorDecl>(GD.getDecl()))
- assert(GD.getDtorType() == Dtor_Deleting);
+ assert(GD.getDtorType() == Dtor_VectorDeleting);
GD = GD.getCanonicalDecl();
diff --git a/clang/lib/CodeGen/CGCXX.cpp b/clang/lib/CodeGen/CGCXX.cpp
index 78a7b021855b7e8..6f47e24eed5b374 100644
--- a/clang/lib/CodeGen/CGCXX.cpp
+++ b/clang/lib/CodeGen/CGCXX.cpp
@@ -175,7 +175,6 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// requires explicit comdat support in the IL.
if (llvm::GlobalValue::isWeakForLinker(TargetLinkage))
return true;
-
// Create the alias with no name.
auto *Alias = llvm::GlobalAlias::create(AliasValueType, 0, Linkage, "",
Aliasee, &getModule());
@@ -201,6 +200,42 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
return false;
}
+/// Emit a definition as a global alias for another definition, unconditionally.
+void CodeGenModule::EmitDefinitionAsAlias(GlobalDecl AliasDecl,
+ GlobalDecl TargetDecl) {
+
+ llvm::Type *AliasValueType = getTypes().GetFunctionType(AliasDecl);
+
+ StringRef MangledName = getMangledName(AliasDecl);
+ llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
+ if (Entry && !Entry->isDeclaration())
+ return;
+ auto *Aliasee = cast<llvm::GlobalValue>(GetAddrOfGlobal(TargetDecl));
+
+ // Determine the linkage type for the alias.
+ llvm::GlobalValue::LinkageTypes Linkage = getFunctionLinkage(AliasDecl);
+
+ // Create the alias with no name.
+ auto *Alias = llvm::GlobalAlias::create(AliasValueType, 0, Linkage, "",
+ Aliasee, &getModule());
+ // Destructors are always unnamed_addr.
+ Alias->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
+ if (Entry) {
+ assert(Entry->getValueType() == AliasValueType &&
+ Entry->getAddressSpace() == Alias->getAddressSpace() &&
+ "declaration exists with different type");
+ Alias->takeName(Entry);
+ Entry->replaceAllUsesWith(Alias);
+ Entry->eraseFromParent();
+ } else {
+ Alias->setName(MangledName);
+ }
+
+ // Set any additional necessary attributes for the alias.
+ SetCommonAttributes(AliasDecl, Alias);
+}
+
llvm::Function *CodeGenModule::codegenCXXStructor(GlobalDecl GD) {
const CGFunctionInfo &FnInfo = getTypes().arrangeCXXStructorDeclaration(GD);
auto *Fn = cast<llvm::Function>(
diff --git a/clang/lib/CodeGen/CGCXXABI.cpp b/clang/lib/CodeGen/CGCXXABI.cpp
index 7c6dfc3e59d8c0e..e109fd0c443f6a0 100644
--- a/clang/lib/CodeGen/CGCXXABI.cpp
+++ b/clang/lib/CodeGen/CGCXXABI.cpp
@@ -273,6 +273,20 @@ void CGCXXABI::ReadArrayCookie(CodeGenFunction &CGF, Address ptr,
numElements = readArrayCookieImpl(CGF, allocAddr, cookieSize);
}
+void CGCXXABI::ReadArrayCookie(CodeGenFunction &CGF, Address ptr,
+ QualType eltTy, llvm::Value *&numElements,
+ llvm::Value *&allocPtr, CharUnits &cookieSize) {
+ assert(eltTy.isDestructedType());
+
+ // Derive a char* in the same address space as the pointer.
+ ptr = ptr.withElementType(CGF.Int8Ty);
+
+ cookieSize = getArrayCookieSizeImpl(eltTy);
+ Address allocAddr = CGF.Builder.CreateConstInBoundsByteGEP(ptr, -cookieSize);
+ allocPtr = allocAddr.emitRawPointer(CGF);
+ numElements = readArrayCookieImpl(CGF, allocAddr, cookieSize);
+}
+
llvm::Value *CGCXXABI::readArrayCookieImpl(CodeGenFunction &CGF,
Address ptr,
CharUnits cookieSize) {
diff --git a/clang/lib/CodeGen/CGCXXABI.h b/clang/lib/CodeGen/CGCXXABI.h
index 687ff7fb844445a..fbbe8f6c6142cb7 100644
--- a/clang/lib/CodeGen/CGCXXABI.h
+++ b/clang/lib/CodeGen/CGCXXABI.h
@@ -251,9 +251,10 @@ class CGCXXABI {
public:
virtual void emitVirtualObjectDelete(CodeGenFunction &CGF,
- const CXXDeleteExpr *DE,
- Address Ptr, QualType ElementType,
- const CXXDestructorDecl *Dtor) = 0;
+ const CXXDeleteExpr *DE, Address Ptr,
+ QualType ElementType,
+ const CXXDestructorDecl *Dtor,
+ bool ArrayDeletion) = 0;
virtual void emitRethrow(CodeGenFunction &CGF, bool isNoReturn) = 0;
virtual void emitThrow(CodeGenFunction &CGF, const CXXThrowExpr *E) = 0;
virtual llvm::GlobalVariable *getThrowInfo(QualType T) { return nullptr; }
@@ -275,6 +276,7 @@ class CGCXXABI {
virtual CatchTypeInfo getCatchAllTypeInfo();
virtual bool shouldTypeidBeNullChecked(QualType SrcRecordTy) = 0;
+ virtual bool hasVectorDeletingDtors() = 0;
virtual void EmitBadTypeidCall(CodeGenFunction &CGF) = 0;
virtual llvm::Value *EmitTypeid(CodeGenFunction &CGF, QualType SrcRecordTy,
Address ThisPtr,
@@ -485,11 +487,10 @@ class CGCXXABI {
llvm::PointerUnion<const CXXDeleteExpr *, const CXXMemberCallExpr *>;
/// Emit the ABI-specific virtual destructor call.
- virtual llvm::Value *
- EmitVirtualDestructorCall(CodeGenFunction &CGF, const CXXDestructorDecl *Dtor,
- CXXDtorType DtorType, Address This,
- DeleteOrMemberCallExpr E,
- llvm::CallBase **CallOrInvoke) = 0;
+ virtual llvm::Value *EmitVirtualDestructorCall(
+ CodeGenFunction &CGF, const CXXDestructorDecl *Dtor, CXXDtorType DtorType,
+ Address This, DeleteOrMemberCallExpr E, llvm::CallBase **CallOrInvoke,
+ bool ArrayDeletion = false) = 0;
virtual void adjustCallArgsForDestructorThunk(CodeGenFunction &CGF,
GlobalDecl GD,
@@ -575,6 +576,12 @@ class CGCXXABI {
QualType ElementType, llvm::Value *&NumElements,
llvm::Value *&AllocPtr, CharUnits &CookieSize);
+ /// Reads the array cookie associated with the given pointer,
+ /// that should have one.
+ virtual void ReadArrayCookie(CodeGenFunction &CGF, Address Ptr,
+ QualType ElementType, llvm::Value *&NumElements,
+ llvm::Value *&AllocPtr, CharUnits &CookieSize);
+
/// Return whether the given global decl needs a VTT parameter.
virtual bool NeedsVTTParameter(GlobalDecl GD);
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index 7a1096fcbca822b..bb94d502c34faf1 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1433,6 +1433,83 @@ static bool CanSkipVTablePointerInitialization(CodeGenFunction &CGF,
return true;
}
+namespace {
+llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
+ const CXXDestructorDecl *DD) {
+ if (Expr *ThisArg = DD->getOperatorDeleteThisArg())
+ return CGF.EmitScalarExpr(ThisArg);
+ return CGF.LoadCXXThis();
+}
+}
+
+void EmitConditionalArrayDtorCall(const CXXDestructorDecl *DD,
+ CodeGenFunction &CGF,
+ llvm::Value *ShouldDeleteCondition) {
+ Address ThisPtr = CGF.LoadCXXThisAddress();
+ llvm::BasicBlock *ScalarBB = CGF.createBasicBlock("dtor.scalar");
+ llvm::BasicBlock *callDeleteBB =
+ CGF.createBasicBlock("dtor.call_delete_after_array_destroy");
+ llvm::BasicBlock *VectorBB = CGF.createBasicBlock("dtor.vector");
+ auto *CondTy = cast<llvm::IntegerType>(ShouldDeleteCondition->getType());
+ llvm::Value *CheckTheBitForArrayDestroy = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition,
+ llvm::Constant::getIntegerValue(CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/2)));
+ llvm::Value *ShouldDestroyArray =
+ CGF.Builder.CreateIsNull(CheckTheBitForArrayDestroy);
+ CGF.Builder.CreateCondBr(ShouldDestroyArray, ScalarBB, VectorBB);
+
+ CGF.EmitBlock(VectorBB);
+
+ llvm::Value *numElements = nullptr;
+ llvm::Value *allocatedPtr = nullptr;
+ CharUnits cookieSize;
+ QualType EltTy = DD->getThisType()->getPointeeType();
+ CGF.CGM.getCXXABI().ReadArrayCookie(CGF, ThisPtr, EltTy, numElements,
+ allocatedPtr, cookieSize);
+
+ // Destroy the elements.
+ QualType::DestructionKind dtorKind = EltTy.isDestructedType();
+
+ assert(dtorKind);
+ assert(numElements && "no element count for a type with a destructor!");
+
+ CharUnits elementSize = CGF.getContext().getTypeSizeInChars(EltTy);
+ CharUnits elementAlign =
+ ThisPtr.getAlignment().alignmentOfArrayElement(elementSize);
+
+ llvm::Value *arrayBegin = ThisPtr.emitRawPointer(CGF);
+ llvm::Value *arrayEnd = CGF.Builder.CreateInBoundsGEP(
+ ThisPtr.getElementType(), arrayBegin, numElements, "delete.end");
+
+ // We already checked that the array is not 0-length before entering vector
+ // deleting dtor.
+ CGF.emitArrayDestroy(arrayBegin, arrayEnd, EltTy, elementAlign,
+ CGF.getDestroyer(dtorKind),
+ /*checkZeroLength*/ false, CGF.needsEHCleanup(dtorKind));
+
+ llvm::BasicBlock *VectorBBCont = CGF.createBasicBlock("dtor.vector.cont");
+ CGF.EmitBlock(VectorBBCont);
+
+ llvm::Value *CheckTheBitForDeleteCall = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition,
+ llvm::Constant::getIntegerValue(CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/1)));
+
+ llvm::Value *ShouldCallDelete =
+ CGF.Builder.CreateIsNull(CheckTheBitForDeleteCall);
+ CGF.Builder.CreateCondBr(ShouldCallDelete, CGF.ReturnBlock.getBlock(),
+ callDeleteBB);
+ CGF.EmitBlock(callDeleteBB);
+ const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CGF.CurCodeDecl);
+ const CXXRecordDecl *ClassDecl = Dtor->getParent();
+ CGF.EmitDeleteCall(Dtor->getOperatorDelete(), allocatedPtr,
+ CGF.getContext().getTagDeclType(ClassDecl));
+
+ CGF.EmitBranchThroughCleanup(CGF.ReturnBlock);
+ CGF.EmitBlock(ScalarBB);
+}
+
/// EmitDestructorBody - Emits the body of the current destructor.
void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CurGD.getDecl());
@@ -1462,7 +1539,9 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
// outside of the function-try-block, which means it's always
// possible to delegate the destructor body to the complete
// destructor. Do so.
- if (DtorType == Dtor_Deleting) {
+ if (DtorType == Dtor_Deleting || DtorType == Dtor_VectorDeleting) {
+ if (CXXStructorImplicitParamValue && DtorType == Dtor_VectorDeleting)
+ EmitConditionalArrayDtorCall(Dtor, *this, CXXStructorImplicitParamValue);
RunCleanupsScope DtorEpilogue(*this);
EnterDtorCleanups(Dtor, Dtor_Deleting);
if (HaveInsertPoint()) {
@@ -1491,6 +1570,7 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
switch (DtorType) {
case Dtor_Comdat: llvm_unreachable("not expecting a COMDAT");
case Dtor_Deleting: llvm_unreachable("already handled deleting case");
+ case Dtor_VectorDeleting: llvm_unreachable("already handled vector deleting case");
case Dtor_Complete:
assert((Body || getTarget().getCXXABI().isMicrosoft()) &&
@@ -1567,13 +1647,6 @@ void CodeGenFunction::emitImplicitAssignmentOperatorBody(FunctionArgList &Args)
}
namespace {
- llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
- const CXXDestructorDecl *DD) {
- if (Expr *ThisArg = DD->getOperatorDeleteThisArg())
- return CGF.EmitScalarExpr(ThisArg);
- return CGF.LoadCXXThis();
- }
-
/// Call the operator delete associated with the current destructor.
struct CallDtorDelete final : EHScopeStack::Cleanup {
CallDtorDelete() {}
@@ -1592,8 +1665,12 @@ namespace {
bool ReturnAfterDelete) {
llvm::BasicBlock *callDeleteBB = CGF.createBasicBlock("dtor.call_delete");
llvm::BasicBlock *continueBB = CGF.createBasicBlock("dtor.continue");
- llvm::Value *ShouldCallDelete
- = CGF.Builder.CreateIsNull(ShouldDeleteCondition);
+ auto *CondTy = cast<llvm::IntegerType>(ShouldDeleteCondition->getType());
+ llvm::Value *CheckTheBit = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition, llvm::Constant::getIntegerValue(
+ CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/1)));
+ llvm::Value *ShouldCallDelete = CGF.Builder.CreateIsNull(CheckTheBit);
CGF.Builder.CreateCondBr(ShouldCallDelete, continueBB, callDeleteBB);
CGF.EmitBlock(callDeleteBB);
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index d5b584ec0f2e95b..da514ff612a32ef 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2104,7 +2104,8 @@ llvm::DISubprogram *CGDebugInfo::CreateCXXMemberFunction(
// Emit MS ABI vftable inf...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesWhereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Fixes #19772 Patch is 78.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126240.diff 33 Files Affected:
diff --git a/clang/include/clang/AST/VTableBuilder.h b/clang/include/clang/AST/VTableBuilder.h
index a5de41dbc22f14b..e1efe8cddcc5e58 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 231bad799a42cbd..48969e4f295c364 100644
--- a/clang/include/clang/Basic/ABI.h
+++ b/clang/include/clang/Basic/ABI.h
@@ -31,10 +31,11 @@ enum CXXCtorType {
/// C++ destructor types.
enum CXXDtorType {
- Dtor_Deleting, ///< Deleting dtor
- Dtor_Complete, ///< Complete object dtor
- Dtor_Base, ///< Base object dtor
- Dtor_Comdat ///< The COMDAT used for dtors
+ Dtor_Deleting, ///< Deleting dtor
+ Dtor_Complete, ///< Complete object dtor
+ Dtor_Base, ///< Base object dtor
+ Dtor_Comdat, ///< The COMDAT used for dtors
+ Dtor_VectorDeleting ///< Vector deleting dtor
};
} // end namespace clang
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index e5eb22eae7dd139..3209c1777fc7f4b 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -6001,6 +6001,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 fe34251688a9888..d5dc7833a7fcc43 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -1484,8 +1484,7 @@ 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");
}
@@ -2886,9 +2885,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
+ // 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) {
+ if (StructorType == Dtor_Deleting ||
+ StructorType == Dtor_VectorDeleting) {
Out << (PointersAre64Bit ? "PEAXI@Z" : "PAXI@Z");
return;
}
@@ -3861,10 +3863,10 @@ void MicrosoftMangleContextImpl::mangleCXXDtorThunk(const CXXDestructorDecl *DD,
const ThunkInfo &Thunk,
bool /*ElideOverrideInfo*/,
raw_ostream &Out) {
- // FIXME: Actually, the dtor thunk should be emitted for vector deleting
- // dtors rather than scalar deleting dtors. Just use the vector deleting dtor
- // mangling manually until we support both deleting dtor types.
- assert(Type == Dtor_Deleting);
+ // The dtor thunk should use vector deleting dtor mangling, however as an
+ // optimization we may end up emitting only scalar deleting dtor body, so just
+ // use the vector deleting dtor mangling manually.
+ assert(Type == Dtor_Deleting || Type == Dtor_VectorDeleting);
msvc_hashing_ostream MHO(Out);
MicrosoftCXXNameMangler Mangler(*this, MHO, DD, Type);
Mangler.getStream() << "??_E";
diff --git a/clang/lib/AST/VTableBuilder.cpp b/clang/lib/AST/VTableBuilder.cpp
index fa3055dd1206f41..9b7f72e2390f445 100644
--- a/clang/lib/AST/VTableBuilder.cpp
+++ b/clang/lib/AST/VTableBuilder.cpp
@@ -1733,8 +1733,8 @@ void ItaniumVTableBuilder::LayoutPrimaryAndSecondaryVTables(
const CXXMethodDecl *MD = I.first;
const MethodInfo &MI = I.second;
if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
- MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)]
- = MI.VTableIndex - AddressPoint;
+ MethodVTableIndices[GlobalDecl(DD, Dtor_Complete)] =
+ MI.VTableIndex - AddressPoint;
MethodVTableIndices[GlobalDecl(DD, Dtor_Deleting)]
= MI.VTableIndex + 1 - AddressPoint;
} else {
@@ -2655,7 +2655,10 @@ class VFTableBuilder {
MethodVFTableLocation Loc(MI.VBTableIndex, WhichVFPtr.getVBaseWithVPtr(),
WhichVFPtr.NonVirtualOffset, MI.VFTableIndex);
if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
- MethodVFTableLocations[GlobalDecl(DD, Dtor_Deleting)] = Loc;
+ if (!Context.getTargetInfo().getCXXABI().isMicrosoft())
+ MethodVFTableLocations[GlobalDecl(DD, Dtor_Deleting)] = Loc;
+ else
+ MethodVFTableLocations[GlobalDecl(DD, Dtor_VectorDeleting)] = Loc;
} else {
MethodVFTableLocations[MD] = Loc;
}
@@ -3285,7 +3288,10 @@ void VFTableBuilder::dumpLayout(raw_ostream &Out) {
const CXXDestructorDecl *DD = Component.getDestructorDecl();
DD->printQualifiedName(Out);
- Out << "() [scalar deleting]";
+ if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+ Out << "() [vector deleting]";
+ else
+ Out << "() [scalar deleting]";
if (DD->isPureVirtual())
Out << " [pure]";
@@ -3756,7 +3762,7 @@ void MicrosoftVTableContext::dumpMethodLocations(
PredefinedIdentKind::PrettyFunctionNoVirtual, MD);
if (isa<CXXDestructorDecl>(MD)) {
- IndicesMap[I.second] = MethodName + " [scalar deleting]";
+ IndicesMap[I.second] = MethodName + " [vector deleting]";
} else {
IndicesMap[I.second] = MethodName;
}
@@ -3873,7 +3879,7 @@ MicrosoftVTableContext::getMethodVFTableLocation(GlobalDecl GD) {
assert(hasVtableSlot(cast<CXXMethodDecl>(GD.getDecl())) &&
"Only use this method for virtual methods or dtors");
if (isa<CXXDestructorDecl>(GD.getDecl()))
- assert(GD.getDtorType() == Dtor_Deleting);
+ assert(GD.getDtorType() == Dtor_VectorDeleting);
GD = GD.getCanonicalDecl();
diff --git a/clang/lib/CodeGen/CGCXX.cpp b/clang/lib/CodeGen/CGCXX.cpp
index 78a7b021855b7e8..6f47e24eed5b374 100644
--- a/clang/lib/CodeGen/CGCXX.cpp
+++ b/clang/lib/CodeGen/CGCXX.cpp
@@ -175,7 +175,6 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// requires explicit comdat support in the IL.
if (llvm::GlobalValue::isWeakForLinker(TargetLinkage))
return true;
-
// Create the alias with no name.
auto *Alias = llvm::GlobalAlias::create(AliasValueType, 0, Linkage, "",
Aliasee, &getModule());
@@ -201,6 +200,42 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
return false;
}
+/// Emit a definition as a global alias for another definition, unconditionally.
+void CodeGenModule::EmitDefinitionAsAlias(GlobalDecl AliasDecl,
+ GlobalDecl TargetDecl) {
+
+ llvm::Type *AliasValueType = getTypes().GetFunctionType(AliasDecl);
+
+ StringRef MangledName = getMangledName(AliasDecl);
+ llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
+ if (Entry && !Entry->isDeclaration())
+ return;
+ auto *Aliasee = cast<llvm::GlobalValue>(GetAddrOfGlobal(TargetDecl));
+
+ // Determine the linkage type for the alias.
+ llvm::GlobalValue::LinkageTypes Linkage = getFunctionLinkage(AliasDecl);
+
+ // Create the alias with no name.
+ auto *Alias = llvm::GlobalAlias::create(AliasValueType, 0, Linkage, "",
+ Aliasee, &getModule());
+ // Destructors are always unnamed_addr.
+ Alias->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+
+ if (Entry) {
+ assert(Entry->getValueType() == AliasValueType &&
+ Entry->getAddressSpace() == Alias->getAddressSpace() &&
+ "declaration exists with different type");
+ Alias->takeName(Entry);
+ Entry->replaceAllUsesWith(Alias);
+ Entry->eraseFromParent();
+ } else {
+ Alias->setName(MangledName);
+ }
+
+ // Set any additional necessary attributes for the alias.
+ SetCommonAttributes(AliasDecl, Alias);
+}
+
llvm::Function *CodeGenModule::codegenCXXStructor(GlobalDecl GD) {
const CGFunctionInfo &FnInfo = getTypes().arrangeCXXStructorDeclaration(GD);
auto *Fn = cast<llvm::Function>(
diff --git a/clang/lib/CodeGen/CGCXXABI.cpp b/clang/lib/CodeGen/CGCXXABI.cpp
index 7c6dfc3e59d8c0e..e109fd0c443f6a0 100644
--- a/clang/lib/CodeGen/CGCXXABI.cpp
+++ b/clang/lib/CodeGen/CGCXXABI.cpp
@@ -273,6 +273,20 @@ void CGCXXABI::ReadArrayCookie(CodeGenFunction &CGF, Address ptr,
numElements = readArrayCookieImpl(CGF, allocAddr, cookieSize);
}
+void CGCXXABI::ReadArrayCookie(CodeGenFunction &CGF, Address ptr,
+ QualType eltTy, llvm::Value *&numElements,
+ llvm::Value *&allocPtr, CharUnits &cookieSize) {
+ assert(eltTy.isDestructedType());
+
+ // Derive a char* in the same address space as the pointer.
+ ptr = ptr.withElementType(CGF.Int8Ty);
+
+ cookieSize = getArrayCookieSizeImpl(eltTy);
+ Address allocAddr = CGF.Builder.CreateConstInBoundsByteGEP(ptr, -cookieSize);
+ allocPtr = allocAddr.emitRawPointer(CGF);
+ numElements = readArrayCookieImpl(CGF, allocAddr, cookieSize);
+}
+
llvm::Value *CGCXXABI::readArrayCookieImpl(CodeGenFunction &CGF,
Address ptr,
CharUnits cookieSize) {
diff --git a/clang/lib/CodeGen/CGCXXABI.h b/clang/lib/CodeGen/CGCXXABI.h
index 687ff7fb844445a..fbbe8f6c6142cb7 100644
--- a/clang/lib/CodeGen/CGCXXABI.h
+++ b/clang/lib/CodeGen/CGCXXABI.h
@@ -251,9 +251,10 @@ class CGCXXABI {
public:
virtual void emitVirtualObjectDelete(CodeGenFunction &CGF,
- const CXXDeleteExpr *DE,
- Address Ptr, QualType ElementType,
- const CXXDestructorDecl *Dtor) = 0;
+ const CXXDeleteExpr *DE, Address Ptr,
+ QualType ElementType,
+ const CXXDestructorDecl *Dtor,
+ bool ArrayDeletion) = 0;
virtual void emitRethrow(CodeGenFunction &CGF, bool isNoReturn) = 0;
virtual void emitThrow(CodeGenFunction &CGF, const CXXThrowExpr *E) = 0;
virtual llvm::GlobalVariable *getThrowInfo(QualType T) { return nullptr; }
@@ -275,6 +276,7 @@ class CGCXXABI {
virtual CatchTypeInfo getCatchAllTypeInfo();
virtual bool shouldTypeidBeNullChecked(QualType SrcRecordTy) = 0;
+ virtual bool hasVectorDeletingDtors() = 0;
virtual void EmitBadTypeidCall(CodeGenFunction &CGF) = 0;
virtual llvm::Value *EmitTypeid(CodeGenFunction &CGF, QualType SrcRecordTy,
Address ThisPtr,
@@ -485,11 +487,10 @@ class CGCXXABI {
llvm::PointerUnion<const CXXDeleteExpr *, const CXXMemberCallExpr *>;
/// Emit the ABI-specific virtual destructor call.
- virtual llvm::Value *
- EmitVirtualDestructorCall(CodeGenFunction &CGF, const CXXDestructorDecl *Dtor,
- CXXDtorType DtorType, Address This,
- DeleteOrMemberCallExpr E,
- llvm::CallBase **CallOrInvoke) = 0;
+ virtual llvm::Value *EmitVirtualDestructorCall(
+ CodeGenFunction &CGF, const CXXDestructorDecl *Dtor, CXXDtorType DtorType,
+ Address This, DeleteOrMemberCallExpr E, llvm::CallBase **CallOrInvoke,
+ bool ArrayDeletion = false) = 0;
virtual void adjustCallArgsForDestructorThunk(CodeGenFunction &CGF,
GlobalDecl GD,
@@ -575,6 +576,12 @@ class CGCXXABI {
QualType ElementType, llvm::Value *&NumElements,
llvm::Value *&AllocPtr, CharUnits &CookieSize);
+ /// Reads the array cookie associated with the given pointer,
+ /// that should have one.
+ virtual void ReadArrayCookie(CodeGenFunction &CGF, Address Ptr,
+ QualType ElementType, llvm::Value *&NumElements,
+ llvm::Value *&AllocPtr, CharUnits &CookieSize);
+
/// Return whether the given global decl needs a VTT parameter.
virtual bool NeedsVTTParameter(GlobalDecl GD);
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index 7a1096fcbca822b..bb94d502c34faf1 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -1433,6 +1433,83 @@ static bool CanSkipVTablePointerInitialization(CodeGenFunction &CGF,
return true;
}
+namespace {
+llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
+ const CXXDestructorDecl *DD) {
+ if (Expr *ThisArg = DD->getOperatorDeleteThisArg())
+ return CGF.EmitScalarExpr(ThisArg);
+ return CGF.LoadCXXThis();
+}
+}
+
+void EmitConditionalArrayDtorCall(const CXXDestructorDecl *DD,
+ CodeGenFunction &CGF,
+ llvm::Value *ShouldDeleteCondition) {
+ Address ThisPtr = CGF.LoadCXXThisAddress();
+ llvm::BasicBlock *ScalarBB = CGF.createBasicBlock("dtor.scalar");
+ llvm::BasicBlock *callDeleteBB =
+ CGF.createBasicBlock("dtor.call_delete_after_array_destroy");
+ llvm::BasicBlock *VectorBB = CGF.createBasicBlock("dtor.vector");
+ auto *CondTy = cast<llvm::IntegerType>(ShouldDeleteCondition->getType());
+ llvm::Value *CheckTheBitForArrayDestroy = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition,
+ llvm::Constant::getIntegerValue(CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/2)));
+ llvm::Value *ShouldDestroyArray =
+ CGF.Builder.CreateIsNull(CheckTheBitForArrayDestroy);
+ CGF.Builder.CreateCondBr(ShouldDestroyArray, ScalarBB, VectorBB);
+
+ CGF.EmitBlock(VectorBB);
+
+ llvm::Value *numElements = nullptr;
+ llvm::Value *allocatedPtr = nullptr;
+ CharUnits cookieSize;
+ QualType EltTy = DD->getThisType()->getPointeeType();
+ CGF.CGM.getCXXABI().ReadArrayCookie(CGF, ThisPtr, EltTy, numElements,
+ allocatedPtr, cookieSize);
+
+ // Destroy the elements.
+ QualType::DestructionKind dtorKind = EltTy.isDestructedType();
+
+ assert(dtorKind);
+ assert(numElements && "no element count for a type with a destructor!");
+
+ CharUnits elementSize = CGF.getContext().getTypeSizeInChars(EltTy);
+ CharUnits elementAlign =
+ ThisPtr.getAlignment().alignmentOfArrayElement(elementSize);
+
+ llvm::Value *arrayBegin = ThisPtr.emitRawPointer(CGF);
+ llvm::Value *arrayEnd = CGF.Builder.CreateInBoundsGEP(
+ ThisPtr.getElementType(), arrayBegin, numElements, "delete.end");
+
+ // We already checked that the array is not 0-length before entering vector
+ // deleting dtor.
+ CGF.emitArrayDestroy(arrayBegin, arrayEnd, EltTy, elementAlign,
+ CGF.getDestroyer(dtorKind),
+ /*checkZeroLength*/ false, CGF.needsEHCleanup(dtorKind));
+
+ llvm::BasicBlock *VectorBBCont = CGF.createBasicBlock("dtor.vector.cont");
+ CGF.EmitBlock(VectorBBCont);
+
+ llvm::Value *CheckTheBitForDeleteCall = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition,
+ llvm::Constant::getIntegerValue(CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/1)));
+
+ llvm::Value *ShouldCallDelete =
+ CGF.Builder.CreateIsNull(CheckTheBitForDeleteCall);
+ CGF.Builder.CreateCondBr(ShouldCallDelete, CGF.ReturnBlock.getBlock(),
+ callDeleteBB);
+ CGF.EmitBlock(callDeleteBB);
+ const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CGF.CurCodeDecl);
+ const CXXRecordDecl *ClassDecl = Dtor->getParent();
+ CGF.EmitDeleteCall(Dtor->getOperatorDelete(), allocatedPtr,
+ CGF.getContext().getTagDeclType(ClassDecl));
+
+ CGF.EmitBranchThroughCleanup(CGF.ReturnBlock);
+ CGF.EmitBlock(ScalarBB);
+}
+
/// EmitDestructorBody - Emits the body of the current destructor.
void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CurGD.getDecl());
@@ -1462,7 +1539,9 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
// outside of the function-try-block, which means it's always
// possible to delegate the destructor body to the complete
// destructor. Do so.
- if (DtorType == Dtor_Deleting) {
+ if (DtorType == Dtor_Deleting || DtorType == Dtor_VectorDeleting) {
+ if (CXXStructorImplicitParamValue && DtorType == Dtor_VectorDeleting)
+ EmitConditionalArrayDtorCall(Dtor, *this, CXXStructorImplicitParamValue);
RunCleanupsScope DtorEpilogue(*this);
EnterDtorCleanups(Dtor, Dtor_Deleting);
if (HaveInsertPoint()) {
@@ -1491,6 +1570,7 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
switch (DtorType) {
case Dtor_Comdat: llvm_unreachable("not expecting a COMDAT");
case Dtor_Deleting: llvm_unreachable("already handled deleting case");
+ case Dtor_VectorDeleting: llvm_unreachable("already handled vector deleting case");
case Dtor_Complete:
assert((Body || getTarget().getCXXABI().isMicrosoft()) &&
@@ -1567,13 +1647,6 @@ void CodeGenFunction::emitImplicitAssignmentOperatorBody(FunctionArgList &Args)
}
namespace {
- llvm::Value *LoadThisForDtorDelete(CodeGenFunction &CGF,
- const CXXDestructorDecl *DD) {
- if (Expr *ThisArg = DD->getOperatorDeleteThisArg())
- return CGF.EmitScalarExpr(ThisArg);
- return CGF.LoadCXXThis();
- }
-
/// Call the operator delete associated with the current destructor.
struct CallDtorDelete final : EHScopeStack::Cleanup {
CallDtorDelete() {}
@@ -1592,8 +1665,12 @@ namespace {
bool ReturnAfterDelete) {
llvm::BasicBlock *callDeleteBB = CGF.createBasicBlock("dtor.call_delete");
llvm::BasicBlock *continueBB = CGF.createBasicBlock("dtor.continue");
- llvm::Value *ShouldCallDelete
- = CGF.Builder.CreateIsNull(ShouldDeleteCondition);
+ auto *CondTy = cast<llvm::IntegerType>(ShouldDeleteCondition->getType());
+ llvm::Value *CheckTheBit = CGF.Builder.CreateAnd(
+ ShouldDeleteCondition, llvm::Constant::getIntegerValue(
+ CondTy, llvm::APInt(CondTy->getBitWidth(),
+ /*val=*/1)));
+ llvm::Value *ShouldCallDelete = CGF.Builder.CreateIsNull(CheckTheBit);
CGF.Builder.CreateCondBr(ShouldCallDelete, continueBB, callDeleteBB);
CGF.EmitBlock(callDeleteBB);
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index d5b584ec0f2e95b..da514ff612a32ef 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2104,7 +2104,8 @@ llvm::DISubprogram *CGDebugInfo::CreateCXXMemberFunction(
// Emit MS ABI vftable inf...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Ping. |
rnk
reviewed
Feb 20, 2025
| @@ -2013,7 +2023,7 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall( | |||
| ASTContext &Context = getContext(); | |||
| llvm::Value *ImplicitParam = llvm::ConstantInt::get( | |||
| llvm::IntegerType::getInt32Ty(CGF.getLLVMContext()), | |||
| DtorType == Dtor_Deleting); | |||
| 2 * (ArrayDeletion) + (DtorType == Dtor_Deleting)); | |||
There was a problem hiding this comment.
This is a bitfield parameter, so I think for readability it would be better to express this with shifts and ors, so something like:
uint32_t Flags = (ArrayDeletion << 1) | (DtorType == Dtor_Deleting);
llvm::Value *ImplicitParam = CGF.Builder.getInt32(Flags);
There was a problem hiding this comment.
Yeah, makes sense, thanks.
| !CGM.classNeedsVectorDestructor(dtor->getParent())) { | ||
| // Create GlobalDecl objects with the correct type for the vector and scalar | ||
| // deleting destructors. | ||
| GlobalDecl VectorDtorGD(dtor, Dtor_VectorDeleting); |
There was a problem hiding this comment.
Let's just use GD here rather than creating a new one, since they should be equivalent and GlobalDecl is a multi-word object.
|
Thanks for the updates, I think this is almost done. @zmodem , this will probably have some impact on Chrome code size and will probably churn many crash stack traces. I think it's safe to rely on all the existing automated testing, but this is a high-risk change to keep in mind if any Windows-specific issues arise. |
Thanks for the heads up! |
rnk
reviewed
Feb 26, 2025
clang/lib/AST/VTableBuilder.cpp
Outdated
| @@ -2657,7 +2657,10 @@ class VFTableBuilder { | |||
| MethodVFTableLocation Loc(MI.VBTableIndex, WhichVFPtr.getVBaseWithVPtr(), | |||
| WhichVFPtr.NonVirtualOffset, MI.VFTableIndex); | |||
| if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) { | |||
| MethodVFTableLocations[GlobalDecl(DD, Dtor_Deleting)] = Loc; | |||
| if (!Context.getTargetInfo().getCXXABI().isMicrosoft()) | |||
There was a problem hiding this comment.
Code golf nit: Rather than duplicating the densemap insertion you can select the appropriate dtor type and construct the GlobalDecl with a dtor type variable.
clang/lib/CodeGen/CGCXXABI.h
Outdated
| @@ -575,6 +576,12 @@ class CGCXXABI { | |||
| QualType ElementType, llvm::Value *&NumElements, | |||
| llvm::Value *&AllocPtr, CharUnits &CookieSize); | |||
|
|
|||
| /// Reads the array cookie associated with the given pointer, | |||
| /// that should have one. | |||
| virtual void ReadArrayCookie(CodeGenFunction &CGF, Address Ptr, | |||
There was a problem hiding this comment.
If there are no overrides, this doesn't need to be virtual.
There was a problem hiding this comment.
Ok, removed virtual. Does it make sense to do that for another overload of ReadArrayCookie as well?
rnk
reviewed
Feb 26, 2025
- avoid duplicating densemap insertion - remove virtual
The bugs seem related by both involving the ICU library, and if squinting a bit it seems they might both involve UnicodeString: It's not really clear which relocations against which .text section the linker is complaining about, but and that's the only vector deleting destructor I find referenced from In the crash from https://crbug.com/402425841 the code seems to segfault while doing something with So maybe there is something wrong with the vector deleting destructor for ICU's UnicodeString or how it's being used. |
|
Reduced linkage failure reproducer (can be reduced further, I'm sure), it consists of 4 files t.h: t.cpp t1.cpp t3.cpp Then t.obj and t1.cpp will trigger vector deleting destructor, t3.cpp will trigger scalar deleting destructor and a weak alias. What I've noticed is that LINK.exe (Microsoft linker) is fine with these 3 object files. I don't know what is going on during linkage when one object file has a definition of vector deleting destructor, but another object file has only an alias. vector deleting destructors are generated by this patch with weak linkage. Perhaps support is needed in lld-link for that case? The reason why it is triggered by sanitizer build, adding -fsanitize=fuzzer-no-link option causes adding a bunch of global variables to IR that reference the destructors and marked with SCOVP$M section: I'm now heading to the weekend, but I'll look a bit more before claiming problems in lld-link @zmodem , thanks for the findings
but, that is still not a great reproducer for a reverted patch. Especially if the link issue is due to a missing support in linker. I would appreciate if you could maybe point to how exactly |
|
I think the |
I'm sorry that we didn't catch this problem sooner and were able to provide better reproducers. I still think reverting was right though. There is clearly a subtle problem here -- both at link time and run time -- and the fewer people who run into that the better. Even if it turns out that the root cause lies in lld. I've uploaded a stand-alone from-source reproducer of the run time crash here: https://crbug.com/402425841#comment10 |
frederik-h
pushed a commit
to frederik-h/llvm-project
that referenced
this pull request
Mar 18, 2025
…#126240)" This caused link errors when building with sancov. See comment on the PR. > Whereas it is UB in terms of the standard to delete an array of objects > via pointer whose static type doesn't match its dynamic type, MSVC > supports an extension allowing to do it. > Aside from array deletion not working correctly in the mentioned case, > currently not having this extension implemented causes clang to generate > code that is not compatible with the code generated by MSVC, because > clang always puts scalar deleting destructor to the vftable. This PR > aims to resolve these problems. > > Fixes llvm#19772 This reverts commit d6942d5.
I see that similar globals generated for scalar deleting destructors are actually in the corresponding comdat groups. I suppose it might be due to this check that |
|
If we can separate the linkage of the alias and the vector deleting destructor definition, that might work. We need to block globalopt and other passes from looking through the alias and inlining scalar deleting destructor calls unless we can prove that the object being destroyed was constructed with non-array I threw together PR #131929 to tweak the condition there in sancov if you want to stamp it. I think it's an improvement on its own. Sancov should put these metadata sections into comdats if the existing function already has a comdat, regardless of what the linkage is. |
Reproduced. Seems limited to 32-bit build (only reproducible with -m32 option) and lld linker. Fine with either standard linker or 64-bit build. |
rnk
added a commit
that referenced
this pull request
Mar 19, 2025
This code avoids adding comdat groups to interposable linkage types (weak, linkonce (non-ODR)) to avoid changing semantics, since comdat elimination happens before weak/strong prevailaing symbol resolution. However, if the function is already in a comdat, we can add to the group without changing the semantics of the linked program. Fixes an issue uncovered in PR #126240
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. Fixes llvm#19772
|
So, the body of the destructor is not the problem. I commented out all new additions to body emission of the destructor and it still crashes. |
|
The problem also is only reproducible when using -start-lib -end-lib options around icu library object files in the linking command. |
|
I also noticed that removing -DU_STATIC_IMPLEMENTATION macro passing fixes the crash. |
|
I reduced main code to In order to avoid UniSet creation. @zmodem any chance you could help to reduce it further? I already spent reasonable amount of time doing this without a good output. This does seem like a problem that may have been introduced by the lld. The fact that it all passes in x64 mode, or without alias emission, or without -start-lib -end-lib options. But I am still not sure. I would appreciate any thoughts on this. @rnk , any chance you might be aware of some linkage-related specific affecting vector deleting destructors when building 32-bit applications? |
|
I'll try to take a look at this when I get a chance, but that looks like it's not happening today. I read the comments a bit and found some codesearch links worth sharing for reference. Here's the original UnicodeString array new operation: UnicodeString is a dynamic, polymorphic object with a virtual destructor, as one would expect. There are no DLLs in the build script. I think removing the U_STATIC_IMPLEMENTATION define makes all these classes dllexport, which disables our vector/scalar alias optimization, hiding the problem. More debugging is required. |
|
A few notes from looking today. Reduced main a bit further: That is, we don't need to run any code from filteredbrk.cpp, just reference it. Since the code won't be run, we can strip as much of it as possible. (Attaching what I got so far: These pieces of code are needed to keep reproducing: So we've got both scalar new and regular construction there. What's really interesting is that if I manually inline the crash goes away. That seems like a major hint. What's different in the object file before/after that manual inlining, and how might that affect linking? |
|
Removing |
|
Soo, I restored all the attributes on vector deleting destructor and the calling convention (so they are the same as in non-failing case) and the crash is gone. I'll put a PR reapplying the patch soon. The reduction helped a lot, thanks! |
Fznamznon
added a commit
to Fznamznon/llvm-project
that referenced
this pull request
Mar 28, 2025
…rs (llvm#126240)"" This reverts commit e11ede5.
Fznamznon
added a commit
that referenced
this pull request
Mar 31, 2025
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by #131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in #126240 (comment) . Fixes #19772
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 31, 2025
…tors (#133451) Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm/llvm-project#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm/llvm-project#126240 (comment) . Fixes llvm/llvm-project#19772
SchrodingerZhu
pushed a commit
to SchrodingerZhu/llvm-project
that referenced
this pull request
Mar 31, 2025
…133451) Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm#126240 (comment) . Fixes llvm#19772
KornevNikita
pushed a commit
to intel/llvm
that referenced
this pull request
May 27, 2025
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it. Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems. It was reverted due to link time errors in chromium with sanitizer coverage enabled, which is fixed by llvm/llvm-project#131929 . The second commit of this PR also contains a fix for a runtime failure in chromium reported in llvm/llvm-project#126240 (comment) . Fixes llvm/llvm-project#19772
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Whereas it is UB in terms of the standard to delete an array of objects via pointer whose static type doesn't match its dynamic type, MSVC supports an extension allowing to do it.
Aside from array deletion not working correctly in the mentioned case, currently not having this extension implemented causes clang to generate code that is not compatible with the code generated by MSVC, because clang always puts scalar deleting destructor to the vftable. This PR aims to resolve these problems.
Fixes #19772