[clang] Fix a crash when dynamic_cast-ing to a final class
#148088
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Oliver Hunt (ojhunt) ChangesThe verification pass correctly identifies that the phi node produced for an exact dynamic_cast that always fails. Rather than trying to construct pretend that we have pass and fail paths through an invalid exact dynamic_cast we handle the failure path explicitly. Full diff: https://github.com/llvm/llvm-project/pull/148088.diff 6 Files Affected:
diff --git a/clang/lib/CodeGen/CGCXXABI.h b/clang/lib/CodeGen/CGCXXABI.h
index 96fe04661944d..80aaacb56d436 100644
--- a/clang/lib/CodeGen/CGCXXABI.h
+++ b/clang/lib/CodeGen/CGCXXABI.h
@@ -296,12 +296,11 @@ class CGCXXABI {
/// Emit a dynamic_cast from SrcRecordTy to DestRecordTy. The cast fails if
/// the dynamic type of Value is not exactly DestRecordTy.
- virtual llvm::Value *emitExactDynamicCast(CodeGenFunction &CGF, Address Value,
- QualType SrcRecordTy,
- QualType DestTy,
- QualType DestRecordTy,
- llvm::BasicBlock *CastSuccess,
- llvm::BasicBlock *CastFail) = 0;
+ virtual std::optional<llvm::Value *>
+ emitExactDynamicCast(CodeGenFunction &CGF, Address Value,
+ QualType SrcRecordTy, QualType DestTy,
+ QualType DestRecordTy, llvm::BasicBlock *CastSuccess,
+ llvm::BasicBlock *CastFail) = 0;
virtual bool EmitBadCastCall(CodeGenFunction &CGF) = 0;
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index 359e30cb8f5cd..6ac3e27299e51 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -2341,8 +2341,15 @@ llvm::Value *CodeGenFunction::EmitDynamicCast(Address ThisAddr,
} else if (IsExact) {
// If the destination type is effectively final, this pointer points to the
// right type if and only if its vptr has the right value.
- Value = CGM.getCXXABI().emitExactDynamicCast(
- *this, ThisAddr, SrcRecordTy, DestTy, DestRecordTy, CastEnd, CastNull);
+ std::optional<llvm::Value *> ExactCast =
+ CGM.getCXXABI().emitExactDynamicCast(*this, ThisAddr, SrcRecordTy,
+ DestTy, DestRecordTy, CastEnd,
+ CastNull);
+ if (!ExactCast) {
+ Value = llvm::Constant::getNullValue(VoidPtrTy);
+ EmitBranch(CastNull);
+ } else
+ Value = *ExactCast;
} else {
assert(DestRecordTy->isRecordType() &&
"destination type must be a record type!");
@@ -2364,7 +2371,7 @@ llvm::Value *CodeGenFunction::EmitDynamicCast(Address ThisAddr,
EmitBlock(CastEnd);
- if (CastNull) {
+ if (CastNull && CastNotNull) {
llvm::PHINode *PHI = Builder.CreatePHI(Value->getType(), 2);
PHI->addIncoming(Value, CastNotNull);
PHI->addIncoming(NullValue, CastNull);
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index f4a99467010af..73d188f6ab998 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -231,11 +231,11 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
QualType DestRecordTy,
llvm::BasicBlock *CastEnd) override;
- llvm::Value *emitExactDynamicCast(CodeGenFunction &CGF, Address ThisAddr,
- QualType SrcRecordTy, QualType DestTy,
- QualType DestRecordTy,
- llvm::BasicBlock *CastSuccess,
- llvm::BasicBlock *CastFail) override;
+ std::optional<llvm::Value *>
+ emitExactDynamicCast(CodeGenFunction &CGF, Address ThisAddr,
+ QualType SrcRecordTy, QualType DestTy,
+ QualType DestRecordTy, llvm::BasicBlock *CastSuccess,
+ llvm::BasicBlock *CastFail) override;
llvm::Value *emitDynamicCastToVoid(CodeGenFunction &CGF, Address Value,
QualType SrcRecordTy) override;
@@ -1681,7 +1681,7 @@ llvm::Value *ItaniumCXXABI::emitDynamicCastCall(
return Value;
}
-llvm::Value *ItaniumCXXABI::emitExactDynamicCast(
+std::optional<llvm::Value *> ItaniumCXXABI::emitExactDynamicCast(
CodeGenFunction &CGF, Address ThisAddr, QualType SrcRecordTy,
QualType DestTy, QualType DestRecordTy, llvm::BasicBlock *CastSuccess,
llvm::BasicBlock *CastFail) {
@@ -1736,8 +1736,7 @@ llvm::Value *ItaniumCXXABI::emitExactDynamicCast(
if (!Offset) {
// If there are no public inheritance paths, the cast always fails.
- CGF.EmitBranch(CastFail);
- return llvm::PoisonValue::get(CGF.VoidPtrTy);
+ return std::nullopt;
}
// Compare the vptr against the expected vptr for the destination type at
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index a181559834296..133a2ee1e2433 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -158,11 +158,11 @@ class MicrosoftCXXABI : public CGCXXABI {
// TODO: Add support for exact dynamic_casts.
return false;
}
- llvm::Value *emitExactDynamicCast(CodeGenFunction &CGF, Address Value,
- QualType SrcRecordTy, QualType DestTy,
- QualType DestRecordTy,
- llvm::BasicBlock *CastSuccess,
- llvm::BasicBlock *CastFail) override {
+ std::optional<llvm::Value *>
+ emitExactDynamicCast(CodeGenFunction &CGF, Address Value,
+ QualType SrcRecordTy, QualType DestTy,
+ QualType DestRecordTy, llvm::BasicBlock *CastSuccess,
+ llvm::BasicBlock *CastFail) override {
llvm_unreachable("unsupported");
}
diff --git a/clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp b/clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
index 9a8ce1997a7f9..bf202d14c3398 100644
--- a/clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
+++ b/clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
@@ -13,3 +13,19 @@ B *exact(A *a) {
// EXACT-NOT: call {{.*}} @__dynamic_cast
return dynamic_cast<B*>(a);
}
+
+struct C {
+ virtual ~C();
+};
+
+struct D final : private C {
+
+};
+
+// CHECK-LABEL: @_Z5exactP1C
+D *exact(C *a) {
+ // INEXACT: call {{.*}} @__dynamic_cast
+ // EXACT: entry:
+ // EXACT-NEXT: ret ptr null
+ return dynamic_cast<D*>(a);
+}
diff --git a/clang/test/CodeGenCXX/dynamic-cast-exact.cpp b/clang/test/CodeGenCXX/dynamic-cast-exact.cpp
index 86e1965b4ba68..5fdb2f7eb3d75 100644
--- a/clang/test/CodeGenCXX/dynamic-cast-exact.cpp
+++ b/clang/test/CodeGenCXX/dynamic-cast-exact.cpp
@@ -9,6 +9,7 @@ struct E : A { int e; };
struct F : virtual A { int f; };
struct G : virtual A { int g; };
struct H final : C, D, E, F, G { int h; };
+struct H1 final: C, private D { int h1; };
// CHECK-LABEL: @_Z7inexactP1A
C *inexact(A *a) {
@@ -77,6 +78,23 @@ H *exact_multi(A *a) {
return dynamic_cast<H*>(a);
}
+// CHECK-LABEL: @_Z19exact_invalid_multiP1D
+H1 *exact_invalid_multi(D* d) {
+ // CHECK: dynamic_cast.end:
+ // CHECK-NEXT: ret ptr null
+ return dynamic_cast<H1*>(d);
+}
+
+// CHECK-LABEL: @_Z19exact_invalid_multiR1D
+H1 &exact_invalid_multi(D& d) {
+ // CHECK: dynamic_cast.notnull:
+ // CHECK-NEXT: br label %dynamic_cast.null
+ // CHECK: dynamic_cast.null:
+ // CHECK-NEXT: call void @__cxa_bad_cast()
+ // CHECK-NEXT: unreachable
+ return dynamic_cast<H1&>(d);
+}
+
namespace GH64088 {
// Ensure we mark the B vtable as used here, because we're going to emit a
// reference to it.
|
|
/ |
cor3ntin
changed the title
final class
| Value = CGM.getCXXABI().emitExactDynamicCast( | ||
| *this, ThisAddr, SrcRecordTy, DestTy, DestRecordTy, CastEnd, CastNull); | ||
| std::optional<llvm::Value *> ExactCast = | ||
| CGM.getCXXABI().emitExactDynamicCast(*this, ThisAddr, SrcRecordTy, |
There was a problem hiding this comment.
Can we restructure this code in a way that's easier to read?
Like, maybe split emitExactDynamicCast into two functions: one to compute the offset, and one to actually do the cast. If we fail to compute the offset, we can skip all the work constructing the control flow, because the result is always just null.
There was a problem hiding this comment.
@efriedma-quic yeah, I was convinced that there must have been a reason this was structured in this way, but having looked at it again I can't work out how/why that would be the case.
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.
The verification pass correctly identifies that the phi node produced for an exact dynamic_cast that always fails. Rather than trying to construct pretend that we have pass and fail paths through an invalid exact dynamic_cast we handle the failure path explicitly.
Fixes #137518