Jump to content

  • Log In with Google      Sign In   
  • Create Account


Odd memory problems with classes


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
45 replies to this topic

#21 laztrezort   Members   -  Reputation: 954

Like
0Likes
Like

Posted 09 September 2011 - 07:29 PM

I took my program to my professor and he recommended getting Visual Studio 2010 Professional.


Wha???

Sponsor:

#22 Trienco   Crossbones+   -  Reputation: 2049

Like
0Likes
Like

Posted 10 September 2011 - 12:05 AM


I took my program to my professor and he recommended getting Visual Studio 2010 Professional.


Wha???


Pretty much my reaction. The only way this recommendation would make any sense might be better ways of debugging memory issues with the Professional version.

Also keep in mind that even professors teaching computer science can be worthless as programmers (pointing out that computer science isn't programming). I could probably point at a few that are so deep into the theoretical side that they haven't touched any programming language for decades.
f@dzhttp://festini.device-zero.de

#23 swilkewitz   Members   -  Reputation: 125

Like
0Likes
Like

Posted 10 September 2011 - 05:53 PM



I took my program to my professor and he recommended getting Visual Studio 2010 Professional.


Wha???


Pretty much my reaction. The only way this recommendation would make any sense might be better ways of debugging memory issues with the Professional version.

Also keep in mind that even professors teaching computer science can be worthless as programmers (pointing out that computer science isn't programming). I could probably point at a few that are so deep into the theoretical side that they haven't touched any programming language for decades.


I don't understand your reactions. He teaches C and C++, and he solved my problem. His suggestion was spot on.

#24 ApochPiQ   Moderators   -  Reputation: 14251

Like
2Likes
Like

Posted 10 September 2011 - 06:48 PM

No, his suggestion was ridiculous.


Think of it this way. You have major problems with recurring migraines. You go to the doctor. He suggests you shoot yourself in the head with a shotgun.

Hey, wow! You don't have migraines anymore!


Was his suggestion "spot on" or ludicrous?

#25 phantom   Moderators   -  Reputation: 6785

Like
2Likes
Like

Posted 10 September 2011 - 06:51 PM

I don't understand your reactions. He teaches C and C++, and he solved my problem. His suggestion was spot on.


No, he didn't solve anything.

This is like noticing you have a crack in a wall, you go to a builder and they put wallpaper over it. Sure you can't see the crack any more but that's not going to help you when, in the future, your house falls down.

#26 laztrezort   Members   -  Reputation: 954

Like
1Likes
Like

Posted 10 September 2011 - 09:11 PM




I took my program to my professor and he recommended getting Visual Studio 2010 Professional.


Wha???


Pretty much my reaction. The only way this recommendation would make any sense might be better ways of debugging memory issues with the Professional version.

Also keep in mind that even professors teaching computer science can be worthless as programmers (pointing out that computer science isn't programming). I could probably point at a few that are so deep into the theoretical side that they haven't touched any programming language for decades.


I don't understand your reactions. He teaches C and C++, and he solved my problem. His suggestion was spot on.


From the previous information you've provided in this thread, your issue seems to be the dreaded "undefined behavior" that makes C++ programmers cringe (unpleasant memories of long, exasperating debugging sessions). These bugs are often caused by memory issues, as has been pointed out. Also, as has been pointed out, the particular version of Visual Studio almost most certainly has nothing to do with it. The fact that the problem seems "fixed" is because it is undefined behavior - your program may still fail, under seemingly random conditions, and it may fail in seemingly random ways. Because of this, these types of bugs are dangerous, since they may not get caught until its too late, and understanding them is presumably extremely important for a programmer to know.

The fact that a C++ professor would gloss over this very important detail, and instead jump to the most-likely wrong "solution", is what prompted my reply.

Of course, it is entirely possible that I do not know the whole story.

#27 Trienco   Crossbones+   -  Reputation: 2049

Like
0Likes
Like

Posted 11 September 2011 - 06:46 AM

I don't understand your reactions. He teaches C and C++, and he solved my problem. His suggestion was spot on.


He solved your problem in the same way some guy fixed his invalid array access by randomly increasing its size by 100. Not only is the "solution" complete nonsensical bs, it is also just sweeping it under the carpet so you can stumble over it even better a few weeks from now. Your bug is most certainly not "gone", it just doesn't have any visible effect for the time being, which can change at any time you touch your code or just compile it with different options.

Sorry, but unless you didn't explain your problem well enough to that professor I have to say that person shouldn't be teaching C++ and shouldn't even be programming in C++ (or probably any language at all). But maybe that's me being frustrated about my predecessor at work having "solved" problems in much the same way. Making arbitrary and often far reaching changes without ever finding or understanding the bug and claiming it "fixed" just because the program didn't crash anymore.
f@dzhttp://festini.device-zero.de

#28 Antheus   Members   -  Reputation: 2393

Like
2Likes
Like

Posted 11 September 2011 - 08:10 AM

Sorry, but unless you didn't explain your problem well enough to that professor I have to say that person shouldn't be teaching C++ and shouldn't even be programming in C++ (or probably any language at all).

Right...

I once helped someone on forum compile a CUDA app. Putting together the linker settings and all that. And then teaching them about false sharing and structuring OMP code.

A week later I got thanked, turns out he was a professor, teaching 3xx parallel programming class at CS university.

I'm aware that this is beneath academia. Typing some command line commands and dealing with blue-collar things like compilers and actually running applications. It's what grad students or technicians should do. It's not even new: Noli turbare circulos meos.


It's a well known problem. New courses are introduced yearly and existing staff must take them over. They might know the theoretical foundation, but so would anyone who reads the book. But there simply aren't enough qualified teachers. Originally, universities would house people who worked on cutting edge research. You might have people like Einstein teaching you relativity.

But it's no longer possible, so today you have blind leading the blind. Add vendor interests in the mix, the broken funding model and general apathy towards learning and you get degree mills.

#29 alnite   Crossbones+   -  Reputation: 2038

Like
0Likes
Like

Posted 11 September 2011 - 11:19 AM

@swilkewitz

Can you post your texture and model loading code? (along with the class that holds the texture/model data)

#30 swilkewitz   Members   -  Reputation: 125

Like
0Likes
Like

Posted 12 September 2011 - 02:57 PM

I see. So you guys don't trust college professors and his suggestion just hid the problem.

As of now I don't use raw pointers anywhere, only shared pointers and sometimes references. When I load my resources I ask my filemanager to load and return them and then I add them to my objects from my game start method.

bool Game::start()
{

	//Game Tools
	timer = Timer();
	fileManager = FileManager();
	controller = Controller();

	//World
	space.start(fileManager.loadTexture("star.bmp"));

	//Camera
	userCamera = Camera();

	//Ship
	GLuint shipTexture = fileManager.loadTexture("ship1.bmp");
	Model shipModel = fileManager.loadModel("ship1.model");
	Model thrusterModel = fileManager.loadModel("thruster.model");
	

	userShip = shared_ptr<Actor>(new Ship());
	//userShip->addThrusters(fileManager.loadThrusters(thrusterModel, "ship1.config"));
	userShip->addTexture(shipTexture);
	userShip->addModel(shipModel);
	userShip->setScale();
	space.addActor(userShip);

	//Control Target
	controlTarget = userShip;
	userCamera.setTarget(userShip);

	//Add Planet
	Body planet = Body(10.0f, -5.0f, 30.0f, fileManager.loadTexture("planet1.bmp"));
	space.addBody(planet);

	//Get Asteroid Assets
	GLuint asteroidTexture = fileManager.loadTexture("asteroid.bmp");
	Model asteroidModel = fileManager.loadModel("asteroid.model");
	
	//Add Asteroids
	for(int i = 0; i < 75; ++i){
		shared_ptr<Actor> tempA = shared_ptr<Actor>(new Asteroid());
		tempA->addTexture(asteroidTexture);
		tempA->addModel(asteroidModel);
		tempA->setScale();
		space.addActor(tempA);
	}

	return true;
}

I've made a lot of changes in this method before and after asking my professor for help.

#31 L. Spiro   Crossbones+   -  Reputation: 12212

Like
0Likes
Like

Posted 12 September 2011 - 05:27 PM

I have taught C++ and game programming at a University before, and I agree that upgrading the compiler only hides the problem. It may link to a different version of the Microsoft Visual Studio runtime libraries which consume a different amount of memory on loading which just happens to leave the heap in just the right state where overflowing your buffer does not lead into a no-access chunk. Etc.


Go back to the old compiler so that the bug returns. It is the only way you will know you have fixed it.

Do you still have the bug?
Did you go back to the old compiler?


L. Spiro
It is amazing how often people try to be unique, and yet they are always trying to make others be like them. - L. Spiro 2011
I spent most of my life learning the courage it takes to go out and get what I want. Now that I have it, I am not sure exactly what it is that I want. - L. Spiro 2013
I went to my local Subway once to find some guy yelling at the staff. When someone finally came to take my order and asked, “May I help you?”, I replied, “Yeah, I’ll have one asshole to go.”
L. Spiro Engine: http://lspiroengine.com
L. Spiro Engine Forums: http://lspiroengine.com/forums

#32 Bregma   Crossbones+   -  Reputation: 4748

Like
1Likes
Like

Posted 12 September 2011 - 06:15 PM

I see. So you guys don't trust college professors and his suggestion just hid the problem.

Yes, that's exactly what they're saying. They do not trust the wisdom of someone who suggests blaming the tools when the problem is the in crafting. It is poor advice, and you will not improve your crafting skills following it.

As of now I don't use raw pointers anywhere, only shared pointers and sometimes references. When I load my resources I ask my filemanager to load and return them and then I add them to my objects from my game start method.

bool Game::start()
{

	//Game Tools
	timer = Timer();
	fileManager = FileManager();
	controller = Controller();

	//World
	space.start(fileManager.loadTexture("star.bmp"));

	//Camera
	userCamera = Camera();

	//Ship
	GLuint shipTexture = fileManager.loadTexture("ship1.bmp");
	Model shipModel = fileManager.loadModel("ship1.model");
	Model thrusterModel = fileManager.loadModel("thruster.model");
	

	userShip = shared_ptr<Actor>(new Ship());
	//userShip->addThrusters(fileManager.loadThrusters(thrusterModel, "ship1.config"));
	userShip->addTexture(shipTexture);
	userShip->addModel(shipModel);
	userShip->setScale();
	space.addActor(userShip);

	//Control Target
	controlTarget = userShip;
	userCamera.setTarget(userShip);

	//Add Planet
	Body planet = Body(10.0f, -5.0f, 30.0f, fileManager.loadTexture("planet1.bmp"));
	space.addBody(planet);

	//Get Asteroid Assets
	GLuint asteroidTexture = fileManager.loadTexture("asteroid.bmp");
	Model asteroidModel = fileManager.loadModel("asteroid.model");
	
	//Add Asteroids
	for(int i = 0; i < 75; ++i){
		shared_ptr<Actor> tempA = shared_ptr<Actor>(new Asteroid());
		tempA->addTexture(asteroidTexture);
		tempA->addModel(asteroidModel);
		tempA->setScale();
		space.addActor(tempA);
	}

	return true;
}

Is this C++? I sense some pretty noisome code smells.

First, it appears (although you have not posted complete code) that you may have default-construct many of your objects and then overwrote them with new default-constructed objects. It kind of looks like you're thinking in Java or something. This may indicate a misunderstanding of the C++ object model.

Second, you are creating local objects that hold resources (models, textures, etc), which is good, and then copy them into other objects. Are you sure your copy constructors and assignment operators are doing the right thing with respect to the resources? Is it really possible to use shared_ptr for all those resources? There's nothing wrong with copying these objects, but it's important to get the semantics right or you will have exactly the problem you have described sooner or later.

Can you post more code for review, such as your fileManager.loadModel() function and/or your Model class?
Stephen M. Webb
Professional Free Software Developer

#33 alnite   Crossbones+   -  Reputation: 2038

Like
2Likes
Like

Posted 12 September 2011 - 06:19 PM

Here's a couple of mishaps I noticed:

1.

Body tempStar = Body(starTex);
Body planet = Body(10.0f, -5.0f, 30.0f, fileManager.loadTexture("planet1.bmp"));
shared_ptr<Actor> tempA = shared_ptr<Actor>(new Asteroid());
I see these everywhere. Did you realize that you are making two objects here? You created one and called the copy-constructors. Have you handled copy constructors correctly? You are risking duplicating and leaking huge memory chunks here especially that you also pass along the Model objects. I think what you meant is:

Body tempStar(starTex);
Body planet(10.0f, -5.0f, 30.0f, fileManager.loadTexture("planet1.bmp"));
shared_ptr<Actor> tempA(new Asteroid());

Also, check your World::start() method, you push_back actual instances of Body class, which triggers the copy constructor, depending on what you have inside Body, this could be huge leak and premature deallocations. And you did this 1000 times!

2. It's been a while since I last touched OpenGL, somebody with more expertise can probably enlighten more, but how did you deallocate these textures and models? I see allocations in the Game::start() method, but where did the deallocation occur?

It seems to me that your program hasn't managed memory consumption correctly, which could lead to the problem you are experiencing.

Now, I am not 100% sure if these are going to fix your problem. If you really want to fix this, I'd suggest going back to your old compiler where you can reproduce this bug and start cleaning up your code.

#34 swilkewitz   Members   -  Reputation: 125

Like
0Likes
Like

Posted 12 September 2011 - 07:55 PM

Ok thanks guys! I'll look into these suggestions asap. I'm not an expert in C++... (I'm self taught)
As for default constructing and overwriting, I'm pretty sure I never call the defaults. I felt like the default constructors were being called automatically, though. My default constructor for my Game class is empty, so would that empty constructor call the default constructors on any objects I failed to instantiate in it?

(and I was taking a high school java class when I started working on this back in january, so I probably did get things mixed up)

#35 swilkewitz   Members   -  Reputation: 125

Like
0Likes
Like

Posted 12 September 2011 - 08:38 PM

I reinstalled VC++ 2010 express, but I wasn't able to reproduce the bug. I'm sure there are still major problems though.

I just realized that I can link to my google code repository!
googlecode.com/*****

I appreciate all of your advice, so if you see any common misconceptions please notify me!

Thanks,
Scott

#36 rip-off   Moderators   -  Reputation: 7641

Like
3Likes
Like

Posted 13 September 2011 - 02:50 AM

I quickly skimmed the header files.

The main thing I'd worry about are the fact that some of your default constructors don't seem to initialise any variables at all! This could cause serious bugs. I'd recommend removing such constructors. To call a superclass constructor explicitly, you use an initialiser list. Calling Base::Base() constructs an anonymous Base value and throws it away. You do not need to do anything special to call a default Base constructor, or a default member constructor.

I don't see any classes that obviously break the rule of three. You could be stepping outside the bounds of an array in some source file though.

Minor points:
  • Header files should not define variables (see main.h). . If you ever #include this file elsewhere you will get "multiple definition" linker errors.
  • Names beginning with an underscore and capital letter are reserved for the "implementation" (i.e. the compiler and standard library), as are names beginning with two underscores, or names beginning with an underscore in the global scope. Your include guards could clash. I'd recommend changing them to something like FILE_NAME_H or H_FILE_NAME, which are safe for you to use.
  • Putting "using namespace" directives in header files is considered poor practise, because it is not possible to "un-use" a namespace. In some cases, adding a using directive in a header can subtly change the meaning of unrelated code in some file that ultimately ends up #including this file.
  • Your global "Game" object runs its constructor before main(). I'd recommend not doing this, code running before main doesn't have a defined order and this can cause problems if globals reference each other during construction. For the moment, you could use a global pointer, pointing at an auto allocated Game object.
  • I'd recommend moving your globals into the source file, in an anonymous namespace. If possible, try to remove them entirely, by making them local variables, passing them to the functions that require them. You could wrap most of those in a little struct to relieve the burden of passing them around.
  • Use initialiser lists for initialising members.


#37 LorenzoGatti   Crossbones+   -  Reputation: 2510

Like
1Likes
Like

Posted 13 September 2011 - 04:00 AM

  • Visual Studio "Professional" adds important features compared to the free version, but magically preventing memory corruption isn't one of them.
  • You didn't even post the Asteroid class, which is the obvious candidate for object slicing. It must be an interesting class: it doesn't even set its own texture and model.
  • If you don't understand and solve the problem, it will come back.
    If you don't know what whole classes of bugs (e.g. object slicing) are, how can you tell that your code doesn't have them? Inspections are difficult enough when one knows what to look for.

Produci, consuma, crepa

#38 Bregma   Crossbones+   -  Reputation: 4748

Like
1Likes
Like

Posted 13 September 2011 - 07:45 AM

I just realized that I can link to my google code repository!
http://code.google.com/p/triax-bridge-command/source/browse/#svn%2Ftrunk

Sweet.

I had a great deal of trouble compiling your code because i do not have Windows. That's not your problem, but a number of things came up that are just plain bad practice.

(1) you should never use a backslash \ in #include file paths. Never. Use a solidus / (aka frontslash to the unedumacated).

(2) the OpenGL headers are in <GL/gl.h> not <gl/GL.h> or any other case variation. Sure, some filesystems confuse letter cases, but not all do and if you get it right once, you get it right for always.

(3) You need to include the right header file for all external functions. std::abs() is in <cmath> and std::rand() is in <cstdlib>.

(4) I got compile failures because GCC balks at using copy-style initialization without a public copy constructor. I have no idea how MSVC++ compiles that code, it shouldn't. Try turning up the warning level on your compiler to 11 ( or at least d10+1).

Here's a diff of what I mean from FileManager.c.
@@ -60,8 +70,7 @@
 	ss << modelPath << id;
 	
 	//Create Stream
-	fstream fs = fstream();
-	ifstream file = ifstream(ss.str().c_str(), ios::binary);
+	ifstream file(ss.str().c_str(), ios::binary);
 	file.seekg(0, ios::beg);

If you're unfamiliar with parsing diffs, your code has a minus sign in column 0 and my replacement has a plus sign.

(5) You don't have any destructors, so you will leak resources. Because many of your objects (textures, etc) are proxied by handles providing the proper semantics in your destructors and copy operators will be tricky. Not impossible, and a good learning experience, but tricky nevertheless.

I would suggest putting print statements (use std::cerr, it's unbuffered) in all your constructors, and add copy constructors, assignment operators, and destructors to all your classes. Have the added copy constructors, assignment operators, and destructors do nothing but log their calls (since this will add no new functionality other than logging). This will help you get an excellent grasp of object lifetime and the interaction of the various components.

If I can get the code to compile and run (without the Windows-specific stuff) I'll run it through valgrind to flag problematic areas, but at least you have some suggestions to get along with.
Stephen M. Webb
Professional Free Software Developer

#39 swilkewitz   Members   -  Reputation: 125

Like
0Likes
Like

Posted 13 September 2011 - 08:36 AM

I've made some minor revisions already, I'll probably have time to fix my object construction later today.

#40 rip-off   Moderators   -  Reputation: 7641

Like
0Likes
Like

Posted 13 September 2011 - 08:51 AM

I got compile failures because GCC balks at using copy-style initialization without a public copy constructor. I have no idea how MSVC++ compiles that code, it shouldn't. Try turning up the warning level on your compiler to 11 ( or at least d10+1).

I believe the compiler is allowed to elide the copy constructor in such a statement (C++ standard section 12.8/15). GCC might be stricter about requiring the copy constructor to be present however, even if it will go unused.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS