commit 9abfed23c0d430aafb85de6397d171316c982792 Author: Paul Floyd Date: Fri Nov 19 08:34:53 2021 +0100 Bug 445504 Using C++ condition_variable results in bogus "mutex is locked simultaneously by two threads" warning(edit) Add intercepts for pthread_cond_clockwait to DRD and Helgrind Also testcase from bugzilla done by Bart, with configure check diff --git a/configure.ac b/configure.ac index e7381f205..cb836dbff 100755 --- a/configure.ac +++ b/configure.ac @@ -1989,6 +1989,27 @@ AC_LANG(C) AM_CONDITIONAL(CXX_CAN_INCLUDE_THREAD_HEADER, test x$ac_cxx_can_include_thread_header = xyes) +# Check whether compiler can process #include without errors + +AC_MSG_CHECKING([that C++ compiler can include header file]) +AC_LANG(C++) +safe_CXXFLAGS=$CXXFLAGS +CXXFLAGS=-std=c++0x + +AC_COMPILE_IFELSE([AC_LANG_SOURCE([ +#include +])], +[ +ac_cxx_can_include_condition_variable_header=yes +AC_MSG_RESULT([yes]) +], [ +ac_cxx_can_include_condition_variable_header=no +AC_MSG_RESULT([no]) +]) +CXXFLAGS=$safe_CXXFLAGS +AC_LANG(C) + +AM_CONDITIONAL(CXX_CAN_INCLUDE_CONDITION_VARIABLE_HEADER, test x$ac_cxx_can_include_condition_variable_header = xyes) # On aarch64 before glibc 2.20 we would get the kernel user_pt_regs instead # of the user_regs_struct from sys/user.h. They are structurally the same diff --git a/drd/drd_pthread_intercepts.c b/drd/drd_pthread_intercepts.c index 8b4454364..95127b42c 100644 --- a/drd/drd_pthread_intercepts.c +++ b/drd/drd_pthread_intercepts.c @@ -1175,6 +1175,30 @@ PTH_FUNCS(int, condZureltimedwait, pthread_cond_timedwait_intercept, (cond, mutex, timeout)); #endif /* VGO_solaris */ + +static __always_inline +int pthread_cond_clockwait_intercept(pthread_cond_t *cond, + pthread_mutex_t *mutex, + clockid_t clockid, + const struct timespec* abstime) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__PRE_COND_WAIT, + cond, mutex, DRD_(mutex_type)(mutex), 0, 0); + CALL_FN_W_WWWW(ret, fn, cond, mutex, clockid, abstime); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__POST_COND_WAIT, + cond, mutex, 1, 0, 0); + return ret; +} + +PTH_FUNCS(int, pthreadZucondZuclockwait, pthread_cond_clockwait_intercept, + (pthread_cond_t *cond, pthread_mutex_t *mutex, + clockid_t clockid, const struct timespec* abstime), + (cond, mutex, clockid, abstime)); + + // NOTE: be careful to intercept only pthread_cond_signal() and not Darwin's // pthread_cond_signal_thread_np(). The former accepts one argument; the latter // two. Intercepting all pthread_cond_signal* functions will cause only one diff --git a/drd/tests/Makefile.am b/drd/tests/Makefile.am index 4cb2f7f84..c804391e8 100755 --- a/drd/tests/Makefile.am +++ b/drd/tests/Makefile.am @@ -105,6 +105,8 @@ EXTRA_DIST = \ circular_buffer.vgtest \ concurrent_close.stderr.exp \ concurrent_close.vgtest \ + condvar.stderr.exp \ + condvar.vgtest \ custom_alloc.stderr.exp \ custom_alloc.vgtest \ custom_alloc_fiw.stderr.exp \ @@ -458,6 +460,11 @@ check_PROGRAMS += \ endif endif +if CXX_CAN_INCLUDE_CONDITION_VARIABLE_HEADER +check_PROGRAMS += \ + condvar +endif + if HAVE_OPENMP check_PROGRAMS += omp_matinv omp_prime omp_printf endif @@ -502,6 +509,8 @@ LDADD = -lpthread bug322621_SOURCES = bug322621.cpp +condvar_SOURCES = condvar.cpp +condvar_CXXFLAGS = $(AM_CXXFLAGS) -std=c++0x concurrent_close_SOURCES = concurrent_close.cpp if !VGCONF_OS_IS_FREEBSD dlopen_main_LDADD = -ldl diff --git a/drd/tests/condvar.cpp b/drd/tests/condvar.cpp new file mode 100644 index 000000000..18ecb3f8a --- /dev/null +++ b/drd/tests/condvar.cpp @@ -0,0 +1,55 @@ +/* See also https://bugs.kde.org/show_bug.cgi?id=445504 */ + +#include +#include +#include +#include +#include +#include + +using lock_guard = std::lock_guard; +using unique_lock = std::unique_lock; + +struct state { + std::mutex m; + std::vector v; + std::condition_variable cv; + + state() { + // Call pthread_cond_init() explicitly to let DRD know about 'cv'. + pthread_cond_init(cv.native_handle(), NULL); + } +}; + +void other_thread(state *sp) { + state &s = *sp; + std::cerr << "Other thread: waiting for notify\n"; + unique_lock l{s.m}; + while (true) { + if (s.cv.wait_for(l, std::chrono::seconds(3)) != + std::cv_status::timeout) { + std::cerr << "Other thread: notified\n"; + break; + } + } + return; +} + + +int main() { + state s; + auto future = std::async(std::launch::async, other_thread, &s); + + if (future.wait_for(std::chrono::seconds(1)) != std::future_status::timeout) { + std::cerr << "Main: other thread returned too early!\n"; + return 2; + } + + { + std::lock_guard g{s.m}; + s.v.push_back(1); + s.v.push_back(2); + s.cv.notify_all(); + } + return 0; +} diff --git a/drd/tests/condvar.stderr.exp b/drd/tests/condvar.stderr.exp new file mode 100644 index 000000000..be1de9f97 --- /dev/null +++ b/drd/tests/condvar.stderr.exp @@ -0,0 +1,5 @@ + +Other thread: waiting for notify +Other thread: notified + +ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) diff --git a/drd/tests/condvar.vgtest b/drd/tests/condvar.vgtest new file mode 100644 index 000000000..2e7d49f5a --- /dev/null +++ b/drd/tests/condvar.vgtest @@ -0,0 +1,3 @@ +prereq: ./supported_libpthread && [ -e condvar ] +vgopts: --check-stack-var=yes --read-var-info=yes +prog: condvar diff --git a/helgrind/hg_intercepts.c b/helgrind/hg_intercepts.c index 866efdbaa..49c3ddcd9 100644 --- a/helgrind/hg_intercepts.c +++ b/helgrind/hg_intercepts.c @@ -1409,6 +1409,88 @@ static int pthread_cond_timedwait_WRK(pthread_cond_t* cond, # error "Unsupported OS" #endif +//----------------------------------------------------------- +// glibc: pthread_cond_clockwait +// +__attribute__((noinline)) +static int pthread_cond_clockwait_WRK(pthread_cond_t* cond, + pthread_mutex_t* mutex, + clockid_t clockid, + struct timespec* abstime, + int timeout_error) +{ + int ret; + OrigFn fn; + unsigned long mutex_is_valid; + Bool abstime_is_valid; + VALGRIND_GET_ORIG_FN(fn); + + if (TRACE_PTH_FNS) { + fprintf(stderr, "<< pthread_cond_clockwait %p %p %p", + cond, mutex, abstime); + fflush(stderr); + } + + /* Tell the tool a cond-wait is about to happen, so it can check + for bogus argument values. In return it tells us whether it + thinks the mutex is valid or not. */ + DO_CREQ_W_WW(mutex_is_valid, + _VG_USERREQ__HG_PTHREAD_COND_WAIT_PRE, + pthread_cond_t*,cond, pthread_mutex_t*,mutex); + assert(mutex_is_valid == 1 || mutex_is_valid == 0); + + abstime_is_valid = abstime->tv_nsec >= 0 && abstime->tv_nsec < 1000000000; + + /* Tell the tool we're about to drop the mutex. This reflects the + fact that in a cond_wait, we show up holding the mutex, and the + call atomically drops the mutex and waits for the cv to be + signalled. */ + if (mutex_is_valid && abstime_is_valid) { + DO_CREQ_v_W(_VG_USERREQ__HG_PTHREAD_MUTEX_UNLOCK_PRE, + pthread_mutex_t*,mutex); + } + + CALL_FN_W_WWWW(ret, fn, cond,mutex,clockid,abstime); + + if (mutex_is_valid && !abstime_is_valid && ret != EINVAL) { + DO_PthAPIerror("Bug in libpthread: pthread_cond_clockwait " + "invalid abstime did not cause" + " EINVAL", ret); + } + + if (mutex_is_valid && abstime_is_valid) { + /* and now we have the mutex again if (ret == 0 || ret == timeout) */ + DO_CREQ_v_WW(_VG_USERREQ__HG_PTHREAD_MUTEX_LOCK_POST, + pthread_mutex_t *, mutex, + long, (ret == 0 || ret == timeout_error) ? True : False); + } + + DO_CREQ_v_WWWW(_VG_USERREQ__HG_PTHREAD_COND_WAIT_POST, + pthread_cond_t*,cond, pthread_mutex_t*,mutex, + long,ret == timeout_error, + long, (ret == 0 || ret == timeout_error) && mutex_is_valid + ? True : False); + + if (ret != 0 && ret != timeout_error) { + DO_PthAPIerror( "pthread_cond_clockwait", ret ); + } + + if (TRACE_PTH_FNS) { + fprintf(stderr, " cotimedwait -> %d >>\n", ret); + } + + return ret; +} + +#if defined(VGO_linux) + PTH_FUNC(int, pthreadZucondZuclockwait, // pthread_cond_clockwait + pthread_cond_t* cond, pthread_mutex_t* mutex, + clockid_t clockid, + struct timespec* abstime) { + return pthread_cond_clockwait_WRK(cond, mutex, clockid, abstime, ETIMEDOUT); + } +#endif + //----------------------------------------------------------- // glibc: pthread_cond_signal@GLIBC_2.0