On Fri, 12 May 2023 at 11:30, Mike Crowe <mac@mcrowe.com> wrote:

> On Thursday 11 May 2023 at 21:52:22 +0100, Jonathan Wakely wrote:
> > On Thu, 11 May 2023 at 13:42, Jonathan Wakely <jwakely@redhat.com>
> wrote:
> >
> > >
> > >
> > > On Thu, 11 May 2023 at 13:19, Mike Crowe <mac@mcrowe.com> wrote:
> > >
> > >> However, ...
> > >>
> > >> > > diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
> > >> > > index 89e7f5f5f45..e2700b05ec3 100644
> > >> > > --- a/libstdc++-v3/acinclude.m4
> > >> > > +++ b/libstdc++-v3/acinclude.m4
> > >> > > @@ -4284,7 +4284,7 @@
> > >> AC_DEFUN([GLIBCXX_CHECK_PTHREAD_COND_CLOCKWAIT], [
> > >> > >        [glibcxx_cv_PTHREAD_COND_CLOCKWAIT=no])
> > >> > >    ])
> > >> > >    if test $glibcxx_cv_PTHREAD_COND_CLOCKWAIT = yes; then
> > >> > > -    AC_DEFINE(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT, 1, [Define if
> > >> > > pthread_cond_clockwait is available in <pthread.h>.])
> > >> > > +    AC_DEFINE(_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT,
> > >> (_GLIBCXX_TSAN==0),
> > >> > > [Define if pthread_cond_clockwait is available in <pthread.h>.])
> > >> > >    fi
> > >>
> > >> TSan does appear to have an interceptor for pthread_cond_clockwait,
> even
> > >> if
> > >> it lacks the others. Does this mean that this part is unnecessary?
> > >>
> > >
> > > Ah good point, thanks. I grepped for clocklock but not clockwait.
> > >
> >
> > In fact it seems like we don't need to change
> > _GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK either, because I don't get any
> tsan
> > warnings for that. It doesn't have interceptors for
> > pthread_rwlock_{rd,wr}lock, but it doesn't complain anyway (maybe it's
> > simply not instrumenting the rwlock functions at all?!)
>
> It looks like TSan does have interceptors for pthread_rwlock_timedrdlock
> etc. I can't explain why this doesn't cause problems when libstdc++ uses
> pthread_rwlock_clockrdlock etc.
>

I think glibc has renamed the rwlock functions, and so the interceptors no
longer work.

# ifdef __USE_XOPEN2K
/* Try to acquire read lock for RWLOCK or return after specfied time.  */
#  ifndef __USE_TIME_BITS64
extern int pthread_rwlock_timedrdlock (pthread_rwlock_t *__restrict
__rwlock,
      const struct timespec *__restrict
      __abstime) __THROWNL __nonnull ((1, 2));
#  else
#   ifdef __REDIRECT_NTHNL
extern int __REDIRECT_NTHNL (pthread_rwlock_timedrdlock,
                             (pthread_rwlock_t *__restrict __rwlock,
                              const struct timespec *__restrict __abstime),
                             __pthread_rwlock_timedrdlock64)
    __nonnull ((1, 2));
#   else
#    define pthread_rwlock_timedrdlock __pthread_rwlock_timedrdlock64
#   endif
#  endif
# endif

If glibc is really providing a function called
__pthread_rwlock_timedrdlock64 then will tsan be able to intercept that?



> > So I'm now retesting with this version of the patch, which only touches
> the
> > USE_PTHREAD_LOCKLOCK macro.
> >
> > Please take another look, thanks.
>
> > commit 4fc14825c125eece32980df21d09da35e3d5bac6
> > Author: Jonathan Wakely <jwakely@redhat.com>
> > Date:   Tue May 9 09:30:48 2023
> >
> >     libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer
> >
> >     As noted in https://github.com/llvm/llvm-project/issues/62623 there
> are
> >     no tsan interceptors for some of the new POSIX-1:202x APIs added by
> >     https://austingroupbugs.net/view.php?id=1216 so tsan gives false
> >     positive warnings for try_lock_for on timed mutexes.
> >
> >     Disable the uses of the new pthread_mutex_clocklock API when tsan is
> >     active. This changes the semantics of the try_lock_for functions,
> >     because it can change which clock is used for the wait. This means
> those
> >     functions might be affected by system clock adjustments when tsan is
> >     used, when they would not be affected otherwise.
> >
> >     libstdc++-v3/ChangeLog:
> >
> >             * acinclude.m4 (GLIBCXX_CHECK_PTHREAD_MUTEX_CLOCKLOCK):
> Define
> >             _GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK in terms of
> _GLIBCXX_TSAN.
> >             * configure: Regenerate.
> >
> > diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4
> > index 89e7f5f5f45..dce3d16aa5c 100644
> > --- a/libstdc++-v3/acinclude.m4
> > +++ b/libstdc++-v3/acinclude.m4
> > @@ -4314,7 +4314,7 @@ AC_DEFUN([GLIBCXX_CHECK_PTHREAD_MUTEX_CLOCKLOCK], [
> >        [glibcxx_cv_PTHREAD_MUTEX_CLOCKLOCK=no])
> >    ])
> >    if test $glibcxx_cv_PTHREAD_MUTEX_CLOCKLOCK = yes; then
> > -    AC_DEFINE(_GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK, 1, [Define if
> pthread_mutex_clocklock is available in <pthread.h>.])
> > +    AC_DEFINE(_GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK, (_GLIBCXX_TSAN==0),
> [Define if pthread_mutex_clocklock is available in <pthread.h>.])
> >    fi
> >
> >    CXXFLAGS="$ac_save_CXXFLAGS"
>
> LGTM.
>
> Mike.
>
>