Boost.Xint is not thread-safe
The proposed Boost.Xint does not fulfill the thread-safety promises that it makes. The library uses a custom allocator (allocator_t), which uses global variables and does not protect them with a synchronization primitive. There appears to be no way to override the use of this custom allocator (the library just passes the user-specified STL allocator to allocator_t, and allocator_t uses the STL allocator to get memory). Can the author of this library please make the custom allocation strategy optional? Merely making allocator_t threadsafe wouldn't be a solution for me unless I could change the synchronization primitive that this allocator uses. I am using this library within a parallel C++ runtime that implements (among other things) a user-threading layer, so I would want to use one of our synchronization primitives, as OS-level (e.g. Boost.Thread) based synchronization would interfere with our user threading subsystem. I think the better solution here is to not tie Boost.Xint to a particular memory allocation scheme. The allocator in question is in <boost/xint/detail/policies.hpp> -- Bryce Lelbach aka wash boost-spirit.com px.cct.lsu.edu github.com/lll-project
On Sun, 19 Jun 2011 12:29:41 -0500 Bryce Lelbach <blelbach@cct.lsu.edu> wrote:
The proposed Boost.Xint does not fulfill the thread-safety promises that it makes. The library uses a custom allocator (allocator_t), which uses global variables and does not protect them with a synchronization primitive. [...]
What global variables are those? The only non-local variables I see from a quick perusal of allocator_t are fixed_length and magnitude_datasize (allocator_t static consts) and minimum_digits (a global const).
Can the author of this library please make the custom allocation strategy optional? [...] I think the better solution here is to not tie Boost.Xint to a particular memory allocation scheme.
If I work on it any further (still undecided), that's already on the to-do list. -- Chad Nelson Oak Circle Software, Inc. * * *
On 2011.06.19_18.08.11, Chad Nelson wrote:
On Sun, 19 Jun 2011 12:29:41 -0500 Bryce Lelbach <blelbach@cct.lsu.edu> wrote:
What global variables are those? The only non-local variables I see from a quick perusal of allocator_t are fixed_length and magnitude_datasize (allocator_t static consts) and minimum_digits (a global const).
List is below. Problems that are immediately visible: * exception_handler uses an unprotected static data member to hold an exception handler; this variable is not once-initialized with boost::call_once. * get_nan() uses static variables and does not initialize them in a thread-safe manner. * nan_text() uses static variables and does not initialize them in a thread-safe manner. * magnitude_manager_t<>::zerodata() uses a static variable, which is not initialized in a thread safe manner. * TUTable (monty.hpp) uses a static std::auto_pt<> which is not initialized in a thread safe manner, and can lead to race-condition induced memory leaks (in addition to data corruption). You check if the auto_ptr is 0 before reseting it, but there is no synchronization there, so two threads could read a zero, assume it's not been initialized, and allocate storage for it, leading to at least one excessive allocation, if not a leak. * is_prime() uses a static std::vector<int> which is not initialized in a thread safe manner. * is_prime() uses an std::auto_ptr<> (to a random generator) which is not initialized in a thread safe manner, and may lead to memory leaks, among other issues. * Your reference counting implementation is not thread safe. The reference counting implementation consists of an std::size_t member in the magnitude_t class, which is incremented by an inc() function, and decrementing by a dec() function. No synchronization is performed before, during, or after increments or decrements. * magnitude_manager_t's copy constructor doesn't copy, it just increments the reference count of the rhs. * raw_integer_t's copy constructor doesn't copy, it does the same thing that magnitude_manager_t does. The raw_integer_t constructor for other instantiations of raw_integer_t does ensure unique copies. * copy semantics are performed lazily as needed by magnitude_manager_t::resize_and_uniquify/raw_integer_t::make_unique (and the backend, allocator_t::realloc_and_uniquify), because of the above three points, this leads to copies that share storage. * integer_t's copy constructor only calls data.make_unique() if Threadsafe == false && force_thread_safety. This is not thread safe as it prevents resize_and_uniquify from being called, which leads to copies of the same integer_t sharing storage, as explained above. Copy assignment and move assignment operators are fine. * The above points mean that the "threadsafe" option isn't actually threadsafe. Other notes: * You should use BOOST_STATIC_CONSTANT instead of defining static constants by hand. * Please use BOOST_ASSERT instead of assert().
If I work on it any further (still undecided), that's already on the to-do list.
If you're not going to make Boost.Xint thread safe, please remove the notices that claim that the library is thread safe. I found a "boost.bigint" library in the Boost sandbox (under 2007 GSoC). Said library is thread safe, albeit slightly slower. I would advise anyone else looking for a Boost-style arbitrary precision integer library to check out bigint as well. Chad, if you wish to use globals, please investigate a thread-safe singleton approach. Robert Ramey has a nice one in Boost.Serialization; attached is the one that we use at work. Is this Xint library accepted? -- Bryce Lelbach aka wash boost-spirit.com px.cct.lsu.edu github.com/lll-project
On Mon, Jun 20, 2011 at 9:02 AM, Bryce Lelbach <blelbach@cct.lsu.edu> wrote:
Is this Xint library accepted?
No, AFAIK and IIRC. Cheers -- Dean Michael Berris http://about.me/deanberris
On Sun, 19 Jun 2011 18:02:16 -0500 Bryce Lelbach <blelbach@cct.lsu.edu> wrote:
What global variables are those? The only non-local variables I see from a quick perusal of allocator_t are fixed_length and magnitude_datasize (allocator_t static consts) and minimum_digits (a global const).
List is below. [...]
Thank you. I'll note those.
Is this Xint library accepted?
No, it was rejected. -- Chad Nelson Oak Circle Software, Inc. * * *
participants (3)
-
Bryce Lelbach -
Chad Nelson -
Dean Michael Berris