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:[EMAIL PROTECTED] 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 [EMAIL PROTECTED] wrote: Author: elemings Date: Thu Apr 3 08:32:56 2008 New Revision: 644364 URL: http://svn.apache.org/viewvc?rev=644364view=rev Log: 2008-04-03 Eric Lemings [EMAIL PROTECTED] 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
-Original Message- From: Martin Sebor [mailto:[EMAIL PROTECTED] 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:[EMAIL PROTECTED] 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 [EMAIL PROTECTED] wrote: Author: elemings Date: Thu Apr 3 08:32:56 2008 New Revision: 644364 URL: http://svn.apache.org/viewvc?rev=644364view=rev Log: 2008-04-03 Eric Lemings [EMAIL PROTECTED] 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
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
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\libdumpbin /exports libstd12d.dll | find setlocale 123 7A 000114C0 [EMAIL PROTECTED]@@[EMAIL PROTECTED]@Z = [EMAIL PROTECTED]@@[EMAIL PROTECTED]@Z (public: __thiscall __rw::__rw_setlocale::__rw_setlocale(char const *,int,int)) 245 F4 00011610 [EMAIL PROTECTED]@@[EMAIL PROTECTED] = [EMAIL PROTECTED]@@[EMAIL PROTECTED] (public: __thiscall __rw::__rw_setlocale::~__rw_setlocale(void)) 523 20A 2C20 [EMAIL PROTECTED]@@QAEXXZ = [EMAIL PROTECTED]@@QAEXXZ (public: void __thiscall __rw::__rw_setlocale::`default constructor closure'(void)) 1693 69C 2C10 [EMAIL PROTECTED]@__rw@@QBEPBDXZ = [EMAIL PROTECTED]@__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
-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:[EMAIL PROTECTED]
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:[EMAIL PROTECTED]
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
[EMAIL PROTECTED] wrote:
Author: elemings
Date: Thu Apr 3 08:32:56 2008
New Revision: 644364
URL: http://svn.apache.org/viewvc?rev=644364view=rev
Log:
2008-04-03 Eric Lemings [EMAIL PROTECTED]
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
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.
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
-Original Message- From: Travis Vitek [mailto:[EMAIL PROTECTED] 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
-Original Message- From: Martin Sebor [mailto:[EMAIL PROTECTED] 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.
