Patchwork [v3] fix libstdc++/53830

login
register
mail settings
Submitter Jonathan Wakely
Date July 4, 2012, 10:17 p.m.
Message ID <CAH6eHdR2GYxs9orW39rdUMd5d4QDEWMdqs1zYw2aP+jJK9MA0w@mail.gmail.com>
Download mbox | patch
Permalink /patch/169053/
State New
Headers show

Comments

Jonathan Wakely - July 4, 2012, 10:17 p.m.
This fixes a deadlock in condition_variable_any, which I should have
fixed as part of PR 50862.

        PR libstdc++/53830
        * include/std/condition_variable (condition_variable_any::wait):
        Move _Unlock type to class scope.
        (condition_variable_any::wait_until): Reuse it.
        * testsuite/30_threads/condition_variable_any/53830.cc: New.

Tested x86_64-linux, committed to trunk. I'm also going to commit it
on the 4.6 and 4.7 branches.
commit 8c3d75d934e3e6d5cf313991c56af8844f725dce
Author: Jonathan Wakely <jwakely.gcc@gmail.com>
Date:   Mon Jul 2 20:54:23 2012 +0100

    	PR libstdc++/53830
    	* include/std/condition_variable (condition_variable_any::wait):
    	Move _Unlock type to class scope.
    	(condition_variable_any::wait_until): Reuse it.
    	* testsuite/30_threads/condition_variable_any/53830.cc: New.

Patch

diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index c4e2080..85b50a7 100644
--- a/libstdc++-v3/include/std/condition_variable
+++ b/libstdc++-v3/include/std/condition_variable
@@ -176,6 +176,26 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     condition_variable			_M_cond;
     mutex				_M_mutex;
 
+    // scoped unlock - unlocks in ctor, re-locks in dtor
+    template<typename _Lock>
+      struct _Unlock
+      {
+	explicit _Unlock(_Lock& __lk) : _M_lock(__lk) { __lk.unlock(); }
+
+	~_Unlock() noexcept(false)
+	{
+	  if (uncaught_exception())
+	    __try { _M_lock.lock(); } __catch(...) { }
+	  else
+	    _M_lock.lock();
+	}
+
+	_Unlock(const _Unlock&) = delete;
+	_Unlock& operator=(const _Unlock&) = delete;
+
+	_Lock& _M_lock;
+      };
+
   public:
 
     condition_variable_any() noexcept;
@@ -202,21 +222,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       wait(_Lock& __lock)
       {
-	// scoped unlock - unlocks in ctor, re-locks in dtor
-	struct _Unlock {
-	  explicit _Unlock(_Lock& __lk) : _M_lock(__lk) { __lk.unlock(); }
-	  ~_Unlock() noexcept(false)
-	  {
-	    if (uncaught_exception())
-	      __try { _M_lock.lock(); } __catch(...) { }
-	    else
-	      _M_lock.lock();
-	  }
-	  _Lock& _M_lock;
-	};
-
 	unique_lock<mutex> __my_lock(_M_mutex);
-	_Unlock __unlock(__lock);
+	_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.
 	unique_lock<mutex> __my_lock2(std::move(__my_lock));
@@ -237,11 +244,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       wait_until(_Lock& __lock,
 		 const chrono::time_point<_Clock, _Duration>& __atime)
       {
-        unique_lock<mutex> __my_lock(_M_mutex);
-        __lock.unlock();
-        cv_status __status = _M_cond.wait_until(__my_lock, __atime);
-        __lock.lock();
-        return __status;
+	unique_lock<mutex> __my_lock(_M_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.
+	unique_lock<mutex> __my_lock2(std::move(__my_lock));
+	return _M_cond.wait_until(__my_lock2, __atime);
       }
 
     template<typename _Lock, typename _Clock,
diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable_any/53830.cc b/libstdc++-v3/testsuite/30_threads/condition_variable_any/53830.cc
new file mode 100644
index 0000000..91aa348
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/condition_variable_any/53830.cc
@@ -0,0 +1,68 @@ 
+// { dg-do run { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++0x -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++0x -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++0x " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+// { dg-require-sched-yield "" }
+// { dg-require-nanosleep "" }
+
+// Copyright (C) 2012 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.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// PR libstdc++/53830
+// Test for deadlock in condition_variable_any::wait_for
+
+#include <thread>
+#include <mutex>
+#include <condition_variable>
+#include <chrono>
+#include <atomic>
+
+std::mutex mutex;
+std::condition_variable_any cv;
+
+std::atomic<int> barrier(0);
+
+// waits for data from another thread
+void wait_for_data()
+{
+  std::unique_lock<std::mutex> lock(mutex);
+  barrier = 1;
+  cv.wait_for(lock, std::chrono::milliseconds(100), []{ return false; });
+  // read data
+}
+
+// passes data to waiting thread
+void provide_data()
+{
+  while (barrier == 0)
+    std::this_thread::yield();
+  std::unique_lock<std::mutex> lock(mutex);
+  // pass data
+  std::this_thread::sleep_for(std::chrono::seconds(1));
+  cv.notify_one();
+}
+
+int main()
+{
+  std::thread thread1(wait_for_data);
+  provide_data();
+  thread1.join();
+  return 0;
+}
+