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=644364&view=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.
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
#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
>-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=644364&view=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. 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
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=644364&view=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=644364&view=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
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:[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=644364&view=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
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 [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
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 [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=644364&view=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
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.
-
$ 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
>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.
[PATCH] STDCXX-423 LIMITS.cpp assumes integers with no padding bits
Changes made:
* fix a bug (not filed) that would potentially access an array using
negative index
* add function compute_type_bits to calculate the number of bits in the
value representation of type T.
* add a check that would only define exact width integer type if the
system supports two's complement
* add a check to ensure that the number of bits in the value
representation and the number of bits in the object representation are
the same before defining exact integer width types.
Index: LIMITS.cpp
===
--- LIMITS.cpp (revision 639462)
+++ LIMITS.cpp (working copy)
@@ -80,7 +80,7 @@
'0','1','2','3','4','5','6','7','8','9', 'a', 'b', 'c', 'd',
'e', 'f'
};
-if (is_max || n >= zero) {
+if (n >= zero) {
T tnstr = n;
@@ -224,6 +224,19 @@
}
+template
+unsigned compute_type_bits()
+{
+T max = T (one);
+T current = T(one);
+int bits = 1;
+
+for (; T (current * 2) > max; current *=2, max *= 2, bits++) { }
+
+return bits;
+}
+
+
// used to compute the size of a pointer to a member function
struct EmptyStruct { };
@@ -397,6 +410,12 @@
// 1 for a 16-bit integer, etc)
int width_bits = 0;
+// store exact bit size of each type
+int ushort_bits = compute_type_bits ();
+int uint_bits = compute_type_bits ();
+int ulong_bits = compute_type_bits ();
+int ullong_bits = compute_type_bits ();
+
#define PRINT_SPECIFIC(width, least, type)
\
do {
\
/* avoid warnings about expression being constant */
\
@@ -406,7 +425,7 @@
"#define _RWSTD_UINT_LEAST%d_T %s _RWSTD_U%s_T\n",
\
width, width < 10 ? " " : "", type,
\
width, width < 10 ? " " : "", type);
\
-else
\
+else if (!no_twos_complement)
\
printf ("#define _RWSTD_INT%d_T %s signed %s\n"
\
"#define _RWSTD_UINT%d_T %s unsigned %s\n",
\
width, width < 10 ? " " : "", type,
\
@@ -430,59 +449,59 @@
PRINT_SPECIFIC (64, "", "char");
}
-if (16 == char_bits * sizeof (short) && !(width_bits & 2)) {
+if (16 == char_bits * sizeof (short) && 16 == ushort_bits &&
!(width_bits & 2)) {
width_bits |= 2;
PRINT_SPECIFIC (16, "", "short");
}
-else if (32 == char_bits * sizeof (short) && !(width_bits & 4)) {
+else if (32 == char_bits * sizeof (short) && 32 == ushort_bits &&
!(width_bits & 4)) {
width_bits |= 4;
PRINT_SPECIFIC (32, "", "short");
}
-else if (64 == char_bits * sizeof (short) && !(width_bits & 8)) {
+else if (64 == char_bits * sizeof (short) && 64 == ushort_bits &&
!(width_bits & 8)) {
width_bits |= 8;
PRINT_SPECIFIC (64, "", "short");
}
-else if (128 == char_bits * sizeof (short) && !(width_bits & 16)) {
+else if (128 == char_bits * sizeof (short) && 128 == ushort_bits &&
!(width_bits & 16)) {
width_bits |= 16;
PRINT_SPECIFIC (128, "", "short");
}
-if (32 == char_bits * sizeof (int) && !(width_bits & 4)) {
+if (32 == char_bits * sizeof (int) && 32 == uint_bits &&
!(width_bits & 4)) {
width_bits |= 4;
PRINT_SPECIFIC (32, "", "int");
}
-else if (64 == char_bits * sizeof (int) && !(width_bits & 8)) {
+else if (64 == char_bits * sizeof (int) && 64 == uint_bits &&
!(width_bits & 8)) {
width_bits |= 8;
PRINT_SPECIFIC (64, "", "int");
}
-else if (128 == char_bits * sizeof (int) && !(width_bits & 16)) {
+else if (128 == char_bits * sizeof (int) && 128 == uint_bits &&
!(width_bits & 16)) {
width_bits |= 16;
PRINT_SPECIFIC (128, "", "int");
}
-if (32 == char_bits * sizeof (long) && !(width_bits & 4)) {
+if (32 == char_bits * sizeof (long) && 32 == ulong_bits &&
!(width_bits & 4)) {
width_bits |= 4;
PRINT_SPECIFIC (32, "", "long");
}
-else if (64 == char_bits * sizeof (long) && !(width_bits & 8)) {
+else if (64 == char_bits * sizeof (long) && 64 == ulong_bits &&
!(width_bits & 8)) {
width_bits |= 8;
PRINT_SPECIFIC (64, "", "long");
}
-else if (128 == char_bits * sizeof (long) && !(width_bits & 16)) {
+else if (128 == char_bits * sizeof (long) && 128 == ulong_bits &&
!(width_bits & 16)) {
width_bits |= 16;
PRINT_SPECIFIC (128, "", "long");
}
-if (32 == char_bits * sizeof (LLong) && !(width_bits & 4)) {
+if (32 == char_bits * sizeof (LLong) && 32 == ullong_bits &&
!(width_bits & 4)) {
width_bits |= 4;
PRINT_SPECIFIC (32, "", llong_name);
}
-else if (64 == char_bits * sizeof (LLong) && !(width_bits & 8)) {
+else if (64 == char_bits * sizeof (LLong) && 64 == ullong_bits &&
!(width_bits & 8)) {
width_bits |= 8;
PRINT_SPECIFIC (64, "", llong_name);
}
-else if (128 == char_bits * sizeo
RE: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp
Travis,
If you could, give the following patch a whirl (or quick review
at least).
-
$ svn diff
Index: tests/src/new.cpp
===
--- tests/src/new.cpp (revision 64)
+++ tests/src/new.cpp (working copy)
@@ -604,6 +604,17 @@
return ret;
}
+static void
+rwt_checkpoint_compare (_RWSTD_SIZE_T* diffs, _RWSTD_SIZE_T n,
+const _RWSTD_SIZE_T* st0, const _RWSTD_SIZE_T*
st1,
+bool& diff_0)
+{
+for (_RWSTD_SIZE_T i = 0; i != n; ++i) {
+diffs [i] = st1 [i] - st0 [i];
+if (diffs [i])
+diff_0 = false;
+}
+}
_TEST_EXPORT rwt_free_store*
rwt_checkpoint (const rwt_free_store *st0, const rwt_free_store *st1)
@@ -616,21 +627,24 @@
// of the free_store specified by the arguments
static rwt_free_store diff;
-
memset (&diff, 0, sizeof diff);
-size_t* diffs= diff.new_calls_;
-const size_t* st0_args = st0->new_calls_;
-const size_t* st1_args = st1->new_calls_;
-
bool diff_0 = true; // difference of 0 (i.e., none)
-for (size_t i = 0; i != 16; ++i) {
-diffs [i] = st1_args [i] - st0_args [i];
-if (diffs [i])
-diff_0 = false;
-}
+#define EXTENT(array) (sizeof (array) / sizeof(*array))
+#define COMPARE(member) \
+rwt_checkpoint_compare (diff.member, EXTENT(diff.member),\
+st0->member, st1->member, diff_0)
+COMPARE(new_calls_);
+COMPARE(delete_calls_);
+COMPARE(delete_0_calls_);
+COMPARE(blocks_);
+COMPARE(bytes_);
+COMPARE(max_blocks_);
+COMPARE(max_bytes_);
+COMPARE(max_block_size_);
+
if (diff_0)
return 0;
-
The 0.new test seems to be okay with it.
Thanks,
Brad.
> -Original Message-
> From: Travis Vitek (JIRA) [mailto:[EMAIL PROTECTED]
> Sent: Thursday, April 03, 2008 3:23 PM
> To: Eric Lemings
> Subject: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of
> bound access in new.cpp
>
>
> [
> https://issues.apache.org/jira/browse/STDCXX-761?page=com.atla
> ssian.jira.plugin.system.issuetabpanels:comment-tabpanel&focus
> edCommentId=12585290#action_12585290 ]
>
> Travis Vitek commented on STDCXX-761:
> -
>
> It looks like someone is being tricky. They are assuming that
> every member in {{rwt_free_store}} is contiguous [i.e. no
> padding], and they are actually comparing each of
> {{new_calls_}}, {{delete_calls_}}, {{blocks_}}, {{bytes_}},
> {{max_blocks_}}, {{max_bytes_}} and {{max_block_size_}} with
> that one loop. If you decide to fix this, you'll end up
> probably end up writing eight loops of two iterations each.
>
> Also, I'd prefer that the 2 isn't hardcoded and that you use
> something like {{sizeof (array) / sizeof (*array)}}, or a
> size macro instead.
>
> > [HP aCC 6.16] Out of bound access in new.cpp
> >
> >
> > Key: STDCXX-761
> > URL:
> https://issues.apache.org/jira/browse/STDCXX-761
> > Project: C++ Standard Library
> > Issue Type: Sub-task
> > Components: Test Driver
> >Affects Versions: 4.2.0
> > Environment: $ uname -sr && aCC -V
> > HP-UX B.11.31
> > aCC: HP C/aC++ B3910B A.06.16 [Nov 26 2007]
> >Reporter: Scott (Yu) Zhong
> >Assignee: Eric Lemings
> > Fix For: 4.2.1
> >
> > Original Estimate: 2h
> > Remaining Estimate: 2h
> >
> > When compiled with HP aCC 6.16 +w +O, the test driver
> source file
> [new.cpp|http://svn.apache.org/repos/asf/stdcxx/trunk/tests/sr
> c/new.cpp] produces the warnings below:
> > {noformat}
> > aCC -c -I$(TOPDIR)/include -I$(BUILDDIR)/include
> -I$(TOPDIR)/tests/include -AA +O2 +DD64 +w \
> > +W392 +W655 +W684 +W818 +W819 +W849 +W2193 +W2236
> +W2261 +W2340 +W2401 +W2487 +W4227 \
> > +W4229 +W4231 +W4235 +W4237 +W4249 +W4255 +W4272 +W4284
> +W4285 +W4286 +W4296 +W4297 +W3348 \
> > $(TOPDIR)/tests/src/new.cpp
> > "$(TOPDIR)/tests/src/new.cpp", line 629, procedure
> rwt_checkpoint: warning #20206-D: Out of bound access (In
> expression "(unsigned long long*)(&diff)->new_calls_+i",
> (&diff)->new_calls_ (type: unsigned long long [2]) has byte
> range [0 .. 15], writing byte range [0 .. 127].)
> > {noformat}
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
Re: svn commit: r644364 - in /stdcxx/trunk: src/locale_global.cpp tests/localization/22.locale.statics.mt.cpp
Eric Lemings wrote: -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)' ? 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: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp
Eric Lemings wrote:
Travis,
If you could, give the following patch a whirl (or quick review
at least).
Is there an easier way to silence the warning than by adding
all these loops? E.g., by assigning the address of the first
member to a local pointer? (Or is the compiler too smart for
that?)
Also, there is no need to use the _RWSTD_SIZE_T macro in .cpp
files. The macro is useful in library and test suite headers
to reduce namespace pollution (so we don't have to #include
.
Last thing: the code formatting convention calls for a space
before each open parenthesis. It might take some getting used
to but once you do, you'll never go back -- just ask Travis ;-)
Martin
-
$ svn diff
Index: tests/src/new.cpp
===
--- tests/src/new.cpp (revision 64)
+++ tests/src/new.cpp (working copy)
@@ -604,6 +604,17 @@
return ret;
}
+static void
+rwt_checkpoint_compare (_RWSTD_SIZE_T* diffs, _RWSTD_SIZE_T n,
+const _RWSTD_SIZE_T* st0, const _RWSTD_SIZE_T*
st1,
+bool& diff_0)
+{
+for (_RWSTD_SIZE_T i = 0; i != n; ++i) {
+diffs [i] = st1 [i] - st0 [i];
+if (diffs [i])
+diff_0 = false;
+}
+}
_TEST_EXPORT rwt_free_store*
rwt_checkpoint (const rwt_free_store *st0, const rwt_free_store *st1)
@@ -616,21 +627,24 @@
// of the free_store specified by the arguments
static rwt_free_store diff;
-
memset (&diff, 0, sizeof diff);
-size_t* diffs= diff.new_calls_;
-const size_t* st0_args = st0->new_calls_;
-const size_t* st1_args = st1->new_calls_;
-
bool diff_0 = true; // difference of 0 (i.e., none)
-for (size_t i = 0; i != 16; ++i) {
-diffs [i] = st1_args [i] - st0_args [i];
-if (diffs [i])
-diff_0 = false;
-}
+#define EXTENT(array) (sizeof (array) / sizeof(*array))
+#define COMPARE(member) \
+rwt_checkpoint_compare (diff.member, EXTENT(diff.member),\
+st0->member, st1->member, diff_0)
+COMPARE(new_calls_);
+COMPARE(delete_calls_);
+COMPARE(delete_0_calls_);
+COMPARE(blocks_);
+COMPARE(bytes_);
+COMPARE(max_blocks_);
+COMPARE(max_bytes_);
+COMPARE(max_block_size_);
+
if (diff_0)
return 0;
-
The 0.new test seems to be okay with it.
Thanks,
Brad.
-Original Message-
From: Travis Vitek (JIRA) [mailto:[EMAIL PROTECTED]
Sent: Thursday, April 03, 2008 3:23 PM
To: Eric Lemings
Subject: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of
bound access in new.cpp
[
https://issues.apache.org/jira/browse/STDCXX-761?page=com.atla
ssian.jira.plugin.system.issuetabpanels:comment-tabpanel&focus
edCommentId=12585290#action_12585290 ]
Travis Vitek commented on STDCXX-761:
-
It looks like someone is being tricky. They are assuming that
every member in {{rwt_free_store}} is contiguous [i.e. no
padding], and they are actually comparing each of
{{new_calls_}}, {{delete_calls_}}, {{blocks_}}, {{bytes_}},
{{max_blocks_}}, {{max_bytes_}} and {{max_block_size_}} with
that one loop. If you decide to fix this, you'll end up
probably end up writing eight loops of two iterations each.
Also, I'd prefer that the 2 isn't hardcoded and that you use
something like {{sizeof (array) / sizeof (*array)}}, or a
size macro instead.
[HP aCC 6.16] Out of bound access in new.cpp
Key: STDCXX-761
URL:
https://issues.apache.org/jira/browse/STDCXX-761
Project: C++ Standard Library
Issue Type: Sub-task
Components: Test Driver
Affects Versions: 4.2.0
Environment: $ uname -sr && aCC -V
HP-UX B.11.31
aCC: HP C/aC++ B3910B A.06.16 [Nov 26 2007]
Reporter: Scott (Yu) Zhong
Assignee: Eric Lemings
Fix For: 4.2.1
Original Estimate: 2h
Remaining Estimate: 2h
When compiled with HP aCC 6.16 +w +O, the test driver
source file
[new.cpp|http://svn.apache.org/repos/asf/stdcxx/trunk/tests/sr
c/new.cpp] produces the warnings below:
{noformat}
aCC -c -I$(TOPDIR)/include -I$(BUILDDIR)/include
-I$(TOPDIR)/tests/include -AA +O2 +DD64 +w \
+W392 +W655 +W684 +W818 +W819 +W849 +W2193 +W2236
+W2261 +W2340 +W2401 +W2487 +W4227 \
+W4229 +W4231 +W4235 +W4237 +W4249 +W4255 +W4272 +W4284
+W4285 +W4286 +W4296 +W4297 +W3348 \
$(TOPDIR)/tests/src/new.cpp
"$(TOPDIR)/tests/src/new.cpp", line 629, procedure
rwt_checkpoint: warning #20206-D: Out of bound access (In
expression "(unsigned long long*)(&diff)->new_calls_+i",
(&diff)->new_calls_ (type: unsigned long long [2]) has byte
range [0 .. 15], writing byte range [0 .. 127].)
{noformat}
--
This message is automatically generated by JIRA.
-
You can reply to this email to ad
Re: spaces in rw_xxx_expand()
Travis Vitek wrote: Martin Sebor wrote: PING? Should I open an issue for this or is it something you're already working on or planning to? Well if we want 100% compatibility with the shell, then it should be implemented. I don't expect rw_shell_expand() to necessarily behave exactly the same way as the shell in every respect but I do think the function should interpret escapes and quotes the same the shell does. In my experience, quoting spaces is more common than escaping them is so must users would find it surprising if quoting didn't work. I took a look, and I think I can fix it by modifying _rw_brace_graph::brace_expand_write() to treat quoted blocks literally and failing if an unescaped quote isn't matched. That will break rw_brace_expand(), but that isn't too big of a deal as we've discussed removing that function several times. Would you please open an issue for this? I would have done this myself, but I'm not sure how high a priority this is for you. I'm assuming that this is not to serious because you should be able to work around it by using escapes. Okay, it's STDCXX-825 http://issues.apache.org/jira/browse/STDCXX-825 I don't think it's a pressing enhancement but it would be nice to get it implemented at some point before we start using the function in our test suite. Martin
Re: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp
Travis Vitek wrote:
Martin Sebor wrote:
Eric Lemings wrote:
Travis,
If you could, give the following patch a whirl (or quick review
at least).
Is there an easier way to silence the warning than by adding
all these loops? E.g., by assigning the address of the first
member to a local pointer? (Or is the compiler too smart for
that?)
That is exactly what the previous code did...
size_t* diffs= diff.new_calls_;
const size_t* st0_args = st0->new_calls_;
const size_t* st1_args = st1->new_calls_;
The compiler complains because the member new_calls_ is an array of
length 2, but we iterate over 16 elements. Provided that there is no
padding between each of the array fields, this would work and we'd just
have to disable the warning.
Wouldn't that be the way to go then? I.e., wrap the loop
in a couple of #pragma diag_suppress/diag_default?
Btw., C99 added this _Pragma() operator. It can be used
just like #pragma except it's not a directive so it can
be the result of preprocessor expansion. aCC seems to
grok it, so instead of adding a pair of
#if 6 <= __HP_aCC
# pragma diag_suppress 20206
#endif
and
#if 6 <= __HP_aCC
# pragma diag_default
#endif
around tricky code all over the place we could have
#if 6 <= __HP_aCC
# define _RWSTD_aCC_PRAGMA(text) _Pragma (text)
#else
# define _RWSTD_aCC_PRAGMA(ignore) (void)0
#endif
and simplify the tricky code guards to:
_RWSTD_aCC_PRAGMA ("diag_suppress 20206")
/* tricky code */
_RWSTD_aCC_PRAGMA ("diag_default")
Martin
I don't really like the macro, probably because it uses local variables
without taking them as parameters. Another option would be to use offset
pointer to member and a nested loop.
typedef size_t (rwt_free_store::* checkpoint_field)[2];
static const checkpoint_field fields [] =
{
&rwt_free_store::new_calls_,
&rwt_free_store::delete_calls_,
&rwt_free_store::delete_0_calls_,
&rwt_free_store::blocks_,
&rwt_free_store::bytes_,
&rwt_free_store::max_blocks_,
&rwt_free_store::max_bytes_,
&rwt_free_store::max_block_size_
};
static rwt_free_store diff;
bool diff_0 = true;
for (size_t f = 0; f < sizeof (fields) / sizeof (*fields); ++f)
{
size_t* diffs= (diff.*fields [f]);
const size_t* st0_args = (st1->*fields [f]);
const size_t* st1_args = (st0->*fields [f]);
for (size_t i = 0; i < 2; ++i)
{
diffs [i] = st1_args [i] - st0_args [i];
if (diffs [i])
diff_0 = false;
}
}
Also, there is no need to use the _RWSTD_SIZE_T macro in .cpp
files. The macro is useful in library and test suite headers
to reduce namespace pollution (so we don't have to #include
.
I was going to mention that.
Last thing: the code formatting convention calls for a space
before each open parenthesis. It might take some getting used
to but once you do, you'll never go back -- just ask Travis ;-)
Uh, yeah.
Travis
RE: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp
>Martin Sebor wrote:
>
>Eric Lemings wrote:
>>
>> Travis,
>>
>> If you could, give the following patch a whirl (or quick review
>> at least).
>
>Is there an easier way to silence the warning than by adding
>all these loops? E.g., by assigning the address of the first
>member to a local pointer? (Or is the compiler too smart for
>that?)
That is exactly what the previous code did...
size_t* diffs= diff.new_calls_;
const size_t* st0_args = st0->new_calls_;
const size_t* st1_args = st1->new_calls_;
The compiler complains because the member new_calls_ is an array of
length 2, but we iterate over 16 elements. Provided that there is no
padding between each of the array fields, this would work and we'd just
have to disable the warning.
I don't really like the macro, probably because it uses local variables
without taking them as parameters. Another option would be to use offset
pointer to member and a nested loop.
typedef size_t (rwt_free_store::* checkpoint_field)[2];
static const checkpoint_field fields [] =
{
&rwt_free_store::new_calls_,
&rwt_free_store::delete_calls_,
&rwt_free_store::delete_0_calls_,
&rwt_free_store::blocks_,
&rwt_free_store::bytes_,
&rwt_free_store::max_blocks_,
&rwt_free_store::max_bytes_,
&rwt_free_store::max_block_size_
};
static rwt_free_store diff;
bool diff_0 = true;
for (size_t f = 0; f < sizeof (fields) / sizeof (*fields); ++f)
{
size_t* diffs= (diff.*fields [f]);
const size_t* st0_args = (st1->*fields [f]);
const size_t* st1_args = (st0->*fields [f]);
for (size_t i = 0; i < 2; ++i)
{
diffs [i] = st1_args [i] - st0_args [i];
if (diffs [i])
diff_0 = false;
}
}
>Also, there is no need to use the _RWSTD_SIZE_T macro in .cpp
>files. The macro is useful in library and test suite headers
>to reduce namespace pollution (so we don't have to #include
>.
I was going to mention that.
>
>Last thing: the code formatting convention calls for a space
>before each open parenthesis. It might take some getting used
>to but once you do, you'll never go back -- just ask Travis ;-)
>
Uh, yeah.
Travis
Re: [jira] Commented: (STDCXX-761) [HP aCC 6.16] Out of bound access in new.cpp
Martin Sebor wrote:
Travis Vitek wrote:
Martin Sebor wrote:
Eric Lemings wrote:
Travis,
If you could, give the following patch a whirl (or quick review
at least).
Is there an easier way to silence the warning than by adding
all these loops? E.g., by assigning the address of the first
member to a local pointer? (Or is the compiler too smart for
that?)
That is exactly what the previous code did...
size_t* diffs= diff.new_calls_;
const size_t* st0_args = st0->new_calls_;
const size_t* st1_args = st1->new_calls_;
The compiler complains because the member new_calls_ is an array of
length 2, but we iterate over 16 elements. Provided that there is no
padding between each of the array fields, this would work and we'd just
have to disable the warning.
Wouldn't that be the way to go then? I.e., wrap the loop
in a couple of #pragma diag_suppress/diag_default?
On second thought, I have to agree that the code is tricky and brittle.
If someone were to change the order of the rwt_free_store members or
add a new one in between the loop would silently break. So maybe the
right solution is as Brad proposed...
Btw., C99 added this _Pragma() operator.
[...]
I don't really like the macro, probably because it uses local variables
without taking them as parameters. Another option would be to use offset
pointer to member and a nested loop.
typedef size_t (rwt_free_store::* checkpoint_field)[2];
static const checkpoint_field fields [] =
{
...or this, except, of course, for the braces which in local/class
scope go on the line above ;-)
You guys decide, either way is fine with me.
Btw., if you go with Brad's approach, there's no need for EXTENT().
We already have an RW_COUNTOF() macro in .
Oh, and another last thing: new.cpp uses a naming convention that's
inconsistent with the rest of the driver where non-member functions
with internal linkage (i.e., namespace-scope statics) start with
the _rw_ prefix, and extern functions with rw_. At some point we
need to change the externs but any new statics should use the
_rw_ convention.
Martin
&rwt_free_store::new_calls_,
&rwt_free_store::delete_calls_,
&rwt_free_store::delete_0_calls_,
&rwt_free_store::blocks_,
&rwt_free_store::bytes_,
&rwt_free_store::max_blocks_,
&rwt_free_store::max_bytes_,
&rwt_free_store::max_block_size_
};
static rwt_free_store diff;
bool diff_0 = true;
for (size_t f = 0; f < sizeof (fields) / sizeof (*fields); ++f)
{
size_t* diffs= (diff.*fields [f]);
const size_t* st0_args = (st1->*fields [f]);
const size_t* st1_args = (st0->*fields [f]);
for (size_t i = 0; i < 2; ++i)
{
diffs [i] = st1_args [i] - st0_args [i];
if (diffs [i])
diff_0 = false;
}
}
Also, there is no need to use the _RWSTD_SIZE_T macro in .cpp
files. The macro is useful in library and test suite headers
to reduce namespace pollution (so we don't have to #include
.
I was going to mention that.
Last thing: the code formatting convention calls for a space
before each open parenthesis. It might take some getting used
to but once you do, you'll never go back -- just ask Travis ;-)
Uh, yeah.
Travis
