is delete on abstract with non-virtal ever safe?

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

is delete on abstract with non-virtal ever safe?

Henry Miller

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.

--
  Henry Miller
  [hidden email]
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Matthieu Monrocq


On Mon, Jul 23, 2012 at 4:29 PM, Henry Miller <[hidden email]> wrote:

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.

--
  Henry Miller
  [hidden email]


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

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

David Blaikie
In reply to this post by Henry Miller
On Mon, Jul 23, 2012 at 7:29 AM, Henry Miller <[hidden email]> wrote:
>
> 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."

> 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.
>
> --
>   Henry Miller
>   [hidden email]
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Eli Friedman
In reply to this post by Henry Miller
On Mon, Jul 23, 2012 at 7:29 AM, Henry Miller <[hidden email]> wrote:
>
> 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?

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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Sebastian Redl
In reply to this post by Henry Miller

On 23.07.2012, at 16:29, Henry Miller wrote:

>
> 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?

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
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Steve Ramsey
In reply to this post by Matthieu Monrocq
On Jul 23, 2012, at 9:52 AM, Matthieu Monrocq wrote:
> 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.

                        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.


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

David Blaikie
On Mon, Jul 23, 2012 at 11:55 AM, Steve Ramsey <[hidden email]> wrote:
> On Jul 23, 2012, at 9:52 AM, Matthieu Monrocq wrote:
>> 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*.

>
>                         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.
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Steve Ramsey
On Jul 23, 2012, at 12:40 PM, David Blaikie wrote:
>> 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


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

David Blaikie
On Mon, Jul 23, 2012 at 1:48 PM, Steve Ramsey <[hidden email]> wrote:

> On Jul 23, 2012, at 12:40 PM, David Blaikie wrote:
>>> 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.

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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Matthieu Monrocq
In reply to this post by Steve Ramsey


On Mon, Jul 23, 2012 at 8:55 PM, Steve Ramsey <[hidden email]> wrote:
On Jul 23, 2012, at 9:52 AM, Matthieu Monrocq wrote:
> 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

 
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.



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

emmanuel.attia
In reply to this post by Henry Miller
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
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Richard Smith
On Thu, Oct 3, 2013 at 2:43 AM, emmanuel.attia <[hidden email]> wrote:
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.
 
Best regards,

Emmanuel



--
View this message in context: http://clang-developers.42468.n3.nabble.com/is-delete-on-abstract-with-non-virtal-ever-safe-tp4025653p4034847.html
Sent from the Clang Developers mailing list archive at Nabble.com.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

emmanuel.attia
> 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 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).
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Jonathan Sauer
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


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

emmanuel.attia
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.
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Richard Smith
In reply to this post by emmanuel.attia


On 4 Oct 2013 02:49, "emmanuel.attia" <[hidden email]> wrote:
>
> > 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.

> > 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).
>
>
>
>
> --
> View this message in context: http://clang-developers.42468.n3.nabble.com/is-delete-on-abstract-with-non-virtal-ever-safe-tp4025653p4034880.html
> Sent from the Clang Developers mailing list archive at Nabble.com.
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Matthieu Monrocq



On Fri, Oct 4, 2013 at 7:15 PM, Richard Smith <[hidden email]> wrote:


On 4 Oct 2013 02:49, "emmanuel.attia" <[hidden email]> wrote:
>
> > 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.

> > 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).
>
>
>
>
> --
> View this message in context: http://clang-developers.42468.n3.nabble.com/is-delete-on-abstract-with-non-virtal-ever-safe-tp4025653p4034880.html
> Sent from the Clang Developers mailing list archive at Nabble.com.
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

emmanuel.attia
In reply to this post by Richard Smith

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



The information contained in this message may be confidential and legally protected under applicable law. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, forwarding, dissemination, or reproduction of this message is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender by return e-mail and destroy all copies of the original message.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

emmanuel.attia
In reply to this post by Matthieu Monrocq

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

 

De : Matthieu Monrocq [mailto:[hidden email]]
Envoyé : samedi 5 octobre 2013 14:22
À : Richard Smith
Cc : ATTIA, Emmanuel; [hidden email] Developers
Objet : Re: [cfe-dev] is delete on abstract with non-virtal ever safe?

 

 

 

On Fri, Oct 4, 2013 at 7:15 PM, Richard Smith <[hidden email]> wrote:


On 4 Oct 2013 02:49, "emmanuel.attia" <[hidden email]> wrote:
>
> > 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.

> > 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).
>
>
>
>
> --
> View this message in context: http://clang-developers.42468.n3.nabble.com/is-delete-on-abstract-with-non-virtal-ever-safe-tp4025653p4034880.html
> Sent from the Clang Developers mailing list archive at Nabble.com.
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

 

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



The information contained in this message may be confidential and legally protected under applicable law. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, forwarding, dissemination, or reproduction of this message is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender by return e-mail and destroy all copies of the original message.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: is delete on abstract with non-virtal ever safe?

Tim Northover-2
> 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.

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
12