CODEWITHSUNDEEP
c++linuxmultithreadinglocking
bugs king
bugs king

Reputation: 564

ThreadSanitizer says my Atomic Inc/Dec has data races, false positive?

I wrote my atomic_inc for increment an integer using asm, it is actually used for referencing counting for shared objects. gcc 4.8.2 -fsanitize=thread reports data races and I finally found it was likely cause by my atomic_inc. I don't believe my code has a bug there to do with data races, is that a false positive by tsan?

static inline int atomic_add(volatile int *count, int add) {
    __asm__ __volatile__(
            "lock xadd %0, (%1);"
            : "=a"(add)
            : "r"(count), "a"(add)
            : "memory"
    );
    return add;
}

void MyClass::Ref() {
    // std::unique_lock<std::mutex> lock(s_ref);
    atomic_add(&_refs, 1);
}
void MyClass::Unref() {
    // std::unique_lock<std::mutex> lock(s_ref);
    int n = atomic_add(&_refs, -1) - 1;
    // lock.unlock();
    assert(n >= 0);
    if (n <= 0) {
        delete this;
    }
}

Upvotes: 2

Views: 1002

Answers (2)

Jesper Juhl
Jesper Juhl

Reputation: 31458

As others have already said, the tool cannot see inside your asm. But you shouldn't do that anyway. Just use std::atomic and be done with it - that's both thread safe and portable and the compiler knows how to optimize it - unlike your current code.

Upvotes: 1

Martin Bonner supports Monica
Martin Bonner supports Monica

Reputation: 29017

Part of your problem is that gcc doesn't look inside the asm.

The other part of your problem is that volatile doesn't make a variable thread-safe.

Given __asm__ means you are committed to gcc, why not use the gcc intrinsics? (They are documented and well tested, and gcc will understand their semantics.)

As to whether the warning is a false positive, I don't know. The safe thing to do is to assume the problem is genuine. It is really hard to see problems in multi-threaded code (even when you know they are there). (Once we ripped out a very clever piece of code using a published mutex algorithm that was failing and replaced it with a simple spin-lock. That fixed the failures, but we never could find why it failed.)

Upvotes: 2

Related Questions