Skip to main content
GameDev.net gamedev.net
🔒 Locked

Segmentation Fault (Continued)

Started by Entheo Jan 24, 2007 at 12:28 AM 8 replies 1.2k views
Original Post
Entheo
Entheo
My question is: is this an SDL issue or an issue with my code? I'll first post the related code and then the gdb output. mm.cpp (the driver program)

#include "SDL_MainWindow.hpp"
#include "MasterMind.hpp"


void initialize_tacs(std::vector<Tac>&, Board&);
void display_tacs(std::vector<Tac>::iterator, std::vector<Tac>::iterator, SDL_Surface*);
void handle_tac_events(std::vector<Tac>::iterator, std::vector<Tac>::iterator, int&, int&, Board&);

int main(int args, char** argv)
{
    SDL_Window window(SCREEN_WIDTH, SCREEN_HEIGHT, BPP, 
                    SDL_HWSURFACE | SDL_DOUBLEBUF, "MasterMind");
    Board board;
  
    std::vector<Tac> vTacs;
    initialize_tacs(vTacs, board);

    bool quit = false;
    int mouse_x, mouse_y;

    while (quit == false)
    {
      SDL_Event event;
      while (SDL_PollEvent(&event)) 
      {  
        if (event.type == SDL_QUIT)
          quit = true;
	if (event.type == SDL_MOUSEBUTTONDOWN)
	{
	  SDL_GetMouseState(&mouse_x, &mouse_y);
	  //handle_tac_events(vTacs.begin(), vTacs.end(), mouse_x, mouse_y, board);
        }
      }
      display_tacs(vTacs.begin(),vTacs.end(), board.me());
      board.drawOn(window.me());      
      SDL_Flip(window.me());
    }
    return 0;
}


void initialize_tacs(std::vector<Tac>& vTacs, Board& board)
{
  for (int i = 0; i < 8 ; i++)
  {
    vTacs.push_back(Tac((Color)i, board));
  }
  return;
}

void display_tacs(std::vector<Tac>::iterator iter, std::vector<Tac>::iterator end, SDL_Surface* board)
{
  for (; iter != end;  ++iter)
  {
    iter->drawOn(board);
  } 
  return;
}

void handle_tac_events(std::vector<Tac>::iterator iter, std::vector<Tac>::iterator end, int& x, int& y, Board& board)
{
  for (; iter != end ; ++iter)
  {
    iter->handleEvents(x, y, board);
  } 
  return;

}


MasterMind.cpp (the implementation file of the header MasterMind.hpp) My guess is that the only parts needed are: Tac::drawOn(args), Tac::Tac().

#include <fstream>
#include <vector>
#include <iostream>
#include <SDL/SDL.h>
#include <SDL/SDL_image.h>
#include "MasterMind.hpp"



SDL_Surface* loadImage(const std::string& filename)
{  
   SDL_Surface* loadedImage = NULL;   
   SDL_Surface* optimizedImage = NULL;   
   loadedImage = IMG_Load(filename.c_str());  
   if (loadedImage != NULL) 
   {
     optimizedImage = SDL_DisplayFormat(loadedImage); //to ensure format is same for image and surface to be blitted. 
     SDL_FreeSurface(loadedImage);  
   } 
   else
     cerr << "Unable to initialize image," << filename << endl;
   return optimizedImage; 
}

//SDL_FreeSurface(SDL_Surface*) //this is a biggy man...

/* 
** Begin Hole
*/

Hole::Hole()
{
}
bool Hole::isOnHole(Point const &point) const
{
  if (point.x <= (center.x + RADIUS) && point.x >= (center.x - RADIUS)
  &&  point.y <= (center.y + RADIUS) && point.y >= (center.x - RADIUS))
    return true;
  return false;
}
/* 
** End Hole
*/

/*
**Begin Tac
*/
Tac::Tac(Color colour, Board& game_board): color(colour), is_clicked(false)
{
  switch (color)
  {
    case BLACK: tac = loadImage(black);
                break;
    case WHITE: tac = loadImage(white);
                break;
    case RED:   tac = loadImage(red);
                break;
    case BLUE:  tac = loadImage(blue);
                break;
    case YELLOW:tac = loadImage(yellow);
                break;
    case PURPLE:tac = loadImage(purple);
                break;
    case GREEN: tac = loadImage(green);
                break;
    case BROWN: tac = loadImage(brown);
                break;
    default: cerr << "unable to load image, invalid color specified." << endl;
  }   
  for (std::vector<Hole>::iterator i(game_board.tacDefaults.begin()); i != 
	 game_board.tacDefaults.end() ; ++i)
  {
    if (i->getID() == colour+1)  //set location to default location for color.
      location = i->getCenter();
  }
}


void Tac::drawOn(SDL_Surface* dest)
{
  SDL_Rect cord;
  cord.x = location.x;
  cord.y = location.y;
  SDL_BlitSurface(tac, NULL, dest, &cord);
}

bool Tac::isOnTac(Point const &point) const
{
  if (point.x <= (location.x + RADIUS*2) && point.x >= (location.x)
      &&  point.y <= (location.y + RADIUS*2) && point.y >= (location.y))
    return true;
  return false;
}

void Tac::handleEvents(int& x, int& y, Board& board)
{
  Point mouseLocation(x,y);
  if (is_clicked)
  {
    std::vector<Hole>::iterator iHoles(board.guess_holes.begin());
    for (; iHoles != board.guess_holes.end(); ++iHoles)
    {  
      if (iHoles->isOnHole(mouseLocation))
      {
        location = iHoles->getCenter();
	location.x -= RADIUS;
	location.y -= RADIUS; //to center out where the tac is drawn
      }
    }
    is_clicked = false;
  }
  else
    is_clicked = isOnTac(mouseLocation);
}
  
Tac::~Tac()
{
  SDL_FreeSurface(tac);
}
/*
** End Tac
*/
/*
** Begin Board
*/
Board::Board(): columns(5), rows(12)
{
  board = loadImage(boardFile);
  std::ifstream guess_holeS (cordFile.c_str());
  std::ifstream tac_defaults (tacFile.c_str());
  int num,x,y;
  guess_holes.reserve(rows*columns+5); //5 is the answer holes.
  tacDefaults.reserve(8); //8 is the # of colors.
  while (guess_holeS >> num)
  {
    guess_holeS >> x;
    guess_holeS >> y; 
    Point cordinate(x,y);
    Hole hole(cordinate, num);   //set cordinate and ID on hole
    guess_holes.push_back(hole); //then add the hole to the vec.    
  } 
  while (tac_defaults >> num)
  {
    tac_defaults >> x; 
    tac_defaults >> y;  
    Point cordinate(x,y);
    Hole hole(cordinate, num);
    tacDefaults.push_back(hole);
  }
}  

Board::~Board()
{
  SDL_FreeSurface(board);
  
}

void Board::drawOn(SDL_Surface* dest)
{
  SDL_Rect location;
  location.x = 0;
  location.y = 0;
  SDL_BlitSurface(board, NULL, dest, &location);
}

/*
** End Board
*/


Here is the debugger output:

(gdb) where
#0  main () at mm.cpp:13
(gdb) n
15          std::vector<Tac> vTacs;
(gdb) n
16          initialize_tacs(vTacs, board);
(gdb) n
*** glibc detected *** /home/Emperor/Programming/SDL/MasterMind/mm: double free or corruption (!prev): 0x082c03f8 ***
======= Backtrace: =========
/lib/libc.so.6[0xb98f18]
/lib/libc.so.6(__libc_free+0x78)[0xb9c3ef]
/usr/lib/libSDL-1.2.so.0(SDL_FreeSurface+0xdc)[0x721700c]
/home/Emperor/Programming/SDL/MasterMind/mm[0x8049f1b]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804cdd3]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804cdf1]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804ce34]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804ce4e]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804d33b]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804d471]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804c5e1]
/home/Emperor/Programming/SDL/MasterMind/mm[0x804c739]
/lib/libc.so.6(__libc_start_main+0xdc)[0xb4a724]
/home/Emperor/Programming/SDL/MasterMind/mm(__gxx_personality_v0+0x8d)[0x80495b1]
======= Memory map: ========
003da000-003e5000 r-xp 00000000 08:02 12799974   /lib/libgcc_s-4.1.1-20060525.so.1
003e5000-003e6000 rwxp 0000a000 08:02 12799974   /lib/libgcc_s-4.1.1-20060525.so.1
003f2000-003f6000 r-xp 00000000 08:02 6783204    /usr/lib/libXfixes.so.3.0.0
003f6000-003f7000 rwxp 00003000 08:02 6783204    /usr/lib/libXfixes.so.3.0.0
007d0000-007d9000 r-xp 00000000 08:02 6783324    /usr/lib/libXcursor.so.1.0.2
007d9000-007da000 rwxp 00008000 08:02 6783324    /usr/lib/libXcursor.so.1.0.2
007dc000-008be000 r-xp 00000000 08:02 6783689    /usr/lib/libstdc++.so.6.0.8
008be000-008c2000 r-xp 000e1000 08:02 6783689    /usr/lib/libstdc++.so.6.0.8
008c2000-008c3000 rwxp 000e5000 08:02 6783689    /usr/lib/libstdc++.so.6.0.8
008c3000-008c9000 rwxp 008c3000 00:00 0
00b17000-00b18000 r-xp 00b17000 00:00 0          [vdso]
00b18000-00b31000 r-xp 00000000 08:02 12799815   /lib/ld-2.4.so
00b31000-00b32000 r-xp 00018000 08:02 12799815   /lib/ld-2.4.so
00b32000-00b33000 rwxp 00019000 08:02 12799815   /lib/ld-2.4.so
00b35000-00c62000 r-xp 00000000 08:02 12799817   /lib/libc-2.4.so
00c62000-00c64000 r-xp 0012d000 08:02 12799817   /lib/libc-2.4.so
00c64000-00c65000 rwxp 0012f000 08:02 12799817   /lib/libc-2.4.so
00c65000-00c68000 rwxp 00c65000 00:00 0
00c6a000-00c8d000 r-xp 00000000 08:02 12799954   /lib/libm-2.4.so
00c8d000-00c8e000 r-xp 00022000 08:02 12799954   /lib/libm-2.4.so
00c8e000-00c8f000 rwxp 00023000 08:02 12799954   /lib/libm-2.4.so
00c91000-00c93000 r-xp 00000000 08:02 12799853   /lib/libdl-2.4.so
00c93000-00c94000 r-xp 00001000 08:02 12799853   /lib/libdl-2.4.so
00c94000-00c95000 rwxp 00002000 08:02 12799853   /lib/libdl-2.4.so
00c97000-00c99000 r-xp 00000000 08:02 6782123    /usr/lib/libXau.so.6.0.0
00c99000-00c9a000 rwxp 00001000 08:02 6782123    /usr/lib/libXau.so.6.0.0
00c9c000-00ca1000 r-xp 00000000 08:02 6782773    /usr/lib/libXdmcp.so.6.0.0
00ca1000-00ca2000 rwxp 00004000 08:02 6782773    /usr/lib/libXdmcp.so.6.0.0
00ca4000-00d9d000 r-xp 00000000 08:02 6782807    /usr/lib/libX11.so.6.2.0
00d9d000-00da1000 rwxp 000f9000 08:02 6782807    /usr/lib/libX11.so.6.2.0
00db8000-00dc7000 r-xp 00000000 08:02 6783081    /usr/lib/libXext.so.6.4.0
00dc7000-00dc8000 rwxp 0000e000 08:02 6783081    /usr/lib/libXext.so.6.4.0
00dca000-00dda000 r-xp 00000000 08:02 12799957   /lib/libpthread-2.4.so
00dda000-00ddb000 r-xp 0000f000 08:02 12799957   /lib/libpthread-2.4.so
00ddb000-00ddc000 rwxp 00010000 08:02 12799957   /lib/libpthread-2.4.so
00ddc000-00dde000 rwxp 00ddc000 00:00 0
00df1000-00df9000 r-xp 00000000 08:02 6782955    /usr/lib/libXrender.so.1.3.0
00df9000-00dfa000 rwxp 00007000 08:02 6782955    /usr/lib/libXrender.so.1.3.0
069ac000-06a7d000 r-xp 00000000 08:02 12799970   /lib/libasound.so.2.0.0
06a7d000-06a82000 rwxp 000d0000 08:02 12799970   /lib/libasound.so.2.0.0
071e2000-07253000 r-xp 00000000 08:02 6783923    /usr/lib/libSDL-1.2.so.0.7.2
07253000-07256000 rwxp 00070000 08:02 6783923    /usr/lib/libSDL-1.2.so.0.7.2
07256000-07271000 rwxp 07256000 00:00 0
08048000-08057000 r-xp 00000000 08:02 14801291   /home/Emperor/Programming/SDL/MasterMind/mm
08057000-08058000 rw-p 0000f000 08:02 14801291   /home/Emperor/Programming/SDL/MasterMind/mm
08058000-08068000 rw-p 08058000 00:00 0
08293000-082d5000 rw-p 08293000 00:00 0          [heap]
b782d000-b7aed000 rw-p b782d000 00:00 0
b7b00000-b7b21000 rw-p b7b00000 00:00 0
b7b21000-b7c00000 ---p b7b21000 00:00 0
b7cfd000-b7fbd000 rw-s 00000000 00:07 1114139    /SYSV00000000 (deleted)
b7fbd000-b7fc0000 rw-p b7fbd000 00:00 0
b7fd5000-b7fd6000 rw-p b7fd5000 00:00 0
bfc35000-bfc4a000 rw-p bfc35000 00:00
Program received signal SIGABRT, Aborted.
0x00b17402 in __kernel_vsyscall ()


I really appreciate your help!
Endurion
Endurion
The problem is the destructor of Tac together with std::vector.

Everytime you push_back an item in std::vector not the actual object gets pushed in but a copy is created (via copy constructor). As soon as the original object leaves the scope it is destroyed thus calling FreeSurface. Now the copy inside the vector still has a pointer to that destroyed surface.

Solution:

Follow the rule of the three: If you need to either create a destructor, copy constructor or assignment operator you need to implement all three of them.

You need to do proper handling of the surface in your copy constructor and you should do it as well in the assignment operator. Either add a ref count to the surfaces or create a new surface from the old one.
Fruny: Ftagn! Ia! Ia! std::time_put_byname! Mglui naflftagn std::codecvt eY'ha-nthlei!,char,mbstate_t>
Entheo
Entheo
I think you are right Endurion!!! Thank you so very much for pointing this out to me... This is by far the biggest hurdle I've had since learning C++ two weeks ago... As of this moment I don't exactly know how to fix it but I'll have it figured out within the hour and I'll report back. Again thanks!
E-Coder
E-Coder
You could fix it by pushing an empty element and copy all the old data to vector (where n is the empty element). Altough... a copy contructor is saver and neat :).
Entheo
Entheo
Yeah so I figured it out. What I did was a deep copy - again thanks so much! I would have never figured that one out on my own.
Aardvajk
Aardvajk
Quote:
Original post by E-Coder
You could fix it by pushing an empty element and copy all the old data to vector (where n is the empty element).


Sadly, that is not a solution. Whenever you push_back a new element to the vector, the entire vector could require reallocating, at which point all the elements will be copied into the new ones and the destructors of the original elements would be called.

If it were not feasible to implement proper deep copy for any reason, the only solutions would be to pre-reserve the maximum vector size and make sure yourself that you didn't go over that, or to store a vector of pointers to the objects so that vector reallocations would just copy the pointers. With the pointer solution you would, of course, have to take responsiblity for freeing up the data yourself when elements were removed.

E-Coder
E-Coder
EasilyConfused, the destructor of the local instance will not be called. A deep copy is required, but this is what I mean:

struct SData{    int id;    char *szName;};vector<SData> vectData;...// Some functionvectData.push_back(empty_object);vectData[last_element].szName = new char[strlen(szName)+1];// copy everything...<tt>destructor</tt>for(int i = 0; i < vectData.size(); i++){    delete[] vectData.szName;}


In this case, the destructor of the 'empty_object' will be called, but that wouldn't affect the vector.

Just forget this :P, the copy-constructor is a much better way.
Entheo
Entheo
E-Coder is correct, but like he said, a copy constructor is a far better solution. But that is something I hadn't thought of E-Coder. Thanks.
intransigent-seal
intransigent-seal
You're helped by the fact that SDL Surfaces are reference counted objects. All you have to do is write a copy constructor that does a shallow copy of the surface (ie, copies the surface pointer), and increments the surface's refcount, and then everything should work fine (the SDL_FreeSurface() call in the destructor will then decrement the refcount and only destroy the surface itself at the right time).

Edit: Stupid me, I should've read the replies properly instead of just skimming them - looks like Endurion already mentioned reference counting in the first reply, although it's useful to note that the SDL_Surface already has a refcount that you can use, you don't have to add your own unless there is other data that you need to protect with it.

John B
The best thing about the internet is the way people with no experience or qualifications can pretend to be completely superior to other people who have no experience or qualifications.
Aardvajk
Aardvajk
You're missing my point. I'm talking about the construction/destruction of the objects already in the vector, not the local object.

Consider this:

#include <iostream>#include <vector>class x{public:    x(){ std::cout << "x constructor\n"; }    x(const x &v){ std::cout << "x copy\n"; }    ~x(){ std::cout << "x destructor\n"; }    const x &operator=(const x &v){ std::cout << "x assign\n"; return *this; }};std::vector<x> v(1);int main(){    v.push_back(x());    v.push_back(x());}


Note I am setting the initial capacity of the vector to 1 in order to force a reallocation when I push the second object.

Compiled with Digital Mars C++, the above produces the following output:

x constructorx copyx destructorx constructorx copyx copyx destructorx destructorx constructorx copyx copyx copyx destructorx destructorx destructorx destructorx destructorx destructor


Clearly more constructors and destructors than for the two objects concerned.

When the second object is pushed, and the vector realises it doesn't have enough capacity to fit it, it needs to reallocate a block of new memory, use the copy constructor to in-place copy-construct the old objects to the new ones, call the destructors for the old objects, then in-place construct the pushed object. A vector cannot and does not simply copy the memory bits across when it reallocates.

So, as I said, unless you are going to enforce that a vector never exceeds its initial capacity, all your objects already in the vector can be copied and destroyed simply by pushing another object into the vector.

Sorry if I didn't explain that properly the first time. Another alternative is to use a std::list or std::deque, which do not require the above copying on reallocation.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.