SDL 1.2 in C: A variable changes when it shouldn't

Question

Well, it shouldn't as far as I can tell. I'll provide context first. I'm wanting to define "Controls" as basically screen widgets:

typedef struct {
    struct Control** children;
    SDL_Surface* surface;
    struct Control* parent;
    char* type;
    int width;
    int height;
    int x;
    int y;
} Control;

The main screen is called a Window, and I made a special init function for it:

Control* createWindow() {
    Control window = {
        .type = "window",
        .surface = SDL_SetVideoMode(WIN_W, WIN_H, 24, SDL_SWSURFACE),
        .height = WIN_H,
        .width = WIN_W,
        .x = 0,
        .y = 0
    };
    return &window;
}

It has a child that's called a Panel, with its own initializer:

Control* createPanel(const char* filepath, Control* parent, int x, int y, int numberOfChildren) {
    Control panel = {
        .parent = parent,
        .children = (Control**)malloc(numberOfChildren * sizeof(Control*)),
        .type = "panel",
        .x = x,
        .y = y,
        .surface = SDL_LoadBMP(filepath)
    };
    return &panel;
}

And the beginning of the main function:

int main(int argc, char* args[]) {

    Control* window = NULL;
    Control* panel = NULL;

    SDL_Init(SDL_INIT_EVERYTHING);

    window = createWindow();
    panel = createPanel("BACKGROUND.bmp", window, 0, 0, 3);

Now, when reaching the createWindow() function, everything is fine and window is well defined. After the next line, the panel initialization, window gets mangled. I just can't figure out why.

I thought it might have been because I sent window to be assigned as panel's parent, so I tried without passing it and removing that assignment. No go, after createPanel() returns, it still messes up the fields of window in the main scope.

I've debugged this issue for a long time now and I just have no leads left. I'm pretty new to C and pointer anomalies can happen and I wouldn't know, so I'm really expecting this to be something totally trivial..

Thank you for your time.

I just found out about this today. Apparently it's a legitimate way to initialize a struct, and it works: http://stackoverflow.com/a/330867/2639558 — Adar Hefer, Oct 04 '14 at 19:39

dtech · Accepted Answer · 2014-10-05T08:27:44.777

Why not simply:

void createWindow(Control * c) {
    *c = {
        .type = "window",
        .surface = SDL_SetVideoMode(WIN_W, WIN_H, 24, SDL_SWSURFACE),
        .height = WIN_H,
        .width = WIN_W,
        .x = 0,
        .y = 0
    };
}

and then:

Control c;
createWindow(&c);

This way you allocate the space for the object in the call location and pass its address to the function to initialize that address, this is effectively a by-hand implementation of RVO, the compiler will most likely be smart enough and figure out the pointer is to a stack object and directly initialize the data in-place without calling the function.

Referencing to memory on the stack to use an object whose function has already returned is a very bad idea. That data might remain there "for a while", but when the stack reaches that depth again, the data will be overwritten and next time you will either get and/or make garbage. You can also dynamically allocated memory and return a pointer to it, WHILE remembering to deal with that data in the calling location or relying on some other management scheme, but that would be overkill with a tiny overhead that can be avoided in your case.

A global would be a rather clumsy solution to your problem, and entirely pointless. If you decide to have more than 1 window, are you going to edit the source and recompile to add another global each time you need one? Doesn't seem like a good idea.

EDIT: I didn't test that code before posting it, assuming it would work but it doesn't seem that designated initializers work with dereferenced pointers. Casting the initializer to the type works as you noted in the comments, and the code generated by the compiler should be identical to initializing "procedurally", i.e. c->type = "window" and so on which is what I would personally do.

Thank you so much for the suggestion. It does seem to make a lot more sense. — Adar Hefer, Oct 04 '14 at 20:29
Strange. The compiler says at the line in which the assignments happens (*c = {...) "Error: expected an expression". Why would that happen? — Adar Hefer, Oct 04 '14 at 20:40
Answering my own question: Had to cast to (Control) before the assignment. Makes sense :) Thank you so much for your help! — Adar Hefer, Oct 04 '14 at 20:51
BTW, as Joachim noted in the comment of his answer - you might just as well return the structure itself. The compiler will likely use RVO instead of actually copying the struct, so it will effectively do implicitly what the code in this answer does explicitly, e.g. use the storage allocated in the call location and store data directly there rather than creating another instance on the stack and copying values over. — dtech, Oct 05 '14 at 08:25

Some programmer dude · Answer 2 · 2014-10-04T20:20:34.470

2

You are returning a pointer to a local variable. You have to remember that (non-static) local variable goes out of scope and wont exist anymore when the function they are defined in returns.

Returning, and using, this pointer will lead to undefined behavior, which is what you're seeing.

My suggestion on how to fix the problem? Don't return a pointer, return a copy.

If you desperately need want to return a pointer, which you don't really need to do in that function, you must allocate it of the heap using malloc. Remember that you then have to free it.

edited Oct 04 '14 at 20:20

answered Oct 04 '14 at 19:59

Some programmer dude

400,186
35
402
621

I see. How would you copy the struct? I think a good solution would be to move window and panel to the global scope. Would you agree? – Adar Hefer Oct 04 '14 at 20:03
1

@AdarHefer Don't return a pointer, just return a `Control` structure. The compiler will handle the copying for you in the most efficient way it can. – Some programmer dude Oct 04 '14 at 20:17

score 1 · Answer 3 · edited May 23 '17 at 12:05

Let's take a look at your CreateWindow function.

Control* createWindow() {
    Control window = {
        .type = "window",
        .surface = SDL_SetVideoMode(WIN_W, WIN_H, 24, SDL_SWSURFACE),
        .height = WIN_H,
        .width = WIN_W,
        .x = 0,
        .y = 0
    };
    return &window;
}

You are creating a new Control structure on the stack, and later returning the address. Since the structure is not on the heap, the data in the place is probably modified. You want save the Window on the heap.

Take a look at this answer: Creating a struct on the heap?

SDL 1.2 in C: A variable changes when it shouldn't

3 Answers3