3. [Problem gelöst] Segmentation fault: Hilfe, was mach ich bloß falsch?

[Problem gelöst] Segmentation fault: Hilfe, was mach ich bloß falsch?

  • A

    Hallo zusammen,

    seit erschreckend kurzer Zeit beschäftige ich mich (nach fünf Jahren Abstinenz) wieder mit C++. Ich versuche gerade mit SDL eine kleine Engine auf die Beine zu stellen, jedoch scheint mein Code irgendwo fehlerhaft zu sein, denn ich bekomme regelmäßig einen segmentation fault zur Laufzeit.

    Ich habe die letzten beiden Tage damit verbracht herauszufinden, wo mein Fehler liegt, aber ich steige da einfach nicht durch. Ich konnte immerhin herausfinden, welche Zeile den Fehler auslöst, aber warum ist mir völlig unklar. Daher füge ich den Code bei und hoffe dass jemand so freundlich ist, einen Blick darauf zu werfen.

    1. Struktur der Engine (bis jetzt):

    cSystem system (initialisiert SDL etc.)
  cGraphics graphics (zeichnet Bilder etc.)
    cImageManager imageManager (lädt und verwaltet Images (cImage) in einer std::map
  cInput input (führt Buch über alle Eingaben)

    2. Ausgabe im Terminal:

    Initializing SDL...                             [DONE]
Hardware acceleration?                          [NO]
Creating surface...                             [DONE]
Image /home/nik/media/tux-icon.jpg loaded successfully.
Ho Ho Ho, it's debug time!
Segmentation fault (core dumped)
nik@Filzlaus:~/development/sdltest/debug/src$

    3. Ablauf
    Um den Code zu testen, werden zu Begin zwei Bilder geladen: Ein Hintergrundbild und ein kleines Tux-Bild, das mit den Cursortasten hin und hergeschoben werden kann.
    Der Segmentation fault tritt jedoch schon beim Laden des ersten Bildes auf, und zwar in der Funktion

    bool cImageManager::addImage(string fname, uint id)

    .

    4. Code

    4.a: global.h

    #ifndef GLOBAL
#define GLOBAL

	#ifdef HAVE_CONFIG_H
	#include <config.h>
	#endif

#include <iostream>
#include <stdlib.h>
#include <map>

#include "SDL.h"
#include "SDL_image.h"

using namespace std;

#endif

    4.b: sdltest.cpp

    #include <global.h>
#include <system.h>

int main(int argc, char *argv[])
{ 
		cSystem system(640, 480, SDL_DOUBLEBUF);		

		system.graphics.imageManager.addImage("/home/nik/media/tux-icon.jpg", 0);
		system.graphics.imageManager.addImage("/home/nik/development/sdltest/background.png", 10);

		//system.graphics.blit(10, 0, 0);
		cout << "Ha ha" << endl;
		system.graphics.blit(0, 310, 230);

		system.gameLoop();	

}

    4.c.1: system.h

    #ifndef SYSTEM
#define SYSTEM

#include <global.h>
#include <graphics.h>
#include <input.h>

enum eHandleEvents
{
	OKAY = 0, QUIT
};

class cSystem
{
	public:
		cSystem(uint width, uint height, Uint32 flags);
		~cSystem();				

		uint	gameLoop();
		eHandleEvents	handleEvents();

		cInput 								input;
		cGraphics 						graphics;
		SDL_Surface 					*surface;
		const SDL_VideoInfo 	*videoinfo;
		Uint32 videoflags;
};

#endif

    4.c.2: system.cpp

    #include <system.h>

uint cSystem::gameLoop()
{
	int x = 310; int y = 230;
	int oldX = x; int oldY = y;
	int changed = false;
	SDL_Rect oldRect;
	eHandleEvents he;
	while(true)
	{
		he = handleEvents();

		if (input.keyboard[SDLK_UP].keyState == DOWN)
		{
			y -= 2;
			changed = true;
		}
		if (input.keyboard[SDLK_DOWN].keyState == DOWN)		
		{
			y += 2;		
			changed = true;
		}
		if (input.keyboard[SDLK_RIGHT].keyState == DOWN)		
		{			
			x += 2;
			changed = true;
		}
		if (input.keyboard[SDLK_LEFT].keyState == DOWN)		
		{
			x -= 2;
			changed = true;
		}

		if (changed == true)
		{
			oldRect.x = oldX; oldRect.y = oldY;
			oldRect.w = graphics.imageManager.images[0].getWidth();
			oldRect.h = graphics.imageManager.images[0].getHeight();

			graphics.blit(10, oldRect, oldRect);

			graphics.blit(0, x, y);

			oldX = x; oldY = y;
			changed = false;
		}
		//SDL_Flip(surface);
		if (he == QUIT) break;
		if (input.keyboard[SDLK_ESCAPE].keyState == DOWN) break;
		SDL_Delay(30);
	}
}

eHandleEvents cSystem::handleEvents()
{
	SDL_Event event;

	while(SDL_PollEvent(&event))
	{
		switch(event.type)
		{
			case SDL_KEYDOWN:				
				input.keyboard[event.key.keysym.sym].keyState = DOWN;
				cout << SDL_GetKeyName(event.key.keysym.sym) << " is down." << endl;
				input.keyboard[event.key.keysym.sym].timestamp = SDL_GetTicks();
				break;

			case SDL_KEYUP:
				input.keyboard[event.key.keysym.sym].keyState = UP;
				cout << SDL_GetKeyName(event.key.keysym.sym) << " is up (" 
						<< SDL_GetTicks() - input.keyboard[event.key.keysym.sym].timestamp 
						<< "ms)." << endl;
				input.keyboard[event.key.keysym.sym].timestamp = SDL_GetTicks();
				break;

			case SDL_QUIT:
				return QUIT;
		}
	}
	return OKAY;	
}

cSystem::cSystem(uint width, uint height, Uint32 flags)
{
	videoflags = flags;

	cout << "Initializing SDL... ";
	if (SDL_Init(
			SDL_INIT_VIDEO | 
			 SDL_INIT_AUDIO | 
			 SDL_INIT_TIMER
							) < 0 )
	{
		cout << "				[ERROR]" << endl;
		cout << "  (" << SDL_GetError() << ")" << endl;
		exit(true);
	}
	else
	{
		cout << "				[DONE]" << endl;
	}

	cout << "Hardware acceleration?";
	videoinfo = SDL_GetVideoInfo();

	if (videoinfo)
	{
		if (videoinfo->hw_available == true)
		{
			cout << "				[YES]" << endl;
			videoflags = videoflags | SDL_HWSURFACE;
		}
		else
		{
			cout << "				[NO]" << endl;
			videoflags = videoflags | SDL_SWSURFACE;
		}
	}
	else
		cout << "				[ERROR]" << endl;

	cout << "Creating surface...  ";
	surface = SDL_SetVideoMode(width, height, 24, videoflags);
	if (surface == NULL)
	{
		cout << "				[ERROR]" << endl;
		cout << "  (" << SDL_GetError() << ")" << endl;
		exit(true);
	}
	else
	{
		cout << "				[DONE]" << endl;
	}
	graphics.setDisplaySurface(surface);

}

cSystem::~cSystem()
{
	cout << "Destroying surface...";
	SDL_FreeSurface(surface);
	cout << "				[DONE]" << endl;

	cout << "Shutting down SDL... ";
	SDL_Quit();
	cout << "				[DONE]" << endl;
	cout << "Good bye." << endl;
}

    4.d.1 graphics.h

    #ifndef GRAPHICS
#define GRAPHICS

#include <global.h>

class cImage
{
	public:		
		SDL_Surface *data;
		std::string filename;
		uint				id;

		cImage();
		~cImage();

		uint getWidth();
		uint getHeight();

};

class cImageManager
{
	public:

		map <uint, cImage> images;
		uint imagecount;

		cImageManager();
		~cImageManager();

		SDL_Surface getImageData(uint id);
		bool addImage(string fname, uint id);
		bool imageExists(uint id);
	//bool addImage(string fname, uint id, SDL_Rect rect);
/*	bool removeImage(uint id);
		bool removeAll();
		bool reloadAll();
		bool reloadImage(uint id);
	*/

};	

class cGraphics
{
	public:
		SDL_Surface *display;

		//cGraphics();
		//~cGraphic();

		cImageManager imageManager;

		void blit(uint id, uint x, uint y);
		void blit(uint id, SDL_Rect src, SDL_Rect dest);
		void setDisplaySurface(SDL_Surface *s);
};

#endif

    4.d.2: graphics.cpp

    #include <graphics.h>

void cGraphics::setDisplaySurface(SDL_Surface *s)
{
	display = s;
}

void cGraphics::blit(uint id, uint x, uint y)
{

	SDL_Rect trect;
	SDL_Rect trect2;
	trect2.x = 0; trect2.y = 0;
	trect2.w = imageManager.images[id].getWidth();
	trect2.h = imageManager.images[id].getHeight();

	trect.x = x; trect.y = y;
	trect.w = imageManager.images[id].getWidth();
	trect.h = imageManager.images[id].getHeight();

	blit(id, trect2, trect);
}

void cGraphics::blit(uint id, SDL_Rect src, SDL_Rect dest)
{
	if (imageManager.imageExists(id))
	{
		SDL_BlitSurface(imageManager.images[id].data, &src, display, &dest);	
		SDL_UpdateRect(display, dest.x, dest.y, dest.w, dest.h);
	}
}

cImageManager::cImageManager()
{	
}

cImageManager::~cImageManager()
{
//	removeAll();
}

bool cImageManager::imageExists(uint id)
{
	map <uint, cImage>::iterator i = images.find(id);
	if (i == images.end())
	{
		cout << "Image ID " << id << " does not exist." << endl;
		return false;
	}
	else
		return true;	
}

bool cImageManager::addImage(string fname, uint id)
{
	SDL_Surface *tempsurface = IMG_Load(fname.c_str());	
	if (tempsurface == NULL)
	{
		cout << "Could not load " << fname << "!" << endl;
		return false;
	}
	cout << "Image " << fname << " loaded successfully." << endl;		
	cout << "Ho Ho Ho, it's debug time!" << endl;
	*images[id].data = *tempsurface;      // !!! Hier passiert der Fehler.
	cout << "Ho Ho Ho, it's debug time! Again!" << endl;
	images[id].filename = fname;
	SDL_FreeSurface(tempsurface);
	return true;
}

SDL_Surface cImageManager::getImageData(uint id)
{
	return *images[id].data;
}

uint cImage::getWidth()
{
	return data->w;
}

uint cImage::getHeight()
{
	return data->h;
}

cImage::cImage()
{}

cImage::~cImage()
{
	SDL_FreeSurface(data);
}

    (input.h hat mit dem Problem definitiv nichts zu tun und fehlt daher)

    Ich hoffe jemand kann mir helfen. Sorry, dass ich den ganzen Code hier eingefügt habe, aber ich bin mit meinem Latein am Ende.

    Bis dann,
    anytime

    0
  • C
     

    *images[id].data = *tempsurface;

    Du dereferenzierst hier einen Zeiger, der (höchstwahrscheinlich) vom Default-Ctor von cImage initialisiert wurde (sprich: gar nicht) - damit versuchst du auf Speicher zuzugreifen, der dir nicht gehört. Lass doch mal die beiden Sternchen in der Zeile weg, dann sollte es besser klappen.

    0
  • A

    Leider nicht. Ursprünglich habe ich keine "Sternchen" verwendet, dann tauchte der Seg-fault auf und nach dem Trial'n'Error Prinzip habe ich es dann so probiert.

    Das Problem besteht also mit und ohne Dereferenzierungsoperatoren.

    0
  • 7

    Wo hält der Debugger an?

    0
  • C

    Ja, wenn du die Bilddaten sofort am Ende der Funktion wieder vernichtest, sind sie weg 😉 Und dann dürfte der nächste Versuch, sie zu lesen, zu einem SegFault führen. Die SDL_FreeSurface() solltest du erst dann aufrufen, wenn du deine Bilddaten nicht mehr benötigst (und am besten überlässt du der cImage-Klasse die Entscheidung, wann das ist).

    0
  • A

    SDL_FreeSurface(tempsurface) war in diesem Fall nötig, weil ich mit

    *images[id].data = *tempsurface;

    den Wert von tempsurface an images[id].data zugewiesen habe und tempsurface damit seinen Zweck erfüllt hatte. Glaub ich 🙂

    Ohne Dereferenzierungsoperator hat SDL_FreeSurface(tempsurface) natürlich keinen Sinn, Ursache des Problems ist es dummerweise aber auch nicht 😞

    0
  • C

    anytime schrieb:

    SDL_FreeSurface(tempsurface) war in diesem Fall nötig, weil ich mit

    *images[id].data = *tempsurface;

    den Wert von tempsurface an images[id].data zugewiesen habe und tempsurface damit seinen Zweck erfüllt hatte. Glaub ich 🙂

    Ja, du hast irgendwas zugewiesen - nur das Problem ist, daß data nicht ordnungsgemäß initialisiert war und das OS etwas degegen hatte, dahinter etwas zuzuweisen.

    Ohne Dereferenzierungsoperator hat SDL_FreeSurface(tempsurface) natürlich keinen Sinn, Ursache des Problems ist es dummerweise aber auch nicht 😞

    Und was ist sonst die Ursache? (bzw. bis wohin kommst du, wenn du die Sterne dort oben weglässt?)

    Anderer Lösungsansatz: Statt einen Zeiger in der cImage zu speichern, könntest du das SDL_Surface als Kopie dort rein packen.

    PS: Ich kenne die Klasse SDL_Surface nicht, aber ist die problemlos kopierbar?

    0
  • A

    Das Programm bricht an derselben Stelle ab wie zuvor. Interessant ist allerdings, dass der Debugger von KDevelop manchmal beim Laden des ersten Bildes, manchmal auch beim zweiten abstürzt. Allerdings an derselben Position:

    bool cImageManager::addImage(string fname, uint id)
{
	SDL_Surface *tempsurface = IMG_Load(fname.c_str());	
	if (tempsurface == NULL)
	{
		cout << "Could not load " << fname << "!" << endl;
		return false;
	}
	cout << "Image " << fname << " loaded successfully." << endl;		
	cout << "Ho Ho Ho, it's debug time!" << endl;
	images[id].data = tempsurface; //<- immer noch ein Fehler
	cout << "Ho Ho Ho, it's debug time! Again!" << endl;
	images[id].filename = fname;	
	return true;
}

    Die umgeschriebene Version dereferenziert tempsurface

    images[id].data = *tempsurface;

    ,
    weil images[id].data hier kein Zeiger mehr ist. Das Resultat bleibt allerdings dasselbe: Segmentation fault.

    SDL ist in C geschrieben, daher ist SDL_Surface auch nur eine einfache struct. Schätze das macht sie "einfach so kopierbar" (glaub ich). I.d.R. verwendet "man" aber Pointer, weil die meisten SDL-Funktionen Pointer auf SDL Datentypen erwarten. Ich hab den Code eben mal so umgeschrieben, dass cImage ein echtes SDL_Surface Objekt erhält.
    Das ändert aber kein bisschen, das Programm verursacht immer noch an genau derselben Stelle einen Seg-fault.

    Edit:
    Wenn ich die fehlerhafte Zeile auskommentiere, ruft

    images[id].filename = fname

    einen Segmentation fault hervor, so dass ich annehme, dass die generelle Wertzuweisung an images[id] das eigentliche Problem ist. Ich habe aber leider viel zu wenig Ahnung, um daraus die richtigen Schlüsse zu ziehen.

    0
  • C

    eine struct ist nicht per se kopierbar (und eigentlich legen Funktionen wie SDL_FreeSurface() nahe, daß dort etwas mehr Verwaltungsaufwand hinter den Kulissen nötig ist). Da könnte es auch Probleme damit geben, daß du in der GetImageData()-Methode selber eine Kopie anlegst.

    (soll cImage eine Wrapper-Klasse um SDL_Surface werden? Wenn ja, brauchst du noch einige mehr als die bisher vorhandenen Methoden)

    0
  • 7

    Bevor du zu lange im Nebel stocherst, wirf mal valgrind auf das Problem: www.valgrind.org

    0
  • A

    Wenn ich mit der Engine fertig werden sollte, trete ich sie sowieso in die Tonne. Ich code nämlich einfach so ins Blaue hinein, ohne großen Plan. Daher ist cImage auch noch recht schmallbrüstig und wird einfach nach Bedarf erweitert. Zudem muss ich den Code noch stärker kapseln, die Mutterobjekte bedienen sich im Moment teilweise recht großzügig an den Klasseninterna ihrer (Kindes)kinder.

    Du hast wohl Recht, SDL_Surface ist nicht einfach so zu kopieren, die darin enthalten Pixeldaten werden etwa auf dem Heap gespeichert. Aber ich glaube auch wirklich nicht, dass es erforderlich ist, etwas anderes als einen Zeiger auf ein Surface zu benutzen.

    Und da der segmentation fault bei allen drei von mir verwendeten Methoden zur selben Zeit an der selben Stelle auftritt, muss es etwas anderes sein.
    Glaub ich mal wieder 🙂

    @7H3 N4C3R:
    Danke, installiere es gerade. Mal sehen ob das wirklich so gut ist, wie die Website mich glauben machen will 😉

    0
  • C

    anytime schrieb:

    Du hast wohl Recht, SDL_Surface ist nicht einfach so zu kopieren, die darin enthalten Pixeldaten werden etwa auf dem Heap gespeichert. Aber ich glaube auch wirklich nicht, dass es erforderlich ist, etwas anderes als einen Zeiger auf ein Surface zu benutzen.

    Da ist es tatsächlich die beste Lösung, cImage als Wrapper um ein SDL_Surface* zu verwenden (das erfordert aber einiges an Aufwand bei den Kopieroperationen und im Destruktor - ich würde vermutlich einen Ansatz mit Referenzzählung verwenden).

    0
  • A

    So, ich habe Valgrind drauf losgelassen, und auch wenn ich mir bei der Ausgabe unter den meisten Dinge etwas vorstellen kann, verstehe ich es doch nicht wirklich (JA, ich bin ein DAU). Habe das Log bei Nopaste online gestellt: http://rafb.net/p/1luF6p57.html. Falls jemand fähig genug ist es zu interpretieren, wäre das eine große Hilfe für mich.

    Da ist es tatsächlich die beste Lösung, cImage als Wrapper um ein SDL_Surface* zu verwenden (das erfordert aber einiges an Aufwand bei den Kopieroperationen und im Destruktor - ich würde vermutlich einen Ansatz mit Referenzzählung verwenden).

    so viel aufwand wird das hoffentlich nicht sein, alles was man jemals von cImage wollen kann, ist ein Zeiger auf die Pixeldaten, ein Zeiger auf die SDL_Surface, getter für Höhe und Breite des Bildes, für den Dateinamen sowie für die transparente Farbe. Für die Verwaltung ist dann cImageManager zuständig.

    Aber was schwafel ich, mit Segmentation fault wird das sowieso nix 🙂

    0
  • 7

    ==14202== Conditional jump or move depends on uninitialised value(s)
    ==14202== at 0x0406f8ab: SDL_FreeSurface (in /usr/lib/libSDL-1.2.so.0.11.0)
    ==14202== by 0x08049277: cImage::~cImage() (graphics.cpp:131)
    ==14202== by 0x08049c7b: std::pair<unsigned ((null):0)
    ==14202== by 0x0804a731: std::map<unsigned, ((null):0)
    ==14202== by 0x080494fe: cImageManager::addImage(std::string, unsigned) (graphics.cpp:98)
    ==14202== by 0x0804aa37: main (sdltest.cpp:34)
    ==14202==
    ==14202== Use of uninitialised value of size 4
    ==14202== at 0x0406f8ad: SDL_FreeSurface (in /usr/lib/libSDL-1.2.so.0.11.0)
    ==14202== by 0x08049277: cImage::~cImage() (graphics.cpp:131)
    ==14202== by 0x08049c7b: std::pair<unsigned ((null):0)
    ==14202== by 0x0804a731: std::map<unsigned, ((null):0)
    ==14202== by 0x080494fe: cImageManager::addImage(std::string, unsigned) (graphics.cpp:98)
    ==14202== by 0x0804aa37: main (sdltest.cpp:34)

    Die beiden Meldungen sagen dir quasi das selbe. in graphics.cpp, Zeile 131 wird anscheinend SDL_FreeSurface mit einem unitialisierten Zeiger gerufen. Die langen Meldungen dadrüber sind möglicherweise kleine Fehler in Systembibliotheken oder SDL selbst, die hoffentlich nicht ins Gewicht fallen sollten.

    Nachtrag:
    Passiert es, dass cSystem-Objekte kopiert werden? Ohne korreten Copy-Konstruktor und Assignment-Operator passiert dabei natürlich Murks. Mach die beiden mal private und schau mal, wo es beim Kompilieren kracht.

    0
  • A

    Heureka!

    7H3 N4C3R, danke für die Hilfe, die war Gold wert.

    Ich habe erst einmal im ctor von cImage den .data Zeiger mit NULL initialisiert, außerdem habe ich herausgefunden, dass es keine gute Idee ist, einer std::map einen Eintrag hinzuzufügen und bei jenem gleichzeitig auf eine Elementvariable zuzugreifen:

    mymap[NEWONE].member = "Wurstwasser";

    (Böse)

    Ich habe also ein Dummyimage erzeugt und dieses an das Mapobjekt zugewiesen:

    cImage tempImage;
	images[id] = tempImage;
        images[id].data = WHATEVER;

    (Gut)

    Wahrscheinlich gehts einfacher... aber es funktioniert. Noch. Glaub ich 🙂

    Nochmals danke für eure Hilfe,
    anytime

    Edit:

    Passiert es, dass cSystem-Objekte kopiert werden? Ohne korreten Copy-Konstruktor und Assignment-Operator passiert dabei natürlich Murks. Mach die beiden mal private und schau mal, wo es beim Kompilieren kracht.

    Nein, cSystem soll eigentlich nur einmal instanziiert werden.

    0
Anmelden zum Antworten