C++ destructor not being called, depending on the linking order

Question

I ran across this issue in my application after checking it for memory leaks, and discovered that some of my classes are not being destroyed at all.

The code below is split into 3 files, it is supposed to implement a pattern called pimpl. The expected scenario is to have both Cimpl constructor and destructor print their messages. However, that's not what I get with g++. In my application, only constructor got called.

classes.h:

#include <memory>

class Cimpl;

class Cpimpl {
    std::auto_ptr<Cimpl> impl;
public:
    Cpimpl();
};

classes.cpp:

#include "classes.h"
#include <stdio.h>

class Cimpl {
public:
    Cimpl() {
        printf("Cimpl::Cimpl()\n");
    }
    ~Cimpl() {
        printf("Cimpl::~Cimpl()\n");
    }
};    

Cpimpl::Cpimpl() {
    this->impl.reset(new Cimpl);
}

main.cpp:

#include "classes.h"

int main() {
    Cpimpl c;
    return 0;
}

Here is what I was able to discover further:

g++ -Wall -c main.cpp
g++ -Wall -c classes.cpp
g++ -Wall main.o classes.o -o app_bug
g++ -Wall classes.o main.o -o app_ok

It looks like the destructor is being called in one of two possible cases, and it depends on the linking order. With app_ok I was able to get the correct scenario, while app_bug behaved exactly like my application.

Is there any bit of wisdom I am missing in this situation? Thanks for any suggestion in advance!

Answer 1

The goal of the pimpl idiom is to not have to expose a definition of the implementation class in the header file. But all the standard smart pointers require a definition of their template parameter to be visible at the point of declaration in order to work correctly.

That means this is one of the rare occasions where you actually want to use new, delete, and a bare pointer. (If I'm wrong about this and there's a standard smart pointer that can be used for pimpl, someone please let me know.)

classes.h

struct Cimpl;

struct Cpimpl
{
    Cpimpl();
    ~Cpimpl();

    // other public methods here

private:
    Cimpl *ptr;

    // Cpimpl must be uncopyable or else make these copy the Cimpl
    Cpimpl(const Cpimpl&);
    Cpimpl& operator=(const Cpimpl&);
};

classes.cpp

#include <stdio.h>

struct Cimpl
{
    Cimpl()
    {
        puts("Cimpl::Cimpl()");
    }
    ~Cimpl()
    {
        puts("Cimpl::~Cimpl()");
    }

    // etc
};

Cpimpl::Cpimpl() : ptr(new Cimpl) {}
Cpimpl::~Cpimpl() { delete ptr; }

// etc

Answer 2

The problem is that at the point of the definition of the auto_ptr<Cimpl> object, Cimpl is an incomplete type, that is, the compiler has only seen a forward declaration of Cimpl. That's okay, but since it eventually deletes the object that it holds a pointer to, you have to comply with this requirement, from [expr.delete]/5:

If the object being deleted has incomplete class type at the point of deletion and the complete class has a non-trivial destructor or a deallocation function, the behavior is undefined.

So this code runs into undefined behavior, and all bets are off.

Answer 3

The code violates the One Definition Rule. There's a definition of the class Cimpl in classes.h, and a different definition of the class Cimpl in the file classes.cpp. The result is undefined behavior. It's okay to have more than one definition of a class, but they must be the same.

Answer 4

Edited for clarity, original retained below.

This code has undefined behavior because in the context of main.cpp the implicit Cpimpl::~Cpimpl destructor only has a forward declaration of Cimpl, but the auto_ptr (or any other form of doing delete) needs a full definition to legally clean up the Cimpl. Given that it's undefined behavior no further explanation for your observations is needed.

Original answer:

I suspect that what's happening here is that the implicit destructor of Cpimpl is being generated in the context of classes.h and not having access to the full definition of Cimpl. Then when the auto_ptr tries to do its thing and clean up its contained pointer, it deletes an incomplete class, which is undefined behavior. Given that it's undefined we don't have to go any further to explain that it's perfectly acceptable for it to work in different ways depending on the link order.

I suspect that an explicit destructor for Cpimpl with a definition in a source file would solve your problem.

EDIT: Actually now that I look at it again, I believe your program violates the one definition rule as it stands. In main.cpp it sees an implicit destructor that doesn't know how to call a destructor of Cimpl (because it only has a forward declaration). In classes.cpp the implicit destructor does have access to Cimpl's definition and thus how to call its destructor.