Bug 7302

Fix:
	If a class has only pure virtual members besides the destructor and
	has no friends, and the destructor is protected or private, then the
	warning shouldn't be shown.

State-Changed-From-To: open->analyzed
State-Changed-Why: Confirmed.  Low priority.

Closing as a dup of bug 15214 because that one has a start on the patch but it also describes another 
situation.

*** This bug has been marked as a duplicate of 15214 ***

This is indeed not a duplicate of PR 15214 though the two reports certainly 
go into the same direction. Let's keep it open then. 
 
W. 

A pure virtual member function can still be defined (and must be, if it is the
destructor).

*** Bug 20570 has been marked as a duplicate of this bug. ***

Created attachment 11068 [details]
proposed patch

patch, proposed at http://bugs.debian.org/356316

Created attachment 11520 [details]
proposed patch (with doc and test changes)

I'd like to point out that structures containing only pure virtual functions should not trigger this diagnostic either. Consider the following:

struct IfacFoo
{
   virtual int a() = 0;
   virtual int b() = 0;
};

There is no destructor declared nor do any members exist to necessitate the implicit creation of one. As of version "4.1.1 20060525 (Red Hat 4.1.1-1)", the diagnostic message is still emitted in both this case and those with a protected "dummy" destructor included.

Lawrence: Every class has a destructor. You're talking about classes that have trivial destructors. Whether a non-virtual destructor is trivial or not has no bearing on the fact that if an instance of a derived class is destroyed through a pointer or reference to a base class with a non-virtual destructor, the result is undefined behaviour.

(In reply to comment #8)
> Created an attachment (id=11520) [edit]
> proposed patch (with doc and test changes)
> 

Thanks for the patch. However, formal submissions are sent to gcc-patches@gcc.gnu.org. An attachment in bugzilla is not considered a formal submission and it may go unnoticed. Please, consider submitting to gcc-patches@gcc.gnu.org (and perhaps also adding it to the patch queue: http://www.dberlin.org/patchdirections.html )

Already posted as <http://gcc.gnu.org/ml/gcc-patches/2006-04/msg00885.html>, with no response. 

(In reply to comment #12)
> Already posted as <http://gcc.gnu.org/ml/gcc-patches/2006-04/msg00885.html>,
> with no response. 
> 

You need to insist. A week is normally considered an acceptable interval between pings. You need to bootstrap + regression test. http://gcc.gnu.org/contribute.html#testing

Short version:
make && make -k check 

Then, compare with the output without your patch.

What I normally do is, for an unpatched tree:
make && (make -k check &> pristine)

then patch and do the same saving the output in another file. Compare the two outputs (there is a compare tests script in the contrib/ directory).

Of course, this is a very dirty and rough explanation. It is better to read the docs.

(In reply to comment #12)
> Already posted as <http://gcc.gnu.org/ml/gcc-patches/2006-04/msg00885.html>,
> with no response. 

this patch doesn't cover one situation:

struct D;
struct C {
        virtual void f() = 0;
protected: // or private
        friend class D;
        ~C();
};

in such case compiler should generate warning, because class D
can delete derived object through the pointer to class C.

Pawel: Yes, any friend class or function can call a protected or private destructor wrongly.  So can members of the class - in fact, even pure virtual members can, since they may still have definitions!

The current implementation warns whenever any function might be able to call a non-virtual destructor in a polymorphic class; this results in many false positives.  False warnings result in programmers disabling the warning, or paying less attention to warnings, or (in this case) introducing a virtual destructor for no good reason.  That is why I consider the behaviour a bug.

quite better ( modulo coding style ) patch is:

--- class.c.orig        2006-10-12 22:02:53.000000000 +0200
+++ class.c     2007-02-22 02:54:11.888652367 +0100
@@ -5105,15 +5105,15 @@
       tree dtor;

       dtor = CLASSTYPE_DESTRUCTORS (t);
-      /* Warn only if the dtor is non-private or the class has
-        friends.  */
       if (/* An implicitly declared destructor is always public.  And,
             if it were virtual, we would have created it by now.  */
          !dtor
          || (!DECL_VINDEX (dtor)
-             && (!TREE_PRIVATE (dtor)
-                 || CLASSTYPE_FRIEND_CLASSES (t)
-                 || DECL_FRIENDLIST (TYPE_MAIN_DECL (t)))))
+                 && (/* public non-virtual */
+                     (!TREE_PRIVATE (dtor) && !TREE_PROTECTED (dtor))
+                     || (/* or non-public non-virtual with friends */
+                         (TREE_PRIVATE (dtor) || TREE_PROTECTED (dtor))
+                          && (CLASSTYPE_FRIEND_CLASSES (t) || DECL_FRIENDLIST (TYPE_MAIN_DECL (t)))))))
        warning (0, "%q#T has virtual functions but non-virtual destructor",
                 t);
     }

it correctly warns on B, C and D classes:

struct A {
        virtual void f() = 0;
protected:
        ~A(); // ok.
};

struct B {
        virtual void f() = 0;
        ~B(); // warn! public non-virtual dtor.
};

struct C {
        virtual void f() = 0;
        // warn! implicit public non-virtual dtor.
};

struct D {
        virtual void f() = 0;
private:
        friend class C;
        ~D(); // warn! can be called from class C.
};

Created attachment 13214 [details]
extended patch against gcc-4.2

(In reply to comment #17)
> Created an attachment (id=13214) [edit]
> extended patch against gcc-4.2
> 

Hi Pawel,

if the bug exists in mainline, the patch should be against mainline (I think this patch will apply to mainline anyway). The patch needs testcases, and it should be sent to gcc-patches@gcc.gnu.org.

Also,

+	warning (0, "%q#T has virtual functions and accessible"
+		 " non-virtual destructor", t)
should be:
+	warning (OPT_Wnon_virtual_dtor, 
+                "%q#T has virtual functions and accessible"
+		 " non-virtual destructor", t)

Also
+			|| DECL_FRIENDLIST( TYPE_MAIN_DECL (t)))))))
should be
+			|| DECL_FRIENDLIST (TYPE_MAIN_DECL (t)))))))

Thanks for the patch.

(In reply to comment #18)

> The patch needs testcases,

i have a testcase but my tcl/autogen/dejagnu crashes
with magic `spawn failed' message :/

e.g.:

(...)
Executing on host: /home/users/pluto/rpm/BUILD/gcc-4.2-20070307/builddir/gcc/xgcc
-B/home/users/pluto/rpm/BUILD/gcc-4.2-20070307/builddir/gcc/
-O0  -w -fno-show-column -c  -o 20000105-1.o
/home/users/pluto/rpm/BUILD/gcc-4.2-20070307/gcc/testsuite/gcc.c-torture/compile/20000105-1.c
   (timeout = 300)

compiler exited with status -1
output is:
spawn failed
FAIL: gcc.c-torture/compile/20000105-1.c  -O0  (test for excess errors)
Excess errors:
spawn failed

(In reply to comment #19)
> (In reply to comment #18)
> 
> > The patch needs testcases,
> 
> i have a testcase but my tcl/autogen/dejagnu crashes
> with magic `spawn failed' message :/
> 

No idea. Ask in the gcc list, perhaps someone could help you. I would just remove completely and reinstall DejaGNU. Or just use the GCC Compile Farm for testing. I have a script there that you just say

PATCH=/path/to/patch ./gccfarming bootstrap

And it bootstraps and regression tests the patch and sends you the results by email. It also has a "patchqueue" mode where filenames of patches are read from a file and bootstrapped+tested in sequence, and you can add more names at the end of the file on the fly.

http://gcc.gnu.org/ml/gcc-patches/2007-03/msg01343.html

(In reply to comment #21)
> http://gcc.gnu.org/ml/gcc-patches/2007-03/msg01343.html
> 

Hint, if you use the patch queue[1], it takes care of adding a comment pointing to the patch. Also, your patch lacks a Changelog [2]. See also an example [3].

[1] http://www.dberlin.org/patchdirections.html
[2] http://gcc.gnu.org/codingconventions.html
[3] http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00008.html

Subject: Bug number PR7302

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-03/msg01347.html

Subject: Bug 7302

Author: jason
Date: Mon Aug 20 15:08:24 2007
New Revision: 127649

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=127649
Log:
        PR c++/7302
        * cp/class.c (finish_struct_1): Warn when a class has virtual
        functions and accessible non-virtual destructor.

        * doc/invoke.texi (-Wnon-virtual-dtor): Update documentation.

        * g++.dg/warn/Wnvdtor-2.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Wnvdtor-2.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/class.c
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog

The testcase seems to be missing one case where it should warn:

class H
{
protected:
	~H();
public:
	virtual void deleteme() = 0;
};

H::~H()
{
}

void
H::deleteme()
{
	delete this;
}

class I : public H
{
protected:
	~I();
public:
	virtual void deleteme() { H::deleteme(); }
	void oops();
};

I::~I()
{
}

void
I::oops()
{
	deleteme();
}

Here, H must have a virtual destructor. The point where it can know it should warn is the "delete this;" line.

(can this bug be un-ASSIGNED?)

(In reply to comment #25)
> Here, H must have a virtual destructor. The point where it can know it should
> warn is the "delete this;" line.

I've posted a patch to http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00191.html which gives this for the code in comment 25

7302.cc:16:12: warning: deleting object of abstract class type ‘H’ which has non-virtual destructor will cause undefined behaviour [-Wdelete-non-virtual-dtor]

Assigned to you Jonathan!

Feel free to close it when your patch goes in. If there is something else left to do, it is always better to open a new PR.

(In reply to comment #26)

> I've posted a patch to http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00191.html
> which gives this for the code in comment 25

BTW, I would suggest that you use warning_at, but I guess this is a lost battle.

For G++ 4.7 I've added -Wdelete-non-virtual-dtor, included in -Wall, to handle the case in comment 25, so closing this as fixed now