Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tsan] False warning when using std::condition_variable #1259

Closed
kamshi opened this issue Jun 4, 2020 · 8 comments
Closed

[tsan] False warning when using std::condition_variable #1259

kamshi opened this issue Jun 4, 2020 · 8 comments
Assignees

Comments

@kamshi
Copy link

kamshi commented Jun 4, 2020

When compiling the following code with clang, utilizing thread sanitizer, I get false warnings.

#include <cassert>
#include <chrono>
#include <condition_variable>
#include <iostream>
#include <mutex>
#include <thread>


class WaitSignalLock {

    private:
        std::condition_variable conditionVariable;
        std::mutex mtx;
        bool condition = false;

    public:
        bool wait(const std::chrono::milliseconds& timeout);
        void signal();

    private:
        bool conditionMet() const;
        bool waitForConditionVariableSignal(std::unique_lock<std::mutex>& lck, const std::chrono::milliseconds& timeout);

};


bool WaitSignalLock::wait(const std::chrono::milliseconds& timeout) {
    std::unique_lock<std::mutex> lck(this->mtx);
    const auto result = waitForConditionVariableSignal(lck, timeout);
    condition = false;
    return result;
}

void WaitSignalLock::signal() {
    // the variable used for checking the condition has to be modified under a lock (even if the variable is atomic),
    // and the condition variable should not be notified under a lock
    {
        std::lock_guard<std::mutex> lck(this->mtx);
        condition = true;
    }
    conditionVariable.notify_one();
}

bool WaitSignalLock::conditionMet() const {
    // do not lock, method will be called under the lock already
    return condition;
}

bool WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>& lck, const std::chrono::milliseconds& timeout) {
    assert(lck.owns_lock());
    if(this->conditionVariable.wait_for(lck, timeout, [this]() {
        return this->conditionMet();
    })) {
        return true;
    }
    else {
        return false;
    }
}


int main(int, char**) {
    WaitSignalLock waitSignalLock;

    std::thread sendSignal([&waitSignalLock]() {
        std::this_thread::sleep_for(std::chrono::milliseconds(500));
        waitSignalLock.signal();
    });

    const auto result = waitSignalLock.wait(std::chrono::seconds(1));
    sendSignal.join();
    return !result;
}

Generated warnings when running the program:

WARNING: ThreadSanitizer: double lock of a mutex (pid=16421)
    #0 pthread_mutex_lock <null> (a.out+0x7e358)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/x86_64-pc-linux-gnu/bits/gthr-default.h:749:12 (a.out+0xd39a6)
    #2 std::mutex::lock() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/std_mutex.h:100:17 (a.out+0xd45e9)
    #3 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/std_mutex.h:159:19 (a.out+0xd4148)
    #4 WaitSignalLock::signal() /home/jae/projects/condition_variable/main.cpp:38:37 (a.out+0xd367e)
    #5 main::$_1::operator()() const /home/jae/projects/condition_variable/main.cpp:67:24 (a.out+0xd3f2f)
    #6 void std::__invoke_impl<void, main::$_1>(std::__invoke_other, main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:60:14 (a.out+0xd3eb1)
    #7 std::__invoke_result<main::$_1>::type std::__invoke<main::$_1>(main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:95:14 (a.out+0xd3e01)
    #8 void std::thread::_Invoker<std::tuple<main::$_1> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:264:13 (a.out+0xd3db9)
    #9 std::thread::_Invoker<std::tuple<main::$_1> >::operator()() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:271:11 (a.out+0xd3d69)
    #10 std::thread::_State_impl<std::thread::_Invoker<std::tuple<main::$_1> > >::_M_run() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:215:13 (a.out+0xd3c9d)
    #11 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80:18 (libstdc++.so.6+0xcfb73)

  Location is stack of main thread.

  Location is global '??' at 0x7ffd19818000 ([stack]+0x00000001ec48)

  Mutex M12 (0x7ffd19836c48) created at:
    #0 pthread_mutex_lock <null> (a.out+0x7e358)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/x86_64-pc-linux-gnu/bits/gthr-default.h:749:12 (a.out+0xd39a6)
    #2 std::mutex::lock() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/std_mutex.h:100:17 (a.out+0xd45e9)
    #3 std::unique_lock<std::mutex>::lock() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/unique_lock.h:138:17 (a.out+0xd46df)
    #4 std::unique_lock<std::mutex>::unique_lock(std::mutex&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/unique_lock.h:68:2 (a.out+0xd40b3)
    #5 WaitSignalLock::wait(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) /home/jae/projects/condition_variable/main.cpp:28:34 (a.out+0xd3563)
    #6 main /home/jae/projects/condition_variable/main.cpp:70:40 (a.out+0xd3860)

SUMMARY: ThreadSanitizer: double lock of a mutex (/home/jae/projects/condition_variable/a.out+0x7e358) in pthread_mutex_lock
==================
==================
WARNING: ThreadSanitizer: data race (pid=16421)
  Write of size 1 at 0x7ffd19836c70 by thread T1 (mutexes: write M12):
    #0 WaitSignalLock::signal() /home/jae/projects/condition_variable/main.cpp:39:19 (a.out+0xd3687)
    #1 main::$_1::operator()() const /home/jae/projects/condition_variable/main.cpp:67:24 (a.out+0xd3f2f)
    #2 void std::__invoke_impl<void, main::$_1>(std::__invoke_other, main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:60:14 (a.out+0xd3eb1)
    #3 std::__invoke_result<main::$_1>::type std::__invoke<main::$_1>(main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:95:14 (a.out+0xd3e01)
    #4 void std::thread::_Invoker<std::tuple<main::$_1> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:264:13 (a.out+0xd3db9)
    #5 std::thread::_Invoker<std::tuple<main::$_1> >::operator()() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:271:11 (a.out+0xd3d69)
    #6 std::thread::_State_impl<std::thread::_Invoker<std::tuple<main::$_1> > >::_M_run() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:215:13 (a.out+0xd3c9d)
    #7 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80:18 (libstdc++.so.6+0xcfb73)

  Previous read of size 1 at 0x7ffd19836c70 by main thread (mutexes: write M12):
    #0 WaitSignalLock::conditionMet() const /home/jae/projects/condition_variable/main.cpp:46:12 (a.out+0xd36ea)
    #1 WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&)::$_0::operator()() const /home/jae/projects/condition_variable/main.cpp:52:18 (a.out+0xd3af1)
    #2 bool std::condition_variable::wait_until<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> >, WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&)::$_0>(std::unique_lock<std::mutex>&, std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > > const&, WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&)::$_0) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/condition_variable:157:10 (a.out+0xd3a37)
    #3 bool std::condition_variable::wait_for<long, std::ratio<1l, 1000l>, WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&)::$_0>(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&, WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&)::$_0) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/condition_variable:185:9 (a.out+0xd378e)
    #4 WaitSignalLock::waitForConditionVariableSignal(std::unique_lock<std::mutex>&, std::chrono::duration<long, std::ratio<1l, 1000l> > const&) /home/jae/projects/condition_variable/main.cpp:51:32 (a.out+0xd3608)
    #5 WaitSignalLock::wait(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) /home/jae/projects/condition_variable/main.cpp:29:25 (a.out+0xd3572)
    #6 main /home/jae/projects/condition_variable/main.cpp:70:40 (a.out+0xd3860)

  As if synchronized via sleep:
    #0 nanosleep <null> (a.out+0x6aadc)
    #1 void std::this_thread::sleep_for<long, std::ratio<1l, 1000l> >(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:405:9 (a.out+0xd556a)
    #2 main::$_1::operator()() const /home/jae/projects/condition_variable/main.cpp:66:9 (a.out+0xd3f1f)
    #3 void std::__invoke_impl<void, main::$_1>(std::__invoke_other, main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:60:14 (a.out+0xd3eb1)
    #4 std::__invoke_result<main::$_1>::type std::__invoke<main::$_1>(main::$_1&&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/invoke.h:95:14 (a.out+0xd3e01)
    #5 void std::thread::_Invoker<std::tuple<main::$_1> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:264:13 (a.out+0xd3db9)
    #6 std::thread::_Invoker<std::tuple<main::$_1> >::operator()() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:271:11 (a.out+0xd3d69)
    #7 std::thread::_State_impl<std::thread::_Invoker<std::tuple<main::$_1> > >::_M_run() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/thread:215:13 (a.out+0xd3c9d)
    #8 execute_native_thread_routine /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80:18 (libstdc++.so.6+0xcfb73)

  Location is stack of main thread.

  Location is global '??' at 0x7ffd19818000 ([stack]+0x00000001ec70)

  Mutex M12 (0x7ffd19836c48) created at:
    #0 pthread_mutex_lock <null> (a.out+0x7e358)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/x86_64-pc-linux-gnu/bits/gthr-default.h:749:12 (a.out+0xd39a6)
    #2 std::mutex::lock() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/std_mutex.h:100:17 (a.out+0xd45e9)
    #3 std::unique_lock<std::mutex>::lock() /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/unique_lock.h:138:17 (a.out+0xd46df)
    #4 std::unique_lock<std::mutex>::unique_lock(std::mutex&) /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/10.1.0/../../../../include/c++/10.1.0/bits/unique_lock.h:68:2 (a.out+0xd40b3)
    #5 WaitSignalLock::wait(std::chrono::duration<long, std::ratio<1l, 1000l> > const&) /home/jae/projects/condition_variable/main.cpp:28:34 (a.out+0xd3563)
    #6 main /home/jae/projects/condition_variable/main.cpp:70:40 (a.out+0xd3860)

  Thread T1 (tid=16423, running) created by main thread at:
    #0 pthread_create <null> (a.out+0x8dbce)
    #1 __gthread_create /build/gcc/src/gcc-build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:663:35 (libstdc++.so.6+0xcfe49)
    #2 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:135:37 (libstdc++.so.6+0xcfe49)
    #3 main /home/jae/projects/condition_variable/main.cpp:65:17 (a.out+0xd3816)

SUMMARY: ThreadSanitizer: data race /home/jae/projects/condition_variable/main.cpp:39:19 in WaitSignalLock::signal()
==================
ThreadSanitizer: reported 2 warnings

I am building the code with the following command:

clang++ -O1 -g -fsanitize=thread -fno-omit-frame-pointer -lpthread main.cpp

The version of my compiler is:

$ clang --version
clang version 10.0.0 
Target: x86_64-pc-linux-gnu
Thread model: posix

tsan library info:

/usr/lib/libtsan.so.0.0.0: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), statically linked, BuildID[sha1]=63b425a5e70aa72476a97ad9f925301c52e71d31, with debug_info, not stripped

I also posted a question regarding this behavior on stackoverflow: https://stackoverflow.com/questions/62188442/clang-thread-sanitizer-reports-issues-when-using-conditon-variables

Apparently, the warnings are only reported when using clang-10.0.0, they are not reported when using older compiler versions. I could reproduce this behavior on two separate machines running Arch Linux with the latest shipped clang version.

@jeremiahar
Copy link

I ran into this issue in spdlog while upgrading a toolchain. I am also able to reproduce this issue on Arch Linux. Compiling with -stdlib=libc++ makes the issue disappear. After a closer look, it seems there were some changes in libstdc++ which cause wait_until to be compiled with pthread_cond_clockwait (new in glibc 2.30).

gcc-mirror/gcc@ad4d1d2

The pthread_cond_clockwait function is available in glibc since the 2.30
release. If this function is available in the C library it can be used
to fix PR libstdc++/41861 by supporting std::chrono::steady_clock
properly with std::condition_variable.  
  
This means that code using std::condition_variable::wait_for or
std::condition_variable::wait_until with std::chrono::steady_clock is no
longer subject to timing out early or potentially waiting for much
longer if the system clock is warped at an inopportune moment.  
  
If pthread_cond_clockwait is available then std::chrono::steady_clock is
deemed to be the "best" clock available which means that it is used for
the relative wait_for calls and absolute wait_until calls using
user-defined clocks. Calls explicitly using std::chrono::system_clock
continue to use CLOCK_REALTIME via __gthread_cond_timedwait.  
  
If pthread_cond_clockwait is not available then
std::chrono::system_clock is deemed to be the "best" clock available
which means that the previous suboptimal behaviour remains.

2019-09-04  Mike Crowe  <mac@mcrowe.com>

The code is gated behind the macro _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT. On Arch Linux this is defined in /usr/include/c++/10.1.0/x86_64-pc-linux-gnu/bits/c++config.h.

/* Define if pthread_cond_clockwait is available in <pthread.h>. */
#define _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT 1

If the above line is commented out, the double lock error disappears. I would assume this means tsan needs to add support for pthread_cond_clockwait.

@fxkaiser
Copy link

I ran into a similar issue, but with libstdc++ it works fine whereas for libc++ those false positives show up. _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT is not defined in /usr/include/x86_64-pc-linux-gnu/c++/9/bits/c++config.h. But a similar symbol _LIBCPP_HAS_COND_CLOCKWAIT is defined in /usr/lib/llvm-10/include/c++/v1/__config. Removing _LIBCPP_HAS_COND_CLOCKWAIT makes the false positives disappear.

Tested system:
Linux 5.4.0.-33-generic x86_64, Ubuntu 20.04 LTS, GLIBC: 2.31, GCC: 9.3.0, CLANG: 10.0.0

@vitalybuka
Copy link
Contributor

Same with debian
GOOD http://snapshot.debian.org/archive/debian/20200806T070000Z libc6-i386 (2.31-3) clang-9 (1:9.0.1-13) libstdc++-9-dev:amd64 (9.3.0-16)
BAD http://snapshot.debian.org/archive/debian/20200810T070000Z libc6-i386 (2.31-3) clang-9 (1:9.0.1-13+b1) libstdc++-10-dev:amd64 (10.2.0-5)

looking what can be done in tsan about pthread_cond_clockwait

@yachoor
Copy link

yachoor commented Nov 13, 2020

looking what can be done in tsan about pthread_cond_clockwait

Adding an interceptor in compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp similar to the one for pthread_cond_timedwait should do it, I think

@knzivid
Copy link

knzivid commented Nov 18, 2020

Great, thank you for the fix

vitalybuka added a commit to llvm/llvm-project that referenced this issue Nov 18, 2020
Disable the test on old systems.
pthread_cond_clockwait is supported by glibc-2.30.
It also supported by Android api 30 even though we
do not run tsan on Android.

Fixes google/sanitizers#1259

Reviewed By: dvyukov
chelini pushed a commit to llvm/Polygeist that referenced this issue Nov 25, 2020
chelini pushed a commit to llvm/Polygeist that referenced this issue Nov 25, 2020
Disable the test on old systems.
pthread_cond_clockwait is supported by glibc-2.30.
It also supported by Android api 30 even though we
do not run tsan on Android.

Fixes google/sanitizers#1259

Reviewed By: dvyukov
@Soukyuu
Copy link

Soukyuu commented Dec 9, 2020

Great! Which clang version will this land in?

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Feb 7, 2021
The iothread pool has a feature where, if the thread is emptied, some
threads will choose to wait around in case new work appears, up to a
certain amount of time (500 msec). This prevents thrashing where new
threads are rapidly created and destroyed as the user types. This is
implemented via `std::condition_variable::wait_for`. However this function
is not properly instrumented under Thread Sanitizer (see
google/sanitizers#1259) so TSan reports false
positives. Just disable this feature under TSan.
ridiculousfish added a commit to fish-shell/fish-shell that referenced this issue Feb 7, 2021
The iothread pool has a feature where, if the thread is emptied, some
threads will choose to wait around in case new work appears, up to a
certain amount of time (500 msec). This prevents thrashing where new
threads are rapidly created and destroyed as the user types. This is
implemented via `std::condition_variable::wait_for`. However this function
is not properly instrumented under Thread Sanitizer (see
google/sanitizers#1259) so TSan reports false
positives. Just disable this feature under TSan.
arichardson pushed a commit to arichardson/llvm-project that referenced this issue Mar 25, 2021
arichardson pushed a commit to arichardson/llvm-project that referenced this issue Mar 25, 2021
Disable the test on old systems.
pthread_cond_clockwait is supported by glibc-2.30.
It also supported by Android api 30 even though we
do not run tsan on Android.

Fixes google/sanitizers#1259

Reviewed By: dvyukov
thomastrapp added a commit to thomastrapp/signal-wrangler that referenced this issue May 18, 2021
Clang's ThreadSanitizer in version 10 and 11 emits a false positive
when using condition variables ("WARNING: ThreadSanitizer: double lock
of a mutex").
This was fixed in version 12 (google/sanitizers#1259).
@mxmlnkn
Copy link

mxmlnkn commented Jun 20, 2021

Great! Which clang version will this land in?

I have false positives in GCC 10 and clang 10 and 11.

The false positives disappear for me in GCC 8, 9 and clang 12.

jrtc27 pushed a commit to CTSRD-CHERI/compiler-rt that referenced this issue Jan 18, 2022
jrtc27 pushed a commit to CTSRD-CHERI/compiler-rt that referenced this issue Jan 18, 2022
Disable the test on old systems.
pthread_cond_clockwait is supported by glibc-2.30.
It also supported by Android api 30 even though we
do not run tsan on Android.

Fixes google/sanitizers#1259

Reviewed By: dvyukov
@jwakely
Copy link

jwakely commented May 11, 2023

See llvm/llvm-project#62623 which points out that interceptors are still missing for pthread_mutex_clocklock, pthread_rwlock_clockrdlock, and pthread_rwlock_clockwrlock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants