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