Patchwork proposed fix for libstdc++/54352

login
register
mail settings
Submitter Jonathan Wakely
Date June 16, 2013, 2:35 p.m.
Message ID <CAH6eHdSWwnhirSQ9u5qsSzKHyEObO1dg8iVLyeE9bOLkbfoXwQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/251684/
State New
Headers show

Comments

Jonathan Wakely - June 16, 2013, 2:35 p.m.
In order to conform to  [thread.condition.condvarany]/5 and fix this
PR we need an ABI change to std::condition_variable_any, so that its
internal mutex can outlive the condition_variable_any while there are
threads blocked on the mutex, which this patch does by using a
shared_ptr to hold the mutex and making waiting threads share
ownership of that mutex.

The ABI change is handled similarly to the recent chrono::steady_clock
changes, using an inline namespace for the new versions and continuing
to export the old version from the library for compatibility with old
code.

Any thoughts or suggestions on improving this before I commit it to trunk?

I can no longer reproduce the valgrind errors, except by hacking in
changes to std::mutex to cause races, so I haven't been able to write
a test case that fails without this change, but I believe it's still
necessary for correctness.

        PR libstdc++/54352
        * include/std/condition_variable (condition_variable_any): Move into
        inline namespace _V2 and replace mutex member with shared_ptr<mutex>.
        * src/c++11/condition_variable.cc (condition_variable_any): Move
        definitions to ...
        * src/c++11/compatibility-condvar.cc (condition_variable_any): Here.
        * src/Makefile.am: Add new source file.
        * src/Makefile.in: Regenerate.
commit 60d115e1b9631adf1476a77c71224257a7c5ca7d
Author: Jonathan Wakely <jwakely.gcc@gmail.com>
Date:   Sun Jun 16 12:38:32 2013 +0100

    	PR libstdc++/54352
    	* include/std/condition_variable (condition_variable_any): Move into
    	inline namespace _V2 and replace mutex member with shared_ptr<mutex>.
    	* src/c++11/condition_variable.cc (condition_variable_any): Move
    	definitions to ...
    	* src/c++11/compatibility-condvar.cc (condition_variable_any): Here.
    	* src/Makefile.am: Add new source file.
    	* src/Makefile.in: Regenerate.
Jonathan Wakely - July 21, 2013, 7:21 p.m.
On 16 June 2013 15:35, Jonathan Wakely wrote:
> In order to conform to  [thread.condition.condvarany]/5 and fix this
> PR we need an ABI change to std::condition_variable_any, so that its
> internal mutex can outlive the condition_variable_any while there are
> threads blocked on the mutex, which this patch does by using a
> shared_ptr to hold the mutex and making waiting threads share
> ownership of that mutex.
>
> The ABI change is handled similarly to the recent chrono::steady_clock
> changes, using an inline namespace for the new versions and continuing
> to export the old version from the library for compatibility with old
> code.
>
> Any thoughts or suggestions on improving this before I commit it to trunk?
>
> I can no longer reproduce the valgrind errors, except by hacking in
> changes to std::mutex to cause races, so I haven't been able to write
> a test case that fails without this change, but I believe it's still
> necessary for correctness.
>
>         PR libstdc++/54352
>         * include/std/condition_variable (condition_variable_any): Move into
>         inline namespace _V2 and replace mutex member with shared_ptr<mutex>.
>         * src/c++11/condition_variable.cc (condition_variable_any): Move
>         definitions to ...
>         * src/c++11/compatibility-condvar.cc (condition_variable_any): Here.
>         * src/Makefile.am: Add new source file.
>         * src/Makefile.in: Regenerate.

I've just committed this to trunk.

Patch

diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 5284655..76a6e94 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -36,7 +36,12 @@ 
 #else
 
 #include <chrono>
-#include <mutex> // unique_lock
+#include <mutex>
+#include <ext/concurrence.h>
+#include <bits/alloc_traits.h>
+#include <bits/allocator.h>
+#include <bits/unique_ptr.h>
+#include <bits/shared_ptr.h>
 
 #if defined(_GLIBCXX_HAS_GTHREADS) && defined(_GLIBCXX_USE_C99_STDINT_TR1)
 
@@ -165,13 +170,15 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
   };
 
+  inline namespace _V2 {
+
   /// condition_variable_any
   // Like above, but mutex is not required to have try_lock.
   class condition_variable_any
   {
     typedef chrono::system_clock	__clock_t;
     condition_variable			_M_cond;
-    mutex				_M_mutex;
+    shared_ptr<mutex>			_M_mutex;
 
     // scoped unlock - unlocks in ctor, re-locks in dtor
     template<typename _Lock>
@@ -194,9 +201,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       };
 
   public:
-
-    condition_variable_any() noexcept;
-    ~condition_variable_any() noexcept;
+    condition_variable_any() : _M_mutex(std::make_shared<mutex>()) { }
+    ~condition_variable_any() = default;
 
     condition_variable_any(const condition_variable_any&) = delete;
     condition_variable_any& operator=(const condition_variable_any&) = delete;
@@ -204,14 +210,14 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     void
     notify_one() noexcept
     {
-      lock_guard<mutex> __lock(_M_mutex);
+      lock_guard<mutex> __lock(*_M_mutex);
       _M_cond.notify_one();
     }
 
     void
     notify_all() noexcept
     {
-      lock_guard<mutex> __lock(_M_mutex);
+      lock_guard<mutex> __lock(*_M_mutex);
       _M_cond.notify_all();
     }
 
@@ -219,10 +225,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       wait(_Lock& __lock)
       {
-	unique_lock<mutex> __my_lock(_M_mutex);
+	shared_ptr<mutex> __mutex = _M_mutex;
+	unique_lock<mutex> __my_lock(*__mutex);
 	_Unlock<_Lock> __unlock(__lock);
-	// _M_mutex must be unlocked before re-locking __lock so move
-	// ownership of _M_mutex lock to an object with shorter lifetime.
+	// *__mutex must be unlocked before re-locking __lock so move
+	// ownership of *__mutex lock to an object with shorter lifetime.
 	unique_lock<mutex> __my_lock2(std::move(__my_lock));
 	_M_cond.wait(__my_lock2);
       }
@@ -241,10 +248,11 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       wait_until(_Lock& __lock,
 		 const chrono::time_point<_Clock, _Duration>& __atime)
       {
-	unique_lock<mutex> __my_lock(_M_mutex);
+	shared_ptr<mutex> __mutex = _M_mutex;
+	unique_lock<mutex> __my_lock(*__mutex);
 	_Unlock<_Lock> __unlock(__lock);
-	// _M_mutex must be unlocked before re-locking __lock so move
-	// ownership of _M_mutex lock to an object with shorter lifetime.
+	// *__mutex must be unlocked before re-locking __lock so move
+	// ownership of *__mutex lock to an object with shorter lifetime.
 	unique_lock<mutex> __my_lock2(std::move(__my_lock));
 	return _M_cond.wait_until(__my_lock2, __atime);
       }
@@ -275,6 +283,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return wait_until(__lock, __clock_t::now() + __rtime, std::move(__p)); }
   };
 
+  } // end inline namespace
+
   // @} group condition_variables
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
diff --git a/libstdc++-v3/src/Makefile.am b/libstdc++-v3/src/Makefile.am
index 092fd1b..3e11aa3 100644
--- a/libstdc++-v3/src/Makefile.am
+++ b/libstdc++-v3/src/Makefile.am
@@ -51,7 +51,8 @@  cxx11_sources = \
 	compatibility-c++0x.cc \
 	compatibility-atomic-c++0x.cc \
 	compatibility-thread-c++0x.cc \
-	compatibility-chrono.cc
+	compatibility-chrono.cc \
+	compatibility-condvar.cc
 
 libstdc___la_SOURCES = $(cxx98_sources) $(cxx11_sources)
 
@@ -103,6 +104,11 @@  compatibility-chrono.lo: compatibility-chrono.cc
 compatibility-chrono.o: compatibility-chrono.cc
 	$(CXXCOMPILE) -std=gnu++11 -c $<
 
+compatibility-condvar.lo: compatibility-condvar.cc
+	$(LTCXXCOMPILE) -std=gnu++11 -c $<
+compatibility-condvar.o: compatibility-condvar.cc
+	$(CXXCOMPILE) -std=gnu++11 -c $<
+
 # A note on compatibility and static libraries.
 # 
 # static lib == linked against only this version, should not need compat
diff --git a/libstdc++-v3/src/Makefile.in b/libstdc++-v3/src/Makefile.in
index a7b3404..93215e0 100644
--- a/libstdc++-v3/src/Makefile.in
+++ b/libstdc++-v3/src/Makefile.in
@@ -93,7 +93,8 @@  am__DEPENDENCIES_1 =
 am__objects_2 = compatibility.lo compatibility-debug_list.lo \
 	compatibility-debug_list-2.lo $(am__objects_1)
 am__objects_3 = compatibility-c++0x.lo compatibility-atomic-c++0x.lo \
-	compatibility-thread-c++0x.lo compatibility-chrono.lo
+	compatibility-thread-c++0x.lo compatibility-chrono.lo \
+	compatibility-condvar.lo
 am_libstdc___la_OBJECTS = $(am__objects_2) $(am__objects_3)
 libstdc___la_OBJECTS = $(am_libstdc___la_OBJECTS)
 DEFAULT_INCLUDES = -I.@am__isrc@ -I$(top_builddir)
@@ -352,7 +353,8 @@  cxx11_sources = \
 	compatibility-c++0x.cc \
 	compatibility-atomic-c++0x.cc \
 	compatibility-thread-c++0x.cc \
-	compatibility-chrono.cc
+	compatibility-chrono.cc \
+	compatibility-condvar.cc
 
 libstdc___la_SOURCES = $(cxx98_sources) $(cxx11_sources)
 libstdc___la_LIBADD = \
@@ -854,6 +856,11 @@  compatibility-chrono.lo: compatibility-chrono.cc
 compatibility-chrono.o: compatibility-chrono.cc
 	$(CXXCOMPILE) -std=gnu++11 -c $<
 
+compatibility-condvar.lo: compatibility-condvar.cc
+	$(LTCXXCOMPILE) -std=gnu++11 -c $<
+compatibility-condvar.o: compatibility-condvar.cc
+	$(CXXCOMPILE) -std=gnu++11 -c $<
+
 # Symbol versioning for shared libraries.
 @ENABLE_SYMVERS_TRUE@libstdc++-symbols.ver:  ${glibcxx_srcdir}/$(SYMVER_FILE) \
 @ENABLE_SYMVERS_TRUE@		$(port_specific_symbol_files)
diff --git a/libstdc++-v3/src/c++11/compatibility-condvar.cc b/libstdc++-v3/src/c++11/compatibility-condvar.cc
new file mode 100644
index 0000000..006ffe4
--- /dev/null
+++ b/libstdc++-v3/src/c++11/compatibility-condvar.cc
@@ -0,0 +1,57 @@ 
+// Compatibility symbols for previous versions, C++0x bits -*- C++ -*-
+
+// Copyright (C) 2013 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// Under Section 7 of GPL version 3, you are granted additional
+// permissions described in the GCC Runtime Library Exception, version
+// 3.1, as published by the Free Software Foundation.
+
+// You should have received a copy of the GNU General Public License and
+// a copy of the GCC Runtime Library Exception along with this program;
+// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+// <http://www.gnu.org/licenses/>.
+
+#include <bits/c++config.h>
+
+#if __cplusplus < 201103L
+# error "compatibility-condvar-c++0x.cc must be compiled with -std=gnu++11"
+#endif
+
+#if defined(_GLIBCXX_HAS_GTHREADS) && defined(_GLIBCXX_USE_C99_STDINT_TR1)
+
+#define condition_variable_any condition_variable_anyXX
+#include <condition_variable>
+#undef condition_variable_any
+
+// XXX GLIBCXX_ABI Deprecated
+// gcc-4.9.0
+// std::condition_variable_any replaced with std::_V2::condition_variable_any
+namespace std _GLIBCXX_VISIBILITY(default)
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+  class condition_variable_any
+  {
+    condition_variable			_M_cond;
+    mutex				_M_mutex;
+
+  public:
+    condition_variable_any() noexcept;
+    ~condition_variable_any() noexcept;
+  };
+  condition_variable_any::condition_variable_any() noexcept = default;
+  condition_variable_any::~condition_variable_any() noexcept = default;
+_GLIBCXX_END_NAMESPACE_VERSION
+} // namespace std
+
+#endif // _GLIBCXX_HAS_GTHREADS && _GLIBCXX_USE_C99_STDINT_TR1
diff --git a/libstdc++-v3/src/c++11/condition_variable.cc b/libstdc++-v3/src/c++11/condition_variable.cc
index 12f475b..ce2bbf6 100644
--- a/libstdc++-v3/src/c++11/condition_variable.cc
+++ b/libstdc++-v3/src/c++11/condition_variable.cc
@@ -77,10 +77,6 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       __throw_system_error(__e);
   }
 
-  condition_variable_any::condition_variable_any() noexcept = default;
-
-  condition_variable_any::~condition_variable_any() noexcept = default;
-
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace