On 10/05/20 23:03 +0200, François Dumont via Libstdc++ wrote:
I just committed this patch.
François
On 03/03/20 10:11 pm, François Dumont wrote:
After the fix of PR 91910 I tried to consider other possible race
condition and I think we still have a problem.
Like stated in the PR when a container is destroyed all associated
iterators are made singular. If at the same time another thread try
to access this iterator the _M_singular check will face a data race
That's undefined behaviour, and the user's fault.
The problem described in the PR is different. It must be safe to
destroy the container and iterator concurrently. It does not need to
be safe to destroy the container and read from the iterator
concurrently.
It might be nice to improve the behaviour on such errors in user code,
but it's not necessary for correctness (unlike the case in the PR).
when accessing _M_sequence member. In case of race condition the
program is likely to abort but maybe because of memory access
violation rather than a clear singular iterator assertion.
I don't think that's a valid assumption, it might terminate with
SIGSEGV, but not SIGABRT.
To avoid this I rework _M_sequence manipulation to use atomic read
when necessary and make sure that otherwise container mutex is
locked.
I'm not very happy with the change. You seem to be trying to make the
debug iterators fully thread-safe, to support arbitrary concurrent
accesses to the iterators and container. Your patch doesn't achieve
that (there are still races due to non-atomic writes that conflict
with reads), and I don't even think it's possible in general. What
the patch does do is put more work inside the critical section
controlled by the mutex, which could make things slower.
