stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Liviu Nicoara <nikko...@hates.ms>
Subject Re: STDCXX-1056 [was: Re: STDCXX forks]
Date Thu, 06 Sep 2012 23:31:42 GMT

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 <msebor@gmail.com>
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
 

Mime
View raw message