On 20/02/25 15:50 +0100, Thomas Schwinge wrote:
Hi!
On 2025-02-20T14:14:44+0000, Jonathan Wakely <jwak...@redhat.com> wrote:
On Thu, 20 Feb 2025 at 14:08, Jonathan Wakely <jwak...@redhat.com> wrote:
On Thu, 20 Feb 2025 at 13:53, Thomas Schwinge <tschwi...@baylibre.com> wrote:
> On 2025-02-20T12:51:15+0000, Jonathan Wakely <jwak...@redhat.com> wrote:
> > On Thu, 20 Feb 2025 at 12:29, Thomas Schwinge <tschwi...@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 <jwak...@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 <tschwi...@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.
