4

started moving some libraries from msvc to mingw, and found really interesting behavior of msvc when one wants to delete an array of upcasted objects. Namely msvc does some dark magic (it seems to love doing that) and the bellow code executes just fine, however in mingw (4.7.2( crashes. I believe that mingw is performing correctly and its the msvc voodoo thats the issue for making a sleeper bug.

Code:

#include <iostream>

class foo{
    static int idgen;
protected:
    int id;
public:
    foo(){
        id = idgen++;
        std::cout << "Hello  ( foo - "<<id<<")"<<std::endl;
    }
    virtual ~foo(){
        std::cout << "Bye bye ( foo - "<<id<<")"<<std::endl;
    };
};

int foo::idgen = 0;


class bar: public foo{
    double some_data[20];
public:
    bar(){

    std::cout << "Hello  ( bar - "<<id<<")"<<std::endl;
}
    ~bar(){
        std::cout << "Bye bye ( bar - "<<id<<")"<<std::endl;
    }
};

int main()
{
    const unsigned int size = 2;
    foo** arr = new foo*[size];
    {
        bar* tmp = new bar[size];
        for(int i=0; i<size; i++)
        {
            arr[i] = &(tmp[i]); //take address of each object
        }
    }

    delete [] arr[0]; //take address of first object, pointer is same as tmp. This also crashes mingw
    delete [] arr;

}

Output from msvc 2010

Hello  ( foo - 0)
Hello  ( bar - 0)
Hello  ( foo - 1)
Hello  ( bar - 1)
Bye bye ( bar - 1)
Bye bye ( foo - 1)
Bye bye ( bar - 0)
Bye bye ( foo - 0)

And mingw (crashed at the destruction)

Hello  ( foo - 0)
Hello  ( bar - 0)
Hello  ( foo - 1)
Hello  ( bar - 1) 

My question is, what is the correct approach to fix this. The current hackfix that I came up with involved just trying to downcast to every possible class and invoking the delete operation on the downcasted pointer:

if(dynamic_cast<bar*>(arr[0]) != 0)
    delete [] dynamic_cast<bar*>(arr[0]); 

Is there a better approach, besides redesigning the library (it isn't mine)?

Honf
  • 215
  • 1
  • 6
  • You need a virtual destructor on the base class –  May 15 '13 at 21:53
  • @RonDahlgren He has one (see `foo` dtor). – Steve May 15 '13 at 21:55
  • Whoops. I should probably take this blindfold off :-/ –  May 15 '13 at 22:00
  • 1
    I tried to figure out why this even works on MSVS, and it looks like you have a curious hack going on here: `&(tmp[0])` returns the same value as `tmp`, so then you delete the whole `tmp` array at once when you call `delete[] arr[0]`. – Sergey Kalinichenko May 15 '13 at 22:07
  • 1
    @dasblinkenlight Now you get it ;) It's really bizarre this works in msvc, but thats where the library was initially developed, causing this to be a really deeply rooted problem. – Honf May 15 '13 at 22:11
  • @Honf Try replacing the `arr[i] = &(tmp[i]);` with `arr[i] = (!i) ? tmp : &(tmp[i]);` and see if it changes anything (although I doubt it)... – Sergey Kalinichenko May 15 '13 at 22:15
  • 1
    @dasblinkenlight `arr[i] = &(tmp[i]);` is the same as `arr[i] = (!i) ? tmp : &(tmp[i]);`, since it takes the same addresses. The issue I suspect is in the metadata `new[]` and `delete[]` use to do their job, since that isn't defined in the standard, leading to this edgecase which is implementation sensitive. The big question for me is, which behavior is correct or is deleting an upcasted array of objects an undefined operation? – Honf May 15 '13 at 22:20
  • gcc 4.7.2 fails even with a simple example here -> http://ideone.com/z876QX#view_edit_box So we can't apparently make use of virtual destructors if it is an array. – Etherealone May 15 '13 at 23:25

2 Answers2

4

In the standard specifications, Section 5.3.5 paragraph 3, regarding the delete operator:

[...] In the second alternative (delete array) if the dynamic type of the object to be deleted differs from its static type, the behavior is undefined.

So indeed, you should not rely on the gentle behaviour of Visual C++ in this case, and try to provide the array delete operator a pointer of the correct type, which basically means dynamic casting in your situation.

You may avoid that problem by using a vector instance to store your upcasted objects allocated one by one.

didierc
  • 14,572
  • 3
  • 32
  • 52
1

gcc 4.7.2 fails even with a simple example here -> ideone.com/z876QX#view_edit_box So we can't apparently make use of virtual destructors if it is an array.

const unsigned int size = 2;
foo* test = new bar[size];
delete[] test;

However, you get away if you use an array of pointers that will allow you to use delete instead of delete[].

http://ideone.com/NfbF3n#view_edit_box

const int size = 5;
foo *test[size];
for(int i = 0; i < size; ++i)
    test[i] = new bar;
for(int i = 0; i < size; ++i)
   delete test[i];
Etherealone
  • 3,488
  • 2
  • 37
  • 56