stdcxx-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stefan Teleman (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (STDCXX-1056) std::moneypunct and std::numpunct implementations are not thread-safe
Date Sun, 23 Sep 2012 18:43:07 GMT

    [ https://issues.apache.org/jira/browse/STDCXX-1056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13461490#comment-13461490
] 

Stefan Teleman commented on STDCXX-1056:
----------------------------------------

The thing I *don't* like about my patch set at all is that it doesn't really and completely
eliminate the cache resize problem, which I strongly suspect is the root cause of the race
conditions, although I do not have a bull's eye proof for it yet. I have plenty of circumstantial
evidence. ;-)

Regardless of the initial cache size we start with, it is still possible that the cache will
have to be resized. Right now, the fix relies on the fact that it is *unlikely* that a program
will ever need more than 32 locales, and the cache never needs to be resized.

The reason the perfect forwarding patch appears to solve the problem is: the instantiation
of the std::string returned-by-value by the facet member functions is now different: the do_*()
functions return a (cast-to) _CharT* pointer. Because of this, the std::string returned by
value is now constructed as a deep copy (see the string::string(const _CharT*) 21.3.1/P9 constructor),
which effectively gives ownership of this string to the caller - this constructor calls traits_type::copy
(_C_data, __s, __n);.

For one, the timing difference caused by having to copy the bits (in the constructor) is enough
to alter the run-time behavior. Second, by owning a deep copy of the returned string, the
caller will still have the bits, although the facet object itself may have long disappeared.

Latest files, diffs and test results are available here:

 http://s247136804.onlinehome.us/stdcxx-1056-20120922/

In this latest version I also fixed a bug I had in my original patchset.

The diffs are next-to-impossible to read. It is much easier to just read the complete files.

The changes I made are in:

locale_body.cpp:

__rw_locale* __rw_locale::_C_manage ( ... );

facet.cpp:

__rw_facet* __rw_facet::_C_manage ( ... );

punct.cpp:

static const void* __rw_get_numpunct ( ... );
static const void* __rw_get_moneypunct ( ... );

In punct.cpp there are two new static inline functions:

static inline const void* __rw_numpunct_switch ( ... );
static inline const void* __rw_moneypunct_switch ( ... );

These two functions are used in replacing the recursive calls present in the original implementation
of __rw_get_numpunct() and __rw_get_moneypunct(). Their names are horrible. 

Recursion is extremely expensive on SPARC (and I strongly suspect on other RISC machines as
well).

I will provide performance data next week. To produce relevant performance data, I need to
do optimized builds. Instrumentation requires non-optimized debug builds, and the instrumentation
part is what I've been focusing on for the past week.








 
                
> std::moneypunct and std::numpunct implementations are not thread-safe
> ---------------------------------------------------------------------
>
>                 Key: STDCXX-1056
>                 URL: https://issues.apache.org/jira/browse/STDCXX-1056
>             Project: C++ Standard Library
>          Issue Type: Bug
>          Components: 22. Localization
>    Affects Versions: 4.2.1, 4.2.x, 4.3.x, 5.0.0
>         Environment: Solaris 10 and 11, RedHat and OpenSuSE Linux, Sun C++ Compilers
12.1, 12.2, 12.3
> Issue is independent of platform and/or compiler.
>            Reporter: Stefan Teleman
>              Labels: thread-safety
>             Fix For: 4.2.x, 4.3.x, 5.0.0
>
>         Attachments: 22.locale.numpunct.mt.out, facet.cpp.diff, locale_body.cpp.diff,
punct.cpp.diff, runtests-linux32-all.out, runtests-linux64-all.out, runtests.out, STDCXX-1056-additional-timings.tgz,
stdcxx-1056.patch, stdcxx-1056-timings.tgz, stdcxx-4.2.x-numpunct-perfect-forwarding.patch,
stdcxx-4.3.x-numpunct-perfect-forwarding.patch
>
>
> several member functions in std::moneypunct<> and std::numpunct<> return
> a std::string by value (as required by the Standard). The implication of return-by-value
> being that the caller "owns" the returned object.
> In the stdcxx implementation, the std::basic_string copy constructor uses a shared
> underlying buffer implementation. This shared buffer creates the first problem for
> these classes: although the std::string object returned by value *appears* to be owned
> by the caller, it is, in fact, not.
> In a mult-threaded environment, this underlying shared buffer can be subsequently modified
by a different thread than the one who made the initial call. Furthermore, two or more different
threads can access the same shared buffer at the same time, and modify it, resulting in undefined
run-time behavior.
> The cure for this defect has two parts:
> 1. the member functions in question must truly return a copy by avoiding a call to the
copy constructor, and using a constructor which creates a deep copy of the std::string.
> 2. access to these member functions must be serialized, in order to guarantee atomicity
> of the creation of the std::string being returned by value.
> Patch for 4.2.1 to follow.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message