Thread: Valgrind error between two items in initialiser list

I am trying to solve valgrind errors in a code, but I am having some trouble… Perhaps someone can give me some hints ?

I have a "Mother" class holding a whole lot of attributes "A, B, C, D, ..." that are all references to objects of the same type (i.e. "SomeClass &")

Code:

class Mother
{
    //
public:
    //
    Mother(foo, bar);
    //
private:
    //
    SomeClass & A;
    SomeClass & B;
    SomeClass & C;
    SomeClass & D;
    etc.
};

Each attribute is constructed within the initialisation list of the Mother() constructor :

Code:

Mother::Mother(foo, bar)
    : A (*new SomeClass(foo, bar, "alpha"))
    , B (*new SomeClass(foo, bar, "beta" ))
    , C (*new SomeClass(foo, bar, "gamma"))
    , D (*new SomeClass(foo, bar, "zeta" ))
    , etc.

The valgrind error occurs after around 50 such references to objects have been initialised and 4 still need to be initialised. The error is of the form :

Code:

==8347== Invalid write of size 8
==8347==    at 0x42776B: Mother::Mother([...]) (mother.cpp:[...])
==8347==    by [...]
==8347==  Address 0x9081fe8 is 0 bytes after a block of size 440 alloc'd
==8347==    at 0x4C2B0E0: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8347==    by [...]

I've tried to run gdb along with valgrind, but gdb gets lost in the code : it thinks it is at some place that is inconsistent with what "std::cout << [...]" instructions print on the screen.

As an alternative, I've added "std::cout << [...]" instructions in the code to locate where the valgrind message occurs. Unfortunately, it occurs after the "std::cout [...]" instruction located at the last line of the SomeClass() constructor and before the first "std::cout [...]" instruction located within the first initialiser list item of the next SomeClass object.

Code:

static dummy([...])
{
    std::cout << "first" << std::endl;
    return [...]
}

SomeClass::SomeClass(foo, bar)
    : A ( dummy( *new SomeClass(foo, bar) ) )
    , B (*new SomeClass(foo, bar))
    , C (*new SomeClass(foo, bar))
    , D (*new SomeClass(foo, bar))
    , etc.
{
    [...]
    std::cout << "last" << std::endl;
}

Swapping the order of two attributes does not change the error.
I.e. if the valgrind error occurs after constructing C and before constructing D then the following code

Code:

Mother::Mother(foo, bar)
    : A (*new SomeClass(foo, bar))
    , B (*new SomeClass(foo, bar))
    , D (*new SomeClass(foo, bar))
    , C (*new SomeClass(foo, bar))
    , etc.

Will show an error after constructing D and before constructing C.

From your experience, would you have any idea/hint regarding what could cause the Valgrind error ? What I should try to do to find the root cause ? Is there is a better way to write my code ?

Which version of GCC?
Which version of Valgrind?

Do you see the problem when you initialise via assignment within the constructor itself, and not via initialiser lists?

It would also help if you could post some actual code we could test with. Code full of ... snipping doesn't help us guess at all the possible things you could have done wrong.

Originally Posted by Galdor

Is there is a better way to write my code ?

Frankly, this looks bad to me:

Code:

class Mother
{
    //
public:
    //
    Mother(foo, bar);
    //
private:
    //
    SomeClass & A;
    SomeClass & B;
    SomeClass & C;
    SomeClass & D;
    etc.
};

Mother::Mother(foo, bar)
    : A (*new SomeClass(foo, bar, "alpha"))
    , B (*new SomeClass(foo, bar, "beta" ))
    , C (*new SomeClass(foo, bar, "gamma"))
    , D (*new SomeClass(foo, bar, "zeta" ))
    , etc.

What if an exception is propagated from say, new SomeClass(foo, bar, "gamma")? It seems to me that you will then have a memory leak as you have no way of invoking delete on what has already been created. I presume that you have some good reason why Mother does not store SomeClass objects instead of references, yet I wonder if it will be better to take a smart pointer approach:

Code:

class Mother
{
    //
public:
    //
    Mother(foo, bar);
    //
private:
    //
    std::unique_ptr<SomeClass> A;
    std::unique_ptr<SomeClass> B;
    std::unique_ptr<SomeClass> C;
    std::unique_ptr<SomeClass> D;
    etc.
};

Mother::Mother(foo, bar)
{
    A.reset(new SomeClass(foo, bar, "alpha"));
    B.reset(new SomeClass(foo, bar, "beta"));
    C.reset(new SomeClass(foo, bar, "gamma"));
    D.reset(new SomeClass(foo, bar, "zeta"));
}

> but unfortunately it feels like the problem is modified any time I make the slightest change…
Well to be honest, if you've got that much fragile code, then chances are the problem is really somewhere else and this is just where the problem eventually manifests itself.

A simplified example would at least prove to yourself that there is nothing actually wrong with the current method (from a memory management point of view at least).

Are threads involved by any chance?

> and since the problem does not occur at the initialisation of the first fifty "SomeClass" objects,
Don't tell me you have 50+ named variables, instead of a vector.

As an alternative, you could try running in the debugger when linked against Electric Fence - eLinux.org

Valgrind also gives you the option of dropping into gdb when a problem is detected.

Originally Posted by Salem

Well to be honest, if you've got that much fragile code, then chances are the problem is really somewhere else and this is just where the problem eventually manifests itself.

I think you are right, but I feel a bit powerless to find the root cause of the issue… I tried several approaches to fix the problem, but I am running out of options… Did you face similar issues in the past ? How did you manage to fix them ? Do you have tricks and/or strategies to do so ?

Originally Posted by Salem

Are threads involved by any chance?

Thank God, no… Otherwise that would be even more of a nightmare… I am having enough of a hard time …

Originally Posted by Salem

Don't tell me you have 50+ named variables, instead of a vector.

I'm afraid I do have a lot of variables… But much less than 50 (I didn't count properly in my first post…). The "SomeClass" objects are wrappers around arrays that represent a physical quantity (temperature, mass density, velocity, etc.) or that are used for debug as a way to save a state of computation. In the "Mother" class there are 15 arrays for physical quantities, followed by 12 arrays for debug purposes, and the code crashed between the 9th and 10th debug object. Each of these arrays represent something different, so a vector won't do. And a map would make the synthax more difficult to read IMHO… So yes I have a lot of arrays to initialise… Would you use container objects anyway ?

Originally Posted by Salem

As an alternative, you could try running in the debugger when linked against Electric Fence - eLinux.org

What's the difference between electric fence and valgrind ?

Originally Posted by Salem

Valgrind also gives you the option of dropping into gdb when a problem is detected.

Yes, that was what I was doing : running valgrind with -vgdb=full option (that's what you meant, right ?)

Originally Posted by Galdor

So the best solution for you would be to have objects as attributes ?

It is the simplest solution, and if you don't need a more complex solution, then the simplest solution is best.

Originally Posted by Galdor

Unless the objects take too much memory in the stack in which case you would use smart pointers instead, as you described ?

Yes. Other reasons include the need for inheritance-based polymorphism, i.e., you store a (smart) pointer to a base class object, which could in fact point to a derived class object. Yet another reason could be that the member is actually optional, in which case you would store a null pointer. If you need to have some kind of container, then generally a separate container object, e.g., a std::vector, would be better.

Originally Posted by Galdor

That's true ! Up until now I have not really cared about that because I was not managing exceptions, but using assertions instead. I know this is bad in a production environment, but in my case I am working on a in-house simulation software to which only myself and a few colleagues have access. And if something goes wrong, we prefer to have the software crash anyway…

Even if you are not throwing your own exceptions, the use of new means that std::bad_alloc could be thrown.

Originally Posted by Galdor

I'm afraid I do have a lot of variables… But much less than 50 (I didn't count properly in my first post…). The "SomeClass" objects are wrappers around arrays that represent a physical quantity (temperature, mass density, velocity, etc.) or that are used for debug as a way to save a state of computation. In the "Mother" class there are 15 arrays for physical quantities, followed by 12 arrays for debug purposes, and the code crashed between the 9th and 10th debug object. Each of these arrays represent something different, so a vector won't do. And a map would make the synthax more difficult to read IMHO… So yes I have a lot of arrays to initialise… Would you use container objects anyway ?

Generally, a class should model one thing and model it well. This means that typically, a well designed class only has a few member variables. With 15 member variables, probably the class is representing too much at once, so you can move some groups of these into another class that models that group of member variables.

You are violating the One Definition Rule (ODR), which is probably why you're seeing weird things. You're in the undefined behaviour land. All source files (translation units) MUST see the exact same class definitions, but due to your macros, they don't.
In essence: don't use macros unless you absolutely have to. If you do, avoid them in headers. If you have to use them in headers, don't use them to change definitions depending on macro conditions.

Originally Posted by Galdor

Few more questions

Now, before I mark this topic as solved here is a few questions I still have :

What should I change in my coding habits for such mistakes not to happen ever again ?

My personal conclusion is : macros are evil, only use them when they are absolutely necessary (in my case they could have been avoided). Judging from my limited experience, it seems to me that in C++ they are only useful when passing flags during compilation (for instance using gcc's -D option)

Do you agree with that ? Is there something else you would recommend ?

Yep that's pretty much right. Macros are a tool of last resort in c++. As you say, they can be used for compile time flags. There are other uses too, but It's always preferable to use a function of constant variable when possible, because those are aware of the C++ type system.