[patch] add -Wdelete-non-virtual-dtor

[patch] add -Wdelete-non-virtual-dtor

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

-Wnon-virtual-dtor isn't always what you want, defining a polymorphic
object without a virtual destructor is not necessarily a mistake. You
may never delete such an object so instead of warning when the class
is defined it's more useful to warn only when the class is deleted, as
Clang does with -Wdelete-non-virtual-dtor

This patch implements the same warning for G++.

Unlike Clang's, this diagnostic distinguishes the case where the
pointer might refer to a base class from the case where it definitely
does because the static type is abstract.  There's no warning for a
C++0x final class, since the pointer must refer to the most-derived
type.    I'm not entirely happy with the diagnostic text as I don't
think we usually refer to undefined behaviour.  Better suggestions
welcome.

The new test uses -std=gnu++0x in order to test a 'final' class,
should that be split into a separate c++0x test, so the rest uses
gnu++98?

I'm not sure why PR c++/7302 is still open, but I think this would
allow it to be closed, as it issues a warning for the delete
expression in comment 25.

Bootstrapped and tested x86_64-linux

OK for trunk?


c-family/ChangeLog:

        * c.opt: Add -Wdelete-non-virtual-dtor.
        * c-opts.c (c_common_handle_option): Include it in -Wall.

cp/ChangeLog:

        * init.c (build_delete): Warn when deleting type with non-virtual
        destructor.

testsuite/ChangeLog:

        * testsuite/g++.dg/warn/delete-non-virtual-dtor.cc: New.

ChangeLog:

        * doc/invoke.texi: Document -Wdelete-non-virtual-dtor.

[-- Attachment #2: delnonvdtor.txt --]
[-- Type: text/plain, Size: 4500 bytes --]

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 174539)
+++ c-family/c.opt	(working copy)
@@ -331,6 +331,10 @@ Wdeclaration-after-statement
 C ObjC Var(warn_declaration_after_statement) Warning
 Warn when a declaration is found after a statement
 
+Wdelete-non-virtual-dtor
+C++ ObjC++ Var(warn_delnonvdtor) Warning
+Warn about deleting polymorphic objects with non-virtual destructors
+
 Wdeprecated
 C C++ ObjC ObjC++ Var(warn_deprecated) Init(1) Warning
 Warn if a deprecated compiler feature, class, method, or field is used
Index: c-family/c-opts.c
===================================================================
--- c-family/c-opts.c	(revision 174539)
+++ c-family/c-opts.c	(working copy)
@@ -405,6 +405,7 @@ c_common_handle_option (size_t scode, co
           warn_sign_compare = value;
 	  warn_reorder = value;
           warn_cxx0x_compat = value;
+          warn_delnonvdtor = value;
 	}
 
       cpp_opts->warn_trigraphs = value;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 174539)
+++ cp/init.c	(working copy)
@@ -3421,6 +3421,31 @@ build_delete (tree type, tree addr, spec
 		}
 	      complete_p = false;
 	    }
+          else if (warn_delnonvdtor && MAYBE_CLASS_TYPE_P (type)
+                   && !CLASSTYPE_FINAL (type) && TYPE_POLYMORPHIC_P (type))
+	    {
+	      tree dtor;
+	      dtor = CLASSTYPE_DESTRUCTORS (type);
+	      if (!dtor || !DECL_VINDEX (dtor))
+	        {
+		  tree x;
+		  bool abstract = false;
+		  for (x = TYPE_METHODS (type); x; x = DECL_CHAIN (x))
+		    if (DECL_PURE_VIRTUAL_P (x))
+		      {
+			abstract = true;
+			break;
+		      }
+		  if (abstract)
+		    warning(OPT_Wdelete_non_virtual_dtor, "deleting object of"
+                            " abstract class type %qT which has non-virtual"
+                            " destructor will cause undefined behaviour", type);
+		  else
+		    warning(OPT_Wdelete_non_virtual_dtor, "deleting object of"
+                            " polymorphic class type %qT which has non-virtual"
+                            " destructor may cause undefined behaviour", type);
+		}
+	    }
 	}
       if (VOID_TYPE_P (type) || !complete_p || !MAYBE_CLASS_TYPE_P (type))
 	/* Call the builtin operator delete.  */
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 174539)
+++ doc/invoke.texi	(working copy)
@@ -2331,6 +2331,15 @@ Warn when a class seems unusable because
 destructors in that class are private, and it has neither friends nor
 public static member functions.
 
+@item -Wdelete-non-virtual-dtor @r{(C++ and Objective-C++ only)}
+@opindex Wdelete-non-virtual-dtor
+@opindex Wno-delete-non-virtual-dtor
+Warn when @samp{delete} is used to destroy an instance of a class which
+has virtual functions and non-virtual destructor. It is unsafe to delete
+an instance of a derived class through a pointer to a base class if the
+base class does not have a virtual destructor.  This warning is enabled
+by @option{-Wall}.
+
 @item -Wnoexcept @r{(C++ and Objective-C++ only)}
 @opindex Wnoexcept
 @opindex Wno-noexcept
Index: testsuite/g++.dg/warn/delete-non-virtual-dtor.C
===================================================================
--- testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
+++ testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
@@ -0,0 +1,44 @@
+// { dg-options "-std=gnu++0x -Wdelete-non-virtual-dtor"
+// { dg-do compile }
+
+struct polyBase { virtual void f(); };
+
+void f(polyBase* p, polyBase* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct polyDerived : polyBase { };
+
+void f(polyDerived* p, polyDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct absDerived : polyBase { virtual void g() = 0; };
+
+void f(absDerived* p, absDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor will" }
+  delete [] arr;
+}
+
+struct finalDerived final : polyBase { };
+
+void f(finalDerived* p, finalDerived* arr)
+{
+  delete p;      // no error for final classes
+  delete [] arr;
+}
+
+struct safeBase { virtual ~safeBase(); };
+struct safeDerived : safeBase { virtual void f(); };
+
+void f(safeDerived* p, safeDerived* arr)
+{
+  delete p;      // no error because base has virtual dtor
+  delete [] arr;
+}
+

Re: [patch] add -Wdelete-non-virtual-dtor

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On 2 June 2011 22:27, Jonathan Wakely wrote:
> -Wnon-virtual-dtor isn't always what you want, defining a polymorphic
> object without a virtual destructor is not necessarily a mistake. You
> may never delete such an object so instead of warning when the class
> is defined it's more useful to warn only when the class is deleted, as
> Clang does with -Wdelete-non-virtual-dtor
>
> This patch implements the same warning for G++.

That patch was the wrong one, with a typo in the new test. The correct
one, as tested, is attached, but only differs in the additional }
character in the testcase.

[-- Attachment #2: delnonvdtor.2.txt --]
[-- Type: text/plain, Size: 4502 bytes --]

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 174539)
+++ c-family/c.opt	(working copy)
@@ -331,6 +331,10 @@ Wdeclaration-after-statement
 C ObjC Var(warn_declaration_after_statement) Warning
 Warn when a declaration is found after a statement
 
+Wdelete-non-virtual-dtor
+C++ ObjC++ Var(warn_delnonvdtor) Warning
+Warn about deleting polymorphic objects with non-virtual destructors
+
 Wdeprecated
 C C++ ObjC ObjC++ Var(warn_deprecated) Init(1) Warning
 Warn if a deprecated compiler feature, class, method, or field is used
Index: c-family/c-opts.c
===================================================================
--- c-family/c-opts.c	(revision 174539)
+++ c-family/c-opts.c	(working copy)
@@ -405,6 +405,7 @@ c_common_handle_option (size_t scode, co
           warn_sign_compare = value;
 	  warn_reorder = value;
           warn_cxx0x_compat = value;
+          warn_delnonvdtor = value;
 	}
 
       cpp_opts->warn_trigraphs = value;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 174539)
+++ cp/init.c	(working copy)
@@ -3421,6 +3421,31 @@ build_delete (tree type, tree addr, spec
 		}
 	      complete_p = false;
 	    }
+          else if (warn_delnonvdtor && MAYBE_CLASS_TYPE_P (type)
+                   && !CLASSTYPE_FINAL (type) && TYPE_POLYMORPHIC_P (type))
+	    {
+	      tree dtor;
+	      dtor = CLASSTYPE_DESTRUCTORS (type);
+	      if (!dtor || !DECL_VINDEX (dtor))
+	        {
+		  tree x;
+		  bool abstract = false;
+		  for (x = TYPE_METHODS (type); x; x = DECL_CHAIN (x))
+		    if (DECL_PURE_VIRTUAL_P (x))
+		      {
+			abstract = true;
+			break;
+		      }
+		  if (abstract)
+		    warning(OPT_Wdelete_non_virtual_dtor, "deleting object of"
+                            " abstract class type %qT which has non-virtual"
+                            " destructor will cause undefined behaviour", type);
+		  else
+		    warning(OPT_Wdelete_non_virtual_dtor, "deleting object of"
+                            " polymorphic class type %qT which has non-virtual"
+                            " destructor may cause undefined behaviour", type);
+		}
+	    }
 	}
       if (VOID_TYPE_P (type) || !complete_p || !MAYBE_CLASS_TYPE_P (type))
 	/* Call the builtin operator delete.  */
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 174539)
+++ doc/invoke.texi	(working copy)
@@ -2331,6 +2331,15 @@ Warn when a class seems unusable because
 destructors in that class are private, and it has neither friends nor
 public static member functions.
 
+@item -Wdelete-non-virtual-dtor @r{(C++ and Objective-C++ only)}
+@opindex Wdelete-non-virtual-dtor
+@opindex Wno-delete-non-virtual-dtor
+Warn when @samp{delete} is used to destroy an instance of a class which
+has virtual functions and non-virtual destructor. It is unsafe to delete
+an instance of a derived class through a pointer to a base class if the
+base class does not have a virtual destructor.  This warning is enabled
+by @option{-Wall}.
+
 @item -Wnoexcept @r{(C++ and Objective-C++ only)}
 @opindex Wnoexcept
 @opindex Wno-noexcept
Index: testsuite/g++.dg/warn/delete-non-virtual-dtor.C
===================================================================
--- testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
+++ testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
@@ -0,0 +1,44 @@
+// { dg-options "-std=gnu++0x -Wdelete-non-virtual-dtor" }
+// { dg-do compile }
+
+struct polyBase { virtual void f(); };
+
+void f(polyBase* p, polyBase* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct polyDerived : polyBase { };
+
+void f(polyDerived* p, polyDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct absDerived : polyBase { virtual void g() = 0; };
+
+void f(absDerived* p, absDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor will" }
+  delete [] arr;
+}
+
+struct finalDerived final : polyBase { };
+
+void f(finalDerived* p, finalDerived* arr)
+{
+  delete p;      // no error for final classes
+  delete [] arr;
+}
+
+struct safeBase { virtual ~safeBase(); };
+struct safeDerived : safeBase { virtual void f(); };
+
+void f(safeDerived* p, safeDerived* arr)
+{
+  delete p;      // no error because base has virtual dtor
+  delete [] arr;
+}
+

Re: [patch] add -Wdelete-non-virtual-dtor

[Jason Merrill]

On 06/02/2011 06:02 PM, Jonathan Wakely wrote:
> +	      if (!dtor || !DECL_VINDEX (dtor))

Do we really want to warn about the case where the class has no/trivial 
destructor?

> +		  bool abstract = false;
> +		  for (x = TYPE_METHODS (type); x; x = DECL_CHAIN (x))
> +		    if (DECL_PURE_VIRTUAL_P (x))
> +		      {
> +			abstract = true;
> +			break;
> +		      }
> +		  if (abstract)

Just check CLASSTYPE_PURE_VIRTUALS.

Jason

Re: [patch] add -Wdelete-non-virtual-dtor

[Jonathan Wakely]

On 3 June 2011 02:55, Jason Merrill wrote:
> On 06/02/2011 06:02 PM, Jonathan Wakely wrote:
>>
>> +             if (!dtor || !DECL_VINDEX (dtor))
>
> Do we really want to warn about the case where the class has no/trivial
> destructor?

I think so.  Definitely if it's an abstract class, since it's
definitely undefined behaviour.  The user might have forgotten to
define the destructor, so the warning would draw their attention to
it.

For example:

struct abc { virtual void f() = 0; };
struct interface : abc { virtual ~interface(); };
void f(abc* p) { delete p; }

The intended use might be to destroy such objects through a pointer to
interface, but f() has been defined to take the wrong type.

>> +                 bool abstract = false;
>> +                 for (x = TYPE_METHODS (type); x; x = DECL_CHAIN (x))
>> +                   if (DECL_PURE_VIRTUAL_P (x))
>> +                     {
>> +                       abstract = true;
>> +                       break;
>> +                     }
>> +                 if (abstract)
>
> Just check CLASSTYPE_PURE_VIRTUALS.

Thanks, I'll retest with that change ...

Re: [patch] add -Wdelete-non-virtual-dtor

[Jason Merrill]

On 06/03/2011 04:20 AM, Jonathan Wakely wrote:
> On 3 June 2011 02:55, Jason Merrill wrote:

>> Do we really want to warn about the case where the class has no/trivial
>> destructor?
>
> I think so.  Definitely if it's an abstract class, since it's
> definitely undefined behaviour.  The user might have forgotten to
> define the destructor, so the warning would draw their attention to
> it.

Makes sense.

Jason

Re: [patch] add -Wdelete-non-virtual-dtor

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

New patch using CLASSTYPE_PURE_VIRTUALS, thanks for that.

Bootstrapped and tested again on x86_64-linux, no regressions.

ChangeLogs as before, OK for trunk?

[-- Attachment #2: delnonvdtor.3.txt --]
[-- Type: text/plain, Size: 4246 bytes --]

Index: c-family/c.opt
===================================================================
--- c-family/c.opt	(revision 174624)
+++ c-family/c.opt	(working copy)
@@ -331,6 +331,10 @@ Wdeclaration-after-statement
 C ObjC Var(warn_declaration_after_statement) Warning
 Warn when a declaration is found after a statement
 
+Wdelete-non-virtual-dtor
+C++ ObjC++ Var(warn_delnonvdtor) Warning
+Warn about deleting polymorphic objects with non-virtual destructors
+
 Wdeprecated
 C C++ ObjC ObjC++ Var(warn_deprecated) Init(1) Warning
 Warn if a deprecated compiler feature, class, method, or field is used
Index: c-family/c-opts.c
===================================================================
--- c-family/c-opts.c	(revision 174624)
+++ c-family/c-opts.c	(working copy)
@@ -405,6 +405,7 @@ c_common_handle_option (size_t scode, co
           warn_sign_compare = value;
 	  warn_reorder = value;
           warn_cxx0x_compat = value;
+          warn_delnonvdtor = value;
 	}
 
       cpp_opts->warn_trigraphs = value;
Index: cp/init.c
===================================================================
--- cp/init.c	(revision 174624)
+++ cp/init.c	(working copy)
@@ -3421,6 +3421,25 @@ build_delete (tree type, tree addr, spec
 		}
 	      complete_p = false;
 	    }
+	  else if (warn_delnonvdtor && MAYBE_CLASS_TYPE_P (type)
+		   && !CLASSTYPE_FINAL (type) && TYPE_POLYMORPHIC_P (type))
+	    {
+	      tree dtor;
+	      dtor = CLASSTYPE_DESTRUCTORS (type);
+	      if (!dtor || !DECL_VINDEX (dtor))
+	        {
+		  if (CLASSTYPE_PURE_VIRTUALS (type))
+		    warning(OPT_Wdelete_non_virtual_dtor,
+			    "deleting object of abstract class type %qT"
+			    " which has non-virtual destructor"
+			    " will cause undefined behaviour", type);
+		  else
+		    warning(OPT_Wdelete_non_virtual_dtor,
+			    "deleting object of polymorphic class type %qT"
+			    " which has non-virtual destructor"
+			    " may cause undefined behaviour", type);
+		}
+	    }
 	}
       if (VOID_TYPE_P (type) || !complete_p || !MAYBE_CLASS_TYPE_P (type))
 	/* Call the builtin operator delete.  */
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 174624)
+++ doc/invoke.texi	(working copy)
@@ -2331,6 +2331,15 @@ Warn when a class seems unusable because
 destructors in that class are private, and it has neither friends nor
 public static member functions.
 
+@item -Wdelete-non-virtual-dtor @r{(C++ and Objective-C++ only)}
+@opindex Wdelete-non-virtual-dtor
+@opindex Wno-delete-non-virtual-dtor
+Warn when @samp{delete} is used to destroy an instance of a class which
+has virtual functions and non-virtual destructor. It is unsafe to delete
+an instance of a derived class through a pointer to a base class if the
+base class does not have a virtual destructor.  This warning is enabled
+by @option{-Wall}.
+
 @item -Wnoexcept @r{(C++ and Objective-C++ only)}
 @opindex Wnoexcept
 @opindex Wno-noexcept
Index: testsuite/g++.dg/warn/delete-non-virtual-dtor.C
===================================================================
--- testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
+++ testsuite/g++.dg/warn/delete-non-virtual-dtor.C	(revision 0)
@@ -0,0 +1,44 @@
+// { dg-options "-std=gnu++0x -Wdelete-non-virtual-dtor" }
+// { dg-do compile }
+
+struct polyBase { virtual void f(); };
+
+void f(polyBase* p, polyBase* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct polyDerived : polyBase { };
+
+void f(polyDerived* p, polyDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor may" }
+  delete [] arr;
+}
+
+struct absDerived : polyBase { virtual void g() = 0; };
+
+void f(absDerived* p, absDerived* arr)
+{
+  delete p;      // { dg-warning "non-virtual destructor will" }
+  delete [] arr;
+}
+
+struct finalDerived final : polyBase { };
+
+void f(finalDerived* p, finalDerived* arr)
+{
+  delete p;      // no error for final classes
+  delete [] arr;
+}
+
+struct safeBase { virtual ~safeBase(); };
+struct safeDerived : safeBase { virtual void f(); };
+
+void f(safeDerived* p, safeDerived* arr)
+{
+  delete p;      // no error because base has virtual dtor
+  delete [] arr;
+}
+

Re: [patch] add -Wdelete-non-virtual-dtor

[Jason Merrill]

On 06/04/2011 08:45 AM, Jonathan Wakely wrote:
> +		  if (CLASSTYPE_PURE_VIRTUALS (type))
> +		    warning(OPT_Wdelete_non_virtual_dtor,
> +			    "deleting object of abstract class type %qT"
> +			    " which has non-virtual destructor"
> +			    " will cause undefined behaviour", type);
> +		  else
> +		    warning(OPT_Wdelete_non_virtual_dtor,
> +			    "deleting object of polymorphic class type %qT"
> +			    " which has non-virtual destructor"
> +			    " may cause undefined behaviour", type);

Space before the (.  And let's use "might" instead of "may".  OK with 
those changes.

Jason

Re: [patch] add -Wdelete-non-virtual-dtor

[Jonathan Wakely]

On 4 June 2011 16:49, Jason Merrill wrote:
> On 06/04/2011 08:45 AM, Jonathan Wakely wrote:
>>
>> +                 if (CLASSTYPE_PURE_VIRTUALS (type))
>> +                   warning(OPT_Wdelete_non_virtual_dtor,
>> +                           "deleting object of abstract class type %qT"
>> +                           " which has non-virtual destructor"
>> +                           " will cause undefined behaviour", type);
>> +                 else
>> +                   warning(OPT_Wdelete_non_virtual_dtor,
>> +                           "deleting object of polymorphic class type
>> %qT"
>> +                           " which has non-virtual destructor"
>> +                           " may cause undefined behaviour", type);
>
> Space before the (.  And let's use "might" instead of "may".  OK with those
> changes.

Fixed and committed, thanks.