https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21334
Jonathan Wakely <redi at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|SUSPENDED |ASSIGNED
Assignee|unassigned at gcc dot gnu.org |redi at gcc dot gnu.org
Keywords| |documentation
--- Comment #52 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to James Kanze from comment #0)
> I am sending this to the g++ bug list on the recommendation of
> Gabriel Dos Reis. From what little I've read in the g++
> documentation, I'm not convinced that the authors of the g++
> library intend for it to be supported, although Posix would seem
> to require it.
I disagree.
> For the record, the statement in Posix is: "Applications shall
> ensure that access to any memory location by more than one
> thread of control (threads or processes) is restricted such that
> no thread of control can read or modify a memory location while
> another thread of control may be modifying it." The obvious
> extension to C++ is that of replacing "memory location" with
> "object";
Agreed so far.
> at the very least, of course, one can only require
> something of "memory locations" which the user sees, directly or
> indirectly. The statement in the libstdc++-v3 FAQ (question
> 5.6) is: "All library objects are safe to use in a multithreaded
> program as long as each thread carefully locks out access by any
> other thread while it uses any object visible to another thread,
> i.e., treat library objects like any other shared resource. In
> general, this requirement includes both read and write access to
> objects; unless otherwise documented as safe, do not assume that
> two threads may access a shared standard library object at the
> same time." A considerably weaker guarantee than what one
> normally expects under Posix.
That was rewritten for Bug 50834 with r0-112642-gec8ab7c4e438c4
The C++11 rules are closer to the POSIX ones.
The C++11 standard makes exemptions for the "standard containers" so that
calling non-const accessor functions which don't actually modify the object
(such as begin(), end(), data(), operator[] and others) do *not* cause
conflicts with potentially concurrent calls on the same object. Just obtaining
the begin() iterator does not actually modify a std::vector even though begin()
is a non-const member function. Just like taking the address of the first
element of a char[] array does not modify that object, it just obtains a
mutable iterator into it.
However, the old Copy-on-Write std::string in libstdc++ is not a "standard
container". The whole reason we spent enormous effort transitioning from the
old std::string to the new SSO one is precisely because it isn't thread-safe as
required for the C++11 standard containers.
The docs should be clear that the old COW std::string is exempt from the
exemptions for standard containers. I will fix that.
So with that said ...
> The following is an example of a program which may cause
> problems:
>
> #include <pthread.h>
> #include <iostream>
> #include <ostream>
> #include <string>
>
> std::string g ;
>
> extern "C" void*
> thread1(
> void* lock )
> {
> std::string t( g ) ;
This is a read of the memory locations corresponding to g.
> pthread_mutex_lock( static_cast< pthread_mutex_t* >( lock ) ) ;
> std::cout << t << '\n' ;
> pthread_mutex_unlock( static_cast< pthread_mutex_t* >( lock ) ) ;
> return NULL ;
> }
>
> extern "C" void*
> thread2(
> void* lock )
> {
> std::string t( g.begin(), g.end() ) ;
This is calling non-const member functions, so is considered as a write to the
memory locations corresponding to g.
So the program has a data race, and its behaviour is undefined. So this is not
a bug in the library according to the thread-safety rules of the C++ standard
(which didn't exist when this bug was opened). And I don't even think it's true
that the COW std::string doesn't meet the POSIX rules, because the original
report is assuming that calling non-const member functions does not modify the
memory location, but the C++ standard formalized a model where (in general) any
call to a non-const member function is considered a modification of the object.
> The problem is, of course, that the sequence which tests whether
> we have to leak, and then leaks, is not atomic.
And it doesn't need to be, because the action of leaking only happens as a
result of calling non-const member functions, which must be correctly
synchronized so that it does not conflict with any other accesses (whether
reads or writes) to the same object.
(In reply to James Kanze from comment #21)
> In typicaly code, the problem occurs mainly with configuration
> variables; they are declared non-const, because they have to be
> set on start up, either from the command line, or from a
> configuration file, and then used as if they were const.
> According to Posix, I should be able to access such variables
> without additional, user defined synchronization.
But only if you call const member functions. C++ extends the POSIX
thread-safety model to apply to objects by defining calls to const member
functions as "reads" and calls to non-const member functions as "writes". As
comment 35 said, if thread2 did:
const std::string& const_g = g;
std::string t(const_g.begin(), const_g.end());
then there would be no conflicting accesses involving non-const member
functions. This would be thread-safe by all relevant definitions.
> Note that the problem can be very, very subtle, and that most of
> the time, the program will work despite the problem -- this is
> one of the most pernicious cases of undefined behavior.
Yes, I'm actually considering this change:
--- a/libstdc++-v3/include/bits/cow_string.h
+++ b/libstdc++-v3/include/bits/cow_string.h
@@ -226,7 +226,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
void
_M_set_leaked() _GLIBCXX_NOEXCEPT
- { this->_M_refcount = -1; }
+ {
+ _Atomic_word __refcount
+ = __gnu_cxx::__exchange_and_add_dispatch(&this->_M_refcount, -1);
+ __glibcxx_assert(__recount == 0);
+ }
void
_M_set_sharable() _GLIBCXX_NOEXCEPT
This would not "fix" the bug, but it would make it more likely to crash noisily
(at least with assertions enabled).
> Other than that, there is no perfect solution with regards to
> exceptions. If an exception requires resources, and they aren't
> there, there's going to be a problem. What happens if you can't
> allocate the memory for the exception itself?
Off-topic for this bug, but these days libstdc++ has an emergency pool used to
allocate exceptions when malloc fails. I also have ideas for changing the C++
standard to permit a singleton std::bad_alloc to be throw. Implementations
could allocate a single std::bad_alloc exception on startup, and then just
throw that one again and again, incrementing its reference count each time.
That needs language changes though, because the standard currently implies that
each throw expression creates a new object with a different address from any
other concurrent exceptions in flight.
(In reply to gianni from comment #35)
> Who said that calling begin() on a non const std::string should be thread
> safe ?
The C++ standard :-) But it says that about "the standard containers" (and
includes std::string), but the libstdc++ COW std::string **does not** conform
to that.
That's why we had to replace it, at great cost.
The fact that the COW std::string does not conform to a rule that necessitated
replacing the COW std::string is known.
So I think after many years, we should close this. But I'll clarify the docs
first.
