Modify

Ticket #5752 (closed Bugs: fixed)

Opened 3 years ago

Last modified 14 months ago

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

atomic_once.patch Download (11.6 KB) - added by andysem 15 months ago.
The patch ports POSIX implementation of call_once to Boost.Atomic.

Change History

comment:1 Changed 2 years ago by viboes

  • Cc viboes added
  • Component changed from threads to thread

comment:2 Changed 2 years ago by viboes

  • Keywords once added

comment:3 Changed 21 months ago by viboes

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 21 months ago by viboes (previous) (diff)

comment:4 Changed 20 months ago by viboes

  • Owner changed from anthonyw to viboes
  • Status changed from new to assigned

comment:5 Changed 20 months ago by viboes

Does sig_atomic_t works for you?

comment:6 Changed 20 months ago by viboes

  • Milestone changed from To Be Determined to Boost 1.52.0

Committed in trunk revision [80078].

Last edited 20 months ago by viboes (previous) (diff)

comment:7 Changed 20 months ago by viboes

  • Milestone changed from Boost 1.52.0 to To Be Determined

Committed in trunk revision [80082].

Last edited 20 months ago by viboes (previous) (diff)

comment:8 Changed 20 months ago by viboes

  • Milestone changed from To Be Determined to Boost 1.52.0

comment:9 follow-up: ↓ 18 Changed 20 months ago by fhess

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 Changed 20 months ago by fhess

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 Changed 20 months ago by fmhess

  • Cc fmhess@… added

comment:12 Changed 20 months ago by viboes

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 Changed 20 months ago by viboes

Committed in trunk revision 80466.

comment:14 follow-up: ↓ 15 Changed 20 months ago by 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.

comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 19 months ago by viboes

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?

comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 19 months ago by 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?

comment:17 in reply to: ↑ 16 Changed 19 months ago by viboes

  • Milestone changed from Boost 1.52.0 to 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 in reply to: ↑ 9 Changed 18 months ago by kab@…

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.

Changed 15 months ago by andysem

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

comment:19 Changed 15 months ago by viboes

  • Milestone changed from To Be Determined to Boost 1.53.0

comment:20 Changed 15 months ago by viboes

Committed in trunk revision [82513].

comment:21 Changed 15 months ago by viboes

  • Milestone changed from Boost 1.53.0 to Boost 1.54.0

comment:22 Changed 14 months ago by viboes

  • Status changed from assigned to closed
  • Resolution set to fixed

merged from trunk [82838]

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


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

 
Note: See TracTickets for help on using tickets.