On Sep 5, 2012, at 4:03 PM, Martin Sebor wrote:
> On 09/05/2012 01:33 PM, Liviu Nicoara wrote:
>> On 09/05/12 15:17, Martin Sebor wrote:
>>> On 09/05/2012 12:40 PM, Liviu Nicoara wrote:
>>>> On 09/05/12 14:09, Stefan Teleman wrote:
>>>>> On Wed, Sep 5, 2012 at 10:52 AM, Martin Sebor <[email protected]> wrote:
>>>>> [...]
>>>>> OK so I did a little bit of testing, after looking at the *right*
>>>>> __rw_guard class. :-)
>>>>>
>>>>> I changed the std::numpunct class thusly:
>>>>> [...]
>>>>
>>>> I am afraid this would be unsafe, too (if I said otherwise earlier I was
>>>> wrong). [...] Thoughts?
>>>
>>> You're right, there's still a problem. We didn't get the double
>>> checked locking quite right.
>>
>> I wish for simplicity: eliminate the lazy initialization, and fully
>> initialize the facet in the constructor. Then we'd need no locking on
>> copying its data (std::string takes care of its own copying).
>
> I'm not sure how easily we can do that. Almost all of locale
> is initialized lazily. Some of the layers might depend on the
> facets being initialized lazily as well. This was a deliberate
> design choice. One of the constraints was to avoid dynamic
> initialization or allocation at startup. [...]
There would be a performance degradation. IMHO, it would be minor and would
simplify the code considerably.
After finally being able to reproduce the defect with SunPro 12.3 on x86_64 I
tried to remove the lazy initialization and a (smaller) test case now passes. I
am attaching the program and the numpunct patch.
Will continue to look into it. Although STDCXX does not have an implementation
of the atomics library we could probably use the existing, unexposed, atomics
API to implement a more robust lazy initialization.
Liviu
$ cat tests/localization/t.cpp; svn diff
#include <locale>
#include <cstdio>
#include <cstdlib>
#include <unistd.h>
#define MAX_THREADS 16
#define MAX_LOOPS 1000
static bool hold = true;
extern "C" {
static void*
f (void* pv)
{
std::numpunct<char> const& fac =
*reinterpret_cast< std::numpunct<char>* > (pv);
while (hold) ;
for (int i = 0; i != MAX_LOOPS; ++i) {
std::string const grp = fac.grouping ();
}
return 0;
}
}
int
main (int argc, char** argv)
{
std::locale const loc = std::locale (argv [1]);
std::numpunct<char> const& fac =
std::use_facet<std::numpunct<char> > (loc);
pthread_t tid [MAX_THREADS] = { 0 };
for (int i = 0; i < MAX_THREADS; ++i) {
if (pthread_create (tid + i, 0, f, (void*)&fac))
exit (-1);
}
sleep (1);
hold = false;
for (int i = 0; i < MAX_THREADS; ++i) {
if (tid [i])
pthread_join (tid [i], 0);
}
return 0;
}
Index: include/loc/_numpunct.h
===================================================================
--- include/loc/_numpunct.h (revision 1381575)
+++ include/loc/_numpunct.h (working copy)
@@ -61,24 +61,40 @@ struct numpunct: _RW::__rw_facet
string_type;
_EXPLICIT numpunct (_RWSTD_SIZE_T __ref = 0)
- : _RW::__rw_facet (__ref), _C_flags (0) { }
+ : _RW::__rw_facet (__ref) {
+ _C_grouping = do_grouping ();
+ _C_truename = do_truename ();
+ _C_falsename = do_falsename ();
+ _C_decimal_point = do_decimal_point ();
+ _C_thousands_sep = do_thousands_sep ();
+ }
virtual ~numpunct () _RWSTD_ATTRIBUTE_NOTHROW;
// 22.2.3.1.1, p1
- char_type decimal_point () const;
+ char_type decimal_point () const {
+ return _C_decimal_point;
+ }
// 22.2.3.1.1, p2
- char_type thousands_sep () const;
+ char_type thousands_sep () const {
+ return _C_thousands_sep;
+ }
// 22.2.3.1.1, p3
- string grouping () const;
+ string grouping () const {
+ return _C_grouping;
+ }
// 22.2.3.1.1, p4
- string_type truename () const;
+ string_type truename () const {
+ return _C_truename;
+ }
// 22.2.3.1.1, p4
- string_type falsename () const;
+ string_type falsename () const {
+ return _C_falsename;
+ }
static _RW::__rw_facet_id id;
@@ -112,15 +128,13 @@ protected:
private:
- int _C_flags; // bitmap of "cached data valid" flags
- string _C_grouping; // cached results of virtual members
+ string _C_grouping;
string_type _C_truename;
string_type _C_falsename;
char_type _C_decimal_point;
char_type _C_thousands_sep;
};
-
#ifndef _RWSTD_NO_SPECIALIZED_FACET_ID
_RWSTD_SPECIALIZED_CLASS
@@ -134,95 +148,6 @@ _RW::__rw_facet_id numpunct<wchar_t>::id
# endif // _RWSTD_NO_WCHAR_T
#endif // _RWSTD_NO_SPECIALIZED_FACET_ID
-
-template <class _CharT>
-inline _TYPENAME numpunct<_CharT>::char_type
-numpunct<_CharT>::decimal_point () const
-{
- if (!(_C_flags & _RW::__rw_dp)) {
-
- numpunct* const __self = _RWSTD_CONST_CAST (numpunct*, this);
-
- // [try to] get the decimal point first (may throw)
- // then set a flag to avoid future initializations
- __self->_C_decimal_point = do_decimal_point ();
- __self->_C_flags |= _RW::__rw_dp;
- }
-
- return _C_decimal_point;
-}
-
-
-template <class _CharT>
-inline _TYPENAME numpunct<_CharT>::char_type
-numpunct<_CharT>::thousands_sep () const
-{
- if (!(_C_flags & _RW::__rw_ts)) {
-
- numpunct* const __self = _RWSTD_CONST_CAST (numpunct*, this);
-
- // [try to] get the thousands_sep first (may throw)
- // then set a flag to avoid future initializations
- __self->_C_thousands_sep = do_thousands_sep ();
- __self->_C_flags |= _RW::__rw_ts;
- }
-
- return _C_thousands_sep;
-}
-
-
-template <class _CharT>
-inline string numpunct<_CharT>::grouping () const
-{
- if (!(_C_flags & _RW::__rw_gr)) {
-
- numpunct* const __self = _RWSTD_CONST_CAST (numpunct*, this);
-
- // [try to] get the grouping first (may throw)
- // then set a flag to avoid future initializations
- __self->_C_grouping = do_grouping ();
- __self->_C_flags |= _RW::__rw_gr;
- }
-
- return _C_grouping;
-}
-
-
-template <class _CharT>
-inline _TYPENAME numpunct<_CharT>::string_type
-numpunct<_CharT>::truename () const
-{
- if (!(_C_flags & _RW::__rw_tn)) {
-
- numpunct* const __self = _RWSTD_CONST_CAST (numpunct*, this);
-
- // [try to] get the true name first (may throw)
- // then set a flag to avoid future initializations
- __self->_C_truename = do_truename ();
- __self->_C_flags |= _RW::__rw_tn;
- }
-
- return _C_truename;
-}
-
-
-template <class _CharT>
-inline _TYPENAME numpunct<_CharT>::string_type
-numpunct<_CharT>::falsename () const
-{
- if (!(_C_flags & _RW::__rw_fn)) {
-
- numpunct* const __self = _RWSTD_CONST_CAST (numpunct*, this);
-
- // [try to] get the false name first (may throw)
- // then set a flag to avoid future initializations
- __self->_C_falsename = do_falsename ();
- __self->_C_flags |= _RW::__rw_fn;
- }
-
- return _C_falsename;
-}
-
// #endif _RWSTD_NO_EXT_NUMPUNCT_PRIMARY
