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.

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

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Jun 12, 2023

<mutex> doesn't need to know the layout of the critical section member. Specifying the layout is both unnecessary and error-prone (if we ever decide to change stl_critical_section_win7).

This also makes it possible to verify that _Critical_section_size matches stl_critical_section_max_size.

This fixes a bug on UWP where locking a mutex could throw a bogus exception.

@cpplearner cpplearner requested a review from a team as a code owner June 12, 2023 04:37
@StephanTLavavej StephanTLavavej self-assigned this Jun 12, 2023
stl/inc/mutex Show resolved Hide resolved
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej removed their assignment Jun 12, 2023
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jun 13, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 13, 2023
Comment on lines 51 to 54
int _Type;
const void* _Vptr;
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;
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.

stl/src/primitives.hpp Show resolved Hide resolved
@@ -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.

@StephanTLavavej StephanTLavavej added bug Something isn't working and removed enhancement Something can be improved labels Jun 14, 2023
@StephanTLavavej StephanTLavavej removed their assignment Jun 14, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit a9e0c12 into microsoft:main Jun 15, 2023
@StephanTLavavej
Copy link
Member

Thanks for this extreme cleanup that ended up finding and fixing a bug! 🧹 🐞 😻

@cpplearner cpplearner deleted the mutex-internal branch June 15, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants