[RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

This patch would avoid TSan false positives when using timed waiting
functions on mutexes and condvars, but as noted below, it changes the
semantics.

I'm not sure whether we want this workaround in place until tsan gets
fixed.

On one hand, there's no guarantee that those functions use the right
clock anyway (and they won't do unless a recent-ish glibc is used). But
on the other hand, if they normally would use the right clock because
you have glibc support, it's not ideal for tsan to cause a different
clock to be used.

-- >8 --

As noted in https://github.com/llvm/llvm-project/issues/62623 there are
no tsan interceptors for the new POSIX-1:202x APIs added by
https://austingroupbugs.net/view.php?id=1216 so tsan gives false
positive warnings.

Disable the uses of the new APIs when tsan is active. This changes the
semantics of those 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_COND_CLOCKWAIT): Define
	_GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT to depend on _GLIBCXX_TSAN.
	(GLIBCXX_CHECK_PTHREAD_MUTEX_CLOCKLOCK): Likewise for
	_GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK.
	(GLIBCXX_CHECK_PTHREAD_RWLOCK_CLOCKLOCK): Likewise for
	_GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK.
	* configure: Regenerate.
---
 libstdc++-v3/acinclude.m4 | 6 +++---
 libstdc++-v3/configure    | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

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
 
   CXXFLAGS="$ac_save_CXXFLAGS"
@@ -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"
@@ -4346,7 +4346,7 @@ AC_DEFUN([GLIBCXX_CHECK_PTHREAD_RWLOCK_CLOCKLOCK], [
       [glibcxx_cv_PTHREAD_RWLOCK_CLOCKLOCK=no])
   ])
   if test $glibcxx_cv_PTHREAD_RWLOCK_CLOCKLOCK = yes; then
-    AC_DEFINE(_GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK, 1, [Define if pthread_rwlock_clockrdlock and pthread_rwlock_clockwrlock are available in <pthread.h>.])
+    AC_DEFINE(_GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK, (_GLIBCXX_TSAN==0), [Define if pthread_rwlock_clockrdlock and pthread_rwlock_clockwrlock are available in <pthread.h>.])
   fi
 
   CXXFLAGS="$ac_save_CXXFLAGS"

Re: [RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]

On Wed, 10 May 2023 at 12:20, Jonathan Wakely via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> This patch would avoid TSan false positives when using timed waiting
> functions on mutexes and condvars, but as noted below, it changes the
> semantics.
>
> I'm not sure whether we want this workaround in place until tsan gets
> fixed.
>
> On one hand, there's no guarantee that those functions use the right
> clock anyway (and they won't do unless a recent-ish glibc is used). But
> on the other hand, if they normally would use the right clock because
> you have glibc support, it's not ideal for tsan to cause a different
> clock to be used.
>

But of course, it's not ideal to get false positives from tsan either
(especially when it looks like a libstdc++ bug, as initially reported to
me).



>
> -- >8 --
>
> As noted in https://github.com/llvm/llvm-project/issues/62623 there are
> no tsan interceptors for the new POSIX-1:202x APIs added by
> https://austingroupbugs.net/view.php?id=1216 so tsan gives false
> positive warnings.
>
> Disable the uses of the new APIs when tsan is active. This changes the
> semantics of those 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_COND_CLOCKWAIT): Define
>         _GLIBCXX_USE_PTHREAD_COND_CLOCKWAIT to depend on _GLIBCXX_TSAN.
>         (GLIBCXX_CHECK_PTHREAD_MUTEX_CLOCKLOCK): Likewise for
>         _GLIBCXX_USE_PTHREAD_MUTEX_CLOCKLOCK.
>         (GLIBCXX_CHECK_PTHREAD_RWLOCK_CLOCKLOCK): Likewise for
>         _GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK.
>         * configure: Regenerate.
> ---
>  libstdc++-v3/acinclude.m4 | 6 +++---
>  libstdc++-v3/configure    | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> 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
>
>    CXXFLAGS="$ac_save_CXXFLAGS"
> @@ -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"
> @@ -4346,7 +4346,7 @@ AC_DEFUN([GLIBCXX_CHECK_PTHREAD_RWLOCK_CLOCKLOCK], [
>        [glibcxx_cv_PTHREAD_RWLOCK_CLOCKLOCK=no])
>    ])
>    if test $glibcxx_cv_PTHREAD_RWLOCK_CLOCKLOCK = yes; then
> -    AC_DEFINE(_GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK, 1, [Define if
> pthread_rwlock_clockrdlock and pthread_rwlock_clockwrlock are available in
> <pthread.h>.])
> +    AC_DEFINE(_GLIBCXX_USE_PTHREAD_RWLOCK_CLOCKLOCK, (_GLIBCXX_TSAN==0),
> [Define if pthread_rwlock_clockrdlock and pthread_rwlock_clockwrlock are
> available in <pthread.h>.])
>    fi
>
>    CXXFLAGS="$ac_save_CXXFLAGS"
>
>

Re: [RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Mike Crowe]

On Wednesday 10 May 2023 at 12:31:12 +0100, Jonathan Wakely wrote:
> On Wed, 10 May 2023 at 12:20, Jonathan Wakely via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
> 
> > This patch would avoid TSan false positives when using timed waiting
> > functions on mutexes and condvars, but as noted below, it changes the
> > semantics.
> >
> > I'm not sure whether we want this workaround in place until tsan gets
> > fixed.
> >
> > On one hand, there's no guarantee that those functions use the right
> > clock anyway (and they won't do unless a recent-ish glibc is used). But
> > on the other hand, if they normally would use the right clock because
> > you have glibc support, it's not ideal for tsan to cause a different
> > clock to be used.
> >
> 
> But of course, it's not ideal to get false positives from tsan either
> (especially when it looks like a libstdc++ bug, as initially reported to
> me).

I think that this is probably the least-worst option in the short term. As
TSan is distributed with GCC this workaround can be removed as soon as its
TSan implementation gains the necessary interceptors. I shall look into
trying to do that.

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?

See: https://github.com/google/sanitizers/issues/1259

Thanks.

Mike.

Re: [RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Thu, 11 May 2023 at 13:19, Mike Crowe <mac@mcrowe.com> wrote:

> On Wednesday 10 May 2023 at 12:31:12 +0100, Jonathan Wakely wrote:
> > On Wed, 10 May 2023 at 12:20, Jonathan Wakely via Libstdc++ <
> > libstdc++@gcc.gnu.org> wrote:
> >
> > > This patch would avoid TSan false positives when using timed waiting
> > > functions on mutexes and condvars, but as noted below, it changes the
> > > semantics.
> > >
> > > I'm not sure whether we want this workaround in place until tsan gets
> > > fixed.
> > >
> > > On one hand, there's no guarantee that those functions use the right
> > > clock anyway (and they won't do unless a recent-ish glibc is used). But
> > > on the other hand, if they normally would use the right clock because
> > > you have glibc support, it's not ideal for tsan to cause a different
> > > clock to be used.
> > >
> >
> > But of course, it's not ideal to get false positives from tsan either
> > (especially when it looks like a libstdc++ bug, as initially reported to
> > me).
>
> I think that this is probably the least-worst option in the short term. As
> TSan is distributed with GCC this workaround can be removed as soon as its
> TSan implementation gains the necessary interceptors. I shall look into
> trying to do that.
>

Right, and before it gets into GCC it will already be upstream in LLVM, so
a recent Clang would support it too by the time we changed anything in
libstdc++.

Another option would be just document how to use
https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions for
runtime suppressions, but that would be far from ideal.




> 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.


>
> See: https://github.com/google/sanitizers/issues/1259
>
>
Thanks, I've added a link to my new tsan issue there.

Re: [RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Thu, May 11, 2023 at 5:21 AM Mike Crowe via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> On Wednesday 10 May 2023 at 12:31:12 +0100, Jonathan Wakely wrote:
> > On Wed, 10 May 2023 at 12:20, Jonathan Wakely via Libstdc++ <
> > libstdc++@gcc.gnu.org> wrote:
> >
> > > This patch would avoid TSan false positives when using timed waiting
> > > functions on mutexes and condvars, but as noted below, it changes the
> > > semantics.
> > >
> > > I'm not sure whether we want this workaround in place until tsan gets
> > > fixed.
> > >
> > > On one hand, there's no guarantee that those functions use the right
> > > clock anyway (and they won't do unless a recent-ish glibc is used). But
> > > on the other hand, if they normally would use the right clock because
> > > you have glibc support, it's not ideal for tsan to cause a different
> > > clock to be used.
> > >
> >
> > But of course, it's not ideal to get false positives from tsan either
> > (especially when it looks like a libstdc++ bug, as initially reported to
> > me).
>
> I think that this is probably the least-worst option in the short term. As
> TSan is distributed with GCC this workaround can be removed as soon as its
> TSan implementation gains the necessary interceptors. I shall look into
> trying to do that.
>
>
I don't have a strong opinion either way on this, but I think documenting
the TSAN suppressions is the option most in keeping with the principle of
Least Astonishment.


> 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?
>
> See: https://github.com/google/sanitizers/issues/1259
>
> Thanks.
>
> Mike.
>
>

Re: [RFC] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On Thu, 11 May 2023 at 16:54, Thomas Rodgers <trodgers@redhat.com> wrote:

>
>
> On Thu, May 11, 2023 at 5:21 AM Mike Crowe via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
>
>> On Wednesday 10 May 2023 at 12:31:12 +0100, Jonathan Wakely wrote:
>> > On Wed, 10 May 2023 at 12:20, Jonathan Wakely via Libstdc++ <
>> > libstdc++@gcc.gnu.org> wrote:
>> >
>> > > This patch would avoid TSan false positives when using timed waiting
>> > > functions on mutexes and condvars, but as noted below, it changes the
>> > > semantics.
>> > >
>> > > I'm not sure whether we want this workaround in place until tsan gets
>> > > fixed.
>> > >
>> > > On one hand, there's no guarantee that those functions use the right
>> > > clock anyway (and they won't do unless a recent-ish glibc is used).
>> But
>> > > on the other hand, if they normally would use the right clock because
>> > > you have glibc support, it's not ideal for tsan to cause a different
>> > > clock to be used.
>> > >
>> >
>> > But of course, it's not ideal to get false positives from tsan either
>> > (especially when it looks like a libstdc++ bug, as initially reported to
>> > me).
>>
>> I think that this is probably the least-worst option in the short term. As
>> TSan is distributed with GCC this workaround can be removed as soon as its
>> TSan implementation gains the necessary interceptors. I shall look into
>> trying to do that.
>>
>>
> I don't have a strong opinion either way on this, but I think documenting
> the TSAN suppressions is the option most in keeping with the principle of
> Least Astonishment.
>

That assumes anybody reads the docs :-)
Getting TSan errors from the std::lib is somewhat astonishing. The errors
could be avoided, at the risk of subtle timing differences between
tsanitized and un-tsanitized builds ... but won't there be subtle diffs
anyway based on the TSan overhead? Admittedly those will just be fairly
constant overhead, and so immune to system clock adjustments.

[PATCH v2] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

[-- Attachment #1.1: Type: text/plain, Size: 1551 bytes --]

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?!)

So I'm now retesting with this version of the patch, which only touches the
USE_PTHREAD_LOCKLOCK macro.

Please take another look, thanks.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1711 bytes --]

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"

Re: [PATCH v2] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Mike Crowe]

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.

> 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.

Re: [PATCH v2] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 4841 bytes --]

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.
>
>

Re: [PATCH v2] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Mike Crowe]

On Friday 12 May 2023 at 11:32:56 +0100, Jonathan Wakely wrote:
> 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?

I'm by no means an expert, but I would guess not. I suspect that the
renaming was introduced as part of the Y2038 fixes and TSan hasn't caught
up with them either.

Mike.

Re: [PATCH v2] libstdc++: Do not use pthread_mutex_clocklock with ThreadSanitizer

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Thu, May 11, 2023 at 1:52 PM Jonathan Wakely <jwakely@redhat.com> 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?!)
>
> So I'm now retesting with this version of the patch, which only touches
> the USE_PTHREAD_LOCKLOCK macro.
>
> Please take another look, thanks.
>
> LGTM.