ansaurus

Question

Valgrind reports "Invalid free() / delete / delete[]" (C++)

Answer 1

+3 A:

I'm guessing that Protocol1 et al. are subclasses of some SuperProtocol?

devices_text_[*it] goes through assuming that it contains something of type SuperProtocol, so delete devices_text_[*it] calls SuperProtocol::~ SuperProtocol().

However, what you really want to call is Protocol1::~Protocol1() if you're destructing a Protocol1; this only works if you mark SuperProtocol::~ SuperProtocol() as virtual.

ephemient 2009-10-29 22:16:16

Bingo that got it! I had two base classes with non-virtual destructors. I didn't know they had to be virtual.

Scott 2009-10-29 22:25:09

See http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7 about when destructors must be virtual. In short: almost always. It's one of C++'s (many) pitfalls.

ephemient 2009-10-29 22:29:42

Indeed, when you say `delete p`, it's going to run the destructor of what `p` points to. If that isn't virtual, then the compiler has no other choice but run the base destructors. By making it virtual, the compiler can find it's way to the correct destructor (which in turn calls the base destructor after running).

GMan 2009-10-29 22:30:56

I wouldn't say almost always, just when you plan on deriving from it.

GMan 2009-10-29 22:32:03

Ok, GMan, you say "which in turn calls the base destructor." Does that mean the derived constructor should explicitly call the base destructor? Or does it cascade automatically?

Scott 2009-10-29 22:35:20

It's automatic. For an object to be constructed, it does: "Base class's constructor, construct all members objects, enter the constructor body." Keep in mind this is recursive, so any base classes will have their bases constructed first, and so on. C++ says that objects are destructed in the reverse order they were constructed. (Try it in a scope, the last variable created is destructed first). So destruction is he reverse of construction: "Enter the destructor body, destruct member objects, run base destructor"

GMan 2009-10-29 22:38:40

Answer 2

+1 A:

Instead of going through the keys, finding it in the map, then deleting it, why not just iterate through the map deleting as you go? I'd make a functor and use for_each (this isn't a guideline or anything, just my opinion),

typedef Generic<LCDText> GenericLCDText;
typedef std::map<std::string, GenericLCDText*> GenericLCDTextMap;
typedef GenericLCDTextMap::value_type GenericLCDTextPair;

struct device_text_deleter : std::unary_function<const GenericLCDTextPair&, void>
{
    void operator()(const GenericLCDTextPair& pPair)
    {
        Error("Deleting %s %p", pPair.first.c_str(), pPair.second);

        delete pPair.second;        
    }
}

std::for_each(devices_text_.begin(), devices_text_.end(), device_text_deleter());
_devices.text_.clear(); // optional, removes the deleted pointers. unnecessary
                        // if this is run in the destructor, since map goes away
                        // shortly after

That said, you're code would be improved by the following:

// typedef's for readability (would be in header, maybe private)
typedef std::vector<std::string> StringVector;
typedef Generic<LCDText> GenericLCDText;
typedef std::map<std::string, GenericLCDText*> GenericLCDTextMap;

for(StringVector::iterator it = display_keys_.begin();
    it != display_keys_.end(); it++)
{
    // find first! otherwise you're looking up the pair every time
    GenericLCDTextMap::iterator pair = devices_text_.find(*it);

    if (p != devices_text_.end())
    {
        // operator-> is overloaded for iterators but might as well use
        // the pair now.
        Error("Deleting %s %p", pair->first.c_str(), pair->second);

        // no need to check for null, delete null is a-ok
        delete pair->second;
    }
}

Hopefully this will make it easier to spot the errors. Make sure any base classes you use have virtual destructors.

Check you haven't added a string in the vector twice (this would be "fixed" buy just iterating through the map, though you'll want to find out why duplicates exist in the first place), etc.

I've never tried this before, but maybe add a double delete macro thing (totally untested):

#define DOUBLE_DELETE_GAURD static bool x = false; assert(x == false); x = true;

Then just add it to your destructor. If you double delete, and the static bool is still around, the assertion will fail. This is completely in undefined la-la land, though.

GMan 2009-10-29 22:18:41

Answer 3

A:

Could it be that

display_keys_ contains the same string more than once and
this string has an associated Generic <LCDText>* in devices_text_?

In this case the pointer would be given to delete twice and this isn't legal...

hjhill 2009-10-29 22:19:40

ansaurus

tags:

views:

answers:

Valgrind reports "Invalid free() / delete / delete[]" (C++)

related questions