Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<mutex>: Make _Mtx_internal_imp_mirror more closely match _Mtx_internal_imp_t #3763

Merged
merged 7 commits into from
Jun 15, 2023
Merged

<mutex>: Make _Mtx_internal_imp_mirror more closely match _Mtx_internal_imp_t #3763

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ea132c
Make `_Mtx_internal_imp_mirror` more closely match `_Mtx_internal_imp_t`
cpplearner Jun 12, 2023
8c339e1
Stop depending on `_STL_CONCRT_SUPPORT`
cpplearner Jun 13, 2023
e3fcbba
`sizeof_stl_MEOW_vista` is now dead.
StephanTLavavej Jun 14, 2023
6f1a6f6
Hardcode size/align, verified via static_assert.
StephanTLavavej Jun 14, 2023
f1fc626
Fuse `sizeof_stl_MEOW_concrt` for simplicity.
StephanTLavavej Jun 14, 2023
bc8714b
`_STL_CONCRT_SUPPORT` is now dead.
StephanTLavavej Jun 14, 2023
536d5a3
`DependsOnConcRT` is now dead.
StephanTLavavej Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions stl/inc/mutex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,22 @@ _EXPORT_STD class condition_variable_any;
struct _Mtx_internal_imp_mirror {
#ifdef _CRT_WINDOWS
#ifdef _WIN64
static constexpr size_t _Critical_section_size = 8;
static constexpr size_t _Critical_section_size = 16;
#else // _WIN64
static constexpr size_t _Critical_section_size = 4;
static constexpr size_t _Critical_section_size = 8;
#endif // _WIN64
#else // _CRT_WINDOWS
#ifdef _WIN64
static constexpr size_t _Critical_section_size = 56;
static constexpr size_t _Critical_section_size = 64;
#else // _WIN64
static constexpr size_t _Critical_section_size = 32;
static constexpr size_t _Critical_section_size = 36;
#endif // _WIN64
#endif // _CRT_WINDOWS
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved

static constexpr size_t _Critical_section_align = alignof(void*);

int _Type;
const void* _Vptr;
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
union {
void* _Srw_lock_placeholder;
unsigned char _Padding[_Critical_section_size];
};
_Aligned_storage_t<_Critical_section_size, _Critical_section_align> _Cs;
long _Thread_id;
int _Count;
Comment on lines 51 to 54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change requested, note to other reviewers: I know this looks really really scary, but I believe it's a good change. This doesn't control the layout, it's just trying to mirror it (hence the name) so the header can directly access _Count. We are now mirroring it exactly:

STL/stl/src/mutex.cpp

Lines 39 to 48 in c8d1efb

struct _Mtx_internal_imp_t { // ConcRT mutex
int type;
typename std::_Aligned_storage<Concurrency::details::stl_critical_section_max_size,
Concurrency::details::stl_critical_section_max_alignment>::type cs;
long thread_id;
int count;
Concurrency::details::stl_critical_section_interface* _get_cs() { // get pointer to implementation
return reinterpret_cast<Concurrency::details::stl_critical_section_interface*>(&cs);
}
};

The header doesn't care about the details of what's stored within the aligned storage, so we don't need to separately describe the vptr and the class data members within. (This also permits further refactoring as described in the PR comments, but it's a good simplification even aside from that.)

The other simplification is that we can now mention the same "size" as primitives.hpp does, which includes the vptr. This avoids having the numbers all be different by 4 bytes for 32-bit and 8 bytes for 64-bit.

};
Expand Down
4 changes: 2 additions & 2 deletions stl/src/cond.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ struct _Cnd_internal_imp_t { // condition variable implementation for ConcRT
}
};

static_assert(sizeof(_Cnd_internal_imp_t) <= _Cnd_internal_imp_size, "incorrect _Cnd_internal_imp_size");
static_assert(alignof(_Cnd_internal_imp_t) <= _Cnd_internal_imp_alignment, "incorrect _Cnd_internal_imp_alignment");
static_assert(sizeof(_Cnd_internal_imp_t) == _Cnd_internal_imp_size, "incorrect _Cnd_internal_imp_size");
static_assert(alignof(_Cnd_internal_imp_t) == _Cnd_internal_imp_alignment, "incorrect _Cnd_internal_imp_alignment");

void _Cnd_init_in_situ(const _Cnd_t cond) { // initialize condition variable in situ
Concurrency::details::create_stl_condition_variable(cond->_get_cv());
Expand Down
10 changes: 8 additions & 2 deletions stl/src/mutex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <cstdio>
#include <cstdlib>
#include <internal_shared.h>
#include <mutex>
#include <type_traits>
#include <xthreads.h>
#include <xtimec.h>
Expand Down Expand Up @@ -47,8 +48,13 @@ struct _Mtx_internal_imp_t { // ConcRT mutex
}
};

static_assert(sizeof(_Mtx_internal_imp_t) <= _Mtx_internal_imp_size, "incorrect _Mtx_internal_imp_size");
static_assert(alignof(_Mtx_internal_imp_t) <= _Mtx_internal_imp_alignment, "incorrect _Mtx_internal_imp_alignment");
static_assert(sizeof(_Mtx_internal_imp_t) == _Mtx_internal_imp_size, "incorrect _Mtx_internal_imp_size");
static_assert(alignof(_Mtx_internal_imp_t) == _Mtx_internal_imp_alignment, "incorrect _Mtx_internal_imp_alignment");

static_assert(
std::_Mtx_internal_imp_mirror::_Critical_section_size == Concurrency::details::stl_critical_section_max_size);
static_assert(
std::_Mtx_internal_imp_mirror::_Critical_section_align == Concurrency::details::stl_critical_section_max_alignment);

void _Mtx_init_in_situ(_Mtx_t mtx, int type) { // initialize mutex in situ
Concurrency::details::create_stl_critical_section(mtx->_get_cs());
Expand Down
7 changes: 2 additions & 5 deletions stl/src/primitives.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,10 @@ namespace Concurrency {
#if defined(_CRT_WINDOWS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change requested, note to other reviewers: I know this looks really scary too, but we had already messed up UWP as @cpplearner verified in #3763 (comment) . This PR fixes things. Note that stl/inc's idea of the necessary size is not changing, so user-compiled code remains binary-compatible. What's happening is that the separately compiled STL DLL/LIB is now performing layout for UWP that matches what the headers expected.

The actual layout mostly didn't matter, because the interface is a flat C API, except for two things:

  1. The space consumed needs to be less than or equal to the space provided by the header, and for UWP we are increasing the smaller MEOW_vista size consumed to the larger MEOW_concrt size which exactly matches the header, and
  2. The location of the _Count needs to match, which it does after this PR.

const size_t stl_critical_section_max_size = sizeof(stl_critical_section_win7);
const size_t stl_condition_variable_max_size = sizeof(stl_condition_variable_win7);
#elif defined(_STL_CONCRT_SUPPORT)
#else // vvv !defined(_CRT_WINDOWS) vvv
const size_t stl_critical_section_max_size = sizeof_stl_critical_section_concrt;
const size_t stl_condition_variable_max_size = sizeof_stl_condition_variable_concrt;
#else // vvv !defined(_CRT_WINDOWS) && !defined(_STL_CONCRT_SUPPORT) vvv
const size_t stl_critical_section_max_size = sizeof_stl_critical_section_vista;
const size_t stl_condition_variable_max_size = sizeof_stl_condition_variable_vista;
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
#endif // ^^^ !defined(_CRT_WINDOWS) && !defined(_STL_CONCRT_SUPPORT) ^^^
#endif // ^^^ !defined(_CRT_WINDOWS) ^^^

// concrt, vista, and win7 alignments are all identical to alignof(void*)
const size_t stl_critical_section_max_alignment = alignof(stl_critical_section_win7);
Expand Down