microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
9.88k stars 1.45k forks source link

Overhaul `condition_variable` #4720

Closed StephanTLavavej closed 1 week ago

StephanTLavavej commented 2 weeks ago

Overview

This improves how we initialize condition_variable and condition_variable_any. It also eliminates layers of confusing machinery like Concurrency::details::stl_condition_variable_win7. I've been careful to preserve bincompat at each step. (All of the other Concurrency::details machinery was dllexported, but not this one.) This also respects the escape hatch macro _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR, for users who aren't following our documented binary compatibility requirements and are mixing new STL headers with a very old STL DLL (older than VS 2022 17.8), so it doesn't affect that situation any further.

Commits

mutex.cpp doesn't use anything from primitives.hpp.


primitives.hpp doesn't need <type_traits>.


Stop calling no-op _Cnd_destroy_in_situ().

~condition_variable() can be defaulted, making it trivially destructible. (Note that this also makes timed_mutex and recursive_timed_mutex trivially destructible.)

~condition_variable_any() can also be defaulted. It's destroying a shared_ptr, so it's not trivially destructible.

Stop declaring _Cnd_destroy_in_situ() in xthreads.h. Note that this doesn't affect the dllexport surface, as we previously ensured that _CRTIMP2_PURE always appears on both declarations and definitions.

Comment the definition of _Cnd_destroy_in_situ() in cond.cpp as preserved for bincompat.


Statically initialize _Cnd_internal_imp_t.

This follows the example of _Mtx_internal_imp_t and _Stl_critical_section above.

Define _Stl_condition_variable mirroring Concurrency::details::stl_condition_variable_win7.

We can mirror CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; with void* _Win_cv = nullptr; because CONDITION_VARIABLE is just a struct wrapping void* (so it has the same size and alignment) and CONDITION_VARIABLE_INIT just initializes it to null.

Then, use the union pattern to keep the size and alignment of _Cnd_internal_imp_t unchanged, while initializing _Stl_condition_variable _Stl_cv{}; with a default member initializer, so its two pointers will be null. (The size is unchanged because _Cv_storage is at least 2 * sizeof(void*). The alignment is unchanged because everything is void*-aligned here.)

Finally, add empty braces when we have _Cnd_internal_imp_t _Cnd_storage{}; data members. These aren't necessary, but they serve as a reminder that we're getting null instead of garbage, and this is consistent with how we have _Mtx_internal_imp_t _Mtx_storage{};.

Note that I chose these names to avoid confusion between the different layers:


Now we can stop calling _Cnd_init_in_situ(), with _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR as the escape hatch.

We just changed condition_variable_any and condition_variable to statically initialize _Cnd_internal_imp_t.

condition_variable's default constructor can now be defaulted.

In xthreads.h, declare _Cnd_init_in_situ() only when needed for the escape hatch. (Again, this doesn't affect bincompat, the definition still has _CRTIMP2_PURE.)

Comment _Cnd_init_in_situ() as preserved for bincompat and the escape hatch. Its only caller is _Cnd_init() immediately below, which is only called by _Thrd_create(), which is preserved for bincompat.

We need the escape hatch for the same reason that mutex does. When a user hasn't followed our documented restrictions for binary compatibility, and mixes new headers with a very old STL DLL (older than VS 2022 17.8, which was the release that shipped #3770), the headers will initialize the vptr to null, but the old STL DLL will want to actually use the vptr. The escape hatch makes the headers call into the old STL DLL to set up a real vptr.


Simplify the definition of _Cnd_init_in_situ().

Constructing _Cnd_internal_imp_t will now perform the initialization that we need.


Stop calling _Cnd_init() and _Cnd_destroy().

We declared them in xthreads.h (guarded by _CRTBLD) because they were defined by cond.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked _CRTIMP2_PURE.

Mark the definitions as preserved for bincompat.

In cthread.cpp _Thrd_create() (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a condition variable. Locally initializing _Cnd_internal_imp_t cond_var{}; and taking its address is sufficient.


Stop calling _Mtx_init() and _Mtx_destroy().

We declared them in xthreads.h (guarded by _CRTBLD) because they were defined by mutex.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked _CRTIMP2_PURE.

Mark the definition of _Mtx_init() as preserved for bincompat. _Mtx_destroy() was already marked.

In cthread.cpp _Thrd_create() (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a mutex. Locally initializing _Mtx_internal_imp_t mtx_var{};, taking its address, and calling _Mtx_init_in_situ() is sufficient. We don't need to call _Mtx_destroy_in_situ() (which isn't declared). It's a no-op except for a debug check "mutex destroyed while busy". std::mutex doesn't bother with that anymore, and we're definitely calling _Mtx_unlock() so we're fine.


Declare _Mtx_init_in_situ() only when needed.

And update its comment accordingly.


Replace Concurrency::details::stl_condition_variable_win7 with non-member functions.

This wasn't dllexported, and the changes are limited to stl/src, so there's no bincompat impact.

I chose the naming scheme _Primitive_wait to avoid confusion with _Cnd_wait which does more work.

The functions don't need to be extern "C", because they don't interact with user code. (They do need to be ugly to avoid conflicts during static linking.)

They have to be inline. I also marked them as noexcept, although that wasn't necessary.

Because they aren't member functions anymore, I have to define _Primitive_wait_for() before _Primitive_wait() can call it.


Simplify _Primitive_wait[_for]() by taking _Mtx_t mtx.

Now that the originally-ConcRT-derived layer has been eradicated, we don't need to make every caller reach into _Mtx_t internals. This also increases consistency with the _Cnd_t cond parameter.


Skip 3 libcxx tests for Clang until LLVM-94907 is merged.

StephanTLavavej commented 2 weeks ago

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej commented 2 weeks ago

I had to push an additional commit to silence a false positive warning from static analysis.