do integer reads need to be critical section protected?

Question

I have come across C++03 some code that takes this form:

struct Foo {
    int a;
    int b;
    CRITICAL_SECTION cs;
}

// DoFoo::Foo foo_;

void DoFoo::Foolish()
{
    if( foo_.a == 4 )
    {
        PerformSomeTask();

        EnterCriticalSection(&foo_.cs);
        foo_.b = 7;
        LeaveCriticalSection(&foo_.cs);
    }
}

Does the read from foo_.a need to be protected? e.g.:

void DoFoo::Foolish()
{
    EnterCriticalSection(&foo_.cs);
    int a = foo_.a;
    LeaveCriticalSection(&foo_.cs);

    if( a == 4 )
    {
        PerformSomeTask();

        EnterCriticalSection(&foo_.cs);
        foo_.b = 7;
        LeaveCriticalSection(&foo_.cs);
    }
}

If so, why?

Please assume the integers are 32-bit aligned. The platform is ARM.

Answer 1

Technically yes, but no on many platforms. First, let us assume that int is 32 bits (which is pretty common, but not nearly universal).

It is possible that the two words (16 bit parts) of a 32 bit int will be read or written to separately. On some systems, they will be read separately if the int isn't aligned properly.

Imagine a system where you can only do 32-bit aligned 32 bit reads and writes (and 16-bit aligned 16 bit reads and writes), and an int that straddles such a boundary. Initially the int is zero (ie, 0x00000000)

One thread writes 0xBAADF00D to the int, the other reads it "at the same time".

The writing thread first writes 0xBAAD to the high word of the int. The reader thread then reads the entire int (both high and low) getting 0xBAAD0000 -- which is a state that the int was never put into on purpose!

The writer thread then writes the low word 0xF00D.

As noted, on some platforms all 32 bit reads/writes are atomic, so this isn't a concern. There are other concerns, however.

Most lock/unlock code includes instructions to the compiler to prevent reordering across the lock. Without that prevention of reordering, the compiler is free to reorder things so long as it behaves "as-if" in a single threaded context it would have worked that way. So if you read a then b in code, the compiler could read b before it reads a, so long as it doesn't see an in-thread opportunity for b to be modified in that interval.

So possibly the code you are reading is using these locks to make sure that the read of the variable happens in the order written in the code.

Other issues are raised in the comments below, but I don't feel competent to address them: cache issues, and visibility.

Answer 2

Looking at this it seems that arm has quite relaxed memory model so you need a form of memory barrier to ensure that writes in one thread are visible when you'd expect them in another thread. So what you are doing, or else using std::atomic seems likely necessary on your platform. Unless you take this into account you can see updates out of order in different threads which would break your example.