Boost::thread + valgrind/drd possible false positive

[Mihail Strashun <m.strashun@…>]

OS: Arch Linux

Boost Library v 1.39

Valgrind v 3.5

Problem description:

Running small degenerated piece of code under valgrind/drd produces "probably a race condition" error on call of "interrupt". I was adviced on #boost channel to make a ticket after short examination of code.

Note:

This [ ​https://svn.boost.org/trac/boost/ticket/3526 ] case was added to valgrind suppression file.

Example code attached.

Compilation:

make all 
Building file: ../src/test.cpp
Invoking: GCC C++ Compiler
g++ -O0 -g3 -Wall -c -fmessage-length=0 -MMD -MP -MF"src/test.d" -MT"src/test.d" -o"src/test.o" "../src/test.cpp"
Finished building: ../src/test.cpp
 
Building target: test
Invoking: GCC C++ Linker
g++  -o"test"  ./src/test.o   -lboost_thread-mt
Finished building target: test

Output:

[mist@fog test]$ valgrind --tool=drd ./Debug/test 
==2759== drd, a thread error detector
==2759== Copyright (C) 2006-2009, and GNU GPL'd, by Bart Van Assche.
==2759== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==2759== Command: ./Debug/test
==2759== 
==2759== Probably a race condition: condition variable 0x5f64180 has been signaled but the associated mutex 0x5f64158 is not locked by the signalling thread.
==2759==    at 0x4C27BF9: pthread_cond_broadcast@* (in /usr/lib/valgrind/vgpreload_drd-amd64-linux.so)
==2759==    by 0x4E3BC88: boost::thread::interrupt() (in /usr/lib/libboost_thread-mt.so.1.39.0)
==2759==    by 0x405D42: stopThread(std::string const&) (test.cpp:45)
==2759==    by 0x405E79: main (test.cpp:75)
==2759== cond 0x5f64180 was first observed at:
==2759==    at 0x4C266D9: pthread_cond_init@* (in /usr/lib/valgrind/vgpreload_drd-amd64-linux.so)
==2759==    by 0x406E0B: boost::condition_variable::condition_variable() (condition_variable_fwd.hpp:30)
==2759==    by 0x406F39: boost::detail::thread_data_base::thread_data_base() (thread_data.hpp:54)
==2759==    by 0x40BF1C: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >::thread_data(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >) (thread.hpp:48)
==2759==    by 0x40B7F1: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >* boost::detail::heap_new_impl<boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >, boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&>(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&) (thread_heap_alloc.hpp:47)
==2759==    by 0x40AA8B: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >* boost::detail::heap_new<boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >, boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&) (thread_heap_alloc.hpp:73)
==2759==    by 0x40A1C0: boost::shared_ptr<boost::detail::thread_data_base> boost::thread::make_thread_info<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >) (thread.hpp:136)
==2759==    by 0x4092E1: boost::thread::thread<cTestRunner, std::string>(cTestRunner, std::string) (thread.hpp:227)
==2759==    by 0x407D28: void startThread<cTestRunner>(std::string const&) (test.cpp:36)
==2759==    by 0x405DDB: main (test.cpp:71)
==2759== mutex 0x5f64158 was first observed at:
==2759==    at 0x4C2C04F: pthread_mutex_init (in /usr/lib/valgrind/vgpreload_drd-amd64-linux.so)
==2759==    by 0x406D31: boost::mutex::mutex() (mutex.hpp:37)
==2759==    by 0x406F27: boost::detail::thread_data_base::thread_data_base() (thread_data.hpp:54)
==2759==    by 0x40BF1C: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >::thread_data(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >) (thread.hpp:48)
==2759==    by 0x40B7F1: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >* boost::detail::heap_new_impl<boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >, boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&>(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&) (thread_heap_alloc.hpp:47)
==2759==    by 0x40AA8B: boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >* boost::detail::heap_new<boost::detail::thread_data<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >, boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >&) (thread_heap_alloc.hpp:73)
==2759==    by 0x40A1C0: boost::shared_ptr<boost::detail::thread_data_base> boost::thread::make_thread_info<boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > > >(boost::_bi::bind_t<void, cTestRunner, boost::_bi::list1<boost::_bi::value<std::string> > >) (thread.hpp:136)
==2759==    by 0x4092E1: boost::thread::thread<cTestRunner, std::string>(cTestRunner, std::string) (thread.hpp:227)
==2759==    by 0x407D28: void startThread<cTestRunner>(std::string const&) (test.cpp:36)
==2759==    by 0x405DDB: main (test.cpp:71)
==2759== 
==2759== 
==2759== For counts of detected and suppressed errors, rerun with: -v
==2759== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 1956 from 80)

[anonymous]

This is still a problem in 1.42. Has it been determined whether this a bug, or a false positive?

[bvanassche@…]

As far as I can see thread_data_base::current_cond is signaled by thread::interrupt() but is never waited upon. That kind of access pattern is safe. This also means that it is safe to remove the member variable thread_data_base::current_cond from the Boost thread library.

[viboes]

Replying to bvanassche@…:

As far as I can see thread_data_base::current_cond is signaled by thread::interrupt() but is never waited upon. That kind of access pattern is safe. This also means that it is safe to remove the member variable thread_data_base::current_cond from the Boost thread library.

Not yet. Have you take a look at libs/thread/src?

grep -rHn current_cond * 
src/pthread/thread.cpp:416:            if(local_thread_info->current_cond)
src/pthread/thread.cpp:419:                BOOST_VERIFY(!pthread_cond_broadcast(local_thread_info->current_cond));

416             if(local_thread_info->current_cond)
417             {
418                 boost::pthread::pthread_mutex_scoped_lock internal_lock(local_thread_info->cond_mutex);
419                 BOOST_VERIFY(!pthread_cond_broadcast(local_thread_info->current_cond));
420             }

[bvanassche@…]

Do you realize that the above grep output supports my claim that it is safe to remove thread_data_base::current_cond ?

[viboes]

In boost/thread

grep -rHn current_cond * | grep -v svn
pthread/thread_data.hpp:60:            pthread_cond_t* current_cond;
pthread/thread_data.hpp:67:                current_cond(0)
pthread/thread_data.hpp:104:                    thread_info->current_cond=cond;
pthread/thread_data.hpp:119:                    thread_info->current_cond=NULL;

Note that in line 104 current_cond stores an external condition that comes from the interruption_checker. It is the user of this external condition that will do the wait, isn't it?

grep -rHn interruption_checker * | grep -v svn
pthread/condition_variable.hpp:53:            detail::interruption_checker check_for_interruption(&internal_mutex,&cond);
pthread/condition_variable.hpp:71:            detail::interruption_checker check_for_interruption(&internal_mutex,&cond);
pthread/condition_variable.hpp:135:                detail::interruption_checker check_for_interruption(&internal_mutex,&cond);
pthread/condition_variable.hpp:159:                detail::interruption_checker check_for_interruption(&internal_mutex,&cond);

So IMO the current_cond can not be removed.

[viboes]

Why the signaling thread needs to lock the associated mutex?

Moved to support request until resolution clarified.

[viboes]

Replying to viboes:

Why the signaling thread needs to lock the associated mutex?

Moved to support request until resolution clarified.

BTW, local_thread_info->current_cond is protected already by local_thread_info->cond_mutex

                boost::pthread::pthread_mutex_scoped_lock internal_lock(local_thread_info->cond_mutex);
                BOOST_VERIFY(!pthread_cond_broadcast(local_thread_info->current_cond));

So, for me this is a false positive.