ThreadSanitizer says my spin_lock has data race, but how?

Question

My spin lock implementation is shown as below, I don't think it can cause any data race, but when I test my code with -fsanitize=thread, it reports spin_unlock has write data races. How could this happen? Is it a false positive report?

#define barrier() asm volatile("": : :"memory")
#define cpu_relax() asm volatile("pause\n": : :"memory")

static inline void spin_lock(volatile int *lock) {
    while (1)
    {
        int i = 0;
        if (!atomic_swap(lock, EBUSY)) return;
        while (*lock) {
            i++;
            if (i == 4000) {
                i = 0;
                thread_yield();
            }
            cpu_relax();
        }
    }
}

static inline void spin_unlock(volatile int *lock) {
    barrier();
    *lock = 0;
}

the atomic_swap is a function as:

static inline int atomic_swap(volatile void *lockword, int value) {
    unsigned long tmp;
    int result;
    __asm__ __volatile__ ("dmb" : : : "memory");
    __asm__ __volatile__("@ atomic_swap\n"
    "1: ldrex   %0, [%2]\n"
    "   strex   %1, %3, [%2]\n"
    "   teq %1, #0\n"
    "   bne 1b"
    : "=&r" (result), "=&r" (tmp)
    : "r" (lockword), "Ir" (value)
    : "cc");

    __asm__ __volatile__ ("dmb" : : : "memory");
    return result;
}

I know nothing about ARM, but I do know a little about race conditions. I suspect your problem is that you don't check the value returned in ldrex. In your loop you check `lock`, but it's possible that between the time thread1 sees it go to 0 and the time ldrex returns a value, thread2 could have swooped in and changed it. So 2 thread will both have written EBUSY. Probably not what you intended. — David Wohlferd, Mar 21 '16 at 03:43
Also, you are changing a value in memory (lockword) without having it listed as an output. Consider changing `[%2]` to `%2`, moving it from an input to an output, and changing its constraint to `+m`. — David Wohlferd, Mar 21 '16 at 05:13
I don't believe 'intex' is a 'platform.' Intex sells [phones](www.intex.in/mobiles/smart-phones/) (ie aqua smartphones) -> their phones contain processors (ie Snapdragon 615). The Snapdragon processor is an ARM platform. `ldrex`, `strex` & `dmb` are all instructions for the ARM platform. But even if I have that wrong, it seems clear you are loading a value from memory (`ldrex %0, [%2]`), but not checking to see what that value is. You are just 'hoping' that between the time you did the `while (*lock)` and the time you did the `ldrex` that nothing has changed. That's a race condition. — David Wohlferd, Mar 21 '16 at 21:23

score 1 · Answer 1 · answered Mar 14 '18 at 16:54

ThreadSanitizer doesn't know about the semantics of your home grown atomic_swap() or barrier() functions. From the ThreadSanitizer FAQ:

Q: What synchronization primitives are supported? TSan supports pthread synchronization primitives, built-in compiler atomic operations (sync/atomic), C++ operations are supported with llvm libc++ (not very throughly [sic] tested, though).

So it doesn't know what atomic_swap() should do and on top of that ThreadSanitizer currently doesn't support memory fences.

ThreadSanitizer says my spin_lock has data race, but how?

1 Answers1