Skip to content

[clang] Fix a crash when dynamic_cast-ing to a final class #148088

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −22
Open

[clang] Fix a crash when dynamic_cast-ing to a final class #148088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions clang/lib/CodeGen/CGCXXABI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
13 changes: 10 additions & 3 deletions clang/lib/CodeGen/CGExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efriedma-quic ah right, the issue is it is simultaneously working out if there is a path and working out the offset that should be taken. But I think that can be computed as a cumulative offset.

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!");
Expand All @@ -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);
Expand Down
15 changes: 7 additions & 8 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/CodeGen/MicrosoftCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand Down
16 changes: 16 additions & 0 deletions clang/test/CodeGenCXX/dynamic-cast-exact-disabled.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
18 changes: 18 additions & 0 deletions clang/test/CodeGenCXX/dynamic-cast-exact.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
Loading