From: Paul Floyd Date: Wed, 1 Dec 2021 23:03:27 +0000 (+0100) Subject: Bug 446139 DRD/Helgrind with std::shared_timed_mutex::try_lock_until and try_lock_sha... X-Git-Tag: VALGRIND_3_19_0~64 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c2607e093c54abe473b1d419cbdbba403cc87cdf;p=thirdparty%2Fvalgrind.git Bug 446139 DRD/Helgrind with std::shared_timed_mutex::try_lock_until and try_lock_shared_until false positives also Bug 446138 DRD/Helgrind with std::timed_mutex::try_lock_until false positives --- diff --git a/.gitignore b/.gitignore index fd85516b28..bf83c0f8c0 100644 --- a/.gitignore +++ b/.gitignore @@ -442,6 +442,7 @@ /drd/tests/sem_as_mutex /drd/tests/sem_open /drd/tests/sem_wait +/drd/tests/shared_timed_mutex /drd/tests/sigalrm /drd/tests/std_atomic /drd/tests/std_list @@ -476,6 +477,7 @@ /drd/tests/threaded-fork /drd/tests/thread_name /drd/tests/thread_name_freebsd +/drd/tests/timed_mutex /drd/tests/trylock /drd/tests/tsan_unittest /drd/tests/unit_bitmap diff --git a/configure.ac b/configure.ac index 4623d12b5b..96df114bae 100755 --- a/configure.ac +++ b/configure.ac @@ -2011,6 +2011,52 @@ AC_LANG(C) AM_CONDITIONAL(CXX_CAN_INCLUDE_CONDITION_VARIABLE_HEADER, test x$ac_cxx_can_include_condition_variable_header = xyes) +# check for std::shared_timed_mutex, this is a C++ 14 feature + +AC_MSG_CHECKING([that C++ compiler can use std::shared_timed_mutex]) +AC_LANG(C++) +safe_CXXFLAGS=$CXXFLAGS +CXXFLAGS="-std=c++1y -pthread" + +AC_COMPILE_IFELSE([AC_LANG_SOURCE([ +#include +std::shared_timed_mutex test_mutex; +])], +[ +ac_cxx_can_use_shared_timed_mutex=yes +AC_MSG_RESULT([yes]) +], [ +ac_cxx_can_use_shared_timed_mutex=no +AC_MSG_RESULT([no]) +]) +CXXFLAGS=$safe_CXXFLAGS +AC_LANG(C) + +AM_CONDITIONAL(CXX_CAN_USE_SHARED_TIMED_MUTEX, test x$ac_cxx_can_use_shared_timed_mutex = xyes) + +# check for std::shared_mutex, this is a C++ 11 feature + +AC_MSG_CHECKING([that C++ compiler can use std::timed_mutex]) +AC_LANG(C++) +safe_CXXFLAGS=$CXXFLAGS +CXXFLAGS="-std=c++0x -pthread" + +AC_COMPILE_IFELSE([AC_LANG_SOURCE([ +#include +std::timed_mutex test_mutex; +])], +[ +ac_cxx_can_use_timed_mutex=yes +AC_MSG_RESULT([yes]) +], [ +ac_cxx_can_use_timed_mutex=no +AC_MSG_RESULT([no]) +]) +CXXFLAGS=$safe_CXXFLAGS +AC_LANG(C) + +AM_CONDITIONAL(CXX_CAN_USE_TIMED_MUTEX, test x$ac_cxx_can_use_timed_mutex = 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 # but we get either one or the other. diff --git a/drd/drd_pthread_intercepts.c b/drd/drd_pthread_intercepts.c index 95127b42c6..c18e888678 100644 --- a/drd/drd_pthread_intercepts.c +++ b/drd/drd_pthread_intercepts.c @@ -1016,6 +1016,26 @@ PTH_FUNCS(int, (mutex, timeout)); #endif /* VGO_solaris */ +static __always_inline +int pthread_mutex_clocklock_intercept(pthread_mutex_t *mutex, + clockid_t clockid, + const struct timespec *abs_timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__PRE_MUTEX_LOCK, + mutex, DRD_(mutex_type)(mutex), 0, 0, 0); + CALL_FN_W_WWW(ret, fn, mutex, clockid, abs_timeout); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__POST_MUTEX_LOCK, + mutex, ret == 0, 0, 0, 0); + return ret; +} + +PTH_FUNCS(int, pthreadZumutexZuclocklock, pthread_mutex_clocklock_intercept, + (pthread_mutex_t *mutex, clockid_t clockid, const struct timespec *abs_timeout), + (mutex, clockid, abs_timeout)); + static __always_inline int pthread_mutex_unlock_intercept(pthread_mutex_t *mutex) { @@ -1794,6 +1814,27 @@ PTH_FUNCS(int, pthreadZurwlockZureltimedrdlockZunp, (rwlock, timeout)); #endif /* VGO_solaris */ +static __always_inline +int pthread_rwlock_clockrdlock_intercept(pthread_rwlock_t* rwlock, + clockid_t clockid, + const struct timespec *timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__PRE_RWLOCK_RDLOCK, + rwlock, 0, 0, 0, 0); + CALL_FN_W_WWW(ret, fn, rwlock, clockid, timeout); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__POST_RWLOCK_RDLOCK, + rwlock, ret == 0, 0, 0, 0); + return ret; +} + +PTH_FUNCS(int, + pthreadZurwlockZuclockrdlock, pthread_rwlock_clockrdlock_intercept, + (pthread_rwlock_t* rwlock, clockid_t clockid, const struct timespec *timeout), + (rwlock, clockid, timeout)); + static __always_inline int pthread_rwlock_timedwrlock_intercept(pthread_rwlock_t* rwlock, const struct timespec *timeout) @@ -1820,6 +1861,28 @@ PTH_FUNCS(int, pthreadZurwlockZureltimedwrlockZunp, (rwlock, timeout)); #endif /* VGO_solaris */ +static __always_inline +int pthread_rwlock_clockwrlock_intercept(pthread_rwlock_t* rwlock, + clockid_t clockid, + const struct timespec *timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__PRE_RWLOCK_WRLOCK, + rwlock, 0, 0, 0, 0); + CALL_FN_W_WWW(ret, fn, rwlock, clockid, timeout); + VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__POST_RWLOCK_WRLOCK, + rwlock, ret == 0, 0, 0, 0); + return ret; +} + +PTH_FUNCS(int, + pthreadZurwlockZuclockwrlock, pthread_rwlock_clockwrlock_intercept, + (pthread_rwlock_t* rwlock, clockid_t clockid, const struct timespec *timeout), + (rwlock, clockid, timeout)); + + static __always_inline int pthread_rwlock_tryrdlock_intercept(pthread_rwlock_t* rwlock) { diff --git a/drd/tests/Makefile.am b/drd/tests/Makefile.am index ac487def3c..d7d73447bd 100755 --- a/drd/tests/Makefile.am +++ b/drd/tests/Makefile.am @@ -275,6 +275,8 @@ EXTRA_DIST = \ sem_open_traced.vgtest \ sem_wait.stderr.exp \ sem_wait.vgtest \ + shared_timed_mutex.stderr.exp \ + shared_timed_mutex.vgtest \ swapcontext.stderr.exp \ swapcontext.vgtest \ sigalrm.stderr.exp \ @@ -369,6 +371,8 @@ EXTRA_DIST = \ threaded-fork.vgtest \ threaded-fork-vcs.stderr.exp \ threaded-fork-vcs.vgtest \ + timed_mutex.stderr.exp \ + timed_mutex.vgtest \ tls_threads.stderr.exp \ tls_threads.vgtest \ trylock.stderr.exp \ @@ -468,6 +472,16 @@ check_PROGRAMS += \ condvar endif +if CXX_CAN_USE_SHARED_TIMED_MUTEX +check_PROGRAMS += \ + shared_timed_mutex +endif + +if CXX_CAN_USE_TIMED_MUTEX +check_PROGRAMS += \ + timed_mutex +endif + if HAVE_OPENMP check_PROGRAMS += omp_matinv omp_prime omp_printf endif @@ -593,6 +607,9 @@ if VGCONF_OS_IS_SOLARIS rwlock_test_CFLAGS += -D__EXTENSIONS__ endif +shared_timed_mutex_SOURCES = shared_timed_mutex.cpp +shared_timed_mutex_CXXFLAGS = $(AM_CXXFLAGS) -std=c++1y + std_atomic_SOURCES = std_atomic.cpp std_atomic_CXXFLAGS = $(AM_CXXFLAGS) -std=c++0x -Wno-sign-compare @@ -628,3 +645,5 @@ if VGCONF_OS_IS_SOLARIS swapcontext_CFLAGS += -D__EXTENSIONS__ endif +timed_mutex_SOURCES = timed_mutex.cpp +timed_mutex_CXXFLAGS = $(AM_CXXFLAGS) -std=c++0x diff --git a/drd/tests/shared_timed_mutex.cpp b/drd/tests/shared_timed_mutex.cpp new file mode 100644 index 0000000000..306d6bf27a --- /dev/null +++ b/drd/tests/shared_timed_mutex.cpp @@ -0,0 +1,63 @@ +#include +#include +#include +#include +#include +#include +#include + +std::shared_timed_mutex test_mutex; +std::mutex cv_mutex; +std::condition_variable cv; +int global; +bool reads_done = false; + +void f() +{ + auto now=std::chrono::steady_clock::now(); + if (test_mutex.try_lock_until(now + std::chrono::seconds(3))) + { + --global; + test_mutex.unlock(); + } + else + { + std::cerr << "Lock failed\n"; + } +} + +void g() +{ + auto now=std::chrono::steady_clock::now(); + if (test_mutex.try_lock_shared_until(now + std::chrono::seconds(2))) + { + test_mutex.unlock_shared(); + } + else + { + std::cerr << "Lock shared failed\n"; + } + std::unique_lock lock(cv_mutex); + reads_done = true; + cv.notify_all(); +} + +int main() +{ + global = 1; + test_mutex.lock_shared(); + std::thread t1(f); + std::thread t2(g); + { + std::unique_lock lock(cv_mutex); + while (!reads_done) + { + cv.wait(lock); + } + } + test_mutex.unlock_shared(); + t1.join(); + t2.join(); + assert(global == 0); +} + diff --git a/drd/tests/shared_timed_mutex.stderr.exp b/drd/tests/shared_timed_mutex.stderr.exp new file mode 100644 index 0000000000..d18786f806 --- /dev/null +++ b/drd/tests/shared_timed_mutex.stderr.exp @@ -0,0 +1,3 @@ + + +ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) diff --git a/drd/tests/shared_timed_mutex.vgtest b/drd/tests/shared_timed_mutex.vgtest new file mode 100644 index 0000000000..e3be8646c5 --- /dev/null +++ b/drd/tests/shared_timed_mutex.vgtest @@ -0,0 +1,3 @@ +prereq: ./supported_libpthread && [ -e shared_timed_mutex ] +vgopts: --check-stack-var=yes --read-var-info=yes +prog: shared_timed_mutex diff --git a/drd/tests/timed_mutex.cpp b/drd/tests/timed_mutex.cpp new file mode 100644 index 0000000000..b1bb61efa6 --- /dev/null +++ b/drd/tests/timed_mutex.cpp @@ -0,0 +1,29 @@ +#include +#include +#include +#include +#include + +std::timed_mutex test_mutex; +int global; + +void f() +{ + auto now=std::chrono::steady_clock::now(); + test_mutex.try_lock_until(now + std::chrono::seconds(11)); + --global; + test_mutex.unlock(); +} + +int main() +{ + global = 0; + auto now=std::chrono::steady_clock::now(); + test_mutex.try_lock_until(now + std::chrono::seconds(11)); + ++global; + std::thread t(f); + test_mutex.unlock(); + t.join(); + assert(global == 0); +} + diff --git a/drd/tests/timed_mutex.stderr.exp b/drd/tests/timed_mutex.stderr.exp new file mode 100644 index 0000000000..d18786f806 --- /dev/null +++ b/drd/tests/timed_mutex.stderr.exp @@ -0,0 +1,3 @@ + + +ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) diff --git a/drd/tests/timed_mutex.vgtest b/drd/tests/timed_mutex.vgtest new file mode 100644 index 0000000000..60769b2d13 --- /dev/null +++ b/drd/tests/timed_mutex.vgtest @@ -0,0 +1,3 @@ +prereq: ./supported_libpthread && [ -e timed_mutex ] +vgopts: --check-stack-var=yes --read-var-info=yes +prog: timed_mutex diff --git a/helgrind/hg_intercepts.c b/helgrind/hg_intercepts.c index 49c3ddcd9e..c9b3f85ef3 100644 --- a/helgrind/hg_intercepts.c +++ b/helgrind/hg_intercepts.c @@ -1110,6 +1110,53 @@ PTH_FUNC(int, pthreadZumutexZureltimedlock, // pthread_mutex_reltimedlock } #endif +//----------------------------------------------------------- +// glibc: pthread_mutex_clocklock +// +// pthread_mutex_clocklock. Identical logic to pthread_mutex_timedlock. +__attribute__((noinline)) +static int mutex_clocklock_WRK(pthread_mutex_t *mutex, + clockid_t clockid, + void *timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + if (TRACE_PTH_FNS) { + fprintf(stderr, "<< pthread_mxclocklock %p %p", mutex, timeout); + fflush(stderr); + } + + DO_CREQ_v_WW(_VG_USERREQ__HG_PTHREAD_MUTEX_LOCK_PRE, + pthread_mutex_t*,mutex, long,1/*isTryLock-ish*/); + + CALL_FN_W_WWW(ret, fn, mutex, clockid, timeout); + + /* There's a hole here: libpthread now knows the lock is locked, + but the tool doesn't, so some other thread could run and detect + that the lock has been acquired by someone (this thread). Does + this matter? Not sure, but I don't think so. */ + + DO_CREQ_v_WW(_VG_USERREQ__HG_PTHREAD_MUTEX_LOCK_POST, + pthread_mutex_t *, mutex, long, (ret == 0) ? True : False); + + if (ret != 0) { + if (ret != ETIMEDOUT) + DO_PthAPIerror( "pthread_mutex_clocklock", ret ); + } + + if (TRACE_PTH_FNS) { + fprintf(stderr, " :: mxclocklock -> %d >>\n", ret); + } + return ret; +} + +PTH_FUNC(int, pthreadZumutexZuclocklock, // pthread_mutex_clocklock + pthread_mutex_t *mutex, + clockid_t clockid, + void *timeout) { + return mutex_clocklock_WRK(mutex, clockid, timeout); +} //----------------------------------------------------------- // glibc: pthread_mutex_unlock @@ -2685,6 +2732,48 @@ PTH_FUNC(int, pthreadZurwlockZutimedrdlock, // pthread_rwlock_timedrdlock # error "Unsupported OS" #endif +//----------------------------------------------------------- +// glibc: pthread_rwlock_clockrdlock +// +__attribute__((noinline)) __attribute__((unused)) +static int pthread_rwlock_clockrdlock_WRK(pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct timespec *timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + if (TRACE_PTH_FNS) { + fprintf(stderr, "<< pthread_rwl_clockrdl %p", rwlock); fflush(stderr); + } + + DO_CREQ_v_WWW(_VG_USERREQ__HG_PTHREAD_RWLOCK_LOCK_PRE, + pthread_rwlock_t *, rwlock, + long, 0/*isW*/, long, 0/*isTryLock*/); + + CALL_FN_W_WWW(ret, fn, rwlock, clockid, timeout); + + DO_CREQ_v_WWW(_VG_USERREQ__HG_PTHREAD_RWLOCK_LOCK_POST, + pthread_rwlock_t *, rwlock, long, 0/*isW*/, + long, (ret == 0) ? True : False); + if (ret != 0) { + DO_PthAPIerror("pthread_rwlock_clockrdlock", ret); + } + + if (TRACE_PTH_FNS) { + fprintf(stderr, " :: rwl_clockrdl -> %d >>\n", ret); + } + return ret; +} +#if defined(VGO_linux) +PTH_FUNC(int, pthreadZurwlockZuclockrdlock, // pthread_rwlock_clockrdlock + pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct timespec *timeout) { + return pthread_rwlock_clockrdlock_WRK(rwlock, clockid, timeout); +} +#endif + //----------------------------------------------------------- // glibc: Unhandled @@ -2746,6 +2835,49 @@ PTH_FUNC(int, pthreadZurwlockZutimedwrlock, // pthread_rwlock_timedwrlock #endif +//----------------------------------------------------------- +// glibc: pthread_rwlock_clockwrlock +// +__attribute__((noinline)) __attribute__((unused)) +static int pthread_rwlock_clockwrlock_WRK(pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct timespec *timeout) +{ + int ret; + OrigFn fn; + VALGRIND_GET_ORIG_FN(fn); + if (TRACE_PTH_FNS) { + fprintf(stderr, "<< pthread_rwl_clockwrl %p", rwlock); fflush(stderr); + } + + DO_CREQ_v_WWW(_VG_USERREQ__HG_PTHREAD_RWLOCK_LOCK_PRE, + pthread_rwlock_t *, rwlock, + long, 1/*isW*/, long, 0/*isTryLock*/); + + CALL_FN_W_WWW(ret, fn, rwlock, clockid, timeout); + + DO_CREQ_v_WWW(_VG_USERREQ__HG_PTHREAD_RWLOCK_LOCK_POST, + pthread_rwlock_t *, rwlock, long, 1/*isW*/, + long, (ret == 0) ? True : False); + if (ret != 0) { + DO_PthAPIerror("pthread_rwlock_clockwrlock", ret); + } + + if (TRACE_PTH_FNS) { + fprintf(stderr, " :: rwl_clockwrl -> %d >>\n", ret); + } + return ret; +} +#if defined(VGO_linux) +PTH_FUNC(int, pthreadZurwlockZuclockwrlock, // pthread_rwlock_clockwrlock + pthread_rwlock_t *rwlock, + clockid_t clockid, + const struct timespec *timeout) { + return pthread_rwlock_clockwrlock_WRK(rwlock, clockid, timeout); +} +#endif + + //----------------------------------------------------------- // glibc: pthread_rwlock_unlock // darwin: pthread_rwlock_unlock