Re: [patch] Make std::tr1::shared_ptr thread-safe.

["Peter Dimov" <pdimov at mmltd dot net>]

Paolo Carlini wrote:
> Gabriel Dos Reis wrote:
>
> > > Why are you using the term "broken"?!? I don't think Alexander and
> > > Peter used the term "broken" anywhere. It's just a matter of
> > > efficiency and, as far as I'm concerned, not checking in invasive
> > > changes so close to the release. Do you disagree with that?!?
> >
> > if it is appropriate for 4.0.1, it ought to be appropriate for 4.0.0.
> Ok, if you think so, and the other maintainers agree, I volunteer to
> help Jonathan preparing a complete patch in time for 4.0.0, which
> would include (to summarize): configury bits for old platforms (386
> and Sparc), resurrection of compare_and_swap, fall-back to Alexander
> and Peter proposal.

I think I need to clarify things a bit.

Whether add_ref_lock uses a CAS (Alexander Terekhov's algorithm) or a mutex 
lock (Tyson Whitehead's modification) is one issue. Both are equally 
correct. The mutex lock is taken only when a shared_ptr is constructed from 
a weak_ptr and does not affect the rest of the implementation.

A separate issue is that there needs to exist proper memory ordering between 
some of the operations, for example, destroy() needs to happen after 
dispose() (but there are other visibility issues, some of them more subtle.)

This can be fixed in one of the following ways:

1. Empty lock/unlock regions on the same mutex as outlined in my and 
Alexander's posts.

2. Full bidirectional memory barriers at the same two places.

3. Making __exchange_and_add offer the following memory synchronization 
guarantees:

- acquire memory semantics when the result is not zero;
- release memory semantics when the result is zero

or introducing a separate __atomic_decrement primitive with these 
properties.

I recommend (1), if Tyson Whitehead's algorithm is used and there is already 
a mutex handy, or (2), which would require the addition of a 
_GLIBCXX_MEMORY_BARRIER primitive.

Alexander would recommend (3), which also fixes two other problems. I think 
that this is not feasible for 4.0.0, but the decision is up to you. The main 
problem with msync guarantees is that they aren't testable.

The next issue on our list is that reference-counted mutable synchronized 
objects would need an empty lock/unlock region at the start of the 
destructor if (3) is not chosen. I consider this unfortunate but acceptable.

The final issue is that the reference counted objects in libstdc++ 
(std::string, std::locale::facet) may also be broken *unless* 
__exchange_and_add has some memory visibility guarantees that I don't 
understand, but Alexander does. (It needs to establish ordering of prior 
loads WRT following stores IIUC). This will be fixed by (3) and may or may 
not be a problem in practice; I'm not an expert in this area and can't 
review the current __exchange_and_add implementations. In any event, 
shared_ptr with the above fixes applied would be no more broken than the 
rest of the library.

If that's going to be of any help, I'm planning to use Whitehead/(1) in 
Boost on libstdc++ pre-4, and whatever you decide to use for 4 and up.

Thank you for reading this far. ;-)