[MS][clang] Fix crash on deletion of array of pointers #134088
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
Sometimes a non-array delete is treated as delete[] when input pointer is pointer to array. With vector deleting destructors support we now generate a virtual destructor call instead of simple loop over the elements. This patch adjusts the codepath that generates virtual call to expect the case of pointer to array.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesSometimes a non-array delete is treated as delete[] when input pointer is pointer to array. With vector deleting destructors support we now generate a virtual destructor call instead of simple loop over the elements. This patch adjusts the codepath that generates virtual call to expect the case of pointer to array. Full diff: https://github.com/llvm/llvm-project/pull/134088.diff 3 Files Affected:
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9d5b4a60c9fe7..e7c3302f24756 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/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 464d4370284fb..ccf24e0a3ebd7 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -2033,6 +2033,9 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
ThisTy = D->getDestroyedType();
}
+ while (const ArrayType *ATy = ThisTy->getAsArrayTypeUnsafe())
+ ThisTy = ATy->getElementType();
+
This = adjustThisArgumentForVirtualFunctionCall(CGF, GD, This, true);
RValue RV =
CGF.EmitCXXDestructorCall(GD, Callee, This.emitRawPointer(CGF), ThisTy,
diff --git a/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
index 439ff84456033..9d23708602a43 100644
--- a/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
+++ b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
@@ -35,6 +35,10 @@ void operator delete(void *p) { i-=2; }
void operator delete[](void *p) { i--; }
};
+struct AllocatedAsArray : public Bird {
+
+};
+
// Vector deleting dtor for Bird is an alias because no new Bird[] expressions
// in the TU.
// X64: @"??_EBird@@UEAAPEAXI@Z" = weak dso_local unnamed_addr alias ptr (ptr, i32), ptr @"??_GBird@@UEAAPEAXI@Z"
@@ -55,6 +59,14 @@ Bird* alloc() {
return P;
}
+
+template<class C>
+struct S {
+ void foo() { void *p = new C(); delete (C *)p; }
+};
+
+S<AllocatedAsArray[1][3]> sp;
+
void bar() {
dealloc(alloc());
@@ -63,6 +75,8 @@ void bar() {
Bird *p = new HasOperatorDelete[2];
dealloc(p);
+
+ sp.foo();
}
// CHECK-LABEL: define dso_local void @{{.*}}dealloc{{.*}}(
@@ -99,6 +113,36 @@ void bar() {
// CHECK: delete.end:
// CHECK-NEXT: ret void
+// Definition of S::foo, check that it has vector deleting destructor call
+// X64-LABEL: define linkonce_odr dso_local void @"?foo@?$S@$$BY102UAllocatedAsArray@@@@QEAAXXZ"
+// X86-LABEL: define linkonce_odr dso_local x86_thiscallcc void @"?foo@?$S@$$BY102UAllocatedAsArray@@@@QAEXXZ"
+// CHECK: delete.notnull: ; preds = %arrayctor.cont
+// CHECK-NEXT: %[[DEL_PTR:.*]] = getelementptr inbounds [1 x [3 x %struct.AllocatedAsArray]], ptr %[[THE_ARRAY:.*]], i32 0, i32 0
+// X64-NEXT: %[[COOKIEGEP:.*]] = getelementptr inbounds i8, ptr %[[DEL_PTR]], i64 -8
+// X86-NEXT: %[[COOKIEGEP:.*]] = getelementptr inbounds i8, ptr %[[DEL_PTR]], i32 -4
+// X64-NEXT: %[[HOWMANY:.*]] = load i64, ptr %[[COOKIEGEP]]
+// X86-NEXT: %[[HOWMANY:.*]] = load i32, ptr %[[COOKIEGEP]]
+// X64-NEXT: %[[ISNOELEM:.*]] = icmp eq i64 %[[HOWMANY]], 0
+// X86-NEXT: %[[ISNOELEM:.*]] = icmp eq i32 %[[HOWMANY]], 0
+// CHECK-NEXT: br i1 %[[ISNOELEM]], label %vdtor.nocall, label %vdtor.call
+// CHECK: vdtor.nocall: ; preds = %delete.notnull
+// X64-NEXT: %[[HOWMANYBYTES:.*]] = mul i64 8, %[[HOWMANY]]
+// X86-NEXT: %[[HOWMANYBYTES:.*]] = mul i32 4, %[[HOWMANY]]
+// X64-NEXT: %[[ADDCOOKIESIZE:.*]] = add i64 %[[HOWMANYBYTES]], 8
+// X86-NEXT: %[[ADDCOOKIESIZE:.*]] = add i32 %[[HOWMANYBYTES]], 4
+// X64-NEXT: call void @"??_V@YAXPEAX_K@Z"(ptr noundef %[[COOKIEGEP]], i64 noundef %[[ADDCOOKIESIZE]])
+// X86-NEXT: call void @"??_V@YAXPAXI@Z"(ptr noundef %[[COOKIEGEP]], i32 noundef %[[ADDCOOKIESIZE]])
+// CHECK-NEXT: br label %delete.end
+// CHECK: vdtor.call: ; preds = %delete.notnull
+// CHECK-NEXT: %[[VTABLE:.*]] = load ptr, ptr %[[DEL_PTR]]
+// CHECK-NEXT: %[[FPGEP:.*]] = getelementptr inbounds ptr, ptr %[[VTABLE]], i64 0
+// CHECK-NEXT: %[[FPLOAD:.*]] = load ptr, ptr %[[FPGEP]]
+// X64-NEXT: %[[CALL:.*]] = call noundef ptr %[[FPLOAD]](ptr noundef nonnull align 8 dereferenceable(8) %[[DEL_PTR]], i32 noundef 3)
+// X86-NEXT: %[[CALL:.*]] = call x86_thiscallcc noundef ptr %[[FPLOAD]](ptr noundef nonnull align 4 dereferenceable(4) %[[DEL_PTR]], i32 noundef 3)
+// CHECK-NEXT: br label %delete.end
+// CHECK: delete.end:
+// CHECK-NEXT: ret void
+
// Vector dtor definition for Parrot.
// X64-LABEL: define weak dso_local noundef ptr @"??_EParrot@@UEAAPEAXI@Z"(
// X64-SAME: ptr {{.*}} %[[THIS:.*]], i32 {{.*}} %[[IMPLICIT_PARAM:.*]]) unnamed_addr
@@ -169,3 +213,6 @@ void bar() {
// CHECK: dtor.call_delete:
// X64-NEXT: call void @"??3HasOperatorDelete@@SAXPEAX@Z"
// X86-NEXT: call void @"??3HasOperatorDelete@@SAXPAX@Z"
+
+// X64: define weak dso_local noundef ptr @"??_EAllocatedAsArray@@UEAAPEAXI@Z"
+// X86: define weak dso_local x86_thiscallcc noundef ptr @"??_EAllocatedAsArray@@UAEPAXI@Z"
|
|
@llvm/pr-subscribers-clang-codegen Author: Mariya Podchishchaeva (Fznamznon) ChangesSometimes a non-array delete is treated as delete[] when input pointer is pointer to array. With vector deleting destructors support we now generate a virtual destructor call instead of simple loop over the elements. This patch adjusts the codepath that generates virtual call to expect the case of pointer to array. Full diff: https://github.com/llvm/llvm-project/pull/134088.diff 3 Files Affected:
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9d5b4a60c9fe7..e7c3302f24756 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/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 464d4370284fb..ccf24e0a3ebd7 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -2033,6 +2033,9 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall(
ThisTy = D->getDestroyedType();
}
+ while (const ArrayType *ATy = ThisTy->getAsArrayTypeUnsafe())
+ ThisTy = ATy->getElementType();
+
This = adjustThisArgumentForVirtualFunctionCall(CGF, GD, This, true);
RValue RV =
CGF.EmitCXXDestructorCall(GD, Callee, This.emitRawPointer(CGF), ThisTy,
diff --git a/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
index 439ff84456033..9d23708602a43 100644
--- a/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
+++ b/clang/test/CodeGenCXX/microsoft-vector-deleting-dtors.cpp
@@ -35,6 +35,10 @@ void operator delete(void *p) { i-=2; }
void operator delete[](void *p) { i--; }
};
+struct AllocatedAsArray : public Bird {
+
+};
+
// Vector deleting dtor for Bird is an alias because no new Bird[] expressions
// in the TU.
// X64: @"??_EBird@@UEAAPEAXI@Z" = weak dso_local unnamed_addr alias ptr (ptr, i32), ptr @"??_GBird@@UEAAPEAXI@Z"
@@ -55,6 +59,14 @@ Bird* alloc() {
return P;
}
+
+template<class C>
+struct S {
+ void foo() { void *p = new C(); delete (C *)p; }
+};
+
+S<AllocatedAsArray[1][3]> sp;
+
void bar() {
dealloc(alloc());
@@ -63,6 +75,8 @@ void bar() {
Bird *p = new HasOperatorDelete[2];
dealloc(p);
+
+ sp.foo();
}
// CHECK-LABEL: define dso_local void @{{.*}}dealloc{{.*}}(
@@ -99,6 +113,36 @@ void bar() {
// CHECK: delete.end:
// CHECK-NEXT: ret void
+// Definition of S::foo, check that it has vector deleting destructor call
+// X64-LABEL: define linkonce_odr dso_local void @"?foo@?$S@$$BY102UAllocatedAsArray@@@@QEAAXXZ"
+// X86-LABEL: define linkonce_odr dso_local x86_thiscallcc void @"?foo@?$S@$$BY102UAllocatedAsArray@@@@QAEXXZ"
+// CHECK: delete.notnull: ; preds = %arrayctor.cont
+// CHECK-NEXT: %[[DEL_PTR:.*]] = getelementptr inbounds [1 x [3 x %struct.AllocatedAsArray]], ptr %[[THE_ARRAY:.*]], i32 0, i32 0
+// X64-NEXT: %[[COOKIEGEP:.*]] = getelementptr inbounds i8, ptr %[[DEL_PTR]], i64 -8
+// X86-NEXT: %[[COOKIEGEP:.*]] = getelementptr inbounds i8, ptr %[[DEL_PTR]], i32 -4
+// X64-NEXT: %[[HOWMANY:.*]] = load i64, ptr %[[COOKIEGEP]]
+// X86-NEXT: %[[HOWMANY:.*]] = load i32, ptr %[[COOKIEGEP]]
+// X64-NEXT: %[[ISNOELEM:.*]] = icmp eq i64 %[[HOWMANY]], 0
+// X86-NEXT: %[[ISNOELEM:.*]] = icmp eq i32 %[[HOWMANY]], 0
+// CHECK-NEXT: br i1 %[[ISNOELEM]], label %vdtor.nocall, label %vdtor.call
+// CHECK: vdtor.nocall: ; preds = %delete.notnull
+// X64-NEXT: %[[HOWMANYBYTES:.*]] = mul i64 8, %[[HOWMANY]]
+// X86-NEXT: %[[HOWMANYBYTES:.*]] = mul i32 4, %[[HOWMANY]]
+// X64-NEXT: %[[ADDCOOKIESIZE:.*]] = add i64 %[[HOWMANYBYTES]], 8
+// X86-NEXT: %[[ADDCOOKIESIZE:.*]] = add i32 %[[HOWMANYBYTES]], 4
+// X64-NEXT: call void @"??_V@YAXPEAX_K@Z"(ptr noundef %[[COOKIEGEP]], i64 noundef %[[ADDCOOKIESIZE]])
+// X86-NEXT: call void @"??_V@YAXPAXI@Z"(ptr noundef %[[COOKIEGEP]], i32 noundef %[[ADDCOOKIESIZE]])
+// CHECK-NEXT: br label %delete.end
+// CHECK: vdtor.call: ; preds = %delete.notnull
+// CHECK-NEXT: %[[VTABLE:.*]] = load ptr, ptr %[[DEL_PTR]]
+// CHECK-NEXT: %[[FPGEP:.*]] = getelementptr inbounds ptr, ptr %[[VTABLE]], i64 0
+// CHECK-NEXT: %[[FPLOAD:.*]] = load ptr, ptr %[[FPGEP]]
+// X64-NEXT: %[[CALL:.*]] = call noundef ptr %[[FPLOAD]](ptr noundef nonnull align 8 dereferenceable(8) %[[DEL_PTR]], i32 noundef 3)
+// X86-NEXT: %[[CALL:.*]] = call x86_thiscallcc noundef ptr %[[FPLOAD]](ptr noundef nonnull align 4 dereferenceable(4) %[[DEL_PTR]], i32 noundef 3)
+// CHECK-NEXT: br label %delete.end
+// CHECK: delete.end:
+// CHECK-NEXT: ret void
+
// Vector dtor definition for Parrot.
// X64-LABEL: define weak dso_local noundef ptr @"??_EParrot@@UEAAPEAXI@Z"(
// X64-SAME: ptr {{.*}} %[[THIS:.*]], i32 {{.*}} %[[IMPLICIT_PARAM:.*]]) unnamed_addr
@@ -169,3 +213,6 @@ void bar() {
// CHECK: dtor.call_delete:
// X64-NEXT: call void @"??3HasOperatorDelete@@SAXPEAX@Z"
// X86-NEXT: call void @"??3HasOperatorDelete@@SAXPAX@Z"
+
+// X64: define weak dso_local noundef ptr @"??_EAllocatedAsArray@@UEAAPEAXI@Z"
+// X86: define weak dso_local x86_thiscallcc noundef ptr @"??_EAllocatedAsArray@@UAEPAXI@Z"
|
rnk
approved these changes
Apr 2, 2025
| @@ -2033,6 +2033,9 @@ llvm::Value *MicrosoftCXXABI::EmitVirtualDestructorCall( | |||
| ThisTy = D->getDestroyedType(); | |||
| } | |||
|
|
|||
| while (const ArrayType *ATy = ThisTy->getAsArrayTypeUnsafe()) | |||
There was a problem hiding this comment.
getAsArrayTypeUnsafe() is "unsafe" for a reason: it discards qualifiers. EmitCXXDestructorCall() queries qualifiers, so discarding them seems like a problem.
There was a problem hiding this comment.
Thanks for the catch! Yeah, I knew it drops qualifiers. I kind of didn't expect that something cares about qualifiers that deep in the codegen.
It seems EmitCXXDestructorCall cares about address spaces. Usually targets that care about address spaces don't support virtual calls. So I'm not sure how I can break or test this.
But I made it "safe" by using ASTContext since it is anyway easily accessible here.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/7144 Here is the relevant piece of the build log for the reference |
zmodem
pushed a commit
that referenced
this pull request
Apr 14, 2025
Finding operator delete[] is still problematic, without it the extension is a security hazard, so reverting until the problem with operator delete[] is figured out. This reverts the following PRs: Reland [MS][clang] Add support for vector deleting destructors (#133451) [MS][clang] Make sure vector deleting dtor calls correct operator delete (#133950) [MS][clang] Fix crash on deletion of array of pointers (#134088) [clang] Do not diagnose unused deleted operator delete[] (#134357) [MS][clang] Error about ambiguous operator delete[] only when required (#135041)
var-const
pushed a commit
to ldionne/llvm-project
that referenced
this pull request
Apr 17, 2025
Finding operator delete[] is still problematic, without it the extension is a security hazard, so reverting until the problem with operator delete[] is figured out. This reverts the following PRs: Reland [MS][clang] Add support for vector deleting destructors (llvm#133451) [MS][clang] Make sure vector deleting dtor calls correct operator delete (llvm#133950) [MS][clang] Fix crash on deletion of array of pointers (llvm#134088) [clang] Do not diagnose unused deleted operator delete[] (llvm#134357) [MS][clang] Error about ambiguous operator delete[] only when required (llvm#135041)
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.
Sometimes a non-array delete is treated as delete[] when input pointer is pointer to array. With vector deleting destructors support we now generate a virtual destructor call instead of simple loop over the elements. This patch adjusts the codepath that generates virtual call to expect the case of pointer to array.