7
\$\begingroup\$

Basic idea is to use a Class, with static methods to add and remove references in a static vector, that keeps track of these references, and check that vector upon exit.

The class is detecting intentional leaks that I create, but maybe you can find a case where it does not detect the leak, that I am not seeing

#include <iostream>
#include <stdexcept>
#include <memory>
#include <vector>
#include <cstdlib> // for REM_LEAKt
#include <algorithm> // for std::remove


void checkLeakStack();
class LeakDbg
{
private:
    LeakDbg()
    {
        /// Constructor will only be called once,
        /// since it's a singlton class
        std::atexit( checkLeakStack );
    }

public:
    struct Pair
    {
        std::string name;
        void* ref;
        bool operator==( const Pair &other ) const { return ref == other.ref; }

    };
    static bool locked;
    static std::vector<Pair> stack;
    static LeakDbg& instance()
    {
        static LeakDbg INSTANCE;
        return INSTANCE;
    }

    static void addRef(const std::string& nm, void* ptr)
    {
        stack.push_back(Pair{ nm, ptr });
    }
    static void remRef(void* ptr)
    {
        /// If it's not enabled, it means
        /// it's OK to remove a ref
        if( !LeakDbg::locked ){
            Pair search = Pair{"",ptr};
            std::vector<Pair> vect;
//            std::remove(vect.begin(), vect.end(), search);
            stack.erase(std::remove(stack.begin(), stack.end(), search), stack.end());
        }
    }

};


bool LeakDbg::locked = false;
std::vector<LeakDbg::Pair> LeakDbg::stack = std::vector<LeakDbg::Pair>();

void checkLeakStack()
{
    /// Here the stack should be emoty
    /// you can print or assert if the stack is not empty
    std::cout << "There are " << LeakDbg::stack.size() << " leaks ..." "\n";
    for ( LeakDbg::Pair pair : LeakDbg::stack) {
        const std::string msg = pair.name + " is leaked";
        std::cout << msg << std::endl;
    }
}

Add defines

#define ADD_LEAK(msg, ptr) LeakDbg::addRef(msg, ptr);
#define REM_LEAK(ptr) LeakDbg::remRef(ptr);
#define CREATE_LEAK_DET() LeakDbg::instance();
#define LCK_LEAK_DET(st) LeakDbg::locked = st;

// ADD_LEAK         -> Add it in a class constructor
// REM_LEAK         -> Add it in a class destructor
// CREATE_LEAK_DET  -> Call it once to make sure "std::atexit" is called
// LCK_LEAK_DET     -> If set to "true", all objects destructed after,
//                  -> will be considered a leak

Test

struct Test {
    Test() {
        ADD_LEAK( "Cls",  this )
    }
    ~Test() {
        REM_LEAK( this )
    }

};


int main() {
    CREATE_LEAK_DET()
    Test *obj1 = new Test();
    Test *obj2 = new Test();
    Test *obj3 = new Test();
    delete obj2;
    LCK_LEAK_DET( true )
}

Update 12/12/2019 If anybody is interested, I refactored the code to be reusable and less intrusive. Github

\$\endgroup\$
  • 2
    \$\begingroup\$ The code is working, I just forgot the headers, fixed now. \$\endgroup\$ – User Dec 8 '19 at 13:26
  • 3
    \$\begingroup\$ Better! I think it's ready for review now. Thank you. \$\endgroup\$ – Edward Dec 8 '19 at 13:27
10
\$\begingroup\$

Here are some things that may help you improve your code.

Use the required #includes

The code uses std::string which means that it should #include <string>. It might compile on your machine because some other header includes that file, but you can't count on that, and it could change with the next compiler update.

Use only necessary #includes

The #include <stdexcept> and #include <memory> lines are not necessary and can be safely removed because nothing from those headers appears to be used here.

Avoid C-style macros

I'd advise not using C-style macros like the ones in this code, preferring either inline functions or even lambdas. See ES.31 for details.

Consider thread safety

If multiple threads are using this code, there is likely to be a problem because the single shared instance of the std::vector is not protected by a mutex. I would also recommend renaming the existing locked variable to something like complete or finished to better distinguish what it's doing.

Avoid singletons

A singleton is basically just another way to create global variables, and we don't like global variables much because they make code linkages much harder to see and understand. See I.3 for more on that. In this case, since you already have two global variables, much of the complexity can easily be avoided by simply using a namespace instead of a class. Here's one way to do that which eliminates the need for instance and CREATE_LEAK_DET:

namespace LeakDbg
{
    struct Pair
    {
        std::string name;
        void* ref;
        bool operator==( const Pair &other ) const { return ref == other.ref; }

    };
    static bool locked = false;
    static std::vector<Pair> stack;

    static void addRef(const std::string& nm, void* ptr)
    {
        stack.emplace_back(Pair{ nm, ptr });
    }
    static void remRef(void* ptr)
    {
        if( !LeakDbg::locked ){
            stack.erase(std::remove(stack.begin(), stack.end(), Pair{"",ptr}), stack.end());
        }
    }
    void checkLeakStack()
    {
        std::cout << "There are " << LeakDbg::stack.size() << " leaks ..." "\n";
        for ( LeakDbg::Pair pair : LeakDbg::stack) {
            std::cout << pair.name << " is leaked\n";
        }
    }
    static const bool registered{std::atexit( checkLeakStack ) == 0};
}

Consider the user

The current code requires that the user explicitly instruments the code, which seems a bit intrusive. Here's an alternative approach the modifies things just slightly, using the Curiously Recurring Template Pattern, or CRTP for short. First we isolate the leak detector bits into a templated class.

template <typename T>
struct LeakDetector {
    LeakDetector() {
        LeakDbg::addRef(typeid(T).name(), this);
    }
    ~LeakDetector() {
        LeakDbg::remRef(this);
    }
};

Now to use it is much simpler than before. No ugly macros are required and we only need to add one simple thing to the declaration of the class to be monitored:

struct Test : public LeakDetector<Test> 
{
    Test() {
    }
    ~Test() {
    }
};

An even less intrusive approach might be to override new and delete as outlined in this question.

Consider alternatives

Leak detection is a worthwhile thing to do, since many C++ bugs stem from that kind of error. However, there are already a number of existing approaches to this, some of which may already be installed on your computer. There is, for example the useful valgrind tool. If you're using clang or gcc and have the libasan library installed, you can get a very nice runtime printout. Just compile the code with

g++ -g -fsanitize=address myprogram.cpp -o myprogram

Then at runtime, a memory leak report might look like this:

There are 2 leaks ...
Cls is leaked
Cls is leaked

=================================================================
==71254==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fe67c2c69d7 in operator new(unsigned long) (/lib64/libasan.so.5+0x10f9d7)
    #1 0x4057a6 in main /home/edward/test/memleak/src/main.cpp:97
    #2 0x7fe67bcbb1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fe67c2c69d7 in operator new(unsigned long) (/lib64/libasan.so.5+0x10f9d7)
    #1 0x405774 in main /home/edward/test/memleak/src/main.cpp:95
    #2 0x7fe67bcbb1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

SUMMARY: AddressSanitizer: 2 byte(s) leaked in 2 allocation(s).
\$\endgroup\$
  • \$\begingroup\$ Thanks for the input, I am developing a GUI, and valgrind makes the interaction really slow, time to change my PC I guess. And the reason I used macros, is to be able to separate debug from release. I guess I will go with inheritance since it's less intrusive \$\endgroup\$ – User Dec 8 '19 at 17:36
  • 1
    \$\begingroup\$ While valgrind does indeed slow things down, the address sanitizer has a lot less runtime impact in my experience. You might try that if you're using a compiler that supports it. \$\endgroup\$ – Edward Dec 8 '19 at 17:39
  • \$\begingroup\$ I will give google/sanitizers a try \$\endgroup\$ – User Dec 8 '19 at 18:17
  • 2
    \$\begingroup\$ Although they can be expensive to record, don't underestimate the value of those callstacks: knowing not just that two "operator new(unsigned long)" leaked somewhere in the entire execution but precisely which objects they are is often the difference between an exhaustive slog through the code and swooping directly onto the root cause. \$\endgroup\$ – Tom Goodfellow Dec 8 '19 at 23:00
  • \$\begingroup\$ inline is no use for functions nowadays. Just write normal functions and let compiler do optimizations. Also, templates are somewhat close to macros too. \$\endgroup\$ – val says Reinstate Monica Dec 9 '19 at 10:01
2
\$\begingroup\$

Seems to be broken:

int main()
{
    CREATE_LEAK_DET()
    Test    obj4;
    Test    obj5(obj4);
    LCK_LEAK_DET( true )
}

Now compile and run:

> g++ -std=c++14 ty.cpp
> ./a.out
There are 1 leaks ...
Cls is leaked
> 
\$\endgroup\$
  • \$\begingroup\$ Yep, you can't forget copy constructors. If I also push the ref to the copy constructor it solves it. But are there other cases the code misses. \$\endgroup\$ – User Dec 8 '19 at 18:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.