Patchwork fix libstdc++/56492

login
register
mail settings
Submitter Jonathan Wakely
Date March 16, 2013, 2:51 a.m.
Message ID <CAH6eHdTscjwTQ0X0ty4OwBNBAHd=8cGw8iVV-xXXSoMd9L=HGw@mail.gmail.com>
Download mbox | patch
Permalink /patch/228198/
State New
Headers show

Comments

Jonathan Wakely - March 16, 2013, 2:51 a.m.
This fixes a non-conformance issue in std::packaged_task which we've
decided should be addressed for 4.8

std::function cannot be used with non-CopyConstructible targets, so
this replaces std::function in the implementation of
std::packaged_task.

        PR libstdc++/56492
        * include/std/future (__future_base::_Result): Add result_type
        typedef.
        (__future_base::_S_allocate_result): Overload for std::allocator.
        (__future_base::_Task_setter): Use _Result::result_type instead of
        deducing the type from the task.
        (__future_base::_Task_state): Store allocator to allow shared state
        to be reset.  Replace std::function with member of target object type
        accessed via ...
        (__future_base::_Task_state_base): New abstract base class.
        (__future_base::_Task_state_base::_M_run): New virtual function to
        invoke type-erased target object.
        (__future_base::_Task_state_base::_M_reset): New virtual function to
        create new shared_state using same target object and allocator.
        (__future_base::__create_task_state): Allocate a new _Task_state.
        (packaged_task::packaged_task): Use __create_task_state.
        (packaged_task::reset): Use _Task_state_base::_M_reset.
        * testsuite/30_threads/packaged_task/cons/56492.cc: New.

Tested x86_64-linux, committed to trunk.
commit c3e4bcc3530743e86f7d2b4dec44785a66117386
Author: Jonathan Wakely <jwakely.gcc@gmail.com>
Date:   Sat Mar 16 02:47:42 2013 +0000

    	PR libstdc++/56492
    	* include/std/future (__future_base::_Result): Add result_type
    	typedef.
    	(__future_base::_S_allocate_result): Overload for std::allocator.
    	(__future_base::_Task_setter): Use _Result::result_type instead of
    	deducing the type from the task.
    	(__future_base::_Task_state): Store allocator to allow shared state
    	to be reset.  Replace std::function with member of target object type
    	accessed via ...
    	(__future_base::_Task_state_base): New abstract base class.
    	(__future_base::_Task_state_base::_M_run): New virtual function to
    	invoke type-erased target object.
    	(__future_base::_Task_state_base::_M_reset): New virtual function to
    	create new shared_state using same target object and allocator.
    	(__future_base::__create_task_state): Allocate a new _Task_state.
    	(packaged_task::packaged_task): Use __create_task_state.
    	(packaged_task::reset): Use _Task_state_base::_M_reset.
    	* testsuite/30_threads/packaged_task/cons/56492.cc: New.

Patch

diff --git a/libstdc++-v3/include/std/future b/libstdc++-v3/include/std/future
index 6cccd3d..30100fe 100644
--- a/libstdc++-v3/include/std/future
+++ b/libstdc++-v3/include/std/future
@@ -214,6 +214,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	bool 			_M_initialized;
 
       public:
+	typedef _Res result_type;
+
 	_Result() noexcept : _M_initialized() { }
 	
 	~_Result()
@@ -281,17 +283,23 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
         typename __traits::allocator_type __a2(__a);
         __result_type* __p = __traits::allocate(__a2, 1);
         __try
-	{
-	  __traits::construct(__a2, __p, __a);
-        }
+	  {
+	    __traits::construct(__a2, __p, __a);
+	  }
         __catch(...)
-        {
-	  __traits::deallocate(__a2, __p, 1);
-          __throw_exception_again;
-        }
+	  {
+	    __traits::deallocate(__a2, __p, 1);
+	    __throw_exception_again;
+	  }
         return _Ptr<__result_type>(__p);
       }
 
+    template<typename _Res, typename _Tp>
+      static _Ptr<_Result<_Res>>
+      _S_allocate_result(const std::allocator<_Tp>& __a)
+      {
+	return _Ptr<_Result<_Res>>(new _Result<_Res>);
+      }
 
     /// Base class for state between a promise and one or more
     /// associated futures.
@@ -482,6 +490,9 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       class _Async_state_impl;
 
     template<typename _Signature>
+      class _Task_state_base;
+
+    template<typename _Fn, typename _Alloc, typename _Signature>
       class _Task_state;
 
     template<typename _BoundFn>
@@ -492,24 +503,15 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       static std::shared_ptr<_State_base>
       _S_make_async_state(_BoundFn&& __fn);
 
-    template<typename _Res_ptr, typename _Res>
+    template<typename _Res_ptr,
+	     typename _Res = typename _Res_ptr::element_type::result_type>
       struct _Task_setter;
 
     template<typename _Res_ptr, typename _BoundFn>
-      class _Task_setter_helper
-      {
-	typedef typename remove_reference<_BoundFn>::type::result_type __res;
-      public:
-	typedef _Task_setter<_Res_ptr, __res> __type;
-      };
-
-    template<typename _Res_ptr, typename _BoundFn>
-      static typename _Task_setter_helper<_Res_ptr, _BoundFn>::__type
+      static _Task_setter<_Res_ptr>
       _S_task_setter(_Res_ptr& __ptr, _BoundFn&& __call)
       {
-	typedef _Task_setter_helper<_Res_ptr, _BoundFn> __helper_type;
-	typedef typename __helper_type::__type _Setter;
-	return _Setter{ __ptr, std::ref(__call) };
+	return _Task_setter<_Res_ptr>{ __ptr, std::ref(__call) };
       }
   };
 
@@ -517,6 +519,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Res>
     struct __future_base::_Result<_Res&> : __future_base::_Result_base
     {
+      typedef _Res& result_type;
+
       _Result() noexcept : _M_value_ptr() { }
 
       void _M_set(_Res& __res) noexcept { _M_value_ptr = &__res; }
@@ -533,6 +537,8 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<>
     struct __future_base::_Result<void> : __future_base::_Result_base
     {
+      typedef void result_type;
+
     private:
       void _M_destroy() { delete this; }
     };
@@ -1197,7 +1203,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       _Ptr_type operator()()
       {
-        __try
+	__try
 	  {
 	    _M_result->_M_set(_M_fn());
 	  }
@@ -1205,7 +1211,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  {
 	    _M_result->_M_error = current_exception();
 	  }
-        return std::move(_M_result);
+	return std::move(_M_result);
       }
       _Ptr_type&                _M_result;
       std::function<_Res()>     _M_fn;
@@ -1216,7 +1222,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       _Ptr_type operator()()
       {
-        __try
+	__try
 	  {
 	    _M_fn();
 	  }
@@ -1231,49 +1237,85 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     };
 
   template<typename _Res, typename... _Args>
-    struct __future_base::_Task_state<_Res(_Args...)> final
+    struct __future_base::_Task_state_base<_Res(_Args...)>
     : __future_base::_State_base
     {
       typedef _Res _Res_type;
 
-      _Task_state(std::function<_Res(_Args...)> __task)
-      : _M_result(new _Result<_Res>()), _M_task(std::move(__task))
-      { }
+      template<typename _Alloc>
+	_Task_state_base(const _Alloc& __a)
+	: _M_result(_S_allocate_result<_Res>(__a))
+	{ }
 
-      template<typename _Func, typename _Alloc>
-        _Task_state(_Func&& __task, const _Alloc& __a)
-        : _M_result(_S_allocate_result<_Res>(__a)),
-	  _M_task(allocator_arg, __a, std::move(__task))
-        { }
+      virtual void
+      _M_run(_Args... __args) = 0;
 
-      void
+      virtual shared_ptr<_Task_state_base>
+      _M_reset() = 0;
+
+      typedef __future_base::_Ptr<_Result<_Res>> _Ptr_type;
+      _Ptr_type _M_result;
+    };
+
+  template<typename _Fn, typename _Alloc, typename _Res, typename... _Args>
+    struct __future_base::_Task_state<_Fn, _Alloc, _Res(_Args...)> final
+    : __future_base::_Task_state_base<_Res(_Args...)>
+    {
+      _Task_state(_Fn&& __fn, const _Alloc& __a)
+      : _Task_state_base<_Res(_Args...)>(__a), _M_impl(std::move(__fn), __a)
+      { }
+
+    private:
+      virtual void
       _M_run(_Args... __args)
       {
-        // bound arguments decay so wrap lvalue references
-	auto __boundfn = std::__bind_simple(std::ref(_M_task),
+	// bound arguments decay so wrap lvalue references
+	auto __boundfn = std::__bind_simple(std::ref(_M_impl._M_fn),
 	    _S_maybe_wrap_ref(std::forward<_Args>(__args))...);
-        auto __setter = _S_task_setter(_M_result, std::move(__boundfn));
-        _M_set_result(std::move(__setter));
+	auto __setter = _S_task_setter(this->_M_result, std::move(__boundfn));
+	this->_M_set_result(std::move(__setter));
       }
 
-      typedef __future_base::_Ptr<_Result<_Res>> _Ptr_type;
-      _Ptr_type _M_result;
-      std::function<_Res(_Args...)> _M_task;
+      virtual shared_ptr<_Task_state_base<_Res(_Args...)>>
+      _M_reset();
 
       template<typename _Tp>
-        static reference_wrapper<_Tp>
-        _S_maybe_wrap_ref(_Tp& __t)
-        { return std::ref(__t); }
+	static reference_wrapper<_Tp>
+	_S_maybe_wrap_ref(_Tp& __t)
+	{ return std::ref(__t); }
 
       template<typename _Tp>
-        static typename enable_if<!is_lvalue_reference<_Tp>::value,
-                        _Tp>::type&&
-        _S_maybe_wrap_ref(_Tp&& __t)
-        { return std::forward<_Tp>(__t); }
+	static
+	typename enable_if<!is_lvalue_reference<_Tp>::value, _Tp>::type&&
+	_S_maybe_wrap_ref(_Tp&& __t)
+	{ return std::forward<_Tp>(__t); }
+
+      struct _Impl : _Alloc
+      {
+	_Impl(_Fn&& __fn, const _Alloc& __a)
+	  : _Alloc(__a), _M_fn(std::move(__fn)) { }
+	_Fn _M_fn;
+      } _M_impl;
     };
 
+    template<typename _Signature, typename _Fn, typename _Alloc>
+      static shared_ptr<__future_base::_Task_state_base<_Signature>>
+      __create_task_state(_Fn&& __fn, const _Alloc& __a)
+      {
+	typedef __future_base::_Task_state<_Fn, _Alloc, _Signature> _State;
+	return std::allocate_shared<_State>(__a, std::move(__fn), __a);
+      }
+
+  template<typename _Fn, typename _Alloc, typename _Res, typename... _Args>
+    shared_ptr<__future_base::_Task_state_base<_Res(_Args...)>>
+    __future_base::_Task_state<_Fn, _Alloc, _Res(_Args...)>::_M_reset()
+    {
+      return __create_task_state<_Res(_Args...)>(std::move(_M_impl._M_fn),
+						 static_cast<_Alloc&>(_M_impl));
+    }
+
   template<typename _Task, typename _Fn, bool
-           = is_same<_Task, typename decay<_Fn>::type>::value>
+	   = is_same<_Task, typename decay<_Fn>::type>::value>
     struct __constrain_pkgdtask
     { typedef void __type; };
 
@@ -1285,7 +1327,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Res, typename... _ArgTypes>
     class packaged_task<_Res(_ArgTypes...)>
     {
-      typedef __future_base::_Task_state<_Res(_ArgTypes...)>  _State_type;
+      typedef __future_base::_Task_state_base<_Res(_ArgTypes...)> _State_type;
       shared_ptr<_State_type>                   _M_state;
 
     public:
@@ -1295,31 +1337,30 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // 2095.  missing constructors needed for uses-allocator construction
       template<typename _Allocator>
-        explicit
-        packaged_task(allocator_arg_t, const _Allocator& __a) noexcept
-        { }
+	packaged_task(allocator_arg_t, const _Allocator& __a) noexcept
+	{ }
 
       template<typename _Fn, typename = typename
-               __constrain_pkgdtask<packaged_task, _Fn>::__type>
-        explicit
-        packaged_task(_Fn&& __fn)
-        : _M_state(std::make_shared<_State_type>(std::forward<_Fn>(__fn)))
-        { }
+	       __constrain_pkgdtask<packaged_task, _Fn>::__type>
+	explicit
+	packaged_task(_Fn&& __fn)
+	: packaged_task(allocator_arg, std::allocator<int>(), std::move(__fn))
+	{ }
 
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
       // 2097.  packaged_task constructors should be constrained
-      template<typename _Fn, typename _Allocator, typename = typename
-               __constrain_pkgdtask<packaged_task, _Fn>::__type>
-        explicit
-        packaged_task(allocator_arg_t, const _Allocator& __a, _Fn&& __fn)
-        : _M_state(std::allocate_shared<_State_type>(__a,
-                                                     std::forward<_Fn>(__fn)))
-        { }
+      template<typename _Fn, typename _Alloc, typename = typename
+	       __constrain_pkgdtask<packaged_task, _Fn>::__type>
+	explicit
+	packaged_task(allocator_arg_t, const _Alloc& __a, _Fn&& __fn)
+	: _M_state(__create_task_state<_Res(_ArgTypes...)>(
+		    std::forward<_Fn>(__fn), __a))
+	{ }
 
       ~packaged_task()
       {
         if (static_cast<bool>(_M_state) && !_M_state.unique())
-          _M_state->_M_break_promise(std::move(_M_state->_M_result));
+	  _M_state->_M_break_promise(std::move(_M_state->_M_result));
       }
 
       // No copy
@@ -1327,24 +1368,22 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       packaged_task& operator=(const packaged_task&) = delete;
 
       template<typename _Allocator>
-        explicit
-        packaged_task(allocator_arg_t, const _Allocator&,
-                      const packaged_task&) = delete;
+	packaged_task(allocator_arg_t, const _Allocator&,
+		      const packaged_task&) = delete;
 
       // Move support
       packaged_task(packaged_task&& __other) noexcept
       { this->swap(__other); }
 
       template<typename _Allocator>
-        explicit
-        packaged_task(allocator_arg_t, const _Allocator&,
-                      packaged_task&& __other) noexcept
-        { this->swap(__other); }
+	packaged_task(allocator_arg_t, const _Allocator&,
+		      packaged_task&& __other) noexcept
+	{ this->swap(__other); }
 
       packaged_task& operator=(packaged_task&& __other) noexcept
       {
-        packaged_task(std::move(__other)).swap(*this);
-        return *this;
+	packaged_task(std::move(__other)).swap(*this);
+	return *this;
       }
 
       void
@@ -1364,15 +1403,17 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       void
       operator()(_ArgTypes... __args)
       {
-        __future_base::_State_base::_S_check(_M_state);
-        _M_state->_M_run(std::forward<_ArgTypes>(__args)...);
+	__future_base::_State_base::_S_check(_M_state);
+	_M_state->_M_run(std::forward<_ArgTypes>(__args)...);
       }
 
       void
       reset()
       {
-        __future_base::_State_base::_S_check(_M_state);
-        packaged_task(std::move(_M_state->_M_task)).swap(*this);
+	__future_base::_State_base::_S_check(_M_state);
+	packaged_task __tmp;
+	__tmp._M_state = _M_state;
+	_M_state = _M_state->_M_reset();
       }
     };
 
diff --git a/libstdc++-v3/testsuite/30_threads/packaged_task/cons/56492.cc b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/56492.cc
new file mode 100644
index 0000000..02296bb
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/56492.cc
@@ -0,0 +1,35 @@ 
+// { dg-do compile { target *-*-freebsd* *-*-netbsd* *-*-linux* *-*-solaris* *-*-cygwin *-*-darwin* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++11 -pthread" { target *-*-freebsd* *-*-netbsd* *-*-linux* powerpc-ibm-aix* } }
+// { dg-options " -std=gnu++11 -pthreads" { target *-*-solaris* } }
+// { dg-options " -std=gnu++11 " { target *-*-cygwin *-*-darwin* } }
+// { dg-require-cstdint "" }
+// { dg-require-gthreads "" }
+// { dg-require-atomic-builtins "" }
+
+// 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.
+
+// 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/>.
+
+#include <future>
+
+struct S
+{
+  S() = default;
+  S(S&&) = default;
+  void operator()() { }
+};
+
+std::packaged_task<void ()> pt{ S{} };