Implement LWG-3343 Ordering of calls to unlock() and notify_all() in Effects element of notify_all_at_thread_exit() should be reversed

[vmichal]

Swap mutex unlock and condition_variable notify_all in _Cnd_do_broadcast_at_thread_exit.

Fixes #5863 (at least partially)

The issue LWG-3343 mentions that he notification is additionally sequenced after destructors of all objects in thread-local storage; it is not clear to me how that is checkable (see #5863 (comment))

[NylteJ]

libc++ has already speculatively implemented this fix, and I believe we can reuse their test case. Specifically, we should delete these lines:

STL/tests/libcxx/expected_results.txt

Lines 612 to 615 in 5459853

	# LWG-3343 "Ordering of calls to unlock() and notify_all() in Effects element of notify_all_at_thread_exit() should be reversed" (Open)
	# libc++ speculatively implements LWG-3343. If we wanted to do the same thing,
	# we'd need to reverse the order of the _Mtx_unlock() and _Cnd_broadcast() calls in xnotify.cpp.
	std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp SKIPPED

(Regardless of whether we write our own test, I think we should enable this one.)

The issue LWG-3343 mentions that he notification is additionally sequenced after destructors of all objects in thread-local storage;

This part seems more like a wording change to me, but I’m not entirely sure, and I haven't found relevant tests either.

[vmichal]

libc++ has already speculatively implemented this fix, and I believe we can reuse their test case. Specifically, we should delete these lines:

STL/tests/libcxx/expected_results.txt

Lines 612 to 615 in 5459853

	# LWG-3343 "Ordering of calls to unlock() and notify_all() in Effects element of notify_all_at_thread_exit() should be reversed" (Open)
	# libc++ speculatively implements LWG-3343. If we wanted to do the same thing,
	# we'd need to reverse the order of the _Mtx_unlock() and _Cnd_broadcast() calls in xnotify.cpp.
	std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp SKIPPED

(Regardless of whether we write our own test, I think we should enable this one.)

Is this test of any value? I understand the intent (trying to reproduce the example from LWG-3343), but it does not appear to do so very well. It has implicit timing requirements that invalidate its effect IMO. If for nothing else, then for std::this_thread::sleep_for(std::chrono::milliseconds(1));.
I will enable the test, but it is passing for both implementations (reversed or not)... Opinions?

This part seems more like a wording change to me, but I’m not entirely sure, and I haven't found relevant tests either.

We could create one by adding side effects to the destructor of TLS object, but that still does not avoid the timing requirements problem.

To observe the order of _Mtx_unlock and _Cnd_broadcast we would need to trigger the specific pathological order of context switches given in LWG-3343.

[NylteJ]

Is this test of any value? I understand the intent (trying to reproduce the example from LWG-3343), but it does not appear to do so very well. It has implicit timing requirements that invalidate its effect IMO. If for nothing else, then for std::this_thread::sleep_for(std::chrono::milliseconds(1));.
I will enable the test, but it is passing for both implementations (reversed or not)... Opinions?

Enabling it seems reasonable to me - it may detect issues, doesn’t produce false positives, and doesn’t consume many resources, so it doesn’t meet our bar for skipping a test, IMO. If it’s not effective enough, we can write a new one.

We could create one by adding side effects to the destructor of TLS object, but that still does not avoid the timing requirements problem.

To observe the order of _Mtx_unlock and _Cnd_broadcast we would need to trigger the specific pathological order of context switches given in LWG-3343.

I don’t know of any way to control context switches, and it seems impossible to simulate a context switch between _Mtx_unlock and _Cnd_broadcast using condition variables without modifying product code. This seems like something we can only leave to chance.

[StephanTLavavej]

As long as they aren't flaky-failing, I prefer to keep as many libcxx tests enabled as possible.

It's okay if we can't find a way to exercise every part of the fix. Having confidence that the source is fixed, and not disrupting existing test coverage, is sufficient.