Sign in to follow this  
archaos90

Hidden segfaults

Recommended Posts

Me and some friends are working on a school-project where we decided to create a RPG game, with a very similar gameplay as Zelda: A link to the past to the SNES. We're constructing our own engine, but the following code contains a segfault which I've been, for the last 3-4 hours, unable to find.
class Sector
{
          public:       int posx;
                        int posy;                        
                        char spritefile[50];
                        bool passable;
};

class Room
{
          public:       Room(const char *file);
                        void LoadRoom(const char *file);
                        void DrawRoom(SDL_Surface *surface);
                        
                        int rootx;
                        int rooty;
                        
                        Sector rmap[100][100];                                                                     
};

Room::Room(const char *file)
{           
     LoadRoom(file);
}

void Room::LoadRoom(const char *file)
{                              
     FILE *roomfile;
     int posx;
     int posy;
     int pass;
     char sprfile[50];
     roomfile = fopen(file, "r");
     char tag[20];
     char params[100];
     while(fscanf(roomfile, "%s = %s", tag, params) != EOF)
     {
              if (strcmp(tag, "WALL") == 0)
              {
                       sscanf(params, "%d - %d - %d - %s", &posx, &posy, &pass, sprfile);
                       rmap[posy][posx].posx = posx;
                       rmap[posy][posx].posy = posy;
                       if (pass == 0)
                       {
                       rmap[posy][posx].passable = true;
                       }
                       else
                       {
                       rmap[posy][posx].passable = false;                           
                       }
                       strcpy(rmap[posy][posx].spritefile, sprfile); 
              }
              else if (strcmp(tag, "COMMENT") == 0)
              {
                       
              }
     }
}


void Room::DrawRoom(SDL_Surface *surface)
{
     int i2, i;
     SDL_Surface *spr;
     SDL_Rect pos;
     for(i2=0;i2<100;i2++)
     {
            for(i=0;i<100;i++)
            {
                  pos.x = rmap[i2][i].posx * 50;
                  pos.y = rmap[i2][i].posy * 50;
                  pos.w = 50;
                  pos.h = 50;
                  spr = SDL_LoadBMP(rmap[i2][i].spritefile);
                  SDL_SetColorKey(spr, SDL_SRCCOLORKEY, SDL_MapRGB(spr->format, 0x00, 0x00, 0xFF));
                  SDL_BlitSurface(spr, &spr->clip_rect, surface, &pos);  
            }
     }
}

Share this post


Link to post
Share on other sites
It's impossible to tell without seeing the data files that are being read. Even then, it may be impossible to tell without see the rest of the game's source code. How are you sure it's in this code?

There are other problems lurking in this code. You're loading the tile's every time they're drawn, for example. You should load them once somewhere besides the drawing function.

Share this post


Link to post
Share on other sites
Maybe it is the consequence of the fscanf call. fscanf returns the number of arguments filled or EOF. Try this:


int scan = fscanf(roomfile, "%s = %s", tag, params);
while(scan != EOF && scan != 0)
{
...



// Repeat it at the end of the loop
scan = fscanf(roomfile, "%s = %s", tag, params);
}





If that doesn't work, please use a just-in-time debugger to determine which line of code causes the segfault. If you don't have one installed, use Dr. MinGW (contained in mingw-utils) - put it somewhere and install it as the default debugger with "drmingw.exe -i -a". Then you need to tell the compiler to create debugging information.

GCC: Use the parameter "-g3"
MS VC++: Look at the project options, there should be a predefined "Debug" target

Share this post


Link to post
Share on other sites
Quote:
Original post by wendigo23
It's impossible to tell without seeing the data files that are being read. Even then, it may be impossible to tell without see the rest of the game's source code. How are you sure it's in this code?

There are other problems lurking in this code. You're loading the tile's every time they're drawn, for example. You should load them once somewhere besides the drawing function.


The data file:


WALL = 10 - 10 - 0 - resources/sprites/church_floor.bmp
WALL = 10 - 10 - 0 - resources/sprites/church_floor.bmp


The BMP file is just, a BMP file, which SDL takes care of which is beyond my control and is as such irrelevant to the code.

My program only crashes when I use this code and occurs when I create an object of the Room-class. It's exactly there the crash occurs.

Yes, I know. I just felt lazy to make a completely optimized drawer for it just when testing.

Share this post


Link to post
Share on other sites
I assume this is C++? If so, it looks like you're basically programming in C (or what we sometimes call 'C with classes').

C++ is a low-level language, but C is lower-level still, and C (or C-ish) code is often replete with opportunities for access violations, segfaults, and other unpleasant behavior. Every fixed-sized buffer, every call to strcpy(), every argument passed via pointer - each is an opportunity for nasty bugs to arise.

C++ is by no means an easy language to program in, but it is much easier to avoid these types of errors in C++, assuming you write idiomatic code and use the tools that the language provides. In your case, for example, replacing character buffers, C-style file access, and raw arrays with string objects, file stream objects, and range-checked containers such as vector or boost::multi_array would go a long way towards making your code safer, more robust, and less error-prone.

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
I assume this is C++? If so, it looks like you're basically programming in C (or what we sometimes call 'C with classes').

C++ is a low-level language, but C is lower-level still, and C (or C-ish) code is often replete with opportunities for access violations, segfaults, and other unpleasant behavior. Every fixed-sized buffer, every call to strcpy(), every argument passed via pointer - each is an opportunity for nasty bugs to arise.

C++ is by no means an easy language to program in, but it is much easier to avoid these types of errors in C++, assuming you write idiomatic code and use the tools that the language provides. In your case, for example, replacing character buffers, C-style file access, and raw arrays with string objects, file stream objects, and range-checked containers such as vector or boost::multi_array would go a long way towards making your code safer, more robust, and less error-prone.


I understand what you mean, and I am a old C-coder since when I was like 11 or 12. To my experience, the string-class generally never works and almost always makes my program crash where a char array works perfectly. Almost everything C++ and whatever it's STL brings, brings nothing more than trouble. But since C++ and C is the same thing to the compilers of today, I see no reason why I shouldn't make use of both specialities by combining structural code with classes of C++ and still using the low-level speciality of C. It's always the same code when compiled anyway ;)

Share this post


Link to post
Share on other sites
Quote:
Original post by archaos90
To my experience, the string-class generally never works and almost always makes my program crash where a char array works perfectly.

That's funny. In my experience the string class works fine (trust me, the millions and millions of actual C++ programmers aren't hallucinating). In contrast, I'm given to understand that one may easily write good-looking C-style C++ which nevertheless contains segfaults which take upwards of 3-4 hours to find. Does that last bit jibe with your own experience?
Quote:
Almost everything C++ and whatever it's STL brings, brings nothing more than trouble.

That's because you don't understand it. It's a common programmer defense mechanism to attribute the problems one has with a language to faults of the language itself, as opposed to an unwillingness to get to know the language well enough. I'm not saying C++ is perfect--far from it, it's almost as crappy as C--but your reasons for disliking it are stupid.

Aaanyways, insults aside. If it's actually taken you 3-4 hours to find the bug so far, it's a cinch that you don't know how to use the debugger properly. The debugger will tell you exactly where the segfault is, and (with a little more effort) how the incorrect memory address got into the pointer in the first place. Trust me, you will save yourself time by learning to use the debugger now instead of doing ad-hoc troubleshooting.

Share this post


Link to post
Share on other sites
Quote:
Original post by archaos90
To my experience, the string-class generally never works and almost always makes my program crash where a char array works perfectly. Almost everything C++ and whatever it's STL brings, brings nothing more than trouble.
We're all free, of course, to develop in the language with which we're most comfortable, and it certainly makes sense that you would be more comfortable with C, given your history with it.

However, for the sake of others who might read this thread, I have to point out that your assessment of the C++ standard library is questionable at best, and completely inaccurate at worst.

Your basic implementation of the basic_string class template (such as those provided along with GCC or VC++) should be, for all practical purposes, bug-free. As such, it's safe to say that the string class works fine. If you found that it always made your program crash, it was most likely that you were using it incorrectly. (There aren't that many ways to screw up when using string objects, but it's still possible - for example, by reading or writing past the end of its allocated buffer.)

As for the rest of what you're referring to as the STL, the statement 'it brings nothing more than trouble' would be contested, I'm sure, by the countless experienced developers who use it every day. On the contrary, the STL-derived portions of the SC++L make things easier and less error-prone, not the other way around. If you've had a bad experience with these parts of the C++ standard library, it was again probably just due to unfamiliarity.

Finally, you say that a 'character array works perfectly'. And yet, here you are, posting on the forums because your program is crashing inexplicably :) As such, I'm not sure how you can say with such confidence that character arrays 'work perfectly' ;)

Anyway, I'll stop now. Language choice is certainly very personal (just peruse any of the recent 'language war' threads for evidence of this), and I don't mean to be overzealous in my advocacy of C++. However, I do feel strongly that the way forward in software development is in higher-level, more abstract languages, which C definitely is not. (Neither is C++, for that matter.)

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
Your basic implementation of the basic_string class template (such as those provided along with GCC or VC++) should be, for all practical purposes, bug-free. As such, it's safe to say that the string class works fine. If you found that it always made your program crash, it was most likely that you were using it incorrectly. (There aren't that many ways to screw up when using string objects, but it's still possible - for example, by reading or writing past the end of its allocated buffer.)

As an aside to this point, I've had a number of crashes in the standard string class, but it was always because I had made a mistake somewhere else and stepped on the string memory (or something along those lines). It seems like it's unusually sensitive to memory management mistakes. Of course I consider that a good thing since it lets me know if there's a problem.

Share this post


Link to post
Share on other sites
I agree that if this is supposed to be C++ then it should be done with standard strings, streams, and containers. Either way, it helps to know your way around a debugger.

It only took a few minutes to build your provided code and step through it to see where the problem lies. Looking at the call stack I see it blows up on this line:

rmap[posy][posx].posx = posx;

Most likely posy or posx are out of bounds. The debugger shows
posx = 0x0000000a and posy = 0xcccccccc (probably their initial uninitialized values).
They should have both been set in your sscanf call:
sscanf(params, "%d - %d - %d - %s", &posx, &posy, &pass, sprfile);

According to your data file params should contain the string:
10 - 10 - 0 - resources/sprites/church_floor.bmp
However, the debugger once again shows that it contains only the string:
10

This leads us to where the whole cascading failure really began with the fscanf call:
fscanf(roomfile, "%s = %s", tag, params)
If you look up the documentation for the formatting parameters of fscanf, it will tell you that %s only parses a string up to the next whitespace character.

Remove the whitespaces from your data file, e.g.:
WALL = 10-10-0-resources/sprites/church_floor.bmp
WALL = 10-10-0-resources/sprites/church_floor.bmp

and change your sscanf to not expect the whitespaces:
sscanf(params, "%d-%d-%d-%s", &posx, &posy, &pass, sprfile);

That should make your code actually behave in the manner you're expecting. However, it doesn't address the fact that it's bad code (if it's supposed to be C++) and prone to all sorts of nasty overflows and other bugs.

Share this post


Link to post
Share on other sites
Quote:
Original post by archaos90

char spritefile[50];
public: Room(const char *file);
void LoadRoom(const char *file);
Room::Room(const char *file)
void Room::LoadRoom(const char *file)
FILE *roomfile;
char sprfile[50];
roomfile = fopen(file, "r");
char tag[20];
char params[100];
while(fscanf(roomfile, "%s = %s", tag, params) != EOF)
if (strcmp(tag, "WALL") == 0)
sscanf(params, "%d - %d - %d - %s", &posx, &posy, &pass, sprfile);
strcpy(rmap[posy][posx].spritefile, sprfile);
else if (strcmp(tag, "COMMENT") == 0)


With code like this, I'm not surprised you have a segfault somewhere. I understand from things like the keyword 'class' that you are trying to write in this new-fangled language called "C++". Have you heard that we have this class that represents a string, called std::string, and a whole new I/O system built on "streams" which are type-safe and work seamlessly with user-defined types? No? Well, the language was only most recently standardized about ten years ago, so...

Not to mention, there is normally no point in factoring out a constructor's work to a separate function like that (don't "reset" a room, just construct another one), no point in an empty else-if block, and it's trivial to convert an integer to a boolean without conditional logic. It's also strange to include "Room" in the name of "Room"'s member functions - do you put wallet money in your wallet? Oh, and a "class" that has everything public is simply a "struct"; C++ does not distinguish those except for private/public considerations.

Oh, and you probably don't want to load the image from the hard drive, every time you draw each tile of the room. Instead, make an associative mapping of sprite file names to the loaded contents. This means you only open each file once. Fortunately, C++ makes this easy, with another helpful gadget called std::map.


#include <string>
#include <fstream>
#include <sstream>
#include <map>

struct Sector {
int posx;
int posy;
std::string spritefile;
bool passable;
};

std::map<std::string, SDL_Surface*> sprite_mapping;

struct Room {
Room(const std::string& filename);
void Draw(SDL_Surface *to);
int rootx;
int rooty;
Sector rmap[100][100];
};

Room::Room(const std::string& filename) {
ifstream file(filename.c_str()); // the "open" is implicit.
std::string line;

while (std::getline(file, line)) {
// There's this other thing that also exists in C called
// "scoping your variables properly"...
std::stringstream ss(line); // what we use to "re-parse" data.
// Instead of having a whole suite of s* functions that mirror the f*
// file I/O, we simply make a different kind of stream (from the string),
// and then forget it's anything different at all. Polymorphism!
std::string tag;
char equals;

ss >> tag >> equals;

if (equals == '=' && tag == "WALL") { // Yes, you really can compare strings just like that!
// We can avoid repeating ourselves, with a reference:
Sector& s = rmap[posy][posx];
// Yes, we use it like a value, even though it's not a copy...
// ... and we can read directly into its members instead of temporaries:
ss >> s.posx >> s.posy >> s.pass >> s.spritefile;
// Notice how we read into a bool, too. There's no magic format specifier
// for that.

// While we're at it, let's do that loading:
std::string& name = s.spritefile;
if (sprite_mapping.find(name) == sprite_mapping.end()) {
SDL_Surface* sprite = SDL_LoadBMP(name);
SDL_SetColorKey(sprite, SDL_SRCCOLORKEY,
SDL_MapRGB(sprite->format, 0x00, 0x00, 0xFF));
sprite_mapping[name] = sprite;
} // Otherwise, it was already loaded.
}
}
}

void Room::Draw(SDL_Surface *surface) {
SDL_Rect pos;

// C++ also lets you declare a counter variable directly in the for loop.
// I think C99 does, too.
for (int i2 = 0; i2 < 100; ++i2) {
for (int i = 0; i < 100; ++i) { // spaces are your friends...
const int TILESIZE = 50; // as are constants.
Sector& s = rmap[i2][i];
pos.x = s.posx * TILESIZE;
pos.y = s.posy * TILESIZE;
pos.w = TILESIZE;
pos.h = TILESIZE;
std::map<std::string, SDL_Surface*>::iterator it = sprite_mapping.find(s.spritefile);
if (it != sprite_mapping.end()) {
SDL_Surface* sprite = it->second;
SDL_BlitSurface(sprite, &sprite->clip_rect, surface, &pos);
}
}
}
}



Of course, it's quite possible that your real problem is that you don't actually have 10,000 Sectors to read in from your file, and therefore you try to draw Sectors that aren't properly set up. The code illustrated will neatly sidestep that (although you will probably run into other issues), by simply trying to find an empty-string spritefile name (because the std::string will get default-constructed, while a char[50] buffer will simply be left to contain whatever was already there in memory), observing that we don't have a corresponding SDL_Surface* loaded, and skipping that Sector.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Quote:
Original post by archaos90

char spritefile[50];
public: Room(const char *file);
void LoadRoom(const char *file);
Room::Room(const char *file)
void Room::LoadRoom(const char *file)
FILE *roomfile;
char sprfile[50];
roomfile = fopen(file, "r");
char tag[20];
char params[100];
while(fscanf(roomfile, "%s = %s", tag, params) != EOF)
if (strcmp(tag, "WALL") == 0)
sscanf(params, "%d - %d - %d - %s", &posx, &posy, &pass, sprfile);
strcpy(rmap[posy][posx].spritefile, sprfile);
else if (strcmp(tag, "COMMENT") == 0)


With code like this, I'm not surprised you have a segfault somewhere. I understand from things like the keyword 'class' that you are trying to write in this new-fangled language called "C++". Have you heard that we have this class that represents a string, called std::string, and a whole new I/O system built on "streams" which are type-safe and work seamlessly with user-defined types? No? Well, the language was only most recently standardized about ten years ago, so...

Not to mention, there is normally no point in factoring out a constructor's work to a separate function like that (don't "reset" a room, just construct another one), no point in an empty else-if block, and it's trivial to convert an integer to a boolean without conditional logic. It's also strange to include "Room" in the name of "Room"'s member functions - do you put wallet money in your wallet? Oh, and a "class" that has everything public is simply a "struct"; C++ does not distinguish those except for private/public considerations.

Oh, and you probably don't want to load the image from the hard drive, every time you draw each tile of the room. Instead, make an associative mapping of sprite file names to the loaded contents. This means you only open each file once. Fortunately, C++ makes this easy, with another helpful gadget called std::map.

*** Source Snippet Removed ***

Of course, it's quite possible that your real problem is that you don't actually have 10,000 Sectors to read in from your file, and therefore you try to draw Sectors that aren't properly set up. The code illustrated will neatly sidestep that (although you will probably run into other issues), by simply trying to find an empty-string spritefile name (because the std::string will get default-constructed, while a char[50] buffer will simply be left to contain whatever was already there in memory), observing that we don't have a corresponding SDL_Surface* loaded, and skipping that Sector.


Thanks alot! Really, I had totally forgotten about the C++ streams and it's magic. My code looks as it does, since the really first language I learned was Perl, so, I hope you understand me :)

Just one thing, I locked my mind for char arrays since SDL uses them, and I have no idea of how to convert a string object into a const char* variable, therefor my use of arrays.

Share this post


Link to post
Share on other sites
Quote:
Original post by LackOfGrace
ever checked the documentation about std::string?

checkout std::string::c_str()

( spoiler! it will return a pointer to a char array )
( you also have .data() wich will give you another type of pointer )


I think... I'm falling in love

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this