[c++0x] std::thread fixes

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

As discussed recently, the names in std::thread don't follow the
libstdc++ coding style.

There's a memory leak in the case where a new thread can't be started,
because of the shared_ptr cycle created in _M_this_ptr.

The std::thread move constructor and assignment operator definitions
are missing.

 _Thread_data_base doesn't need a mutex, and doesn't need to derive
from enable_shared_from_this.

I don't think std::thread needs a mutex either, because it's undefined
for users to do any concurrent non-const operations. If the only
concurrent operations are const e.g. calling joinable() and get_id(),
then there's no need to lock a mutex.  If we don't want to crash and
burn when broken programs /do/ perform unsequenced mutating
operations, that can still be done without a mutex, by accessing the
thread data through a local copy of the shared_ptr, e.g.

id get_id() const
{
  if (_Thread_data_ptr __t = _M_thread_data)
    return id(__t->_M_thread_handle);
  else
    return id();
}

Taking concurrent copies of the shared_ptr /is/ safe, and ensures the
thread data is still valid even if_M_thread_data has been reset
between the test and the dereference. This costs a reference-count
increment and decrement in all the members that use the thread data.
I chose not to make this change to support broken code, but could be
persuaded otherwise.

Although join() and detach() have a precondition that the thread is
joinable, the standard also says that they should throw if the thread
is not joinable (to handle the case where pthread_join returns
EINVAL.)  This patch changes join() and detach() so that they throw if
called on an unjoinable thread, instead of silently doing nothing, and
adds tests for the exception.

Although the standard says joinable returns get_id() != id() I've
changed that to just test the thread data ptr, which should be allowed
under the as-if rule. That saves constructing two thread::id objects
and comparing them.

The behaviour of native_handle() is implementation-defined, currently
this implementation will crash if it's called on an un-joinable
thread. That could be changed to throw an exception.

        * include/std/thread (_Thread_data_base, thread): Remove mutexes.
        Change names to match coding style.
        (thread::thread,thread::operator=): Define move operations.
        (thread::joinable): Test if data is non-empty.
        (thread::_M_start_thread): Break shared_ptr cycle on error.
        (thread::id::id): Make constructor explicit.
        * src/thread.cc (thread::join,thread::detach): Throw if not joinable.
        (__thread_proxy): Use shared_ptr swap instead of copy and reset.
        Change names to match coding style.
        * testsuite/30_threads/thread/members/4.cc: New.
        * testsuite/30_threads/thread/members/5.cc: New.

tested x86_64 linux - is this OK for trunk, or should I use shared_ptr
copies to prevent (some) crashes in broken programs?

Jonathan

Index: include/std/thread
===================================================================
--- include/std/thread	(revision 143555)
+++ include/std/thread	(working copy)
@@ -41,46 +41,44 @@
 #else
 
 #include <chrono>
-#include <exception>
 #include <functional>
 #include <memory>
 #include <mutex>
 #include <condition_variable>
-#include <type_traits>
 #include <cstddef>
 #include <bits/functexcept.h>
+#include <bits/move.h>
 #include <bits/gthr.h>
 
 #if defined(_GLIBCXX_HAS_GTHREADS) && defined(_GLIBCXX_USE_C99_STDINT_TR1)
 
 namespace std
 {
-  class __thread_data_base;
+  class _Thread_data_base;
 
-  typedef shared_ptr<__thread_data_base> __thread_data_ptr;
+  typedef shared_ptr<_Thread_data_base> _Thread_data_ptr;
 
-  class __thread_data_base : public enable_shared_from_this<__thread_data_base>
+  class _Thread_data_base
   {
   public:
-    __thread_data_base() = default;
-    virtual ~__thread_data_base() = default;
+    _Thread_data_base() = default;
+    virtual ~_Thread_data_base() = default;
     
-    virtual void __run() = 0;
+    virtual void _M_run() = 0;
     
     __gthread_t 	_M_thread_handle;
-    __thread_data_ptr 	_M_this_ptr;
-    mutex 		_M_data_mutex;
+    _Thread_data_ptr 	_M_this_ptr;
   };
   
   template<typename _Callable>
-    class __thread_data : public __thread_data_base
+    class _Thread_data : public _Thread_data_base
     {
     public:
-      __thread_data(_Callable&& __f)
+      _Thread_data(_Callable&& __f)
       : _M_func(std::forward<_Callable>(__f))
       { }
 
-      void __run()
+      void _M_run()
       { _M_func(); }
 
     private:
@@ -100,29 +98,41 @@ namespace std
     
     template<typename _Callable>
       explicit thread(_Callable __f)
-      : _M_thread_data(__make_thread_data(__f))
-      { __start_thread(); }
+      : _M_thread_data(_M_make_thread_data(__f))
+      { _M_start_thread(); }
 
     template<typename _Callable, typename... _Args>
       thread(_Callable&& __f, _Args&&... __args)
-      : _M_thread_data(__make_thread_data(std::bind(__f, __args...)))
-      { __start_thread(); }
+      : _M_thread_data(_M_make_thread_data(std::bind(__f, __args...)))
+      { _M_start_thread(); }
 
     ~thread()
-    { detach(); }
+    {
+      if (joinable())
+        detach();
+    }
 
     thread(const thread&) = delete;
-    thread(thread&&);
+    thread(thread&& __t)
+    { swap(__t); }
+
     thread& operator=(const thread&) = delete;
-    thread& operator=(thread&&);
+    thread& operator=(thread&& __t)
+    {
+      if (joinable())
+        detach();
+      swap(__t);
+      return *this;
+    }
 
     // members
     void 
     swap(thread&& __t)
-    { std::swap(_M_thread_data, __t._M_thread_data); }
+    { _M_thread_data.swap(__t._M_thread_data); }
 
     bool 
-    joinable() const;
+    joinable() const
+    { return _M_thread_data; }
 
     void 
     join();
@@ -133,37 +143,27 @@ namespace std
     thread::id
     get_id() const;
 
-    native_handle_type 
+    /** @pre thread is joinable
+     */
+    native_handle_type
     native_handle()
     { return _M_thread_data->_M_thread_handle; }
 
     // static members
     static unsigned hardware_concurrency();
 
-    __thread_data_ptr
-    _M_get_thread_data() const
-    {
-      lock_guard<mutex> __l(_M_thread_data_mutex);
-      return _M_thread_data;
-    }
-
   private:
     template<typename _Callable>
-      __thread_data_ptr 
-      __make_thread_data(_Callable&& __f)
-      { 
-	return __thread_data_ptr(
-	  new __thread_data<_Callable>(std::forward<_Callable>(__f)));
+      _Thread_data_ptr 
+      _M_make_thread_data(_Callable&& __f)
+      {
+        return std::make_shared<_Thread_data<_Callable>>(
+            std::forward<_Callable>(__f));
       }
     
-    __thread_data_ptr
-    __make_thread_data(void(*__f)())
-    { return __thread_data_ptr(new __thread_data<void(*)()>(__f)); }
-    
-    void __start_thread();
+    void _M_start_thread();
 
-    __thread_data_ptr 	_M_thread_data;
-    mutable mutex 	_M_thread_data_mutex;
+    _Thread_data_ptr _M_thread_data;
   };
 
   inline void
@@ -229,8 +229,7 @@ namespace std
 
     friend bool 
     operator==(thread::id __x, thread::id __y)
-    { return static_cast<bool>(__gthread_equal(__x._M_thread_id,
-					       __y._M_thread_id)); }
+    { return __gthread_equal(__x._M_thread_id, __y._M_thread_id); }
 
     friend bool
     operator<(thread::id __x, thread::id __y)
@@ -240,6 +239,7 @@ namespace std
       friend basic_ostream<_CharT, _Traits>&
       operator<<(basic_ostream<_CharT, _Traits>&& __out, thread::id __id); 
 
+    explicit
     id(__gthread_t __id)
     : _M_thread_id(__id)
     { }
@@ -273,14 +273,10 @@ namespace std
 	return __out << __id._M_thread_id;
     }  
 
-  inline bool
-  thread::joinable() const
-  { return get_id() != thread::id(); }
-
   inline thread::id
   thread::get_id() const
   {
-    if(_M_thread_data)
+    if (_M_thread_data)
       return thread::id(_M_thread_data->_M_thread_handle);
     else
       return thread::id();
Index: src/thread.cc
===================================================================
--- src/thread.cc	(revision 143555)
+++ src/thread.cc	(working copy)
@@ -28,7 +28,6 @@
 // the GNU General Public License.
 
 #include <thread>
-#include <bits/move.h> // std::move
 
 #if defined(_GLIBCXX_HAS_GTHREADS) && defined(_GLIBCXX_USE_C99_STDINT_TR1)
 
@@ -40,20 +39,20 @@ namespace std
     {
       void* __thread_proxy(void* __p)
       {
-	__thread_data_base* __t = static_cast<__thread_data_base*>(__p);
-	__thread_data_ptr __local_thread_data = __t->_M_this_ptr;
-	__t->_M_this_ptr.reset();
-
-	try
-	  {
-	    __local_thread_data->__run();
-	  }
-	catch(...)
-	  {
-	    std::terminate();
-	  }
+        _Thread_data_base* __t = static_cast<_Thread_data_base*>(__p);
+        _Thread_data_ptr __local_thread_data;
+        __local_thread_data.swap(__t->_M_this_ptr); // break cycle
+
+        try
+        {
+          __local_thread_data->_M_run();
+        }
+        catch(...)
+        {
+          std::terminate();
+        }
 
-	return 0;
+        return 0;
       }
     }
   }
@@ -61,40 +60,46 @@ namespace std
   void
   thread::join()
   {
-    if(joinable())
-      {
-	void* __r = 0;
-	int __e = __gthread_join(_M_thread_data->_M_thread_handle, &__r);
-	if(__e)
-	  __throw_system_error(__e);
+    int __e = EINVAL;
 
-	lock_guard<mutex> __lock(_M_thread_data_mutex);
-	_M_thread_data.reset();
-      }
+    if (_M_thread_data)
+    {
+      void* __r = 0;
+      __e = __gthread_join(_M_thread_data->_M_thread_handle, &__r);
+    }
+
+    if (__e)
+      __throw_system_error(__e);
+
+    _M_thread_data.reset();
   }
 
   void
   thread::detach()
-  {    
-    if(joinable())
-      {
-	int __e = __gthread_detach(_M_thread_data->_M_thread_handle);
-	if(__e)
-	  __throw_system_error(__e);
+  {
+    int __e = EINVAL;
 
-	lock_guard<mutex> __lock(_M_thread_data_mutex);
-	_M_thread_data.reset();
-      }
+    if (_M_thread_data)
+      __e = __gthread_detach(_M_thread_data->_M_thread_handle);
+
+    if (__e)
+      __throw_system_error(__e);
+
+    _M_thread_data.reset();
   }
 
   void 
-  thread::__start_thread()
+  thread::_M_start_thread()
   {
+    // create cycle in reference count so that data cannot go out of scope
     _M_thread_data->_M_this_ptr = _M_thread_data;
     int __e = __gthread_create(&_M_thread_data->_M_thread_handle, 
 			       &__thread_proxy, _M_thread_data.get());
-    if(__e)
+    if (__e)
+    {
+      _M_thread_data->_M_this_ptr.reset(); // break cycle
       __throw_system_error(__e);
+    }
   }
 }
 
Index: testsuite/30_threads/thread/member/4.cc
===================================================================
--- testsuite/30_threads/thread/member/4.cc	(revision 0)
+++ testsuite/30_threads/thread/member/4.cc	(revision 0)
@@ -0,0 +1,58 @@
+// { dg-do run { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++0x " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+
+// Copyright (C) 2009 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 2, 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 COPYING.  If not, write to the Free
+// Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
+// USA.
+
+// As a special exception, you may use this file as part of a free software
+// library without restriction.  Specifically, if other files instantiate
+// templates or use macros or inline functions from this file, or you compile
+// this file and link it with other files to produce an executable, this
+// file does not by itself cause the resulting executable to be covered by
+// the GNU General Public License.  This exception does not however
+// invalidate any other reasons why the executable file might be covered by
+// the GNU General Public License.
+
+#include <thread>
+#include <system_error>
+#include <testsuite_hooks.h>
+
+int main()
+{
+  bool test = false;
+
+  std::thread t;
+
+  try 
+  {
+    t.join();
+    VERIFY( false );
+  }
+  catch (const std::system_error&)
+  {
+    test = true;
+  }
+
+  VERIFY( test );
+
+  return 0;
+}
Index: testsuite/30_threads/thread/member/5.cc
===================================================================
--- testsuite/30_threads/thread/member/5.cc	(revision 0)
+++ testsuite/30_threads/thread/member/5.cc	(revision 0)
@@ -0,0 +1,58 @@
+// { dg-do run { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* alpha*-*-osf* mips-sgi-irix6* } }
+// { dg-options " -std=gnu++0x -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++0x " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+
+// Copyright (C) 2009 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 2, 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 COPYING.  If not, write to the Free
+// Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
+// USA.
+
+// As a special exception, you may use this file as part of a free software
+// library without restriction.  Specifically, if other files instantiate
+// templates or use macros or inline functions from this file, or you compile
+// this file and link it with other files to produce an executable, this
+// file does not by itself cause the resulting executable to be covered by
+// the GNU General Public License.  This exception does not however
+// invalidate any other reasons why the executable file might be covered by
+// the GNU General Public License.
+
+#include <thread>
+#include <system_error>
+#include <testsuite_hooks.h>
+
+int main()
+{
+  bool test = false;
+
+  std::thread t;
+
+  try 
+  {
+    t.detach();
+    VERIFY( false );
+  }
+  catch (const std::system_error&)
+  {
+    test = true;
+  }
+
+  VERIFY( test );
+
+  return 0;
+}