Modify

Opened 4 years ago

Closed 4 years ago

#8596 closed Bugs (fixed)

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

Reported by: Kees-Jan Dijkzeul <kees-jan.dijkzeul@…> Owned by: viboes
Milestone: Boost 1.54.0 Component: thread
Version: Boost 1.53.0 Severity: Problem
Keywords: Cc:

Description

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

Attachments (1)

test-application.cc (709 bytes) - added by Kees-Jan Dijkzeul <kees-jan.dijkzeul@…> 4 years ago.
Small program showing the problem.

Download all attachments as: .zip

Change History (13)

Changed 4 years ago by Kees-Jan Dijkzeul <kees-jan.dijkzeul@…>

Attachment: test-application.cc added

Small program showing the problem.

comment:1 Changed 4 years ago by viboes

Owner: changed from Anthony Williams to viboes
Status: newassigned

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?

comment:2 in reply to:  1 Changed 4 years ago by 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

Last edited 4 years ago by viboes (previous) (diff)

comment:3 Changed 4 years ago by 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)

comment:4 in reply to:  3 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by 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!

comment:6 Changed 4 years ago by 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.

comment:7 Changed 4 years ago by 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

comment:8 Changed 4 years ago by viboes

Milestone: To Be DeterminedBoost 1.54.0

comment:9 Changed 4 years ago by viboes

Committed revision [84414].

With this change set everything is working now.

comment:10 Changed 4 years ago by 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? ;-)

comment:11 in reply to:  10 Changed 4 years ago by 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.

comment:12 Changed 4 years ago by viboes

Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain viboes.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.