Access Violation / Seg Fault

This is a discussion on Access Violation / Seg Fault within the C++ Programming forums, part of the General Programming Boards category; Hello all, I'm currently coding a simulation for a theoretical game called Angels and Mortals. As a simulation, it plays ...

  1. #1
    Confused
    Join Date
    Nov 2002
    Location
    Warwick, UK
    Posts
    209

    Access Violation / Seg Fault

    Hello all,

    I'm currently coding a simulation for a theoretical game called Angels and Mortals. As a simulation, it plays itself out without user input, and depending on different starting conditions, the results will be different.

    So far, my backend is nearly complete, and will eventually feed into an OpenGL frontend where I'll be able to see the "game board" to see what's going on. Right now, output is to the console :P

    The backend is what's causing me the problem so far. My game "pieces" are coded as class objects, and represented in a doubly-linked list. The problem is, I seem to have developed an access violation, but I have no idea why - this exact line was working previously and didn't throw up any seg faults as it does now. Essentially - I have no idea what's going wrong, and I'm pretty confused as to how to fix this.

    Here's the line in question that my debugger ( CodeBlocks ) tells me is causing the problem :

    Code:
    angels = angels->next;
    To put that into context, here's the relevant class :

    Code:
    class Angel
    {
          private:
                  int xcoord, ycoord;
          public:
                 void create(int, int);
                 void move();
                 void setcoords(int, int);
                 void editcoords(int, int);
                 int readxc();
                 int readyc();
                 Angel *next;
                 Angel *prev;
                 ~Angel();
    };
    And the object angels is just an object of class Angel.


    To aid in further contexting, here's the loop in which I supposedly get a seg fault :

    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
    
                        }
    
                }


    For those who are either very brave or very kind, or have too much time on their hands, here's a pastebin of the entire application :

    Main .cpp - http://pastebin.com/m58bfdeb0 - culprit on line 184
    Angel.h - http://pastebin.com/m166973f4
    Mortal.h - http://pastebin.com/m50f40c64
    Settings.h - http://pastebin.com/m768f6067


    My coding environment is Dev-CPP on Windows Vista.

    Thanks for any help,
    Quentin

  2. #2
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,892
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  3. #3
    Confused
    Join Date
    Nov 2002
    Location
    Warwick, UK
    Posts
    209
    Using std::list isn't going to teach me to build custom lists, or how to handle seg faults :P

  4. #4
    Banned master5001's Avatar
    Join Date
    Aug 2001
    Location
    Visalia, CA, USA
    Posts
    3,685
    A seg fault is the equivilent of driving up on the curb when you are learning to drive. You don't practice how to drive on the curb more gracefully. You gracefully avoid driving on the curb.

  5. #5
    Confused
    Join Date
    Nov 2002
    Location
    Warwick, UK
    Posts
    209
    I know a seg fault isn't something I want to do. Yet, I have one, and need to find it and fix it. I'll most likely have others in the future at some point, so being able to do so is essential. Such answers, master5001, aren't helping in the slightest, I'm sorry to say.

    So yes, the curb is a problem. But I'm on the curb - teach me how to get off it.

  6. #6
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,167
    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.
    Last edited by Elysia; 09-05-2008 at 02:09 PM.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  7. #7
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,045
    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]
    Last edited by dwks; 09-05-2008 at 02:08 PM.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  8. #8
    Banned master5001's Avatar
    Join Date
    Aug 2001
    Location
    Visalia, CA, USA
    Posts
    3,685
    I need more code than this to give you a better explanation of what is going wrong. Or at the very least I need to confirm my hypothesis. You are probably not correctly setting next, or perhaps you are not correctly allocating the initial node in your list.

    [edit]Sorry, I just noticed the links... I am a bit sleep deprived today.[/edit]

  9. #9
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  10. #10
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,045
    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.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  11. #11
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,167
    Make the constructor set all pointers to NULL, as well. Good practice! Less bugs and easier to track down problems!
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  12. #12
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  13. #13
    Confused
    Join Date
    Nov 2002
    Location
    Warwick, UK
    Posts
    209
    Thanks to everyone for their help so far !

    matsp was correct, it was a non-NULL pointer error. I've now added
    Code:
    next = NULL;
    prev = NULL;
    to my constructors, and that's solved the problem - only to reveal a further segfault, a little further down the line.


    Line is now 292, giving the same error. I'm trying to get my head around which part I messed up this time :P

    Code:
                        else // If it dies this turn, create link between nodes and destroy current node
                        {
                                mortals->next->prev = mortals->prev; // The next node's previous pointer points to the previous node
    
    // FOLLOWING LINE CAUSES SEG FAULT
                                mortals->prev->next = mortals->next; // The previous node's next pointer points to the next node
    
                                mortals->~Mortal(); // Call Destructor
                                deaths++;
     
                        }

    If anyone sees something drastically obvious, please feel free to point it out. If not, I might work it out myself in a few



    [EDIT]
    Would it be something to do with mortals->next not having a value ( non-NULL, again ) ? *Checks*
    [/EDIT]

  14. #14
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  15. #15
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,045
    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]
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

Page 1 of 3 123 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. access violation segmentation fault raised in your program
    By dinamit875 in forum C++ Programming
    Replies: 8
    Last Post: 04-23-2009, 11:44 AM
  2. Getting Access Violation (Segmentation Fault)
    By Brownie in forum C++ Programming
    Replies: 2
    Last Post: 09-26-2008, 11:43 AM
  3. Replies: 3
    Last Post: 07-17-2008, 08:45 AM
  4. access violation (segmentation fault)
    By MarlonDean in forum C++ Programming
    Replies: 7
    Last Post: 05-16-2008, 05:02 AM
  5. Locating A Segmentation Fault
    By Stack Overflow in forum C Programming
    Replies: 12
    Last Post: 12-14-2004, 12:33 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21