Potential bug with compiler optimizations and initialization

[mjbshaw]

sfml-main, main.cpp has:

    while (!states->initialized)
    {
        states->mutex.unlock();
        sf::sleep(sf::milliseconds(20));
        states->mutex.lock();
    }

The compiler may optimize away the retrieval of the value of states->initialized. The initialized should be volatile to force a memory access.

Additionally, mutexes aren't necessary for simple flag checking like this, though this isn't related to the potential bug.

[mjbshaw]

Similar issue with states->terminated in:

    while (!states->terminated)
    {
        states->mutex.unlock();
        sf::sleep(sf::milliseconds(20));
        states->mutex.lock();
    }

(same file)

[intjelic]

Interesting.

I just read about the volatile keyword and I'm still confuse. So basically all shared variables listed in ActivityStates structure should be volatile as they "can't change on their own" and as most of the time we're locking them with mutexes until their state changes.

If you are aware of what you're doing, go ahead and make the changes :)

[intjelic]

I'll reread the code later and try to figure out by myself, I'm just too busy right now :p

[mjbshaw]

To simplify it, the compiler can detect that states->initialized (or terminated) is never modified during the loop, and thus is allowed to optimize the code such that the value of states->initialized is only retrieved once (because it will never change, according to the compiler's perspective). The volatile keyword exists to "disable" this optimization and force it to re-retrieve the value.

I just learned that pthread locks act as memory barriers and thus memory/thread caches are synced with the pthread locking functions. From my reading this doesn't prevent the value from being cached in a register, though. But the sequence point from the function calls might force the flag to be re-retrieved, even if it's in a register. So this code might be forced to execute as expected, though I'd say it's "lucky."

Anyway, on second thought, volatile is still not the "right" solution. The "right" solution would be using pthread_cond_wait(), like android_app_free() does in native_app_glue.c, though I'm not sure how to use pthread functions with SFML's generic threads/mutexes.