Improve safe iterator move semantic

[Jonathan Wakely]

On 10/08/18 11:00 +0100, Jonathan Wakely wrote:
>This valid program shows data races with -fsanitize=thread after your
>patch is applied:
>
>#define _GLIBCXX_DEBUG
>#include <vector>
>#include <thread>
>
>void thrash(std::vector<int>::iterator& iter)
>{
> for (int i = 0; i < 1000; ++i)
> {
>   auto jiter = std::move(iter);
>   iter = std::move(jiter);
>   jiter = std::move(iter);
>   iter = std::move(jiter);
> }
>}
>
>int main()
>{
> std::vector<int> v{1, 2, 3};
> auto it1 = v.begin();
> auto it2 = v.begin();
> std::thread t1(thrash, std::ref(it1));
> thrash(it2);
> t1.join();
>}

Doing a test like this with TSan should be the absolute minimum
required for any change to the mutex locking policy.

You want to reduce the locks on move operations of iterators? OK,
perform lots of move operations on two iterators from the same
sequence and see if ThreadSanitizer shows errors.

If TSan doesn't show errors, adjust the test and try harder to prove
it's correct. Maybe your test was flawed. For example, my first
attempt to prove the data race passed the iterators by value not by
reference. That meant there were more than two debug iterators in the
linked list, and so the two iterators being tested were not adjacent
in the list, and didn't conflict. Because I'd already proved there was
a bug by inspecting the code I knew my test was flawed, so I changed
it. But even if I hadn't already proved a bug, I would have kept
testing different variations to prove whether the code was correct or
not.

Our debug iterators make "non-local" modifications to other objects
from the same sequence. That needs careful synchronisation. If that
makes the code a bit slower, then we just have to live with it.
Correct and slow is better than fast and wrong.

I'm not aware of people complaining about the performance of debug
mode anyway. Everybody I speak to is happy to accept a performance hit
in order to get checking.

I think https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86843 would be
more helpful to our users, as it would allow some Debug Mode checks to
be enabled in programs that can't currently use it (because
recompiling the entire program is not possible).