With C++0x enabled, boost::packaged_task stores a reference to function objects, instead of a copy

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

I have build a Treadpool using boost::thread. In porting it to boost 1.53, I've found a regression.

Consider attached test-program. It runs correctly on boost 1.49 and earlier. It runs correctly on boost 1.53, with C++0x disabled. With C++0x enabled, it crashes due to an uncaught exception "call to empty boost::function"

Failure has been observed on Ubuntu Saucy, I.e. boost 1.53, gcc 4.8.0, linux kernel 3.9.0.

After some debugging, I believe the problem is caused by packaged_task storing a reference to the boost::function object, instead of a copy. As the boost::function object is a temporary, this leads to undefined behavior further on.

I'm guessing this problem is introduced in [81117] By below patch to boost/thread/future.hpp

 #ifndef BOOST_NO_CXX11_RVALUE_REFERENCES
         template <class F>
         explicit packaged_task(BOOST_THREAD_RV_REF(F) f
             , 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 typename remove_cv<typename remove_reference<F>::type>::type FR;
+          typedef F FR;
 #if defined BOOST_THREAD_PROVIDES_SIGNATURE_PACKAGED_TASK
....

Attached: Small application showing the problem

For original code please visit ​https://github.com/kees-jan/scroom/blob/master/inc/scroom/impl/threadpoolimpl.hh#L90

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

Small program showing the problem.

[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?

[viboes]

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

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

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]

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.

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

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!

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

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]

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

[viboes]

Committed revision [84414].

With this change set everything is working now.

[Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>]

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]

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.

[viboes]

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