Opened 11 years ago
Closed 10 years ago
#5752 closed Bugs (fixed)
boost::call_once() is unreliable on some platforms
Reported by: | Owned by: | viboes | |
---|---|---|---|
Milestone: | Boost 1.54.0 | Component: | thread |
Version: | Boost 1.47.0 | Severity: | Problem |
Keywords: | once | Cc: | viboes, fmhess@… |
Description
boost::call_once() is an implementation of Mike Burrows's fast_pthread_once() algorithm, as described in http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2660.htm. In the correctness proof, there is a requirement that loads and stores of the epoch value are atomic (i.e., do not exhibit word tearing). In Mike's example implementation, the sig_atomic_t type is used, which is required by the C standard to support atomic loads and stores.
However, in the boost::call_once() implementation, the epoch value is defined as uintmax_t, which has no such guarantee, and in practice is not atomic on some architectures. E.g., on OpenBSD/i386, uintmax_t is a 64-bit type and assignments to a 64-bit memory address must be split into two (non-atomic) store instructions.
Therefore, thread/pthread/once.hpp should be changed to use a type that is guaranteed to support atomic loads and stores instead of uintmax_t. Additionally, since once_flag::epoch is accessed by multiple threads without any synchronization, it should be marked volatile.
(Alternatively, the new C++0x atomic operations library appears suitable for this use as well.)
Attachments (1)
Change History (23)
comment:1 by , 11 years ago
Cc: | added |
---|---|
Component: | threads → thread |
comment:2 by , 11 years ago
Keywords: | once added |
---|
comment:4 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 10 years ago
Milestone: | To Be Determined → Boost 1.52.0 |
---|
Committed in trunk revision [80078].
comment:7 by , 10 years ago
Milestone: | Boost 1.52.0 → To Be Determined |
---|
Committed in trunk revision [80082].
comment:8 by , 10 years ago
Milestone: | To Be Determined → Boost 1.52.0 |
---|
follow-up: 18 comment:9 by , 10 years ago
When SIG_ATOMIC_MAX is defined (that is, when I compile on Linux with g++ 4.4.5 and -std=c++0x) call_once does not work. It never calls my function at all. I debugged it, it happens because in thread/pthread/once.hpp in the call_once method it compares epoch and this_thread_epoch. epoch is set to 0 and this_thread_epoch is set to -1 so the comparison is false and call once does nothing. It seems the problem is due to uintmax_atomic_t being defined as a sig_atomic_t which turns out to be a signed rather than unsigned type.
comment:10 by , 10 years ago
Some more info, it seems the problem is due to SIG_ATOMIC_MAX being defined in my code when I include once.hpp, but it is not defined in once.cpp when the libboost_thread is compiled. By default, cstdint doesn't define SIG_ATOMIC_MAX when compiling in c++ mode, unless you define __STDC_LIMIT_MACROS
before stdint.h is included.
comment:11 by , 10 years ago
Cc: | added |
---|
comment:12 by , 10 years ago
I will add it
Index: pthread/once.hpp =================================================================== --- pthread/once.hpp (revision 80450) +++ pthread/once.hpp (working copy) @@ -17,6 +17,8 @@ #include <boost/thread/pthread/pthread_mutex_scoped_lock.hpp> #include <boost/cstdint.hpp> #include <boost/thread/detail/delete.hpp> +// Force SIG_ATOMIC_MAX tobe defined +#define __STDC_LIMIT_MACROS #include <csignal> #include <boost/config/abi_prefix.hpp>
follow-up: 15 comment:14 by , 10 years ago
SIG_ATOMIC_MAX actually gets defined by stdint.h/cstdint not csignal so it seems like your define should be earlier. Actually, even then there's still a problem since the user might include stdint.h before they include the once.hpp, without having __STD_LIMIT_MACROS
defined. In that case, SIG_ATOMIC_MAX won't be defined in their code but it will be defined in the thread library, leading to incompatible types again.
follow-up: 16 comment:15 by , 10 years ago
Replying to fmhess:
SIG_ATOMIC_MAX actually gets defined by stdint.h/cstdint not csignal so it seems like your define should be earlier. Actually, even then there's still a problem since the user might include stdint.h before they include the once.hpp, without having
__STD_LIMIT_MACROS
defined. In that case, SIG_ATOMIC_MAX won't be defined in their code but it will be defined in the thread library, leading to incompatible types again.
It seems then that I can not relay on whether SIG_ATOMIC_MAX is defined or not, but I need a value to mean not-a-valid value. Have a suggestion?
follow-up: 17 comment:16 by , 10 years ago
Replying to viboes:
It seems then that I can not relay on whether SIG_ATOMIC_MAX is defined or not, but I need a value to mean not-a-valid value. Have a suggestion?
How about using a boost::atomic<unsigned long> for the epoch?
comment:17 by , 10 years ago
Milestone: | Boost 1.52.0 → To Be Determined |
---|
Replying to fmhess:
Replying to viboes:
It seems then that I can not relay on whether SIG_ATOMIC_MAX is defined or not, but I need a value to mean not-a-valid value. Have a suggestion?
How about using a boost::atomic<unsigned long> for the epoch?
I will try to use it when released. I can not depend on a non released library.
comment:18 by , 10 years ago
Replying to fhess:
When SIG_ATOMIC_MAX is defined ... call_once does not work. ... It seems the problem is due to uintmax_atomic_t being defined as a sig_atomic_t which turns out to be a signed rather than unsigned type.
This problem with using sig_atomic_t has been reported separately: https://svn.boost.org/trac/boost/ticket/7499 Too bad I didn't know about this sooner.
by , 10 years ago
Attachment: | atomic_once.patch added |
---|
The patch ports POSIX implementation of call_once to Boost.Atomic.
comment:19 by , 10 years ago
Milestone: | To Be Determined → Boost 1.53.0 |
---|
comment:21 by , 10 years ago
Milestone: | Boost 1.53.0 → Boost 1.54.0 |
---|
comment:22 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
merged from trunk [82838]
I would like to make it more reliable, but I don't know on which platforms uintmax_t is unreliable and which type will be reliable for these platforms. Is sig_atomic_t available and reliable on these platforms? Could some one help on this?