Opened 11 years ago

Closed 10 years ago

#5752 closed Bugs (fixed)

boost::call_once() is unreliable on some platforms

Reported by: Matthew Dempsky <matthew@…> 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)

atomic_once.patch (11.6 KB ) - added by Andrey Semashev 10 years ago.
The patch ports POSIX implementation of call_once to Boost.Atomic.

Download all attachments as: .zip

Change History (23)

comment:1 by viboes, 11 years ago

Cc: viboes added
Component: threadsthread

comment:2 by viboes, 11 years ago

Keywords: once added

comment:3 by viboes, 10 years ago

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?

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

comment:4 by viboes, 10 years ago

Owner: changed from Anthony Williams to viboes
Status: newassigned

comment:5 by viboes, 10 years ago

Does sig_atomic_t works for you?

comment:6 by viboes, 10 years ago

Milestone: To Be DeterminedBoost 1.52.0

Committed in trunk revision [80078].

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

comment:7 by viboes, 10 years ago

Milestone: Boost 1.52.0To Be Determined

Committed in trunk revision [80082].

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

comment:8 by viboes, 10 years ago

Milestone: To Be DeterminedBoost 1.52.0

comment:9 by fhess, 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 fhess, 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 Frank Mori Hess, 10 years ago

Cc: fmhess@… added

comment:12 by viboes, 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>

comment:13 by viboes, 10 years ago

Committed in trunk revision 80466.

comment:14 by Frank Mori Hess, 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.

in reply to:  14 ; comment:15 by viboes, 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?

in reply to:  15 ; comment:16 by Frank Mori Hess, 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?

in reply to:  16 comment:17 by viboes, 10 years ago

Milestone: Boost 1.52.0To 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.

in reply to:  9 comment:18 by kab@…, 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 Andrey Semashev, 10 years ago

Attachment: atomic_once.patch added

The patch ports POSIX implementation of call_once to Boost.Atomic.

comment:19 by viboes, 10 years ago

Milestone: To Be DeterminedBoost 1.53.0

comment:20 by viboes, 10 years ago

Committed in trunk revision [82513].

comment:21 by viboes, 10 years ago

Milestone: Boost 1.53.0Boost 1.54.0

comment:22 by viboes, 10 years ago

Resolution: fixed
Status: assignedclosed

merged from trunk [82838]

Note: See TracTickets for help on using tickets.