You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@stdcxx.apache.org by el...@apache.org on 2008/04/03 17:32:57 UTC
svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Author: elemings
Date: Thu Apr 3 08:32:56 2008
New Revision: 644364
URL: http://svn.apache.org/viewvc?rev=644364&view=rev
Log:
2008-04-03 Eric Lemings <er...@roguewave.com>
STDCXX-811
* src/locale_global.cpp (std::locale::global): Replace call to
non-reentrant setlocale() C function with reentrant
__rw_setlocale object.
* tests/localization/22.locale.statics.mt.cpp (test_global):
Fix [un]signed integer conversion warnings.
Modified:
stdcxx/trunk/src/locale_global.cpp
stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp
Modified: stdcxx/trunk/src/locale_global.cpp
URL: http://svn.apache.org/viewvc/stdcxx/trunk/src/locale_global.cpp?rev=644364&r1=644363&r2=644364&view=diff
==============================================================================
--- stdcxx/trunk/src/locale_global.cpp (original)
+++ stdcxx/trunk/src/locale_global.cpp Thu Apr 3 08:32:56 2008
@@ -37,6 +37,7 @@
#include <loc/_locale.h>
#include "locale_body.h"
+#include "setlocale.h"
@@ -53,7 +54,8 @@
// assumes all locale names (i.e., including those of combined
// locales) are in a format understandable by setlocale()
- setlocale (_RWSTD_LC_ALL, rhs.name ().c_str ());
+ const _RW::__rw_setlocale clocale (rhs.name().c_str(), _RWSTD_LC_ALL);
+ _RWSTD_UNUSED(clocale);
// FIXME:
// handle unnamed locales (i.e., locales containing non-standard
Modified: stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp
URL: http://svn.apache.org/viewvc/stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp?rev=644364&r1=644363&r2=644364&view=diff
==============================================================================
--- stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp (original)
+++ stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp Thu Apr 3 08:32:56 2008
@@ -88,9 +88,9 @@
static void*
test_global (void*)
{
- for (int i = 0; i != opt_nloops; ++i) {
+ for (std::size_t i = 0; i != opt_nloops; ++i) {
- const int inx = i % nlocales;
+ const std::size_t inx = i % nlocales;
const std::locale last (std::locale::global (locales [inx]));
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
>
> Martin Sebor wrote:
>> Travis Vitek wrote:
>>>
>>>
>>> Right. It seems that for this to be mt-safe with respect to other
>>> library code that calls setlocale(), we would need to lock the
>>> same lock that is used by _RW::__rw_setlocale. If don't use the
>>> same lock it would be possible for the std::locale::global() call
>>> to change the locale while some other library code is reading
>>> locale specific data under protection of an active
>>> _RW::__rw_setlocale instance.
>> Good point. That probably means extending the __rw_setlocale
>> interface to release the lock without restoring the original
>> locale name.
>>
>>
>
> Is there a chance that such a change would not be forward binary
> compatible?
__rw_setlocale is an implementation class that's not exposed
in our headers so there shouldn't be any changes to it that
we could make that would be binary incompatible.
Martin
>
> C:\Build\stdcxx\trunk\msvc-8.0\12d\lib>dumpbin /exports libstd12d.dll |
> find "setlocale"
> 123 7A 000114C0 ??0__rw_setlocale@__rw@@QAE@PBDHH@Z =
> ??0__rw_setlocale@__rw@@QAE@PBDHH@Z (public: __thiscall
> __rw::__rw_setlocale::__rw_setlocale(char const *,int,int))
> 245 F4 00011610 ??1__rw_setlocale@__rw@@QAE@XZ =
> ??1__rw_setlocale@__rw@@QAE@XZ (public: __thiscall
> __rw::__rw_setlocale::~__rw_setlocale(void))
> 523 20A 00002C20 ??_F__rw_setlocale@__rw@@QAEXXZ =
> ??_F__rw_setlocale@__rw@@QAEXXZ (public: void __thiscall
> __rw::__rw_setlocale::`default constructor closure'(void))
> 1693 69C 00002C10 ?name@__rw_setlocale@__rw@@QBEPBDXZ =
> ?name@__rw_setlocale@__rw@@QBEPBDXZ (public: char const * __thiscall
> __rw::__rw_setlocale::name(void)const )
>
> Travis
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Travis Vitek <Tr...@roguewave.com>.
Martin Sebor wrote:
>
>Travis Vitek wrote:
>>
>>
>> Right. It seems that for this to be mt-safe with respect to other
>> library code that calls setlocale(), we would need to lock the
>> same lock that is used by _RW::__rw_setlocale. If don't use the
>> same lock it would be possible for the std::locale::global() call
>> to change the locale while some other library code is reading
>> locale specific data under protection of an active
>> _RW::__rw_setlocale instance.
>
>Good point. That probably means extending the __rw_setlocale
>interface to release the lock without restoring the original
>locale name.
>
>
Is there a chance that such a change would not be forward binary
compatible?
C:\Build\stdcxx\trunk\msvc-8.0\12d\lib>dumpbin /exports libstd12d.dll |
find "setlocale"
123 7A 000114C0 ??0__rw_setlocale@__rw@@QAE@PBDHH@Z =
??0__rw_setlocale@__rw@@QAE@PBDHH@Z (public: __thiscall
__rw::__rw_setlocale::__rw_setlocale(char const *,int,int))
245 F4 00011610 ??1__rw_setlocale@__rw@@QAE@XZ =
??1__rw_setlocale@__rw@@QAE@XZ (public: __thiscall
__rw::__rw_setlocale::~__rw_setlocale(void))
523 20A 00002C20 ??_F__rw_setlocale@__rw@@QAEXXZ =
??_F__rw_setlocale@__rw@@QAEXXZ (public: void __thiscall
__rw::__rw_setlocale::`default constructor closure'(void))
1693 69C 00002C10 ?name@__rw_setlocale@__rw@@QBEPBDXZ =
?name@__rw_setlocale@__rw@@QBEPBDXZ (public: char const * __thiscall
__rw::__rw_setlocale::name(void)const )
Travis
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Travis Vitek [mailto:Travis.Vitek@roguewave.com]
> Sent: Thursday, April 03, 2008 1:05 PM
> To: dev@stdcxx.apache.org
> Subject: RE: svn commit: r644364 - in /stdcxx/trunk:
> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
>
>
> >Eric Lemings wrote:
> >
> >Please peer review the following patch at your earliest convenience.
> >
>
> Brad,
>
> I don't think that this patch is going to work. The
> unnecessary changes
> to the __rw_setlocale ctor and dtor will cause new failures.
>
> The __rw_setlocale type is supposed to hold the lock for its
> lifetime to
> prevent other __rw_setlocale instances from being created and changing
> the locale. Your change prevents concurrent calls to
> setlocale() through
> __rw_setlocale() instances, but it doesn't prevent multiple
> __rw_setlocale instances from existing simultaneously.
So if thread A creates one of these objects, thread B can't change the
global locale until thread A destroys the __rw_setlocale object? That's
some lengthy syncronization but if the code depends on it...
Will fix according to Martin's suggestion to expose the mutex.
Brad.
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Travis Vitek <Tr...@roguewave.com>.
>Eric Lemings wrote:
>
>Please peer review the following patch at your earliest convenience.
>
Brad,
I don't think that this patch is going to work. The unnecessary changes
to the __rw_setlocale ctor and dtor will cause new failures.
The __rw_setlocale type is supposed to hold the lock for its lifetime to
prevent other __rw_setlocale instances from being created and changing
the locale. Your change prevents concurrent calls to setlocale() through
__rw_setlocale() instances, but it doesn't prevent multiple
__rw_setlocale instances from existing simultaneously.
Travis
>-----
>$ svn diff
>Index: src/setlocale.h
>===================================================================
>--- src/setlocale.h (revision 644426)
>+++ src/setlocale.h (working copy)
>@@ -58,6 +58,9 @@
> return _C_name;
> }
>
>+ static const char* __rw_curlocale (int);
>+ static bool __rw_newlocale (const char*, int);
>+
> private:
>
> __rw_setlocale (const __rw_setlocale&); // not defined
>Index: src/setlocale.cpp
>===================================================================
>--- src/setlocale.cpp (revision 644426)
>+++ src/setlocale.cpp (working copy)
>@@ -80,11 +80,8 @@
> __rw_setlocale::__rw_setlocale (const char *locname, int cat, int
>nothrow)
> : _C_name (0), _C_guard (1), _C_cat (cat)
> {
>- // acquire global per-process lock
>- __rw_setlocale_mutex._C_acquire ();
>-
> // retrieve previous locale name and check if it is already set
>- const char* const curname = ::setlocale (cat, 0);
>+ const char* const curname = __rw_curlocale (cat);
>
> if (curname) {
>
>@@ -101,14 +98,13 @@
>
> ::memcpy (_C_name, curname, size);
>
>- if (::setlocale (cat, locname)) {
>+ if (__rw_newlocale (locname, cat)) {
>
> // a NULL name is only used when we want to use the object
> // as a retriever for the name of the current locale;
> // no need for a lock then
> if (!locname) {
> _C_guard = 0;
>- __rw_setlocale_mutex._C_release ();
> }
>
> return;
>@@ -122,7 +118,6 @@
>
> // bad locales OR bad locale names OR bad locale categories
> _C_guard = 0;
>- __rw_setlocale_mutex._C_release ();
>
> // failure in setting the locale
> _RWSTD_REQUIRES (nothrow, (_RWSTD_ERROR_LOCALE_BAD_NAME,
>@@ -131,6 +126,25 @@
> }
>
>
>+/*static*/ const char*
>+__rw_setlocale::__rw_curlocale (int cat)
>+{
>+ __rw_setlocale_mutex._C_acquire ();
>+ const char* const curname = ::setlocale (cat, 0);
>+ __rw_setlocale_mutex._C_release ();
>+ return curname;
>+}
>+
>+/*static*/ bool
>+__rw_setlocale::__rw_newlocale (const char* locname, int cat)
>+{
>+ __rw_setlocale_mutex._C_acquire ();
>+ bool changed = (0 != ::setlocale (cat, locname));
>+ __rw_setlocale_mutex._C_release ();
>+ return changed;
>+}
>+
>+
> // dtor restores the C library locale that
> // was in effect when the object was constructed
> __rw_setlocale::~__rw_setlocale ()
>Index: src/locale_global.cpp
>===================================================================
>--- src/locale_global.cpp (revision 644426)
>+++ src/locale_global.cpp (working copy)
>@@ -54,8 +54,7 @@
>
> // assumes all locale names (i.e., including those of combined
> // locales) are in a format understandable by setlocale()
>- const _RW::__rw_setlocale clocale (rhs.name().c_str(),
>_RWSTD_LC_ALL);
>- _RWSTD_UNUSED(clocale);
>+ _RW::__rw_setlocale::__rw_newlocale (rhs.name().c_str(),
>_RWSTD_LC_ALL);
>
> // FIXME:
> // handle unnamed locales (i.e., locales containing
>non-standard
>-----
>
>Thanks,
>Brad.
>
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
>
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:sebor@roguewave.com]
>> Sent: Thursday, April 03, 2008 1:02 PM
>> To: dev@stdcxx.apache.org
>> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
>> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>>
> ...
>> __rw_setlocale code. The fix to locale::global() will then
>> be to add
>>
>> _RWSTD_MT_GUARD (__rw_setlocale::_C_mutex);
>>
>> at the top of the function.
>
> You mean `_RWSTD_MT_GUARD (__rw_setlocale_mutex)' ?
I was suggesting to make __rw_setlocale_mutex a static member
of __rw_setlocale and rename it to _C_mutex. But this will
work too.
Martin
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Thursday, April 03, 2008 1:02 PM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
...
> __rw_setlocale code. The fix to locale::global() will then
> be to add
>
> _RWSTD_MT_GUARD (__rw_setlocale::_C_mutex);
>
> at the top of the function.
You mean `_RWSTD_MT_GUARD (__rw_setlocale_mutex)' ?
Brad.
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Posted by Martin Sebor <se...@roguewave.com>.
Eric Lemings wrote:
[...]
> Please peer review the following patch at your earliest convenience.
>
> -----
> $ svn diff
> Index: src/setlocale.h
> ===================================================================
> --- src/setlocale.h (revision 644426)
> +++ src/setlocale.h (working copy)
> @@ -58,6 +58,9 @@
> return _C_name;
> }
>
> + static const char* __rw_curlocale (int);
> + static bool __rw_newlocale (const char*, int);
> +
> private:
>
> __rw_setlocale (const __rw_setlocale&); // not defined
> Index: src/setlocale.cpp
> ===================================================================
> --- src/setlocale.cpp (revision 644426)
> +++ src/setlocale.cpp (working copy)
> @@ -80,11 +80,8 @@
> __rw_setlocale::__rw_setlocale (const char *locname, int cat, int
> nothrow)
> : _C_name (0), _C_guard (1), _C_cat (cat)
> {
> - // acquire global per-process lock
> - __rw_setlocale_mutex._C_acquire ();
> -
> // retrieve previous locale name and check if it is already set
> - const char* const curname = ::setlocale (cat, 0);
> + const char* const curname = __rw_curlocale (cat);
>
> if (curname) {
>
> @@ -101,14 +98,13 @@
>
> ::memcpy (_C_name, curname, size);
This doesn't seem safe: some other thread might have called
setlocale by now and invalidated our curname. The ctor needs
to hold the lock until it's finished copying the name of the
locale returned by setlocale().
IMO, the least invasive change to __rw_setlocale is to expose
the __rw_setlocale_mutex object (currently defined static in
setlocale.cpp) so that it can be used with the _RWSTD_MT_GUARD()
macro. That way we won't have to change any existing
__rw_setlocale code. The fix to locale::global() will then
be to add
_RWSTD_MT_GUARD (__rw_setlocale::_C_mutex);
at the top of the function.
Martin
>
> - if (::setlocale (cat, locname)) {
> + if (__rw_newlocale (locname, cat)) {
>
> // a NULL name is only used when we want to use the object
> // as a retriever for the name of the current locale;
> // no need for a lock then
> if (!locname) {
> _C_guard = 0;
> - __rw_setlocale_mutex._C_release ();
> }
>
> return;
> @@ -122,7 +118,6 @@
>
> // bad locales OR bad locale names OR bad locale categories
> _C_guard = 0;
> - __rw_setlocale_mutex._C_release ();
>
> // failure in setting the locale
> _RWSTD_REQUIRES (nothrow, (_RWSTD_ERROR_LOCALE_BAD_NAME,
> @@ -131,6 +126,25 @@
> }
>
>
> +/*static*/ const char*
> +__rw_setlocale::__rw_curlocale (int cat)
> +{
> + __rw_setlocale_mutex._C_acquire ();
> + const char* const curname = ::setlocale (cat, 0);
> + __rw_setlocale_mutex._C_release ();
> + return curname;
> +}
> +
> +/*static*/ bool
> +__rw_setlocale::__rw_newlocale (const char* locname, int cat)
> +{
> + __rw_setlocale_mutex._C_acquire ();
> + bool changed = (0 != ::setlocale (cat, locname));
> + __rw_setlocale_mutex._C_release ();
> + return changed;
> +}
> +
> +
> // dtor restores the C library locale that
> // was in effect when the object was constructed
> __rw_setlocale::~__rw_setlocale ()
> Index: src/locale_global.cpp
> ===================================================================
> --- src/locale_global.cpp (revision 644426)
> +++ src/locale_global.cpp (working copy)
> @@ -54,8 +54,7 @@
>
> // assumes all locale names (i.e., including those of combined
> // locales) are in a format understandable by setlocale()
> - const _RW::__rw_setlocale clocale (rhs.name().c_str(),
> _RWSTD_LC_ALL);
> - _RWSTD_UNUSED(clocale);
> + _RW::__rw_setlocale::__rw_newlocale (rhs.name().c_str(),
> _RWSTD_LC_ALL);
>
> // FIXME:
> // handle unnamed locales (i.e., locales containing
> non-standard
> -----
>
> Thanks,
> Brad.
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Eric Lemings
> Sent: Thursday, April 03, 2008 11:12 AM
> To: 'dev@stdcxx.apache.org'
> Subject: RE: svn commit: r644364 - in /stdcxx/trunk:
> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
>
>
> > -----Original Message-----
> > From: Martin Sebor [mailto:sebor@roguewave.com]
> > Sent: Thursday, April 03, 2008 11:06 AM
> > To: dev@stdcxx.apache.org
> > Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> > src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
> >
> > Travis Vitek wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Martin Sebor [mailto:sebor@roguewave.com]
> > >> Sent: Thursday, April 03, 2008 8:55 AM
> > >> To: dev@stdcxx.apache.org
> > >> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> > >> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
> > >>
> > >> elemings@apache.org wrote:
> > >>> Author: elemings
> > >>> Date: Thu Apr 3 08:32:56 2008
> > >>> New Revision: 644364
> > >>>
> > >>> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
> > >>> Log:
> > >>> 2008-04-03 Eric Lemings <er...@roguewave.com>
> > >>>
> > >>> STDCXX-811
> > >>> * src/locale_global.cpp (std::locale::global):
> Replace call to
> > >>> non-reentrant setlocale() C function with reentrant
> > >>> __rw_setlocale object.
> > >> I don't think that's correct. The object's dtor restores
> > >> the original locale. We need a mutex around the call to
> > >> setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
> > >> _RWSTD_CLASS_GUARD() macros.
> > >>
> > >
> > > Right. It seems that for this to be mt-safe with respect to other
> > > library code that calls setlocale(), we would need to lock
> > the same lock
> > > that is used by _RW::__rw_setlocale. If don't use the same
> > lock it would
> > > be possible for the std::locale::global() call to change
> the locale
> > > while some other library code is reading locale specific
> data under
> > > protection of an active _RW::__rw_setlocale instance.
> >
> > Good point. That probably means extending the __rw_setlocale
> > interface to release the lock without restoring the original
> > locale name.
>
> I'll whip up a patch to src/setlocale.[h/cpp] and post it for
> review before submitting the next change.
Please peer review the following patch at your earliest convenience.
-----
$ svn diff
Index: src/setlocale.h
===================================================================
--- src/setlocale.h (revision 644426)
+++ src/setlocale.h (working copy)
@@ -58,6 +58,9 @@
return _C_name;
}
+ static const char* __rw_curlocale (int);
+ static bool __rw_newlocale (const char*, int);
+
private:
__rw_setlocale (const __rw_setlocale&); // not defined
Index: src/setlocale.cpp
===================================================================
--- src/setlocale.cpp (revision 644426)
+++ src/setlocale.cpp (working copy)
@@ -80,11 +80,8 @@
__rw_setlocale::__rw_setlocale (const char *locname, int cat, int
nothrow)
: _C_name (0), _C_guard (1), _C_cat (cat)
{
- // acquire global per-process lock
- __rw_setlocale_mutex._C_acquire ();
-
// retrieve previous locale name and check if it is already set
- const char* const curname = ::setlocale (cat, 0);
+ const char* const curname = __rw_curlocale (cat);
if (curname) {
@@ -101,14 +98,13 @@
::memcpy (_C_name, curname, size);
- if (::setlocale (cat, locname)) {
+ if (__rw_newlocale (locname, cat)) {
// a NULL name is only used when we want to use the object
// as a retriever for the name of the current locale;
// no need for a lock then
if (!locname) {
_C_guard = 0;
- __rw_setlocale_mutex._C_release ();
}
return;
@@ -122,7 +118,6 @@
// bad locales OR bad locale names OR bad locale categories
_C_guard = 0;
- __rw_setlocale_mutex._C_release ();
// failure in setting the locale
_RWSTD_REQUIRES (nothrow, (_RWSTD_ERROR_LOCALE_BAD_NAME,
@@ -131,6 +126,25 @@
}
+/*static*/ const char*
+__rw_setlocale::__rw_curlocale (int cat)
+{
+ __rw_setlocale_mutex._C_acquire ();
+ const char* const curname = ::setlocale (cat, 0);
+ __rw_setlocale_mutex._C_release ();
+ return curname;
+}
+
+/*static*/ bool
+__rw_setlocale::__rw_newlocale (const char* locname, int cat)
+{
+ __rw_setlocale_mutex._C_acquire ();
+ bool changed = (0 != ::setlocale (cat, locname));
+ __rw_setlocale_mutex._C_release ();
+ return changed;
+}
+
+
// dtor restores the C library locale that
// was in effect when the object was constructed
__rw_setlocale::~__rw_setlocale ()
Index: src/locale_global.cpp
===================================================================
--- src/locale_global.cpp (revision 644426)
+++ src/locale_global.cpp (working copy)
@@ -54,8 +54,7 @@
// assumes all locale names (i.e., including those of combined
// locales) are in a format understandable by setlocale()
- const _RW::__rw_setlocale clocale (rhs.name().c_str(),
_RWSTD_LC_ALL);
- _RWSTD_UNUSED(clocale);
+ _RW::__rw_setlocale::__rw_newlocale (rhs.name().c_str(),
_RWSTD_LC_ALL);
// FIXME:
// handle unnamed locales (i.e., locales containing
non-standard
-----
Thanks,
Brad.
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Eric Lemings <Er...@roguewave.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Thursday, April 03, 2008 11:06 AM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
> Travis Vitek wrote:
> >
> >
> >> -----Original Message-----
> >> From: Martin Sebor [mailto:sebor@roguewave.com]
> >> Sent: Thursday, April 03, 2008 8:55 AM
> >> To: dev@stdcxx.apache.org
> >> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> >> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
> >>
> >> elemings@apache.org wrote:
> >>> Author: elemings
> >>> Date: Thu Apr 3 08:32:56 2008
> >>> New Revision: 644364
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
> >>> Log:
> >>> 2008-04-03 Eric Lemings <er...@roguewave.com>
> >>>
> >>> STDCXX-811
> >>> * src/locale_global.cpp (std::locale::global): Replace call to
> >>> non-reentrant setlocale() C function with reentrant
> >>> __rw_setlocale object.
> >> I don't think that's correct. The object's dtor restores
> >> the original locale. We need a mutex around the call to
> >> setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
> >> _RWSTD_CLASS_GUARD() macros.
> >>
> >
> > Right. It seems that for this to be mt-safe with respect to other
> > library code that calls setlocale(), we would need to lock
> the same lock
> > that is used by _RW::__rw_setlocale. If don't use the same
> lock it would
> > be possible for the std::locale::global() call to change the locale
> > while some other library code is reading locale specific data under
> > protection of an active _RW::__rw_setlocale instance.
>
> Good point. That probably means extending the __rw_setlocale
> interface to release the lock without restoring the original
> locale name.
I'll whip up a patch to src/setlocale.[h/cpp] and post it for
review before submitting the next change.
Brad.
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Travis Vitek <Tr...@roguewave.com>.
Eric Lemings wrote:
>
>
>file src/setlocale.cpp:
> 83 // acquire global per-process lock
> 84 __rw_setlocale_mutex._C_acquire ();
> 85
> 86 // retrieve previous locale name and check if it is already set
> 87 const char* const curname = ::setlocale (cat, 0);
>
>The mutex doesn't need to be locked just to read the current
>locale name, does it?
>
According to the documentation on Solaris, it does not...
Using setlocale() to query the current locale is safe
and can be used anywhere in a multithreaded application.
I haven't seen any documentation on any of the other platforms to
indicate that it is safe, I'd say it is safest to always lock.
Travis
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Eric Lemings <Er...@roguewave.com>.
file src/setlocale.cpp:
83 // acquire global per-process lock
84 __rw_setlocale_mutex._C_acquire ();
85
86 // retrieve previous locale name and check if it is already set
87 const char* const curname = ::setlocale (cat, 0);
The mutex doesn't need to be locked just to read the current
locale name, does it?
Brad.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Thursday, April 03, 2008 11:06 AM
> To: dev@stdcxx.apache.org
> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
> Travis Vitek wrote:
> >
> >
> >> -----Original Message-----
> >> From: Martin Sebor [mailto:sebor@roguewave.com]
> >> Sent: Thursday, April 03, 2008 8:55 AM
> >> To: dev@stdcxx.apache.org
> >> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
> >> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
> >>
> >> elemings@apache.org wrote:
> >>> Author: elemings
> >>> Date: Thu Apr 3 08:32:56 2008
> >>> New Revision: 644364
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
> >>> Log:
> >>> 2008-04-03 Eric Lemings <er...@roguewave.com>
> >>>
> >>> STDCXX-811
> >>> * src/locale_global.cpp (std::locale::global): Replace call to
> >>> non-reentrant setlocale() C function with reentrant
> >>> __rw_setlocale object.
> >> I don't think that's correct. The object's dtor restores
> >> the original locale. We need a mutex around the call to
> >> setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
> >> _RWSTD_CLASS_GUARD() macros.
> >>
> >
> > Right. It seems that for this to be mt-safe with respect to other
> > library code that calls setlocale(), we would need to lock
> the same lock
> > that is used by _RW::__rw_setlocale. If don't use the same
> lock it would
> > be possible for the std::locale::global() call to change the locale
> > while some other library code is reading locale specific data under
> > protection of an active _RW::__rw_setlocale instance.
>
> Good point. That probably means extending the __rw_setlocale
> interface to release the lock without restoring the original
> locale name.
>
> >
> > Of course the _RW::__rw_setlocale objects will always be
> able to call
> > setlocale() and mess up the state of anything expecting
> locale specific
> > behavior to work correctly. i.e. std::locale::global() is
> required to
> > call setlocale(), but the library may need to call setlocale()
> > internally [through a _RW::__rw_setlocale instance]. When
> this happens,
> > the global locale state [the C/C++ locale set by the setlocale()
> > function] will be temporarily overridden behind the users
> back, possibly
> > resulting in some interesting behavior.
>
> Right. There's no avoiding it. The best we can do (and what
> the test tries to exercise) is that locale::global() doesn't
> crash when called simultaneously from multiple threads.
>
> Martin
>
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
>
>> -----Original Message-----
>> From: Martin Sebor [mailto:sebor@roguewave.com]
>> Sent: Thursday, April 03, 2008 8:55 AM
>> To: dev@stdcxx.apache.org
>> Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
>> src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>>
>> elemings@apache.org wrote:
>>> Author: elemings
>>> Date: Thu Apr 3 08:32:56 2008
>>> New Revision: 644364
>>>
>>> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
>>> Log:
>>> 2008-04-03 Eric Lemings <er...@roguewave.com>
>>>
>>> STDCXX-811
>>> * src/locale_global.cpp (std::locale::global): Replace call to
>>> non-reentrant setlocale() C function with reentrant
>>> __rw_setlocale object.
>> I don't think that's correct. The object's dtor restores
>> the original locale. We need a mutex around the call to
>> setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
>> _RWSTD_CLASS_GUARD() macros.
>>
>
> Right. It seems that for this to be mt-safe with respect to other
> library code that calls setlocale(), we would need to lock the same lock
> that is used by _RW::__rw_setlocale. If don't use the same lock it would
> be possible for the std::locale::global() call to change the locale
> while some other library code is reading locale specific data under
> protection of an active _RW::__rw_setlocale instance.
Good point. That probably means extending the __rw_setlocale
interface to release the lock without restoring the original
locale name.
>
> Of course the _RW::__rw_setlocale objects will always be able to call
> setlocale() and mess up the state of anything expecting locale specific
> behavior to work correctly. i.e. std::locale::global() is required to
> call setlocale(), but the library may need to call setlocale()
> internally [through a _RW::__rw_setlocale instance]. When this happens,
> the global locale state [the C/C++ locale set by the setlocale()
> function] will be temporarily overridden behind the users back, possibly
> resulting in some interesting behavior.
Right. There's no avoiding it. The best we can do (and what
the test tries to exercise) is that locale::global() doesn't
crash when called simultaneously from multiple threads.
Martin
RE: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Posted by Travis Vitek <Tr...@roguewave.com>.
>-----Original Message-----
>From: Martin Sebor [mailto:sebor@roguewave.com]
>Sent: Thursday, April 03, 2008 8:55 AM
>To: dev@stdcxx.apache.org
>Subject: Re: svn commit: r644364 - in /stdcxx/trunk:
>src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
>
>elemings@apache.org wrote:
>> Author: elemings
>> Date: Thu Apr 3 08:32:56 2008
>> New Revision: 644364
>>
>> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
>> Log:
>> 2008-04-03 Eric Lemings <er...@roguewave.com>
>>
>> STDCXX-811
>> * src/locale_global.cpp (std::locale::global): Replace call to
>> non-reentrant setlocale() C function with reentrant
>> __rw_setlocale object.
>
>I don't think that's correct. The object's dtor restores
>the original locale. We need a mutex around the call to
>setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
>_RWSTD_CLASS_GUARD() macros.
>
Right. It seems that for this to be mt-safe with respect to other
library code that calls setlocale(), we would need to lock the same lock
that is used by _RW::__rw_setlocale. If don't use the same lock it would
be possible for the std::locale::global() call to change the locale
while some other library code is reading locale specific data under
protection of an active _RW::__rw_setlocale instance.
Of course the _RW::__rw_setlocale objects will always be able to call
setlocale() and mess up the state of anything expecting locale specific
behavior to work correctly. i.e. std::locale::global() is required to
call setlocale(), but the library may need to call setlocale()
internally [through a _RW::__rw_setlocale instance]. When this happens,
the global locale state [the C/C++ locale set by the setlocale()
function] will be temporarily overridden behind the users back, possibly
resulting in some interesting behavior.
Travis
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp
tests/localization/22.locale.statics.mt.cpp
Posted by Martin Sebor <se...@roguewave.com>.
elemings@apache.org wrote:
> Author: elemings
> Date: Thu Apr 3 08:32:56 2008
> New Revision: 644364
>
> URL: http://svn.apache.org/viewvc?rev=644364&view=rev
> Log:
> 2008-04-03 Eric Lemings <er...@roguewave.com>
>
> STDCXX-811
> * src/locale_global.cpp (std::locale::global): Replace call to
> non-reentrant setlocale() C function with reentrant
> __rw_setlocale object.
I don't think that's correct. The object's dtor restores
the original locale. We need a mutex around the call to
setlocale. Look for/at the _RWSTD_STATIC_GUARD() and
_RWSTD_CLASS_GUARD() macros.
Martin
> * tests/localization/22.locale.statics.mt.cpp (test_global):
> Fix [un]signed integer conversion warnings.
>
>
> Modified:
> stdcxx/trunk/src/locale_global.cpp
> stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp
>
> Modified: stdcxx/trunk/src/locale_global.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/trunk/src/locale_global.cpp?rev=644364&r1=644363&r2=644364&view=diff
> ==============================================================================
> --- stdcxx/trunk/src/locale_global.cpp (original)
> +++ stdcxx/trunk/src/locale_global.cpp Thu Apr 3 08:32:56 2008
> @@ -37,6 +37,7 @@
> #include <loc/_locale.h>
>
> #include "locale_body.h"
> +#include "setlocale.h"
>
>
>
> @@ -53,7 +54,8 @@
>
> // assumes all locale names (i.e., including those of combined
> // locales) are in a format understandable by setlocale()
> - setlocale (_RWSTD_LC_ALL, rhs.name ().c_str ());
> + const _RW::__rw_setlocale clocale (rhs.name().c_str(), _RWSTD_LC_ALL);
> + _RWSTD_UNUSED(clocale);
>
> // FIXME:
> // handle unnamed locales (i.e., locales containing non-standard
>
> Modified: stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp
> URL: http://svn.apache.org/viewvc/stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp?rev=644364&r1=644363&r2=644364&view=diff
> ==============================================================================
> --- stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp (original)
> +++ stdcxx/trunk/tests/localization/22.locale.statics.mt.cpp Thu Apr 3 08:32:56 2008
> @@ -88,9 +88,9 @@
> static void*
> test_global (void*)
> {
> - for (int i = 0; i != opt_nloops; ++i) {
> + for (std::size_t i = 0; i != opt_nloops; ++i) {
>
> - const int inx = i % nlocales;
> + const std::size_t inx = i % nlocales;
>
> const std::locale last (std::locale::global (locales [inx]));
>
>
>