is delete on abstract with non-virtal ever safe?

Clang Frontend
1

I'm trying to settle an argument we are having at work. Right now our
code doesn't compile with clang because of a code like below.

I understand (and agree with) the warning, however the owner of this
code argues that only the default destructors are used, and they have
procedures in place to ensure that no derived class will ever have
members that need destruction. So the question is he correct that the
destructor for derived doesn't need to be called? If so is there a way
to shut clang up without disabling the warning (which has already found
several real problems I've since corrected). If not can someone point
out why we need a virtual destructor in this case.

    class base
    {
    public:
      virtual void operator() () const = 0;
    };

    template<typename T>
    class derived : public base
    {
    public:
       derived(T& d) : data(d) {}
       void operator() () const {data.DoSomething();}

    private:
       T& data; // note that the only data is a reference!
    };

    class myClass
    {
    public:
       void DoSomething() {}
    };

    int main(int, char **)
    {
        myClass cl;
        base* foo = new derived<myClass>(cl);
        delete foo;
    }

clang++ ex.cpp
ex.cpp:29:5: warning: delete called on 'base' that is abstract but has
      non-virtual destructor [-Wdelete-non-virtual-dtor]
    delete foo;

Note, we are comparing to gcc 4.4. I'm given to understand that gcc4.7
also has this warning, but we don't have the ability to upgrade right
now.

2

I am very glad to hear that this warning has already been useful, it’s probably my only direct contribution to Clang :slight_smile:

Regarding shutting it up… well unfortunately whether the destructors are trivial or not does not matter as far as the Standard is concerned:

[expr.delete] 3/ 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.

So even though in practice it probably won’t hurt (for now), it is forbidden by the Standard and any future optimization (of the compiler) or refactoring (of the code) could potentially trigger a bug.

=> Do you really want to live with that sword of Damocles above your head ?

So I would argue against the owner here: what is the cost, really, of declaring the destructor virtual ? (especially since there is already another virtual method anyway)

– Matthieu

3

I'm trying to settle an argument we are having at work. Right now our
code doesn't compile with clang because of a code like below.

I understand (and agree with) the warning, however the owner of this
code argues that only the default destructors are used, and they have
procedures in place to ensure that no derived class will ever have
members that need destruction. So the question is he correct that the
destructor for derived doesn't need to be called?

According to the letter of the standard, he is not correct:

5.3.5 [expr.delete] p3:

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

4

Strictly speaking, the C standard says it's undefined behavior. It's
not completely unrealistic that it could break with a really well
optimized implementation of "delete". That said, you can probably get
away with it on existing C++ implementations.

-Eli

5

Here are some things that don't work correctly if you fail to provide a virtual destructor:
- Destruction of members in derived classes. (You say that is never necessary.)
- If the derived class has other bases besides base, and base is not the very first in the object layout, or else if base is a virtual base class (or part of a virtual base class), the code will pass the wrong pointer to operator delete.
- If you overload operator new and delete for some derived class, you won't get the correct operator delete called.
- For that matter, if you overload operator new and delete in the base class, and you use the pointer+size version of operator delete, the passed size will be incorrect.

There may be more, those are just those I can think of off the top of my head.

Sebastian

6

Well, if you want to avoid deprecated behavior, once you declare a virtual destructor, even if it’s just defaulted, you also have to declare the five other special member functions to maintain the existing behavior.

Which reminds me: I don’t follow the standard proceedings at all, but why shouldn’t the implicitly declared destructor in a class with a virtual member function not be made virtual by default? The only other option that makes sense is to make it protected and nonvirtual, though that is probably less often what the class designer wants. I thought this might change in C++11, and find it makes even more sense given the new rules about special member functions.

      Steve

P.S. If it were me, I’d just suck it up and fix the base class. The maintainer here seems to be spending more time arguing than it would take to implement the correct action.

7

So I would argue against the owner here: what is the cost, really, of declaring the destructor virtual ? (especially since there is already another virtual method anyway)

Well, if you want to avoid deprecated behavior, once you declare a virtual destructor, even if it’s just defaulted, you also have to declare the five other special member functions to maintain the existing behavior.

Which reminds me: I don’t follow the standard proceedings at all, but why shouldn’t the implicitly declared destructor in a class with a virtual member function not be made virtual by default? The only other option that makes sense is to make it protected and nonvirtual, though that is probably less often what the class designer wants. I thought this might change in C++11, and find it makes even more sense given the new rules about special member functions.

I'm not sure if this answers your question, but here's a simple
example that's not uncommon/unreasonable:
{
derive d;
doBaseStuff(d);
}

The doBaseStuff calls virtual functions from a base reference (perhaps
the author doesn't want the code bloat of a template, or it's
statically compiled in another library, etc) but the object itself is
never polymorphically owned so it's not polymorphically destroyed and
thus doesn't need a virtual destructor.

Another less obvious example:

{
std::shared_ptr<base> sp = make_shared<derived>();
doStuff(sp);
}

derived's dtor doesn't need to be virtual as shared_ptr's type erasure
machinery guarantees that it'll be destroyed the same way make_shared
created it, with a derived*, not a base*.

8

Which reminds me: I don’t follow the standard proceedings at all, but why shouldn’t the implicitly declared destructor in a class with a virtual member function not be made virtual by default? The only other option that makes sense is to make it protected and nonvirtual, though that is probably less often what the class designer wants. I thought this might change in C++11, and find it makes even more sense given the new rules about special member functions.

The doBaseStuff calls virtual functions from a base reference (perhaps
the author doesn't want the code bloat of a template, or it's
statically compiled in another library, etc) but the object itself is
never polymorphically owned so it's not polymorphically destroyed and
thus doesn't need a virtual destructor.

Hence why I mentioned protected non-virtual destructors. If your destructor is public, no force on Earth will keep some ninny from calling it directly at some point, documentation and good sense be damned.

derived's dtor doesn't need to be virtual as shared_ptr's type erasure
machinery guarantees that it'll be destroyed the same way make_shared
created it, with a derived*, not a base*.

Once again, a protected nonvirtual destructor guards against this. But, once again, since it’s the less frequently desired behavior, it would make sense for the default to be a virtual public destructor for classes that have a vtable anyway, and let implementors explicitly declare it otherwise if that’s what they desire.

      Steve

9

Which reminds me: I don’t follow the standard proceedings at all, but why shouldn’t the implicitly declared destructor in a class with a virtual member function not be made virtual by default? The only other option that makes sense is to make it protected and nonvirtual, though that is probably less often what the class designer wants. I thought this might change in C++11, and find it makes even more sense given the new rules about special member functions.

The doBaseStuff calls virtual functions from a base reference (perhaps
the author doesn't want the code bloat of a template, or it's
statically compiled in another library, etc) but the object itself is
never polymorphically owned so it's not polymorphically destroyed and
thus doesn't need a virtual destructor.

Hence why I mentioned protected non-virtual destructors. If your destructor is public, no force on Earth will keep some ninny from calling it directly at some point, documentation and good sense be damned.

Right, sorry - then the issue is if you have non-abstract non-final
classes which are admittedly less common, but I'd find it surprising
if the language made choices/defaults based on that being such a
vanishingly narrow case as to be irrelevant.

Anyway, sorry for derailing the thread (I think what needed to be said
was said by the first replies: the code has UB) - this is probably a
conversation for the C++ standards body.

10

So I would argue against the owner here: what is the cost, really, of declaring the destructor virtual ? (especially since there is already another virtual method anyway)

Well, if you want to avoid deprecated behavior, once you declare a virtual destructor, even if it’s just defaulted, you also have to declare the five other special member functions to maintain the existing behavior.

Which is probably a good thing, by the way.

The default (copy|move) (constructor|assignment operator) open the gate to undiagnosed object slicing, so in the worst case they would have to be protected (+defaulted) and in the best case disabling is just a bonus.

– Matthieu

11

Hi,

Sorry to dig out an old topic, but there is one case where delete on
abstract with non-virtual is safe.
If you make a component model for plugin managment and you don't want to
enforce any compiler / compiler settings / runtime settings, you have to
delegate the delete operator to make sure deletion happen in the same module
as allocation.

Here is a small example of this: (suppose main.cpp and toto.cpp are in
different module, for instance main.exe and toto.dll).

// common.h (visible in main.cpp and toto.cpp)
struct my_destroy_interface
{
    virtual void Destroy() = 0;

    inline void operator delete (void * ptr) {
        if (ptr != NULL) static_cast<my_destroy_interface
*>(ptr)->Destroy();
    }
};

my_destroy_interface * createToto();

// toto.cpp
struct my_destroy_default_impl : public my_destroy_interface
{
protected:
    virtual ~my_destroy_default_impl()
    {
        printf("~my_destroy_default_impl()\n");
    }

    virtual void Destroy()
    {
        ::delete this;
    }
};

struct Toto : public my_destroy_default_impl
{
protected:
    ~Toto()
    {
        printf("~Toto()\n");
    }
};

my_destroy_interface * createToto()
{
    return new Toto;
}

// main.cpp
int main(int argc, char* argv)
{
    delete createToto();
  return 0;
}

Result:
~Toto()
~my_destroy_default_impl()

This is perfectly valid but gets a warning:
warning : delete called on 'my_destroy_interface' that is abstract but has
non-virtual destructor [-Wdelete-non-virtual-dtor]

If I would have put the virtual destructor in the interface definition,
"delete createToto()" would have called ~Toto(), but also "::delete this".

Usually this is not an issue because in this kind of component model, the
practice is to call ->Destroy() (more oftently called Release() ) directly.

Using the delete operator allows to combine this kind of objects with smart
pointer classes, without the smart pointer classes having any knowledge that
they handle objects from another "plugin".

An alternative to this without warning would be to:
* add an empty virtual destructor to the interface.
* call ::operator delete(this) instead of ::delete this in
my_destroy_default_impl::Destroy to trigger only memory freeing and not
destructor calling.
Both method are valid but not having to add a empty virtual destructor to
each interface is nice (plus interface are not supposed to have implemented
functions)

One could live with ignoring this warning, but I guess when the operator
delete is custom, it is safe not to emit this warning.

Best regards,

Emmanuel

12

Hi,

Sorry to dig out an old topic, but there is one case where delete on
abstract with non-virtual is safe.
If you make a component model for plugin managment and you don't want to
enforce any compiler / compiler settings / runtime settings, you have to
delegate the delete operator to make sure deletion happen in the same
module
as allocation.

Here is a small example of this: (suppose main.cpp and toto.cpp are in
different module, for instance main.exe and toto.dll).

// common.h (visible in main.cpp and toto.cpp)
struct my_destroy_interface
{
    virtual void Destroy() = 0;

    inline void operator delete (void * ptr) {
        if (ptr != NULL) static_cast<my_destroy_interface
*>(ptr)->Destroy();

This has undefined behavior. You're calling a virtual function on an object
whose lifetime has ended (the destructor has already been called).

    }

};

my_destroy_interface * createToto();

// toto.cpp
struct my_destroy_default_impl : public my_destroy_interface
{
protected:
    virtual ~my_destroy_default_impl()
    {
        printf("~my_destroy_default_impl()\n");
    }

    virtual void Destroy()
    {
        ::delete this;
    }
};

struct Toto : public my_destroy_default_impl
{
protected:
    ~Toto()
    {
        printf("~Toto()\n");
    }
};

my_destroy_interface * createToto()
{
    return new Toto;
}

// main.cpp
int main(int argc, char* argv)
{
    delete createToto();

This has undefined behavior (you're using 'delete', the static and dynamic
types don't match, and your base class does not have a virtual destructor).

        return 0;

}

Result:
~Toto()
~my_destroy_default_impl()

This is perfectly valid

Nope. See above. Or try giving my_destroy_interface a user-declared
(non-virtual) destructor, and watch as it either gets called twice or your
program starts to die due to a pure virtual function call (depending on
implementation details).

but gets a warning:

warning : delete called on 'my_destroy_interface' that is abstract but has
non-virtual destructor [-Wdelete-non-virtual-dtor]

If I would have put the virtual destructor in the interface definition,
"delete createToto()" would have called ~Toto(), but also "::delete this".

Usually this is not an issue because in this kind of component model, the
practice is to call ->Destroy() (more oftently called Release() ) directly.

Using the delete operator allows to combine this kind of objects with smart
pointer classes, without the smart pointer classes having any knowledge
that
they handle objects from another "plugin".

An alternative to this without warning would be to:
* add an empty virtual destructor to the interface.
* call ::operator delete(this) instead of ::delete this in
my_destroy_default_impl::Destroy to trigger only memory freeing and not
destructor calling.
Both method are valid but not having to add a empty virtual destructor to
each interface is nice (plus interface are not supposed to have implemented
functions)

One could live with ignoring this warning, but I guess when the operator
delete is custom, it is safe not to emit this warning.

No, the warning is always correct.

13

This has undefined behavior. You're calling a virtual function on an object

whose lifetime has ended (the destructor has already been called).

my_destroy_interface has no destructor so the vtable is not cleared until
the memory is released (since nothing alters it before i get in the operator
delete overload).

Referring to the standards (i'm not sure its the right pdf):
"The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor
call starts, or
— the storage which the object occupies is reused or released."

And that make totally sense since if i add a (non-virtual) destructor it
still flies, unless the destructor is non trivial (in that case i get as
expected a pure virtual function call).

This has undefined behavior (you're using 'delete', the static and dynamic
types don't match, and your base class does not have a virtual
destructor).

That's the point, i call the overload "my_destroy_interface::operator
delete" and not the global delete operator. Since my_destroy_interface has
no (non trivial) destructor this is the only thing happening when calling
delete on a pointer to my_destroy_interface.

Nope. See above. Or try giving my_destroy_interface a user-declared
(non-virtual) destructor, and watch as it either gets called twice or your
program starts to die due to a pure virtual function call (depending on
implementation details).

Giving to my_destructor_interface a virtual destructor would make it called
twice as you said, thats why it's not there in this construct (and it makes
sense since its an interface).

14

Hello,

This has undefined behavior. You're calling a virtual function on an object

whose lifetime has ended (the destructor has already been called).

my_destroy_interface has no destructor so the vtable is not cleared until

That is an implementation detail, though (just as are vtables).

the memory is released (since nothing alters it before i get in the operator
delete overload).

Referring to the standards (i'm not sure its the right pdf):
"The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor
call starts, or
— the storage which the object occupies is reused or released."
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf

And that make totally sense since if i add a (non-virtual) destructor it
still flies, unless the destructor is non trivial (in that case i get as
expected a pure virtual function call).

But the standard talks about *trivial* destructors, not *virtual* ones.

From §12.4p5:

A destructor is trivial if it is not user-provided and if:

So the moment you define a destructor you have a non-trivial one.

Unless I'm missing something.

Jonathan

15

Jonathan Sauer wrote

But the standard talks about *trivial* destructors, not *virtual* ones.
From §12.4p5:

> A destructor is trivial if it is not user-provided and if:

So the moment you define a destructor you have a non-trivial one.

In my example my_destroy_interface has no destructor.
So when delete (my_destroy_interface*) is called:
* no (non-trivial) destructor is called
* operator delete is called
* in the body of operator delete, the object is still valid according to the
rule, until the memory is released.

16

This has undefined behavior. You’re calling a virtual function on an object
whose lifetime has ended (the destructor has already been called).

my_destroy_interface has no destructor so the vtable is not cleared until
the memory is released (since nothing alters it before i get in the operator
delete overload).

Referring to the standards (i’m not sure its the right pdf):
“The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor
call starts, or
— the storage which the object occupies is reused or released.”
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2005/n1905.pdf

And that make totally sense since if i add a (non-virtual) destructor it
still flies, unless the destructor is non trivial (in that case i get as
expected a pure virtual function call).

This is sort of beside the point, since you had undefined behaviour earlier:

This has undefined behavior (you’re using ‘delete’, the static and dynamic
types don’t match, and your base class does not have a virtual
destructor).

That’s the point, i call the overload “my_destroy_interface::operator
delete” and not the global delete operator. Since my_destroy_interface has
no (non trivial) destructor this is the only thing happening when calling
delete on a pointer to my_destroy_interface.

You can’t reason like that about undefined behaviour. You deleted an object thorough the wrong static type and the dtor is not virtual, so you’ve already lost.

17

I think I found a quote in the n3485 C++ Working Draft that explains a tad
more, in 3.8 [basic.life]:

5/ Before the lifetime of an object has started but after the storage which
the object will occupy has been
allocated or, after the lifetime of an object has ended and before the
storage which the object occupied is
reused or released, any pointer that refers to the storage location where
the object will be or was located
may be used but only in limited ways. [...] The program has undefined
behavior if:

[...]

— the pointer is used to access a non-static data member or call a
non-static member function of the
object, or

[...]

— the pointer is used as the operand of a static_cast (5.2.9), except when
the conversion is to pointer
to cv void, or to pointer to cv void and subsequently to pointer to either
cv char or cv unsigned
char, or

[...]

So unfortunately the scheme you are proposing is violating the Standard at
least twice, because as far as the Standard goes your object is already
dead (whether its destructor had any effect or not does not matter).
Specifically, I could see one of `-fsanitize=undefined` or
`-fsanitize=memory` (for example) overwriting the memory with 0xdeadbeef
between the call to the destructor and the call to your specific operator
delete.

I am sorry I had not remembered this point in our prior discussion; I had
forgotten that operator new and operator delete are only supposed to deal
with raw memory and not objects.

-- Matthieu

18

You can’t reason like that about undefined behaviour. You deleted an object thorough the wrong static type and the dtor is not virtual, so you’ve already lost.

I didn’t delete the object I called a specific delete operator on its pointer. There is a big difference.

19

/ Before the lifetime of an object has started but after the storage which the object will occupy has been
allocated or, after the lifetime of an object has ended and before the storage which the object occupied is
reused or released, any pointer that refers to the storage location where the object will be or was located
may be used but only in limited ways. […] The program has undefined behavior if:

But the lifetime of the object has not ended.

It would only end when either a non-trivial destructor is called either when the memory is released.

This doesn’t mean by chance there is no destructor so nothing bad happens, it mean if there is no destructor so, according to the standard, the compiler should consider the object alive until the memory is released.

I’m sorry but the statement is clear enough:

“The lifetime of an object of type T ends when:
— if T is a class type with a non-trivial destructor (12.4), the destructor call starts, or
— the storage which the object occupies is reused or released.”

Envoyé : samedi 5 octobre 2013 14:22

20

This doesn’t mean by chance there is no destructor so nothing bad happens,
it mean if there is no destructor so, according to the standard, the
compiler should consider the object alive until the memory is released.

There are multiple sub-objects under discussion here and only one of
them has a trivial destructor.

The standard appears to have stopped specifying behaviour at 5.3.5
("thou shalt have a virtual destructor") so the entire discussion is
moot. But I think it's difficult to argue that, had it skipped that
bit and gone on to define behaviour for your case, either the
my_destroy_default_impl or Toto objects would still be alive.

Tim.