why the program using atomic CAS cannot keep thread safety?

Question

int main(){

    atomic<bool> atomic_lock(false);
    std::atomic_flag lock_flag = ATOMIC_FLAG_INIT;
    int count = 0;


    auto f = [&](){
        bool flag = false;
        
        for( int i = 0; i< 10000000; ++i){
          while(!atomic_lock.compare_exchange_strong(flag, true)){}
          //while(lock_flag.test_and_set(std::memory_order_seq_cst));

          ++count;
          //lock_flag.clear(std::memory_order_seq_cst);
          atomic_lock.store(false, std::memory_order_seq_cst);
        }
    };

    thread t1(f);
    thread t2(f);
    t1.join();
    t2.join();

    cout<<count<<endl;

    return 0;
}

here is my program, I want to replace mutex with CAS, but output which is not 20000000 shows it is not a thread safety program, where is wrong? However, I replace atomic with atomic_flag show as above, output is right

Answer 1

You forgot to set flag = false before each CAS attempt to make sure you're only succeeding when you CAS from false to true.

Remember that on CAS failure, the "expected" (old value first arg) is updated with the current value of flag; that's why it takes it by reference (cppref). See also Understanding std::atomic::compare_exchange_weak() in C++11.

After every failure, your code is looping until it can CAS from whatever it saw last iteration. This succeeds very easily, and you will quickly have two threads inside the critical section at once, creating data-race UB (which causes a real problem on multi-core systems, and even on single-core systems if the increment doesn't compile to one instruction.)

---

It might have been a better design for C++ std::atomic:CAS to take that arg by pointer instead of non-const reference, like the non-member std::atomic_compare_exchange_weak(std::atomic<T>* obj, T* expected, T desired) which matches the C11 interface. Or maybe not, but it would have avoided this hidden pitfall.