Boost C++ Libraries: Ticket #8596: With C++0x enabled, boost::packaged_task stores a reference to function objects, instead of a copy

attachment set

Mon, 20 May 2013 11:59:51 GMT

attachment → test-application.cc

Small program showing the problem.

owner, status changed

viboes — Mon, 20 May 2013 13:24:17 GMT

owner changed from Anthony Williams to viboes
status new → assigned

Hi and thanks for the report. The concerned code is not any more on trunk or release branch. Please could you test your example with one of them?

viboes — Mon, 20 May 2013 13:44:43 GMT

Replying to viboes:

Hi and thanks for the report. The concerned code is not any more on trunk or release branch. Please could you test your example with one of them?

Forget this. I was thinking you were proposing to apply the patch above.

Please could you try this patch

 svn diff future.hpp detail/config.hpp
Index: future.hpp
===================================================================
--- future.hpp	(revision 84336)
+++ future.hpp	(working copy)
@@ -2841,8 +2841,8 @@
             , typename disable_if<is_same<typename decay<F>::type, packaged_task>, dummy* >::type=0
             )
         {
-          //typedef typename remove_cv<typename remove_reference<F>::type>::type FR;
-          typedef F FR;
+          typedef typename remove_cv<typename remove_reference<F>::type>::type FR;
+          //typedef F FR;
 #if defined BOOST_THREAD_PROVIDES_SIGNATURE_PACKAGED_TASK
   #if defined(BOOST_THREAD_PROVIDES_VARIADIC_THREAD)
             typedef detail::task_object<FR,R(ArgTypes...)> task_object_type;
@@ -2923,8 +2923,8 @@
         template <class F, class Allocator>
         packaged_task(boost::allocator_arg_t, Allocator a, BOOST_THREAD_RV_REF(F) f)
         {
-          //typedef typename remove_cv<typename remove_reference<F>::type>::type FR;
-          typedef F FR;
+          typedef typename remove_cv<typename remove_reference<F>::type>::type FR;
+          //typedef F FR;
 #if defined BOOST_THREAD_PROVIDES_SIGNATURE_PACKAGED_TASK
   #if defined(BOOST_THREAD_PROVIDES_VARIADIC_THREAD)
           typedef detail::task_object<FR,R(ArgTypes...)> task_object_type;
Index: detail/config.hpp
===================================================================
--- detail/config.hpp	(revision 84336)
+++ detail/config.hpp	(working copy)
@@ -95,7 +95,7 @@
 #endif
 /// RVALUE_REFERENCES_DONT_MATCH_FUNTION_PTR
-#if defined BOOST_NO_CXX11_RVALUE_REFERENCES || defined BOOST_MSVC
+#if defined BOOST_NO_CXX11_RVALUE_REFERENCES || defined BOOST_MSVC || (defined __GNUC__ && ! defined __clang__)
 #define BOOST_THREAD_RVALUE_REFERENCES_DONT_MATCH_FUNTION_PTR
 #endif

Mon, 20 May 2013 13:51:33 GMT

Forgive me, as I am new to contributing to boost. You are proposing that I

Download and build boost
Apply the patch
Build again
Verify that the problem is gone
Run all tests included within boost to verify that nothing else breaks

As opposed to you (very familiar with boost)

applying the patch to your existing tree
downloading and building 26 lines of code and see what happens

At this point, I'm only guessing that the patch in the bug description introduces the bug. I was hoping that you (having written the code :-) would confirm or deny my guesses and save me the time of further testing my guesses (considering that it took me two days to get this far)

viboes — Mon, 20 May 2013 15:16:35 GMT

Replying to Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>:

Forgive me, as I am new to contributing to boost.

No problem.

You are proposing that I

Download and build boost
Apply the patch
Build again
Verify that the problem is gone
Run all tests included within boost to verify that nothing else breaks

I don't know if you cannot apply the patch directly or not. I wanted just to be sure the patch worked for you on your specific example, not to run all the regression test, what an idea. I wanted just to unblock you as soon as possible.

As opposed to you (very familiar with boost)

applying the patch to your existing tree
downloading and building 26 lines of code and see what happens
At this point, I'm only guessing that the patch in the bug description introduces the bug. I was hoping that you (having written the code :-) would confirm or deny my guesses and save me the time of further testing my guesses (considering that it took me two days to get this far)

I confirm there is a bug and I can reproduce it. This is why I have accepted to manage the ticket.

I'm now seen if everything is ok with my more than 15 configurations on at least 3 platforms.

You know, this takes a lot of time also to me. Take in account that the library is developed without having access to future versions of future compilers (punned intended). When a new version of a compiler is available, the user expects that the library must work with it, but often this is not the case. I'm really sorry for you.

Hoping you understand now why I expect some collaboration from the people that are using the library.

Mon, 20 May 2013 15:39:22 GMT

I see. It was unclear to me that you already accepted my ticket as a bug. In that case, I probably overreacted. I apologize. It is true, though, that I have high expectations of you guys. You yourselves contribute to that in large part by mostly living up to them :-)

On the other hand, I was kind of proud having analysed this issue as far as I did. Being assigned more work without it being made explicitly clear that I was on to something and that it was for my own immediate benefit is kind of daunting :-)

As for unblocking me: Your patch (and several variations I tried) triggers a build error, either because boost::forward() cannot handle what I throw at it, or because the task_object constructor expects only an rvalue, but not an lvalue.

Taking a different approach, I observed that the packaged_task seems to store a reference only because I pass it one. Adding a static cast like so:

  return boost::packaged_task<int>(static_cast<boost::function<int()>(fn));

also seems to work around the problem. You can consider me unblocked :-)

Thanks very much for your support!

Mon, 20 May 2013 16:10:22 GMT

Just to be sure: In an earlier comment you hint that a new compiler (version) is possibly to blame for this problem. I've managed to reproduce it on Ubuntu Quantal (i.e. gcc 4.7.2) after downloading and building boost 1.53.

Niall Douglas — Tue, 21 May 2013 16:33:32 GMT

Excellent to see this bug report - I can confirm this bug. My GSoC student candidates found it during the programming task I set them two weeks ago. It causes substantial memory corruption throughout the process.

I might add I saw different behaviour between BOOST_THREAD_VERSION=3 and BOOST_THREAD_VERSION=4. This may be important.

C++11's std::packaged_task<> is very clear that it takes a copy of the passed callable. I appreciate that compiler oddities may make that hard.

Niall

milestone changed

viboes — Tue, 21 May 2013 22:51:20 GMT

milestone To Be Determined → Boost 1.54.0

viboes — Wed, 22 May 2013 05:43:15 GMT

Committed revision [84414].

With this change set everything is working now.

Wed, 22 May 2013 10:39:00 GMT

Indeed. I cannot reproduce this problem on the latest Trunk.

Now all I need to do is convince Ubuntu to switch to 1.54 :-). Do you guys have a best-guess release date? ;-)

Marshall Clow — Wed, 22 May 2013 15:54:38 GMT

Replying to Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>:

Now all I need to do is convince Ubuntu to switch to 1.54 :-). Do you guys have a best-guess release date? ;-)

Yes, we do.

The general answer is to check the calendar at http://www.boost.org/development/index.html , but I can tell you that 1.54 is scheduled to be released on 3-July.

status changed; resolution set

viboes — Fri, 24 May 2013 21:27:56 GMT

status assigned → closed
resolution → fixed

(In [84468]) Thread: merge 84414 to fix #8596.