diff mbox

[v3] PR 53270 fix hppa-linux bootstrap regression

Message ID CAH6eHdSmzPFwWXD2Y8fEzzMZkR-w_uKgkZZbkX8FOy59k-g9YA@mail.gmail.com
State New
Headers show

Commit Message

Jonathan Wakely July 7, 2012, 5:16 p.m. UTC 
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 :-)
commit b609d324bdbc2916d13205108377883962e9bbca
Author: Jonathan Wakely <jwakely.gcc@gmail.com>
Date:   Sat Jul 7 14:40:43 2012 +0100

    	PR libstdc++/53270
    	* include/ext/concurrence.h (__mutex, __recursive_mutex, __cond): Use
    	NSDMI in C++98 mode too.
    	* include/ext/rope (_Refcount_Base): Likewise. Add system_header
    	pragma.

Comments

Jonathan Wakely July 16, 2012, 9:48 p.m. UTC | #1 
On 7 July 2012 18:16, Jonathan Wakely wrote:
>
> 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 :-)

No comments?

Then I'm going for my preferences as stated above.
diff mbox

Patch

diff --git a/libstdc++-v3/include/ext/concurrence.h b/libstdc++-v3/include/ext/concurrence.h
index 25e218b..79bf0b6 100644
--- a/libstdc++-v3/include/ext/concurrence.h
+++ b/libstdc++-v3/include/ext/concurrence.h
@@ -143,8 +143,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   class __mutex 
   {
   private:
-#if __GTHREADS && defined __GTHREAD_MUTEX_INIT \
-    && defined __GXX_EXPERIMENTAL_CXX0X__
+#if __GTHREADS && defined __GTHREAD_MUTEX_INIT
     __gthread_mutex_t _M_mutex = __GTHREAD_MUTEX_INIT;
 #else
     __gthread_mutex_t _M_mutex;
@@ -156,19 +155,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   public:
     __mutex() 
     { 
-#if __GTHREADS
+#if __GTHREADS && ! defined __GTHREAD_MUTEX_INIT
       if (__gthread_active_p())
-	{
-#if defined __GTHREAD_MUTEX_INIT
-# ifndef __GXX_EXPERIMENTAL_CXX0X__
-	  __gthread_mutex_t __tmp = __GTHREAD_MUTEX_INIT;
-	  _M_mutex = __tmp;
-# endif
-#else
-	  __GTHREAD_MUTEX_INIT_FUNCTION(&_M_mutex); 
+	__GTHREAD_MUTEX_INIT_FUNCTION(&_M_mutex);
 #endif
-	}
-#endif 
     }
 
 #if __GTHREADS && ! defined __GTHREAD_MUTEX_INIT
@@ -208,8 +198,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   class __recursive_mutex 
   {
   private:
-#if __GTHREADS && defined __GTHREAD_RECURSIVE_MUTEX_INIT \
-    && defined __GXX_EXPERIMENTAL_CXX0X__
+#if __GTHREADS && defined __GTHREAD_RECURSIVE_MUTEX_INIT
     __gthread_recursive_mutex_t _M_mutex = __GTHREAD_RECURSIVE_MUTEX_INIT;
 #else
     __gthread_recursive_mutex_t _M_mutex;
@@ -221,19 +210,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   public:
     __recursive_mutex() 
     { 
-#if __GTHREADS
+#if __GTHREADS && ! defined __GTHREAD_RECURSIVE_MUTEX_INIT
       if (__gthread_active_p())
-	{
-#if defined __GTHREAD_RECURSIVE_MUTEX_INIT
-# ifndef __GXX_EXPERIMENTAL_CXX0X__
-	  __gthread_recursive_mutex_t __tmp = __GTHREAD_RECURSIVE_MUTEX_INIT;
-	  _M_mutex = __tmp;
-# endif
-#else
-	  __GTHREAD_RECURSIVE_MUTEX_INIT_FUNCTION(&_M_mutex); 
+	__GTHREAD_RECURSIVE_MUTEX_INIT_FUNCTION(&_M_mutex);
 #endif
-	}
-#endif 
     }
 
 #if __GTHREADS && ! defined __GTHREAD_RECURSIVE_MUTEX_INIT
@@ -333,8 +313,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   class __cond
   {
   private:
-#if __GTHREADS && defined __GTHREAD_COND_INIT \
-    && defined __GXX_EXPERIMENTAL_CXX0X__
+#if __GTHREADS && defined __GTHREAD_COND_INIT
     __gthread_cond_t _M_cond = __GTHREAD_COND_INIT;
 #else
     __gthread_cond_t _M_cond;
@@ -346,19 +325,10 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   public:
     __cond() 
     { 
-#if __GTHREADS
+#if __GTHREADS && ! defined __GTHREAD_COND_INIT
       if (__gthread_active_p())
-	{
-#if defined __GTHREAD_COND_INIT
-# ifndef __GXX_EXPERIMENTAL_CXX0X__
-	  __gthread_cond_t __tmp = __GTHREAD_COND_INIT;
-	  _M_cond = __tmp;
-# endif
-#else
-	  __GTHREAD_COND_INIT_FUNCTION(&_M_cond);
+	__GTHREAD_COND_INIT_FUNCTION(&_M_cond);
 #endif
-	}
-#endif 
     }
 
 #if __GTHREADS && ! defined __GTHREAD_COND_INIT
diff --git a/libstdc++-v3/include/ext/rope b/libstdc++-v3/include/ext/rope
index 15cb423..3364f46 100644
--- a/libstdc++-v3/include/ext/rope
+++ b/libstdc++-v3/include/ext/rope
@@ -44,6 +44,8 @@ 
 #ifndef _ROPE
 #define _ROPE 1
 
+#pragma GCC system_header
+
 #include <algorithm>
 #include <iosfwd>
 #include <bits/stl_construct.h>
@@ -458,7 +460,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     volatile _RC_t _M_ref_count;
 
     // Constructor
-#if defined __GTHREAD_MUTEX_INIT && defined __GXX_EXPERIMENTAL_CXX0X__
+#ifdef __GTHREAD_MUTEX_INIT
     __gthread_mutex_t _M_ref_count_lock = __GTHREAD_MUTEX_INIT;
 #else
     __gthread_mutex_t _M_ref_count_lock;
@@ -466,16 +468,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     _Refcount_Base(_RC_t __n) : _M_ref_count(__n), _M_ref_count_lock()
     {
-#ifdef __GTHREAD_MUTEX_INIT
-# ifndef __GXX_EXPERIMENTAL_CXX0X__
-      __gthread_mutex_t __tmp = __GTHREAD_MUTEX_INIT;
-      _M_ref_count_lock = __tmp;
-# endif
-#elif defined(__GTHREAD_MUTEX_INIT_FUNCTION)
+#ifndef __GTHREAD_MUTEX_INIT
+#ifdef __GTHREAD_MUTEX_INIT_FUNCTION
       __GTHREAD_MUTEX_INIT_FUNCTION (&_M_ref_count_lock);
 #else
 #error __GTHREAD_MUTEX_INIT or __GTHREAD_MUTEX_INIT_FUNCTION should be defined by gthr.h abstraction layer, report problem to libstdc++@gcc.gnu.org.
 #endif
+#endif
     }
 
 #ifndef __GTHREAD_MUTEX_INIT