Memory trashing bug

[reubert]

Hi

Edit: Just realised I didn't mention the language, its C++

I've just spent about 2 hours trying to figure out why random values were showing up in seemingly innocent code.

When going through the code in gdb I found that a call to Vector(0.0f) was ending up in a completely unrelated part of code and returning bits of memory from somewhere else.

I had no idea where to look to find the cause of such a thing, but think I may have found a clue when using MallocDebug. There was an option there to reveal Over/underruns. This told me that the following line of code was overrunning on new:
ParticleEmitterType* particleEmitterType = new ParticleEmitterType(configFile, ConfigKey());

I'm not actually sure if "overrunning on new" makes sense but it seemed to me that MallocDebug was saying that the amount of memory allocated by new was not enough to hold what was being created in the constructor... or something.

Anyway, on a hunch I changed that line of code to the following:
ConfigKey key = ConfigKey();
ParticleEmitterType* particleEmitterType = new ParticleEmitterType(configFile, key);

And the problem appears to have gone away.

For reference, ConfigKey() is the constructor for a class, and that constructor is empty.

So, I am very confused by this. Can anyone see why I should need to do that? Has that even fixed the problem, or simply rearranged my code so that the real memory trasher lies dormant?

[Fenris]

You've just put it to sleep. My recommendation (as always) is to turn on GuardMalloc from the Debug menu in XCode, hit Cmd-Y and go grab a cup of coffee. It'll find the real culprit line in minutes. :)

[DoG]

Can you post the prototypes of the constructurs in question?

[akb825]

You may still have a memory error, but from how the mapping is different by declaring a ConfigKey before the constructor call could hide it. (for example, I've had a few instances where I'd put in a print statement and a memory error would continue executing rather than crash)

BTW, you don't need to use = Constructor() for a stack-based object. You could do this instead:
ConfigKey key;
for the default constructor, or
ConfigKey key(args)
for another constructor.

[DoG]

(for example, I've had a few instances where I'd put in a print statement and a memory error would continue executing rather than crash)

That's typical for stack corruption. Could be a compiler bug, could even be something largely unrelated causing the error, and random changes either hide it or bring it out.

[akb825]

I call it the Heisenberg Law of Computing. :p (you know that there's an error, but you can't tell what it is through print statements ;)) Fortunately, each time I had the problem, I was still able to figure it out.

[reubert]

Thanks Guys, You have confirmed my worst fears.

I have searched high and low for stupid things like returning arrays allocated on the stack, deleting anything and then using it, going over the bounds of an array, and cannot find anything. I have also removed all but 2 memory leaks. One is in my modified SDL, so can't be bothered fixing it, and the other I believe is caused by the trasher.

GuardMalloc doesn't break anywhere. :(

The prototypes are:
ConfigKey()
{
}

and

ParticleEmitterType(MJConfigFile *configFile, ConfigKey key);

[akb825]

Just out of curiosity, are you expecting the copy of ConfigKey to be the same in your ParticleEmitterType object to be the same? Assuming you have an object of type ConfigKey in your object as opposed to a pointer to an object, you're going to have 3 copies: one when you call the constructor, a copy of that inside the constructor, and another copy stored in the object. If you have a pointer to a ConfigKey object inside your class that's pointing to the object that you passed in, it's not going to be valid: it will have been destroyed as the constructor exits. If you want to have the same copy of the object as you passed in, you have 2 objects: pointers and reference. For a pointer, you will need to pass in the address of the object, and then store the object as a pointer. If you want to use references to make it seem as if it was simply an object being stored, you would have to have to structure it like this:
//.h file
class ParticleEmitterType
{
//stuff
ConfigKey &configKey
//stuff
public:
ParticleEmitterType(MJConfigFile *configFile, ConfigKey &key);
//stuff
};

//.cpp file
//stuff
ParticleEmitterType::ParticleEmitterType(MJConfigF ile *configFile, ConfigKey &key)
: configKey(key)
{
//initialization stuff
}

//stuff
If you have other constructors you might want to use but also keep your own copy of the object, you would want to have a reference in the constructor for ParticleEmitterType, then use a copy constructor to initialize the object stored in the class. (using the guaranteed initialization method that I showed with initializing the reference; also, make sure the copy constructor takes a reference) Also, I'm assuming that configKey has some stuff in it, otherwise you wouldn't be passing in a copy. (instead you'd create one in the constructor) So in that case, you aren't assuming that everything is 0 initialized, are you? (it's not) BTW, if you just want a blank constructor that takes nothing, you don't have to bother making one: it's implicit.

[reubert]

I did it this way because I don't have to worry about accidentally modifying the ConfigKey in the constructor, or about managing the memory myself.

The ConfigKey just has a std::vector to a bunch of structs, so unless I misunderstand std::vectors, there is no need to initialize it and a copy of an empty object is just fine.

On the rare occasion that I want to keep a key around I just copy it in the constructor, so no worries there.

So I am quite happy with the way this stuff works currently, but thanks for the thoughts anyway.

Still have no idea how to track down this trasher... I'm scanning 20,000 lines of code for something I'm not even sure I'll recognise when I see it. :(