STL List copies a struct, but the copied values are offset by two memory addresses.

Question

I'm compiling using Code::Blocks on Windows 7 using the MinGW compiler (which I can only assume is the latest version; both Code::Blocks and MinGW were installed this past week). My issue crops up under a particular circumstance, and my attempts to write a simpler script that demonstrates the problem have failed (which implies that there is something wrong with my structure). Also, my apologies for how long this post is.

Currently, I'm rolling with one class, FXSDL, which will act as an SDL wrapper:

class FXSDL
{
    public:
        FXSDL();
        virtual ~FXSDL();
        int Initialize();
        int Render();

        FXID CreateCharacter(FXID hRefID, string fpImage, int wpxTile, int hpxTile, map<int, vector<int> > htAnims);
        int SetAnim(FXID hRefID, FXID hAnimID);
        FXID hPlayer;
    protected:
    private:
        list<FXSurface> m_lstFXObjects;
        list<FXSurface>::iterator m_liFirst;
        SDL_Surface* m_lpsfSDLScreen;
        Uint32 m_tmOld;
        Uint32 m_tmFrame;
};

The value type of my list is:

struct FXSurface
{
    FXID hRefID;
    int wpxTile;
    int hpxTile;
    int wpxTotal;
    int hpxTotal;
    int cntTiles;

    map<int, vector<int> > htAnims; // All animations
    map<int, vector<int> >::iterator vCurr; // Currently active animation
    vector<int>::iterator fiCurr; // Currently active frame
    SDL_Surface* lpsfSDL;
    SDL_Rect* lprcTiles;    // Predefined frame positions
    string* fpImage;
};

I've implemented very simple initialize and render function. The CreateCharacter function takes a few parameters, the most important of which is htAnims, a map of integer vectors (idea being: I define numeric ids with easy-to-remember representations, such as FXA_IDLE or FXA_WALK, as the key, and the series of number values representing frames for the animation as a vector). This could be fairly easily implemented as a multidimensional integer array, but animations are variable in length and I want to be able to add new anims (or redefine existing ones) without having to recast an array.

The CreateCharacter function is simple. It creates a new FXSurface, populates it with the required data, and pushes the new FXSurface onto the list:

FXID FXSDL::CreateCharacter(FXID hRefID, string fpImage, int wpxTile, int hpxTile, map<int, vector<int> > htAnims)
{
    //list<FXSurface>::iterator lpsfTemp;
    FXSurface lpsfTemp;
    list<FXSurface>::iterator lpsfPos;
    SDL_Rect* lprcCurr = NULL;
    int cntTileW = 0;
    int cntTileH = 0;
    int cntCurr = 0;

    // Start off by initializing our container struct
    //lpsfTemp = new FXSurface();
    lpsfTemp.lpsfSDL = IMG_Load(fpImage.c_str()); // Try to load the requested image
    if(lpsfTemp.lpsfSDL != NULL) // If we didn't fail to
    {
        // Assign some variables for tracking
        lpsfTemp.hRefID = hRefID;
        lpsfTemp.fpImage = &fpImage;
        lpsfTemp.wpxTotal = lpsfTemp.lpsfSDL->w;
        lpsfTemp.hpxTotal = lpsfTemp.lpsfSDL->h;

        // If a tile width was specified, use it
        if(wpxTile != 0)
        {
            lpsfTemp.wpxTile = wpxTile;
            lpsfTemp.hpxTile = hpxTile;
        } // Otherwise, assume one tile
        else
        {
            lpsfTemp.wpxTile = lpsfTemp.wpxTotal;
            lpsfTemp.hpxTile = lpsfTemp.hpxTotal;
        }

        // Determine the tiles per row and column for later
        cntTileW = lpsfTemp.wpxTotal / lpsfTemp.wpxTile;
        cntTileH = lpsfTemp.hpxTotal / lpsfTemp.hpxTile;

        // And the total number of tiles
        lpsfTemp.cntTiles = cntTileW * cntTileH;
        lpsfTemp.lprcTiles = new SDL_Rect[cntTileW*cntTileH];

        // So we don't calculate this every time, determine each frame's coordinates and store them
        for(int h = 0; h < cntTileH; h++)
        {
            for(int w = 0; w < cntTileW; w++)
            {
                cntCurr = (h*cntTileW)+w;
                lprcCurr = new SDL_Rect;
                lprcCurr->w = lpsfTemp.wpxTile;
                lprcCurr->h = lpsfTemp.hpxTile;
                lprcCurr->x = w*lpsfTemp.wpxTile;
                lprcCurr->y = h*lpsfTemp.hpxTile;

                lpsfTemp.lprcTiles[cntCurr] = *lprcCurr;
                lprcCurr = NULL;
            }
        }

        // Now acquire our list of animations and set the default
        //lpsfTemp.htAnims = new map<int, vector<int> >(*htAnims);
        lpsfTemp.htAnims = htAnims;
        lpsfTemp.vCurr = lpsfTemp.htAnims.find(FXA_WALK_EAST);
        lpsfTemp.fiCurr = lpsfTemp.vCurr->second.begin();

        this->m_lstFXObjects.push_back(lpsfTemp);
    }
    else
    {
        hRefID = 0;
    }

    return hRefID;
}

It is precisely as the object is pushed that the error occurs. I've stepped through the code numerous times. Initially, I was only able to tell that my iterators were unable to dereference to the FXSurface object. After using watches to identify the exact memory address that the iterator and list objects pointed to, and dereferencing the address, I noticed the reason for my segfaults: all the values which I put into the original FXSurface were pushed down two memory blocks when the list object copied it!

My process for doing this is simple. I set up a breakpoint at the return statement for CreateCharacter, which gives me a view of lpsfTemp (the FXSurface I later add to the list) and m_lstFXObjects (the list I add it to). I scroll through the members of m_lstFXObjects, which brings me to _M_node, which contains the memory address of the only object I have added so far. I add a watch to this address in the form of (FXSurface)-hex address here-

First, find the address:

(There should be a picture here showing the highlighted _M_node attribute containing the list item's address, but I can't post pictures, and I can only post one URL. The second one is by far more important. It's located at h t t p : / / www.fauxsoup.net / so / address.jpg)

Next, we cast and deference the address. This image shows both lpsfTemp and the copy in m_lstFXObjects; notice the discrepancy?

http://www.fauxsoup.net/so/dereferenced.jpg - See? All the values are in the correct order, just offset by two listings

I had initially been storing fpImages as a char*, so I thought that may have been throwing things off, but now it's just a pointer and the problem persists. Perhaps this is due to the map<int, vector<int> > I store?

I don't know. My brain is fried. Any help would be appreciated!

Answer 1

At a glance, I'd say you're double-deleting lpsfSDL and/or lprcTiles. When you have raw pointers in your structure, you need to follow the rule-of-three (implement copy constructor, assignment operator, and destructor) to properly manage the memory.

Answer 2

These lines look wrong to me:

            lprcCurr = new SDL_Rect;
            lprcCurr->w = lpsfTemp.wpxTile;
            lprcCurr->h = lpsfTemp.hpxTile;
            lprcCurr->x = w*lpsfTemp.wpxTile;
            lprcCurr->y = h*lpsfTemp.hpxTile;

            lpsfTemp.lprcTiles[cntCurr] = *lprcCurr;
            lprcCurr = NULL;

lpsfTemp.lprcTiles is a SDL_Rect*. lprcTemp.lprcTiles[cntCurr] is a SDL_Rect. You should be writing this, IMHO:

            SDL_Rect tmpRect;
            tmpRect.w = lpsfTemp.wpxTile;
            tmpRect.h = lpsfTemp.hpxTile;
            tmpRect.x = w*lpsfTemp.wpxTile;
            tmpRect.y = h*lpsfTemp.hpxTile;

            lpsfTemp.lprcTiles[cntCurr] = tmpRect;

Dump the lprcCurr entirely.

Now this code:

    lpsfTemp.vCurr = lpsfTemp.htAnims.find(FXA_WALK_EAST);
    lpsfTemp.fiCurr = lpsfTemp.vCurr->second.begin();

This is bad. These iterators are invalid as soon as the push_back completes. That push_back is making a copy of lpsfTemp. The map and vector members are going to copy themselves and those iterators will copy themselves but they will be pointing to lpsfTemp's members which are going to be destroyed as soon as CreateCharacter exits.

One way to fix that would be to push_back a FXSurface object at the beginning, use back() to get its reference and operate on that instead of lpsfTemp. Then the iterators would stay consistent and they should stay consistent since you are using a list which does not copy its objects around. If you were using a vector or deque or anything other than a list you would need to manage all those pointers and iterators in the copy constructor and assignment operator.

Another thing: Double and triple check your array bounds when you access that lprcTiles array. Any mistake there and you could be scribbling over who knows what.

I don't know if any of that will help you.