ansaurus

Question

c++ destructor mess, impossible to debug...

Answer 1

+2 A:

First step is to compile your program with -g 3 switch so that you get much more debugging info.

There's not a lot to go on, but there is a possibility that your ~MSG() is getting called when the vector reallocates to grow.

Here's the most simple version of your code:

#include <vector>

struct MSG {
  MSG(int count);
  ~MSG();
  void destroy();
  char* payload;
  int payloadLen;
};

MSG::MSG(int count): payload(new char[count]), payloadLen(count) {}

MSG::~MSG() {
    destroy();
}

void MSG::destroy() {
    if (payload != NULL)
        delete[] payload;
    payload = NULL;
    payloadLen = 0;
}

std::vector<MSG> queueto1;

int main() {
    queueto1.push_back(MSG(10));
    return 0;
}

And G++ says:

block freed twice

Exited: ExitFailure 127

Now when the push_back(MSG(10)) is being called, it is creating a temporary MSG instance and then using the default copy constructor to set the item in the vector, before calling ~MSG() on the temporary (which deletes the payload that the item in the vector now points at).

You can implement the copy constructor MSG(const MSG& copy) to transfer ownership, but that requires a const_cast on the copy and is very very messy.

Two easy options are available: MSG::payload is a shared pointer, or queueto1 is a std::vector<MSG*>

Will 2009-12-04 17:54:24

unfortunately, it's done upon destruction of the vector.

gotch4 2009-12-04 17:56:43

exactly, the backtrace you posted is just missing debug information! also remove -Ox (-O1 -O2 or whatever was used) option. you used gdb, right?

frunsi 2009-12-04 18:01:04

no optimization and debugging level is maximum...

gotch4 2009-12-04 18:09:00

payload is initialized at NULL by the DEFAULT CONSTRUCTOR. If I compile your example G++ doesn't give any error. Anyway I think that upon copy, the default constructor is called, then payload whould be NULL and no problem should arise I think.

gotch4 2009-12-04 18:13:37

Default constructor isn't called on copy; a copy constructor is called on copy. If you did not write a copy constructor for your class, then compiler will automatically provide one (which will do a memberwise copy) for you. Ditto for `operator=`.

Pavel Minaev 2009-12-04 18:15:18

really, follow the link on the phrase "And G++ says" to see what G++ says!

Will 2009-12-04 18:15:39

Answer 2

+1 A:

to trace memory allocation and corruption problems you can use valgrind or Rational Purify

catwalk 2009-12-04 17:54:53

I tried valgrind... says a bunch of stuff about STL templates and doesn't get me point out the problem...

gotch4 2009-12-04 17:56:05

Answer 3

A:

Printf the "payLoad" pointer and a counter as the deletes happen. You can then see a double free happening and which deletes it is ...

Goz 2009-12-04 17:57:08

Answer 4

+12 A:

Judging by your Valgrind output, the problem is that an object pointed to by a shared_ptr is being deleted twice. One possibility of getting that is if you initialize two shared_ptr with the same raw pointer, e.g.:

int* p = new int(123);
shared_ptr<int> sp1(p);
shared_ptr<int> sp2(p);

shared_ptr is not magical, and it cannot know if the object you ask it to take care of is already owned by some other unrelated shared_ptr, if all you give it is a raw pointer. In the example above, each shared_ptr will create its own reference counter, initialized to 1; when they die, each will decrement its own counter, see that it is 0, and delete the object, producing double deletion. I suspect your case is similar. If you show the code used to initialize shared_ptr objects that are added to the vector, this can be verified.

[EDIT] Since this is validated to be the cause of the crash now, let me elaborate a bit on how to properly use shared_ptr.

First of all, the nature of the problem. The way shared_ptr is written, it works with any C++ type, and provides reference counting semantics. The obvious problem is that most types do not provide any space to store the reference counter (e.g. consider shared_ptr<int> - there's no extra space "inside" int). To work around this, for every shared object, a separate block of memory is allocated that contains the reference counter. This is done whenever you create a shared_ptr from a raw pointer. The shared_ptr object itself then stores the original raw pointer, and the pointer to reference counter (which is why it's more "fat" than a raw pointer, which can be trivially checked with sizeof). When you create one shared_ptr from another (using copy constructor or assignment operator), it copies the pointer to reference counter, so all shared_ptr instances created from one another maintain a single counter, and guarantee correct deletion. But if you have two unrelated "families" of shared_ptr objects to the same objects (where two or more pointers were created from the same raw pointer), those "families" do not know about each other, and will refcount separately, and each will delete when it hits 0.

What this means in practice is that, when using shared_ptr, one must adhere to certain rules. Those depend on which implementation you use.

With std::tr1::shared_ptr, or older Boost versions, the only fully safe pattern for object allocation is this:

shared_ptr<T> x(new T(...));

In other words, the result of new should be immediately put into a shared_ptr - you can then copy the latter as much as you want.

A reasonably safe pattern is also this:

auto_ptr<T> x(new T);
...
shared_ptr<T> y(x);

shared_ptr implements the usual transfer-of-ownership when initializing from auto_ptr, and the semantics of the latter (so long as they're correctly followed) ensure that only one auto_ptr to an object should exist; thus, it is safe to construct a shared_ptr from that.

Sometimes you also have to deal with C++ libraries which do not use auto_ptr to indicate transfer of pointer ownership, but simply document the intent for specific functions. In those cases it should also be safe to use shared_ptr, but of course you should be sure that you've understood the documentation correctly...

In C++0x std::shared_ptr, and in newer versions of boost::shared_ptr, there's a helper provided to ensure correct instantiation of shared objects:

shared_ptr<int> p = make_shared<int>(123);

The return type of make_shared<T>() is already shared_ptr<T>, so at no point you're dealing with raw pointers in your code, reducing chance to get something wrong.

Pavel Minaev 2009-12-04 18:10:32

S**T, you hit the problem man... It's the first time I used shared_ptr sorry :D I was passing the same raw pointer to many shared_ptr... this was because I replaced regular pointers whit shared ones and I forgot to change a couple of things!!!! Thanks!

gotch4 2009-12-04 18:17:04

Answer 5

A:

According to Valgrind the crash is taking place in the destruction of your CONSTCHP2MSLST instance. The problem likely resides in your messageListener implementation. Could you post the relevant contstructors and copy constructors for that class?

fbrereto 2009-12-04 18:16:21

Answer 6

A:

A side issue: you said you didn't define destructors, but rather used STL containers and shared pointers. What the containers and shared pointers do for you is call your destructors for you. They don't replace the destructor; they replace the need to figure out when to use the destructor.

This means that your classes got the default destructor, which consists of calling whatever destructors there are on base classes and members. If your classes don't actually own any resources except through smart pointers and the like, AND you're not using polymorphism, this can work. It does need more care than "I'm using STL constructs and smart pointers, so I don't need destructors".

If you have class A and class B: public A, and you ever have a pointer to A pointing to a B (raw pointer, smart pointer, it doesn't matter), you need a virtual destructor. Otherwise, when the pointer to A is deleted, it will destroy all subclasses and members of A, and ignore anything added in B. Therefore, you should have a virtual destructor in any base class that will be used in this way, and for safety's sake you should have a virtual destructor in any base class with any virtual function. Even if you don't have anything to get rid of in the destructor, you should have virtual A::~A() {} in your definition of A.

David Thornley 2009-12-04 18:56:56

ansaurus

tags:

views:

answers:

c++ destructor mess, impossible to debug...

related questions