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
>> > 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?
>> 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
+ using mutex = __gnu_cxx::__mutex;
#endif
+
+#if ! USE_ATOMIC_SHARED_PTR
inline mutex& list_mutex()
{
#ifdef __GTHREAD_MUTEX_INIT
--
2.34.1
