Re: [v3] PR 53270 fix hppa-linux bootstrap regression

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 14 June 2012 23:23, Jonathan Wakely wrote:
> We've known for ages that it's not portable to do:
>
> __gthread_mutex_t tmp = __GTHREAD_MUTEX_INIT;
> _M_mutex = __tmp;
>
> As PR 53270 shows, the copy assignment now actually fails in C++11
> mode on platforms using LinuxThreads, because the mutex has a volatile
> member so in C++11 mode the copy assignment operator is deleted.
>
> This patch changes <ext/concurrence.h> to use a
> brace-or-equals-initializer for the mutexes and condition variable in
> C++11 mode, allowing hppa-linux to bootstrap again and avoiding the
> non-portable construct everywhere (this is already the approach we
> take for std::mutex etc. in <mutex>). It also makes the same change to
> the mutex used in <ext/rope> and fixes a resource leak in that header
> by ensuring the mutex is destroyed when it was initialized by
> __gthread_mutex_init_function.
>
>         PR libstdc++/53270
>         * include/ext/concurrence.h (__mutex, __recursive_mutex, __cond): Use
>         NSDMI in C++11 mode.
>         * include/ext/rope (_Refcount_Base): Likewise. Destroy mutex in
>         destructor when initialized by function.
>
> Tested x86_64-linux, powerpc64-linux, hppa-linux and x86_64-netbsd (on
> the 4.7 branch instead, as netbsd doesn't bootstrap at the moment.)
> Committed to trunk.

I've come to the conclusion it would be better to use NSDMIs even in
C++98 mode, which works as a GCC extension, without a warning in
system headers.  This means using -Wsystem-headers -pedantic-errors
breaks libstdc++, but that's already the case for our uses of variadic
templates in C++98 mode.

The attached patch does that, any objections to me committing it to trunk?

Jason, I assume it's safe to rely on NSDMI working correctly in C++98
mode, or you'd have made it an error not a warning?

> For 4.6.4 and 4.7.2 I plan to make a less intrusive change, #undef'ing
> the __GTHREAD_MUTEX_INIT, _GTHREAD_RECURSIVE_MUTEX_INIT and
> __GTHREAD_COND_INIT macros on hppa-linux in C++11 mode, so that the
> init functions are used instead.  This fixes the bootstrap regression
> on hppa-linux without affecting other targets.

I've struggled to fix this on the release branches in a clean way.

Currently on the 4.7 branch config/os/gnu-linux/os_defines contains:

#if defined(__hppa__) && defined(__GXX_EXPERIMENTAL_CXX0X__)
# define _GTHREAD_USE_MUTEX_INIT_FUNC
# define _GTHREAD_USE_RECURSIVE_MUTEX_INIT_FUNC
# define _GTHREAD_USE_COND_INIT_FUNC
#endif

Those macros instruct gthr-posix.h to suppress the definition of
__GTHREADS_xxx_INIT, forcing the definition and use of
__gthreads_xxx_init_function and __ghtreads_xxx_destroy instead.

This works, but it forces the use of the init functions even for NPTL,
and IIUC the bootstrap failure only occurs when using LinuxThreads.
(Dave said in the PR trail "I know parisc had switched to NPTL in
Debian lenny.")

Always using the init functions is sub-optimal when the macros would
work.  I think there's also a correctness issue on hppa-linux with
NPTL because of the change in behaviour: if a __gnu_cxx::__mutex is
initialized by code compiled with GCC 4.7.2 and then destroyed by code
compiled with GCC 4.5.4 it will be initialized by pthread_mutex_init
but pthread_mutex_destroy will not be called.  I don't believe that's
a problem for either LinuxThreads or NPTL, as the destroy functions
don't actually deallocate or clean up anything.

A better fix is tricky so I'd like some second opinions.

I added a check to acinclude.m4 and configure.ac to define a new macro
in c++config.h indicating the INIT macros fail in C++11 mode, which
would be set when using LinuxThreads and not when not using NPTL.  But
macros defined by configure.ac only come after os_defines.h is
included, so the new macro can't be used in os_defines.h to control
whether to define the _GTHREAD_USE_xxx_INIT_FUNC macros.

The new macro defined by configure.ac can't be used directly in
<ext/concurrence.h> e.g.

#if __GTHREADS && defined __GTHREADS_MUTEX_INIT \
  && ! defined _GLIBCXX_USE_GTHREAD_INIT_FUNCTIONS_FOR_CXX11

because that would try to use __GTHREADS_MUTEX_INIT_FUNCTION, but
gthr-posix.h only defines *either* the INIT macro *or* the
INIT_FUNCTION macro and associated function.

That means changes are needed to gthr-posix.h so that it
unconditionally defines the INIT_FUNCTIONs, in case some other code
wants to use that function even when the macro is available, as shown
above (this is option 5 below) or change gthr-posix.h so it knows
about the new macro in addition to all the other macros it already
knows about (this is options 6 and 7 below).

A different fix would be to make configure.ac define the
_GTHREADS_USE_xxx_INIT_FUNC macros, using the existing preprocessor
conditions in gthr-posix.h to force use of the init functions not the
macros.  AFAIK configure.ac can't define something conditionally on
C++98/C++11 mode, so that would be a change in behaviour for
hppa-linux using LinuxThreads in C++98 mode compared to GCC 4.5 (the
last release that bootstrapped on that platform.)  (This is option 4
below)

So the options I see are:

1) (Only possible for 4.7, one of the changes below is needed for 4.6)
Use NSDMI as done on the trunk.  This touches code for all platforms,
but not in an incompatible way. It just replaces default init + copy
assignment with direct init using NSDMI.

2) Make the same change to 4.6 as is currently on the 4.7 branch,
forcing use of the INIT_FUNCTIONs on hppa-linux even for NPTL when it
isn't strictly needed.

3) Revert my previous change to os_defines.h on the 4.7 branch,
hppa-linux+LinuxThreads stays broken for 4.6 and 4.7, hppa-linux+NPTL
goes back to previous behaviour. Other platforms untouched.

4) Configure sets the _GTHREAD_USE_xxx_INIT_FUNC macros on
hppa-linux+LinuxThreads to force use of INIT_FUNCTIONs instead of INIT
macros, for both C++98 and C++11.  This is a change from GCC 4.5
behaviour on hppa-linux+LinuxThreads. hppa-linux+NPTL and other
platforms are untouched.

5) Change gthr-posix.h to always define the INIT_FUNCTIONS even if the
macro is defined.  Configure sets a new macro on
hppa-linux+LinuxThreads that tells <ext/concurrence.h> to use the
INIT_FUNCTIONs in C++11 mode, so mutex and condvar init/destroy
differs between c++98 and c++11 mode. Other platforms are unaffected
apart from some redundant code in gthr-posix.h.

6) Change gthr-posix.h to check the new macro and suppress definition
of the INIT macros. Configure sets that new macro on
hppa-linux+LinuxThreads. This is a change from GCC 4.5 behaviour on
hppa-linux+LinuxThreads, hppa-linux+NPTL and other platforms are
untouched.

7) Same as (6) but make gthr-posix.h only check the new macro in C++11
mode. Preserves GCC 4.5 behaviour for C++98 mode, means mutexes and
condvars init/destroy differs between C++98 and C++11 modes.

8) ???

AFAICT the changes in behaviour noted above, which involve calling
init/destroy functions instead of using the INIT macros, are harmless
for both LinuxThreads and NPTL. They do introduce ODR violations
though, when mixing code compiled with previous releases or in c++98
mode if that still uses the INIT macros.

My preference is (1) for 4.7 and (4) for 4.6 but the changes are not
ones I feel comfortable making on release branches without other
opinions.  Please comment :-)

Attachment: patch.txt
Description: Text document