Thread: Access Violation / Seg Fault

You shouldn't manage the list yourself unless you've proven you need to do that. It would be far better and more robust to use e.g. std::list.

How do we solve a seg fault?
Well, there are generally several ways.
First, you can try: take the address of the pointer. Use a proper debugger (such as VC), and add a breakpoint when the data in the memory area where the pointer resides changes. It should break when created, when assigned and (IMPORTANT!) when DELETED. Go from there and try to figure WHY it was deleted when it shouldn't.

This is probably the biggest type of seg faults in a list.
Another is data corruption. You can tell them apart if you have a good debugger (like VC), since it might fill deleted data with a known pattern (0xFEFEFEFE for VC). If it isn't deleted, then it's probably corrupted or something else.

And if dwks's hypothesis is correct - you can know by ALWAYS setting ALL pointers to NULL before ANY use and ANY assignment.

Looking at your code:

Code:

         if(mortals->next != NULL) // ... check that there are actually mortals in play.
         {
                   while(angels->next != NULL)
                   {

                         angels->move();

                         if(angels->readxc() == 0) angels->editcoords(1,0); // fix coord == 0
                         if(angels->readyc() == 0) angels->editcoords(0,1); // fix coord == 0
                         if(angels->readxc() > BOARD_X) angels->editcoords(-1,0);
                         if(angels->readyc() > BOARD_Y) angels->editcoords(0,-1);


                         gotoxy(angels->readxc(), angels->readyc()); // go to new coords

                         cout << "O";

                         angels = angels->next; // next node

                    }

            }

What if mortals or angels is NULL to begin with? Of course, that shouldn't cause it to segfault at the "next node" line. But it's still something to consider.

Anyway . . . I think this code is a little mixed-up.

Code:

    for(int i = 0; i < NUMBER_MORTALS; i++)
    {
            mortals->create(rand() &#37; PIECE_SPREAD + 1, rand() % PIECE_SPREAD + 1); // create at a random position
            mortals->next = new Mortal; // Current mortal's next pointer now points to a new Mortal
            mortals->next->prev = mortals; // The new node's previous pointer points to the current node
 
            // DEBUG
            /*
            cout << "Mortal created at " << mortals->readxc() << ", " << mortals->readyc() << ".\n"; // DEBUG LINE
            Sleep(100); // DEBUG LINE
            */
 
            mortals = mortals->next; // Move to the newly-created Mortal node
            Sleep(1);
    }

The call to mortals->create() initializes the mortals object -- the already-existing mortals object, mind. Then you go and add a new object, which is not initialized -- and guess what? You never set its next pointer, so it's probably set to something strange.

I think you meant to call mortals->next->create().

On a side note, create() would work very well as a constructor.

[edit] Okay, I guess me finding (possible?) segfaults in your code isn't going to help you find them, either.

How did I find it? I figured that angels->next must be invalid. So the first place to check was where angels->next was initialized. That led me to the block of code I posted.

I guess the rest was being suspicious about whether angels->next->next really was initialized.

[Assuming my analysis is right, of course, and that it actually isn't.]
[/edit]

Actually, the problem is that when you create angels (and mortals, you just don't get far enough to encounter that problem), the next-pointer of the NEW object (after you do angels = angels->next at the end of the loop is not set to NULL, so you never know that you reach the end of the list.

--
Mats

Following up on master5001's second thought:

Code:

    Angel *angelroot; // Root note
    Angel *angels; // Conductor node
    angelroot = new Angel; // Pointer *angelroot points to an Angel object
    angelroot->next = NULL; // The next pointer is NULL.
    angelroot->prev = NULL;
 
    // Set the conductor pointer to the root of the list
    mortals = mortalroot;
    angels = angelroot;

It looks okay, although that sort of code should really be in the constructor for Angel.

I was going to mention all of those public variables you have in your classes -- that's generally not a good idea. The class should handle its internal variables itself, most of the time.

Make the constructor set all pointers to NULL, as well. Good practice! Less bugs and easier to track down problems!

And whilst we are on the bandwagon of "telling you what to improve":
Make a list-class, then either include that inside your Angel/Mortal classes, or inherit from it.

Make the list class have all the necessary functions to add, remove and find objects within the class. That is a MUCH better way to practice than to write the same bit of code several times over.

Also, when learning new things, it's a good idea to try on ONE thing [e.g. make sure you can create a bunch of angels and find them back again, before you try making mortals - or the other way around].

--
Mats

Code:

                            mortals->prev->next = mortals->next; // The previous node's next pointer points to the next node
                            mortals->~Mortal(); // Call Destructor

1. Don't ever call the destructor manually! You probably want to make a copy of mortals->next, and then delete mortals [or delete a copy of mortals after you do mortals = mortals->next - whichever you prefer].

2. You are trying to set the next pointer of a prev-pointer that is NULL. Check for mortals->prev == NULL and skip this step [you probably should move mortalsroot instead in that case].

--
Mats

Code:

mortals->~Mortal(); // Call Destructor

Never, ever call a destructor directly. Seriously. If you dynamically allocated memory (which you did), just delete it.

Code:

delete mortals;

[edit] And then your mortals variable will be invalid, but matsp has already pointed that out. [/edit]