Skip to content

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

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

Merged
Stephan T. Lavavej (StephanTLavavej) 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 all 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
2 changes: 1 addition & 1 deletion stl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "")
set(CMAKE_SHARED_LINKER_FLAGS_RELEASE "")

# Toolset options must appear before add_library() for the options to take effect.
add_compile_definitions(_CRTBLD _VCRT_ALLOW_INTERNALS _HAS_OLD_IOSTREAMS_MEMBERS=1 _STL_CONCRT_SUPPORT)
add_compile_definitions(_CRTBLD _VCRT_ALLOW_INTERNALS _HAS_OLD_IOSTREAMS_MEMBERS=1)

# /Z7 for MSVC, /Zi for MASM
set(CMAKE_MSVC_DEBUG_INFORMATION_FORMAT "Embedded")
Expand Down
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
Comment thread
StephanTLavavej marked this conversation as resolved.

static constexpr size_t _Critical_section_align = alignof(void*);

int _Type;
const void* _Vptr;
Comment thread
StephanTLavavej marked this conversation as 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

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) Jun 14, 2023

Copy link
Copy Markdown
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
2 changes: 0 additions & 2 deletions stl/msbuild/stl_base/libcp.settings.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
<TargetType>LIBRARY</TargetType>
<TargetAppFamily Condition="'$(MsvcpFlavor)' == 'app'">true</TargetAppFamily>
<TargetCoreSystem Condition="'$(MsvcpFlavor)' == 'onecore'">true</TargetCoreSystem>
<DependsOnConcRT Condition="'$(MsvcpFlavor)' == 'kernel32'">true</DependsOnConcRT>
<Arm64CombinedPdb>true</Arm64CombinedPdb>
</PropertyGroup>

Expand All @@ -26,7 +25,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

<PropertyGroup>
<ClProgramDataBaseFileName>$(OutputLibPdbPath)$(OutputName)$(PdbVerName).pdb</ClProgramDataBaseFileName>
<ClDefines Condition="'$(DependsOnConcRT)' == 'true'">$(ClDefines);_STL_CONCRT_SUPPORT</ClDefines>
<ClDefines>$(ClDefines);_VCRT_ALLOW_INTERNALS;_ANNOTATE_VECTOR;_ANNOTATE_STRING</ClDefines>
</PropertyGroup>

Expand Down
2 changes: 0 additions & 2 deletions stl/msbuild/stl_base/msvcp.settings.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

<DependsOnVCStartupLib>$(MsvcpFlavor)</DependsOnVCStartupLib>
<DependsOnVCRuntimeLib>$(MsvcpFlavor)</DependsOnVCRuntimeLib>
<DependsOnConcRT Condition="'$(MsvcpFlavor)' == 'kernel32'">true</DependsOnConcRT>

<TargetAppFamily Condition="'$(MsvcpFlavor)' == 'app'">true</TargetAppFamily>
<TargetCoreSystem Condition="'$(MsvcpFlavor)' == 'onecore'">true</TargetCoreSystem>
Expand Down Expand Up @@ -40,7 +39,6 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
<IntermediateImportLibOutput>$(CrtBuildDir)\msvcprt_base$(BuildSuffix).$(MsvcpFlavor).import_only.lib</IntermediateImportLibOutput>
<DllDefName>$(LibOutputFileName).$(MsvcpFlavor)</DllDefName>
<DllDef>$(IntermediateOutputDirectory)\$(DllDefName).def</DllDef>
<ClDefines Condition="'$(DependsOnConcRT)' == 'true'">$(ClDefines);_STL_CONCRT_SUPPORT</ClDefines>

<LinkGenerateDebugInformation>true</LinkGenerateDebugInformation>
<LinkProgramDataBaseFileName>$(OutputPath)\$(OutputName)$(_PDB_VER_NAME_)$(DllPdbFlavorSuffix)</LinkProgramDataBaseFileName>
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
39 changes: 13 additions & 26 deletions stl/src/primitives.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,31 +108,18 @@ namespace Concurrency {
new (p) stl_condition_variable_win7;
}

#ifdef _WIN64
const size_t sizeof_stl_critical_section_concrt = 64;
const size_t sizeof_stl_condition_variable_concrt = 72;
const size_t sizeof_stl_critical_section_vista = 48;
const size_t sizeof_stl_condition_variable_vista = 16;
#else // ^^^ 64-bit / 32-bit vvv
const size_t sizeof_stl_critical_section_concrt = 36;
const size_t sizeof_stl_condition_variable_concrt = 40;
const size_t sizeof_stl_critical_section_vista = 28;
const size_t sizeof_stl_condition_variable_vista = 8;
#endif // ^^^ 32-bit ^^^

#if defined(_CRT_WINDOWS)
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)
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;
Comment thread
StephanTLavavej marked this conversation as resolved.
#endif // ^^^ !defined(_CRT_WINDOWS) && !defined(_STL_CONCRT_SUPPORT) ^^^

// concrt, vista, and win7 alignments are all identical to alignof(void*)
const size_t stl_critical_section_max_alignment = alignof(stl_critical_section_win7);
const size_t stl_condition_variable_max_alignment = alignof(stl_condition_variable_win7);
#if defined(_CRT_WINDOWS) // for Windows-internal code
const size_t stl_critical_section_max_size = 2 * sizeof(void*);
const size_t stl_condition_variable_max_size = 2 * sizeof(void*);
#elif defined(_WIN64) // ordinary 64-bit code
const size_t stl_critical_section_max_size = 64;
const size_t stl_condition_variable_max_size = 72;
#else // vvv ordinary 32-bit code vvv
const size_t stl_critical_section_max_size = 36;
const size_t stl_condition_variable_max_size = 40;
#endif // ^^^ ordinary 32-bit code ^^^

const size_t stl_critical_section_max_alignment = alignof(void*);
const size_t stl_condition_variable_max_alignment = alignof(void*);
} // namespace details
} // namespace Concurrency