Fix 'libstdc++-v3/src/c++20/tzdb.cc' build for '__GTHREADS && !__GTHREADS_CXX0X' configurations (was: libstdc++: '_GLIBCXX_HAS_GTHREADS' -- '__GTHREADS' vs. '__GTHREADS_CXX0X')

[Jonathan Wakely]

On 20/02/25 15:50 +0100, Thomas Schwinge wrote:
>Hi!
>
>On 2025-02-20T14:14:44+0000, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On Thu, 20 Feb 2025 at 14:08, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> On Thu, 20 Feb 2025 at 13:53, Thomas Schwinge <tschwinge@baylibre.com> wrote:
>>> > On 2025-02-20T12:51:15+0000, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> > > On Thu, 20 Feb 2025 at 12:29, Thomas Schwinge <tschwinge@baylibre.com> wrote:
>>> > >> I'm still working on libstdc++ support for GCC's GPU targets: GCN, nvptx.
>>> > >> These configurations are (in typical use) multi-threaded, and support
>>> > >> mutexes etc., but they don't support dynamic spawning of threads etc.
>>> > >> Therefore, re 'libgcc/gthr.h', they define '__GTHREADS' but *not*
>>> > >> '__GTHREADS_CXX0X'.  (See 'libgcc/config/gcn/gthr-gcn.h'; nvptx still to
>>> > >> be done, currently 'Thread model: single', very likely bogus...)
>>> > >>
>>> > >> Now, 'libstdc++-v3/acinclude.m4:GLIBCXX_CHECK_GTHREADS' does:
>>> > >>
>>> > >>     [...]
>>> > >>       AC_MSG_CHECKING([for gthreads library])
>>> > >>
>>> > >>       AC_TRY_COMPILE([#include "gthr.h"],
>>> > >>         [
>>> > >>           #ifndef __GTHREADS_CXX0X
>>> > >>           #error
>>> > >>           #endif
>>> > >>         ], [ac_has_gthreads=yes], [ac_has_gthreads=no])
>>> > >>       else
>>> > >>         ac_has_gthreads=no
>>> > >>       fi
>>> > >>
>>> > >>       AC_MSG_RESULT([$ac_has_gthreads])
>>> > >>
>>> > >>       if test x"$ac_has_gthreads" = x"yes"; then
>>> > >>         AC_DEFINE(_GLIBCXX_HAS_GTHREADS, 1,
>>> > >>                   [Define if gthreads library is available.])
>>> > >>     [...]
>>> > >>
>>> > >> That is, it defines '_GLIBCXX_HAS_GTHREADS' per '__GTHREADS_CXX0X'.
>>> > >> Dependent on this '_GLIBCXX_HAS_GTHREADS',
>>> > >> 'libstdc++-v3/include/bits/std_mutex.h' then enables 'class mutex'.  In
>>> > >> other words, in the current GCN configuration ('__GTHREADS', but not
>>> > >> '__GTHREADS_CXX0X'), 'class mutex' (and probably more) is not available,
>>> > >> which leads to build issues (and presumably a lot of noise in the test
>>> > >> suite).
>>> > >
>>> > > What are the build issues? There shouldn't be any, because we should
>>> > > be checking _GLIBCXX_HAS_GTHREADS as needed.
>>> >
>>> > The build issue is 'libstdc++-v3/src/c++20/tzdb.cc', which checks
>>> > '#if defined __GTHREADS' (which GCN defines), and then assumes that
>>> > 'mutex' is available,
>>>
>>> Right, that's a bug (and not the first time I've done that).
>
>Specifically:
>
>    ../../../../../source-gcc/libstdc++-v3/src/c++20/tzdb.cc:687:9: error: ‘mutex’ does not name a type; did you mean ‘minutes’? [-Wtemplate-body]
>      687 |         mutex infos_mutex;
>          |         ^~~~~
>          |         minutes

Ah right, so the error is later.

>>> > but that one via '_GLIBCXX_HAS_GTHREADS' is guarded
>>> > on '__GTHREADS_CXX0X' (which GCN doesn't define).
>>> >
>>> > Now clearly 'std::chrono::tzdb' is not going to be used in offloaded code
>>> > regions, but (a) famous last words..., and (b) I was more worried about
>>> > the general case, where code (user code or libstdc++-internal) would see
>>> > '!_GLIBCXX_HAS_GTHREADS', and from that deduce that locking for
>>> > concurrent access etc. is not necessary.
>>> >
>>> > > And the testsuite should use dg-require-gthreads.
>>> >
>>> > > So instead of trying to make the C++11 thread library work without the
>>> > > necessary gthreads support
>>> >
>>> > Right, we're not attempting to make 'std::thread::detach' work, for
>>> > example.  But I was assuming that we could/had to make 'std::mutex'
>>> > etc. work, so that algorithms in presence of "external parallelism" still
>>> > do the appropriate locking etc.
>>> >
>>> > Like, when used within a GPU parallel kernel, 'std::chrono::tzdb' needs
>>> > to 'mutex' its internal state, thus, guarding that on '__GTHREADS' would
>>> > seem correct, even if '!__GTHREADS_CXX0X'?
>>>
>>> Yes. Elsewhere in the library we do that with __gnu_cxx::__mutex
>>> instead of std::mutex.
>
>So you're saying, for '__GTHREADS && !__GTHREADS_CXX0X' configurations,
>libstdc++ internally still does locking (via '__GTHREADS' primitives),
>just the user-facing 'std::mutex' etc. isn't available?

Yes, there's a __gnu_cxx::__mutex type that has existed since before
__GTHREADS_CXX0X was added, and only requires __GTHREADS.

>>> I think this should fix it:
>>>
>>> --- a/libstdc++-v3/src/c++20/tzdb.cc
>>> +++ b/libstdc++-v3/src/c++20/tzdb.cc
>>> @@ -48,6 +48,7 @@
>>> #else
>>> # define USE_ATOMIC_LIST_HEAD 0
>>> # define USE_ATOMIC_SHARED_PTR 0
>>> +# include <ext/concurrence.h>
>>> #endif
>>>
>>> #if USE_ATOMIC_SHARED_PTR && ! USE_ATOMIC_LIST_HEAD
>>> @@ -101,6 +102,8 @@ namespace std::chrono
>>> #ifndef __GTHREADS
>>>     // Dummy no-op mutex type for single-threaded targets.
>>>     struct mutex { void lock() { } void unlock() { } };
>>> +#elif ! defined _GLIBCXX_HAS_GHTREADS
>>
>> Oops, s/GHTR/GTHR/
>>
>>> +    using mutex = __gnu_cxx::__mutex;
>>> #endif
>>>     inline mutex& list_mutex()
>>>     {
>
>GCN has 'defined __GTHREADS && ATOMIC_POINTER_LOCK_FREE == 2', so we
>have:
>
>    #if defined __GTHREADS && ATOMIC_POINTER_LOCK_FREE == 2
>    # define USE_ATOMIC_LIST_HEAD 1
>    // TODO benchmark atomic<shared_ptr<>> vs mutex.
>    # define USE_ATOMIC_SHARED_PTR 1
>
>Maybe the attached
>"Fix 'libstdc++-v3/src/c++20/tzdb.cc' build for '__GTHREADS && !__GTHREADS_CXX0X' configurations"?
>Only GCN, nvptx build-tested so far.
>
>
>Grüße
> Thomas
>
>
>>> We could also enable std::mutex for the __GTHREADS && !
>>> __GTHREADS_CXX0X case but that should probably wait for stage 1.
>>>
>>>
>>>
>>> >
>>> > > maybe you just need to identify a handful
>>> > > of bugs where we aren't checking the right macros in the libstdc++
>>> > > sources?
>>> > >
>>> > >> Is '__GTHREADS != __GTHREADS_CXX0X' simply a configuration that libstdc++
>>> > >> so far has not attempted to support?  To make that work, do we need to
>>> > >> consider '__GTHREADS' vs. '__GTHREADS_CXX0X' individually in libstdc++,
>>> > >> for guarding the respective features?
>>> >
>>> > Another option for GCN, nvptx -- and maybe that's actually easiest: we
>>> > define '__GTHREADS_CXX0X' in addition to '__GTHREADS', and implement the
>>> > additional interfaces as "operation is not supported, -1 is returned"?
>>> > That should make libstdc++ "happy" and give a consistent user experience
>>> > (like, when the OS is out of resources)?
>>> >
>>> >
>>> > Grüße
>>> >  Thomas
>>> >
>
>

>>From 820e015494e25187c9a5ffbd69911ec6ce612789 Mon Sep 17 00:00:00 2001
>From: Jonathan Wakely <jwakely@redhat.com>
>Date: Thu, 20 Feb 2025 14:08:11 +0000
>Subject: [PATCH] Fix 'libstdc++-v3/src/c++20/tzdb.cc' build for '__GTHREADS &&
> !__GTHREADS_CXX0X' configurations
>
>	libstdc++-v3/
>	* src/c++20/tzdb.cc [__GTHREADS && !__GTHREADS_CXX0X]: Use
>	'__gnu_cxx::__mutex'.
>
>Co-authored-by: Thomas Schwinge <tschwinge@baylibre.com>
>---
> libstdc++-v3/src/c++20/tzdb.cc | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/src/c++20/tzdb.cc b/libstdc++-v3/src/c++20/tzdb.cc
>index 7e8cce7ce8c..70ba7b0ef53 100644
>--- a/libstdc++-v3/src/c++20/tzdb.cc
>+++ b/libstdc++-v3/src/c++20/tzdb.cc
>@@ -35,6 +35,9 @@
> #include <atomic>     // atomic<T*>, atomic<int>
> #include <memory>     // atomic<shared_ptr<T>>
> #include <mutex>      // mutex
>+#if defined __GTHREADS && ! defined _GLIBCXX_HAS_GHTREADS
>+# include <ext/concurrence.h> // __gnu_cxx::__mutex
>+#endif
> #include <filesystem> // filesystem::read_symlink
> 
> #ifndef _AIX
>@@ -97,11 +100,14 @@ namespace std::chrono
> {
>   namespace
>   {
>-#if ! USE_ATOMIC_SHARED_PTR
> #ifndef __GTHREADS
>     // Dummy no-op mutex type for single-threaded targets.
>     struct mutex { void lock() { } void unlock() { } };
>+#elif ! defined _GLIBCXX_HAS_GHTREADS

This still has my GHTREADS typo.

A comment here would be good too:

     // Use __gnu_cxx::__mutex is std::mutex isn't available.

>+    using mutex = __gnu_cxx::__mutex;
> #endif
>+
>+#if ! USE_ATOMIC_SHARED_PTR
>     inline mutex& list_mutex()
>     {
> #ifdef __GTHREAD_MUTEX_INIT

Strictly speaking, we also need a change here, because unlike
std::mutex, __gnu_cxx::__mutex can't be initialized with `constinit`.
But that can wait, because it's not needed for your configuration.