r257939 - Make -Wdelete-non-virtual-dtor warn on explicit `a->~A()` dtor calls too.
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 15 13:45:31 PST 2016
Author: nico
Date: Fri Jan 15 15:45:31 2016
New Revision: 257939
URL: http://llvm.org/viewvc/llvm-project?rev=257939&view=rev
Log:
Make -Wdelete-non-virtual-dtor warn on explicit `a->~A()` dtor calls too.
-Wdelete-non-virtual-dtor warns if A is a type with virtual functions but
without virtual dtor has its constructor called via `delete a`. This makes the
warning also fire if the dtor is called via `a->~A()`. This would've found a
security bug in Chromium at compile time. Fixes PR26137.
To fix the warning, add a virtual destructor, make the class final, or remove
its other virtual methods. If you want to silence the warning, there's also
a fixit that shows how:
test.cc:12:3: warning: destructor called on 'B' ... [-Wdelete-non-virtual-dtor]
b->~B();
^
test.cc:12:6: note: qualify call to silence this warning
b->~B();
^
B::
http://reviews.llvm.org/D16206
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaExprCXX.cpp
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/test/SemaCXX/destructor.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=257939&r1=257938&r2=257939&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jan 15 15:45:31 2016
@@ -5807,12 +5807,14 @@ def warn_non_virtual_dtor : Warning<
"%0 has virtual functions but non-virtual destructor">,
InGroup<NonVirtualDtor>, DefaultIgnore;
def warn_delete_non_virtual_dtor : Warning<
- "delete called on non-final %0 that has virtual functions "
- "but non-virtual destructor">,
+ "%select{delete|destructor}0 called on non-final %1 that has "
+ "virtual functions but non-virtual destructor">,
InGroup<DeleteNonVirtualDtor>, DefaultIgnore;
+def note_delete_non_virtual : Note<
+ "qualify call to silence this warning">;
def warn_delete_abstract_non_virtual_dtor : Warning<
- "delete called on %0 that is abstract but has non-virtual destructor">,
- InGroup<DeleteNonVirtualDtor>;
+ "%select{delete|destructor}0 called on %1 that is abstract but has "
+ "non-virtual destructor">, InGroup<DeleteNonVirtualDtor>;
def warn_overloaded_virtual : Warning<
"%q0 hides overloaded virtual %select{function|functions}1">,
InGroup<OverloadedVirtual>, DefaultIgnore;
Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=257939&r1=257938&r2=257939&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Fri Jan 15 15:45:31 2016
@@ -4698,6 +4698,10 @@ public:
ExprResult ActOnCXXDelete(SourceLocation StartLoc,
bool UseGlobal, bool ArrayForm,
Expr *Operand);
+ void CheckVirtualDtorCall(CXXDestructorDecl *dtor, SourceLocation Loc,
+ bool IsDelete, bool CallCanBeVirtual,
+ bool WarnOnNonAbstractTypes,
+ SourceLocation DtorLoc);
DeclResult ActOnCXXConditionDeclaration(Scope *S, Declarator &D);
ExprResult CheckConditionVariable(VarDecl *ConditionVar,
Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=257939&r1=257938&r2=257939&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Fri Jan 15 15:45:31 2016
@@ -2765,30 +2765,10 @@ Sema::ActOnCXXDelete(SourceLocation Star
return ExprError();
}
- // C++ [expr.delete]p3:
- // In the first alternative (delete object), if the static type of the
- // object to be deleted is different from its dynamic type, the static
- // type shall be a base class of the dynamic type of the object to be
- // deleted and the static type shall have a virtual destructor or the
- // behavior is undefined.
- //
- // Note: a final class cannot be derived from, no issue there
- if (PointeeRD->isPolymorphic() && !PointeeRD->hasAttr<FinalAttr>()) {
- CXXDestructorDecl *dtor = PointeeRD->getDestructor();
- if (dtor && !dtor->isVirtual()) {
- if (PointeeRD->isAbstract()) {
- // If the class is abstract, we warn by default, because we're
- // sure the code has undefined behavior.
- Diag(StartLoc, diag::warn_delete_abstract_non_virtual_dtor)
- << PointeeElem;
- } else if (!ArrayForm) {
- // Otherwise, if this is not an array delete, it's a bit suspect,
- // but not necessarily wrong.
- Diag(StartLoc, diag::warn_delete_non_virtual_dtor) << PointeeElem;
- }
- }
- }
-
+ CheckVirtualDtorCall(PointeeRD->getDestructor(), StartLoc,
+ /*IsDelete=*/true, /*CallCanBeVirtual=*/true,
+ /*WarnOnNonAbstractTypes=*/!ArrayForm,
+ SourceLocation());
}
if (!OperatorDelete)
@@ -2817,6 +2797,45 @@ Sema::ActOnCXXDelete(SourceLocation Star
return Result;
}
+void Sema::CheckVirtualDtorCall(CXXDestructorDecl *dtor, SourceLocation Loc,
+ bool IsDelete, bool CallCanBeVirtual,
+ bool WarnOnNonAbstractTypes,
+ SourceLocation DtorLoc) {
+ if (!dtor || dtor->isVirtual() || !CallCanBeVirtual)
+ return;
+
+ // C++ [expr.delete]p3:
+ // In the first alternative (delete object), if the static type of the
+ // object to be deleted is different from its dynamic type, the static
+ // type shall be a base class of the dynamic type of the object to be
+ // deleted and the static type shall have a virtual destructor or the
+ // behavior is undefined.
+ //
+ const CXXRecordDecl *PointeeRD = dtor->getParent();
+ // Note: a final class cannot be derived from, no issue there
+ if (!PointeeRD->isPolymorphic() || PointeeRD->hasAttr<FinalAttr>())
+ return;
+
+ QualType ClassType = dtor->getThisType(Context)->getPointeeType();
+ if (PointeeRD->isAbstract()) {
+ // If the class is abstract, we warn by default, because we're
+ // sure the code has undefined behavior.
+ Diag(Loc, diag::warn_delete_abstract_non_virtual_dtor) << (IsDelete ? 0 : 1)
+ << ClassType;
+ } else if (WarnOnNonAbstractTypes) {
+ // Otherwise, if this is not an array delete, it's a bit suspect,
+ // but not necessarily wrong.
+ Diag(Loc, diag::warn_delete_non_virtual_dtor) << (IsDelete ? 0 : 1)
+ << ClassType;
+ }
+ if (!IsDelete) {
+ std::string TypeStr;
+ ClassType.getAsStringInternal(TypeStr, getPrintingPolicy());
+ Diag(DtorLoc, diag::note_delete_non_virtual)
+ << FixItHint::CreateInsertion(DtorLoc, TypeStr + "::");
+ }
+}
+
/// \brief Check the use of the given variable as a C++ condition in an if,
/// while, do-while, or switch statement.
ExprResult Sema::CheckConditionVariable(VarDecl *ConditionVar,
Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=257939&r1=257938&r2=257939&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOverload.cpp Fri Jan 15 15:45:31 2016
@@ -12236,6 +12236,17 @@ Sema::BuildCallToMemberFunction(Scope *S
<< MD->getDeclName();
}
}
+
+ if (CXXDestructorDecl *DD =
+ dyn_cast<CXXDestructorDecl>(TheCall->getMethodDecl())) {
+ // a->A::f() doesn't go through the vtable, except in AppleKext mode.
+ bool CallCanBeVirtual = !cast<MemberExpr>(NakedMemExpr)->hasQualifier() ||
+ getLangOpts().AppleKext;
+ CheckVirtualDtorCall(DD, MemExpr->getLocStart(), /*IsDelete=*/false,
+ CallCanBeVirtual, /*WarnOnNonAbstractTypes=*/true,
+ MemExpr->getMemberLoc());
+ }
+
return MaybeBindToTemporary(TheCall);
}
Modified: cfe/trunk/test/SemaCXX/destructor.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/destructor.cpp?rev=257939&r1=257938&r2=257939&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/destructor.cpp (original)
+++ cfe/trunk/test/SemaCXX/destructor.cpp Fri Jan 15 15:45:31 2016
@@ -257,6 +257,7 @@ void nowarnnonpoly() {
}
}
+// FIXME: Why are these supposed to not warn?
void nowarnarray() {
{
B* b = new B[4];
@@ -311,6 +312,14 @@ void nowarn0() {
}
}
+void nowarn0_explicit_dtor(F* f, VB* vb, VD* vd, VF* vf) {
+ f->~F();
+ f->~F();
+ vb->~VB();
+ vd->~VD();
+ vf->~VF();
+}
+
void warn0() {
{
B* b = new B();
@@ -326,6 +335,17 @@ void warn0() {
}
}
+void warn0_explicit_dtor(B* b, B& br, D* d) {
+ b->~B(); // expected-warning {{destructor called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
+ b->B::~B(); // No warning when the call isn't virtual.
+
+ br.~B(); // expected-warning {{destructor called on non-final 'dnvd::B' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
+ br.B::~B();
+
+ d->~D(); // expected-warning {{destructor called on non-final 'dnvd::D' that has virtual functions but non-virtual destructor}} expected-note{{qualify call to silence this warning}}
+ d->D::~D();
+}
+
void nowarn1() {
{
simple_ptr<F> f(new F());
More information about the cfe-commits
mailing list