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

Bug (segmentation error)

Started by Entheo Jan 23, 2007 at 1:29 AM 13 replies 2.5k views
Original Post
Entheo
Entheo
Hi, I cannot figure this one out... 

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


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

using std::cerr;
using std::endl;

int main(int args, char** argv)
{
  try
  { 
    SDL_Window window(SCREEN_WIDTH, SCREEN_HEIGHT, BPP, 
                    SDL_HWSURFACE | SDL_DOUBLEBUF, "MasterMind");
    MasterMind::Board board;
  
    std::vector<MasterMind::Tac>* pTacs = new std::vector<MasterMind::Tac>;
    pTacs->reserve(board.size());
    initialize_tacs(pTacs, board);
    std::vector<MasterMind::Tac>::iterator iTacs(pTacs->begin());
    cerr << "vec size: " << pTacs->size() << endl;

    bool quit = false;
    while (quit == false)
    {
      SDL_Event event;
      while (SDL_PollEvent(&event)) 
      {  
        if (event.type == SDL_QUIT)
          quit = true;
	if (event.type == SDL_MOUSEBUTTONDOWN)
        {

        }
      }
      iTacs = pTacs->begin();
      display_tacs(iTacs, board);
      board.drawOn(window.me());      
      SDL_Flip(window.me());
    }
    delete[] pTacs;
  }
  catch(...)
  {
    cerr << "An error occurred" << endl;
  }
  return 0;
}


void initialize_tacs(std::vector<MasterMind::Tac>* pTacs, MasterMind::Board& board)
{
  for (int i = 0; i < 8 ; i++)
  {
    MasterMind::Tac tac((Color)i, board);  //i represents the color constant.
    pTacs->push_back(tac);
    cerr << i << endl;
  }
  return;
}

void display_tacs(std::vector<MasterMind::Tac>::iterator iter, MasterMind::Board& board)
{
  for (int i = 0 ; i < board.numberOfTacs; i++)
  {
    cerr << "me" << endl;
    iter->drawOn(board.me());
    cerr << "me too" << endl;
    iter++;

  } 
  return;
}




Output:  
1
2
3
4
5
6
7
vec size: 8
me
Fatal signal: Segmentation Fault (SDL Parachute Deployed)
CRACK123
CRACK123
Hi,

Have you run it under a debugger. Its probably a junk pointer somewhere. I suggest run it under gdb and type "where" to see where exacly it crashed.


The more applications I write, more I find out how less I know
Entheo
Entheo
i downloaded gdb and it just says "no stack". Although i reckon i might not be using it right


i type: [$]gdb myExecutable
(gdb) where
No Stack.

I'll read gdb documentation to find out more...
Entheo
Entheo
Alright I got some info. Can anyone make sense of it though?

Program received signal SIGSEGV, Segmentation fault.0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0(gdb) where#0  0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0#1  0x072173e9 in SDL_UpperBlit () from /usr/lib/libSDL-1.2.so.0#2  0x0804a07f in MasterMind::Tac::drawOn (this=0x9821bb8, dest=0x9821320)    at MasterMind.cpp:90#3  0x0804c49f in display_tacs (iter={_M_current = 0x9821bb8},    board=@0xbfa36870) at mm.cpp:69#4  0x0804c7af in main () at mm.cpp:39
StarikKalachnikov
StarikKalachnikov 
std::vector<MasterMind::Tac>* pTacs = new std::vector<MasterMind::Tac>;delete[] pTacs;


with these statements you don't initialize an array, just one vector.
If you then try to delete it with delete[] it probably has some unexpected behaviour. Try delete without [].

that's my thought, might probably help, it might not :)
Next time I give my advice, I'll buy some bubblegum so I won't your kick ass!
E-Coder
E-Coder
You are pushing back 8 elements, but are you sure you reserved 8 elements?

    pTacs->reserve(board.size());


What did board.size() returned?

EDIT:
StarikKalachnikov:

	int *ptr = new int;	delete[] ptr;


Works perfectly... but [] is only needed if you reserve more elements ;).
JuNC
JuNC
Quote:
Original post by Entheo
Alright I got some info. Can anyone make sense of it though?

Program received signal SIGSEGV, Segmentation fault.0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0(gdb) where#0  0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0#1  0x072173e9 in SDL_UpperBlit () from /usr/lib/libSDL-1.2.so.0#2  0x0804a07f in MasterMind::Tac::drawOn (this=0x9821bb8, dest=0x9821320)    at MasterMind.cpp:90#3  0x0804c49f in display_tacs (iter={_M_current = 0x9821bb8},    board=@0xbfa36870) at mm.cpp:69#4  0x0804c7af in main () at mm.cpp:39


Reading this you can tell where the problem is, the call stack is showing you:

display_tacs -> MasterMind::Tac::drawOn -> SDL_Blit*

Now if I recall correctly, LowerBlit/UpperBlit are internal functions to SDL, so you probably call something like SDL_Blit() in drawOn. You don't show code for drawOn or what board.me() is so at a guess I'd say you're either not initializing SDL correctly (check your error codes from SDL init functions), you're not correctly initializing a surface (again check codes), or you're doing something screwy with the blitting (e.g. trying to pass a junk pointer).

You should also make sure you're not hitting the end of the Tac vector when doing your iteration, I don't quite understand your logic there, but I guess I just don't know what it's supposed to be doing.
Entheo
Entheo
Here's the drawOn function:
void Tac::drawOn(SDL_Surface* dest){  SDL_Rect cord;  cord.x = location.x;  cord.y = location.y;  SDL_BlitSurface(tac, NULL, dest, &cord);}


board.me() returns a SDL_Surface* which is the board image. So in effect, tac.drawOn(board.me()) is blitting an image to board.

I should also mention that all my code worked without any errors until I wrapped it in a namespace. After I made that MasterMind namespace it all went to hell and I don't know how.
JuNC
JuNC
Did you make sure to recompile your entire project?

If it worked before the only way adding a namespace could affect things is if you are (were) shadowing a function/variable name, or its possible the template parameters aren't correct now. Make sure you're recompiling all pre-compiled headers too.

Also, if you're going to ask for a bug hunt, it helps to post all relevant code (I know you've posted your whole project in a different thread, but thats a pain).
CRACK123
CRACK123
Quote:
Original post by Entheo
Alright I got some info. Can anyone make sense of it though?

Program received signal SIGSEGV, Segmentation fault.0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0(gdb) where#0  0x07217143 in SDL_LowerBlit () from /usr/lib/libSDL-1.2.so.0#1  0x072173e9 in SDL_UpperBlit () from /usr/lib/libSDL-1.2.so.0#2  0x0804a07f in MasterMind::Tac::drawOn (this=0x9821bb8, dest=0x9821320)    at MasterMind.cpp:90#3  0x0804c49f in display_tacs (iter={_M_current = 0x9821bb8},    board=@0xbfa36870) at mm.cpp:69#4  0x0804c7af in main () at mm.cpp:39


Hey,

This has a lot of info. If you look at it closely it crashed somewhere on line 90 or MasterMind.cpp. I would break at line 90 by using "br MasterMind.cpp:90" and run the code. Then inspect the values of all variables at line 90. Use print variableName to print information on the gdb console. Also check if the pointers are actually pointing to valid memory and not to something like 0xffffeeee or 0xcccc or some nonsense like that.

Hope this helps.
The more applications I write, more I find out how less I know
Enigma
Enigma
Quote:
Original post by E-Coder
int *ptr = new int;delete[] ptr;
Works perfectly... but [] is only needed if you reserve more elements ;).
Incorrect. Memory allocated by new must be deallocated using delete. Memory allocated by new[] must be deallocated using delete[]. Anything else is undefined behaviour. Note however that undefined behaviour can include doing what you expect, although this can change with compiler, different executions of the program, phases of the moon, etc. Never rely on undefined behaviour just because it seems to work on your compiler.

Σnigma
MaulingMonkey
MaulingMonkey
Quote:
Original post by Enigma
Quote:
Original post by E-Coder
int *ptr = new int;delete[] ptr;
Works perfectly... but [] is only needed if you reserve more elements ;).
Incorrect. Memory allocated by new must be deallocated using delete. Memory allocated by new[] must be deallocated using delete[]. Anything else is undefined behaviour. Note however that undefined behaviour can include doing what you expect, although this can change with compiler, different executions of the program, phases of the moon, etc. Never rely on undefined behaviour just because it seems to work on your compiler.

Σnigma


Quoted for added emphisis

In general, for anything with a non-trivial destructor (even if auto-generated) will not work correctly on any compiler.

Example:

#include <iostream>struct foo {	~foo() {		std::cout << "~foo" << std::endl;	}};int main () {	foo * f = new foo;	delete[] f;}


Running this code in debug mode on MSVC causes the allocation guard before the foo -- initialized to around 0xFEFEFEFE or so to be interpreted as the index. This means "~foo" is displayed about 4,278,124,286 times before the program finishes.

If I had used a non-static variable, either directly or indirectly (by calling a function which did, for example, or by having a member or base class's destructor doing so), this will quickly crash after many ~foos, on account of accessing memory out of bounds.

E-Coder:

push_back() will automatically reserve() as needed. The only difference in using it explicitly will be a possible performance gain by saving on the number of reallocations required. (Well, also, if you make sure you reserve() enough capacity() that it won't need to reserve() again, you can control when the iterators are invalidated, but that doesn't look to be the problem here).

It's also not a good idea to confuse "works (now) in my C++ compiler" with "legal, sane, or guaranteed to work in the future -- or even three seconds from now on the same compiler". C++ is full of things which violate such expectations.


Entheos:

Segmentation faults usually occur because you're accessing memory that you don't have access to. Without inspecting your code too carefully, I'm guessing iter advances beyond the end of the list, and you still try to use it.

Edit: Need to read closer. Please post drawOn, that appears to be where the problem is occuring. Original hypthesis may still hold, and even if it doesn't, it's worth mentioning for general good code practices, so I'm leaving it here:

A problem could occur if board.numberOfTacs is incorrect (>= the vector's size()), or iter starts out past begin (and after numberOfTacs iterations, is beyond end()).

This problem can be avoided entirely by following the idiomatic C++ pattern of always passing two iterators - begin and end.

Refactoring your example function:

void display_tacs(std::vector<MasterMind::Tac>::iterator iter, std::vector<MasterMind::Tac>::iterator end, MasterMind::Board& board){  for ( ; iter != end ; ++iter)  {    cerr << "me" << endl;    iter->drawOn(board.me());    cerr << "me too" << endl;  }   return;}


Note that this is a bit simpler, eliminating the variable i which basically duplicates iter in keeping track of where we are in the list. It also eliminates the need for board.numberOfTacs.

I'm applying the principle of Don't Repeat Yourself (DRY). Repeated data leads to extra code just to keep things in sync, and often hard to debug errors when you make a mistake in doing so, as I strongly suspect has occured here.
Zahlman
Zahlman
You're making things more complicated than they need to be. In particular, the vector does not outlive its scope, so there is no need or reason to allocate it dynamically. Also, the initialize and display interfaces should be (more) consistent, and should pass the container of Tacs by reference (a const reference for display).

Also, what do these .me() member functions return? They look suspicious.

#include "SDL_MainWindow.hpp"#include "MasterMind.hpp"// I recommend avoiding explicit prototypes by putting the declaration before// first use. This saves redundant typing (like MM said, don't repeat yourself)// and highlights any circular dependencies (potential mutual recursion) where// they occur.// Also, this will help us out a lot here:typedef std::vector<MasterMind::Tac> Taclist;using MasterMind::Board;using std::cerr;using std::endl;void initialize_tacs(Taclist& tacs, Board& board) {  for (int i = 0; i < 8; ++i) {    // Don't use C-style casts. Also, you can construct anonymous instances    // and push them back like so:    tacs.push_back(MasterMind::Tac(static_cast<Color>(i), board));    // But why do Tacs need to know what Board they're in, anyway?    cerr << i << endl;  }  // Why explicitly return?}void display_tacs(const Taclist& tacs, Board& board) {  for (Taclist::iterator it = tacs.begin(); it != tacs.end(); ++it) {    cerr << "me" << endl;    it->drawOn(board.me());    cerr << "me too" << endl;  } }int main(int args, char** argv) {  try {     SDL_Window window(SCREEN_WIDTH, SCREEN_HEIGHT, BPP,                       SDL_HWSURFACE | SDL_DOUBLEBUF, "MasterMind");    Board board;    Taclist tacs;    tacs.reserve(board.size()); // probably should go in initialization logic :P    initialize_tacs(tacs, board);    // debug:    cerr << "vec size: " << tacs.size() << endl;    bool quit = false;    while (!quit) { // don't compare to boolean literals      SDL_Event event;      while (SDL_PollEvent(&event)) {          if (event.type == SDL_QUIT) {          quit = true;        }      }      display_tacs(tacs, board);      board.drawOn(window.me());            SDL_Flip(window.me());    }  } catch(...) {    cerr << "An error occurred" << endl;  }}


As for the segfault: Enigma and MM are very correct, but I don't think it's what's causing what you describe (the improper delete[] shouldn't have happened yet anyway).

Also, how is the Board's "number of tacs" getting set, and why wouldn't you draw all the tacs? Oh, did you mean for the tac vector to hold "prototype" images, and give the board a vector of handles for those prototypes?
Entheo
Entheo
I've learned a lot from this even if the issue hasn't been solved. Thanks guys.

For those that asked to know what .me() does and the implementation of drawOn(), that has been done in a previous post I made, about 5 posts ago, after someone already asked me.

To be honest - Even though I am a serious noob, it may be possible that none of my code is offending a segmentation fault. The debugger keeps mentioning SDL_lowerBlit(). I changed my code significantly and it still says the error occured in SDL_lowerBlit() so I'm inclined to think that SDL sucks!
E-Coder
E-Coder
Quote:
Original post by MaulingMonkey
Quote:
Original post by Enigma
Quote:
Original post by E-Coder
int *ptr = new int;delete[] ptr;
Works perfectly... but [] is only needed if you reserve more elements ;).
Incorrect. Memory allocated by new must be deallocated using delete. Memory allocated by new[] must be deallocated using delete[]. Anything else is undefined behaviour. Note however that undefined behaviour can include doing what you expect, although this can change with compiler, different executions of the program, phases of the moon, etc. Never rely on undefined behaviour just because it seems to work on your compiler.

Σnigma


Quoted for added emphisis

In general, for anything with a non-trivial destructor (even if auto-generated) will not work correctly on any compiler.

Example:

*** Source Snippet Removed ***

Running this code in debug mode on MSVC causes the allocation guard before the foo -- initialized to around 0xFEFEFEFE or so to be interpreted as the index. This means "~foo" is displayed about 4,278,124,286 times before the program finishes.

If I had used a non-static variable, either directly or indirectly (by calling a function which did, for example, or by having a member or base class's destructor doing so), this will quickly crash after many ~foos, on account of accessing memory out of bounds.


You are right... sorry for the misstake. I didn't mean that the code was correct :P, I tough delete[] would delete the pointer with 1 as the array-dimension. (So it would delete 1 element).

Topic Locked

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

Sign in to reply to this topic.