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

Cleanups for mutex utilities #3912

Merged
merged 12 commits into from
Aug 11, 2023
Merged

Conversation

achabense
Copy link
Contributor

  • Removes declaration of _Mtx_timedlock and _Mtx_getconcrtcs from <xthreads.h>, as they are not used in the library.
  • Slightly simplifies mtx_do_lock in "mutex.cpp"

@achabense achabense requested a review from a team as a code owner July 30, 2023 07:39
stl/src/mutex.cpp Outdated Show resolved Hide resolved
stl/src/mutex.cpp Outdated Show resolved Hide resolved
stl/inc/xthreads.h Show resolved Hide resolved
stl/src/mutex.cpp Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 31, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 31, 2023
Also mark `_Thrd_abort`, `_Thrd_create`, `_Thrd_current`, `_Thrd_equal`, `_Thrd_exit`, `_Thrd_start` as `__cdecl`; see MSVC-PR-135576.

Wrap `cond.cpp` and `mutex.cpp` in `_EXTERN_C`, copied from `xthreads.h`. This also affects the following:

* `struct _Cnd_internal_imp_t`
  + Good, it was already declared within `_EXTERN_C` in `xthreads.h`.
* `enum class __stl_sync_api_modes_enum`
  + Good, it's used only as a parameter type for `__set_stl_sync_api_mode()` which is `extern "C"`.
* `get_srw_lock()`
  + Good, it's `static`.
* `mtx_do_lock()`
  + Good, it's `static`.
stl/inc/xthreads.h Show resolved Hide resolved
stl/src/mutex.cpp Outdated Show resolved Hide resolved
stl/src/mutex.cpp Outdated Show resolved Hide resolved
stl/inc/xthreads.h Show resolved Hide resolved
stl/src/mutex.cpp Show resolved Hide resolved
stl/src/mutex.cpp Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I've pushed changes to address accumulated comments:

  • Style: Use auto after static_cast.
  • Style: After if-return, avoid else.
  • Comment that _Mtx_plain is for _Thrd_create.

I've pushed one enhancement (related to a comment) that hides more declarations from users. We've seen this pattern elsewhere in the STL - there are some separately compiled functions that are used only between separately compiled STL TUs (in the DLL or static LIB), but we marked them as dllexported and declared them for users. We can at least hide them from users, but we have to be careful to preserve the dllexporting:

  • Enhancement: Guard 6 declarations with #ifdef _CRTBLD, comment why.

Finally, there is a major correctness fix. (Check the output of dumpbin /exports if you don't believe me 😸) This is related to pre-existing weirdness in the STL that it's time to fix up:

  • Fix bincompat: Copy _CRTIMP2_PURE and __cdecl from xthreads.h.
    • Also mark _Thrd_abort, _Thrd_create, _Thrd_current, _Thrd_equal, _Thrd_exit, _Thrd_start as __cdecl. This is the default when we build the STL, and this is also how they were originally declared (see internal MSVC-PR-135576 when the declarations were removed).
    • Wrap cond.cpp and mutex.cpp in _EXTERN_C, copied from xthreads.h. (Other functions declared in xthreads.h were already being defined in _EXTERN_C blocks.) This also affects the following:
      • struct _Cnd_internal_imp_t
        • Good, it was already declared within _EXTERN_C in xthreads.h.
      • enum class __stl_sync_api_modes_enum
        • Good, it's used only as a parameter type for __set_stl_sync_api_mode() which is extern "C".
      • get_srw_lock() and mtx_do_lock()
        • Good, they're static.

I've verified that dumpbin /exports is now unaffected.

@achabense
Copy link
Contributor Author

I've made another push to remove two redundant assignments (res = WAIT_TIMEOUT)

@StephanTLavavej
Copy link
Member

After carefully looking at the control flow, I agree that the assignments were redundant. (Had to consider the impact of loops.)

stl/inc/xthreads.h Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Aug 9, 2023
@StephanTLavavej
Copy link
Member

@CaseyCarter I've pushed a commit to drop the "used only by" comments as they are superseded by _CRTBLD.

@StephanTLavavej StephanTLavavej self-assigned this Aug 10, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Copy link
Member

I pushed a merge with main to resolve a trivial conflict in stl/src/cond.cpp where #3935 removed static_asserts next to where this PR is adding _CRTIMP2_PURE and __cdecl to _Cnd_init_in_situ.

@StephanTLavavej StephanTLavavej merged commit ee3df46 into microsoft:main Aug 11, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for eliminating these unnecessary declarations, and prompting us to fix long-standing codebase inconsistency! 🎉 😸 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants