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> : std::mutex::mutex is not constexpr #2285

Closed
tiagomacarios opened this issue Oct 19, 2021 · 13 comments · Fixed by #3824
Closed

<mutex> : std::mutex::mutex is not constexpr #2285

tiagomacarios opened this issue Oct 19, 2021 · 13 comments · Fixed by #3824
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@tiagomacarios
Copy link
Member

tiagomacarios commented Oct 19, 2021

This is mentioned in other issues, but I wanted a bug report tracking this explicitly. For the curious see #946.

std::mutex::mutex is constexpr as of c++11, but even in c++17 MSVC will not support it.
Minimal repro:
https://godbolt.org/z/5Yc56hEG7

#include <mutex>

struct S {
    constexpr S() noexcept = default;
    std::mutex m_mutex;
};

static constexpr S s{};
@CaseyCarter CaseyCarter added bug Something isn't working vNext Breaks binary compatibility labels Oct 19, 2021
@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 20, 2021

It is not even documented. I added a PR form std::mutex documentation: MicrosoftDocs/cpp-docs#3467
But probably it also should be mentioned here: https://docs.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-160

@AdamBucior
Copy link
Contributor

Could we make shared_mutex constructor constexpr so that people who need it can have an alternative?

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 20, 2021

Could we make shared_mutex constructor constexpr so that people who need it can have an alternative?

9-Best-Grumpy-Cat-Memes-1

See #845:

We are explicitly forbidden from strengthening constexpr by WG21-N4861 16.5.5.7 [constexpr.functions]/1: "An implementation shall not declare any standard library function signature as constexpr except for those where it is explicitly required."

@StephanTLavavej
Copy link
Member

Perhaps an LWG issue should be submitted to allow shared_mutex's constructor to be constexpr, as static initialization is preferable to dynamic initialization. As we just store nullptr, we would easily be able to implement such a resolution.

@CaseyCarter
Copy link
Contributor

Perhaps an LWG issue should be submitted to allow shared_mutex's constructor to be constexpr, as static initialization is preferable to dynamic initialization. As we just store nullptr, we would easily be able to implement such a resolution.

Looks like it would be feasible for POSIX platforms as well (https://godbolt.org/z/nqxs1n9xn) if they implement shared_mutex naively.

@CaseyCarter CaseyCarter removed the vNext Breaks binary compatibility label Oct 22, 2021
@CaseyCarter
Copy link
Contributor

Per the discussion in #2286, we believe mutex's default constructor can feasibly be made constexpr absent the need to support Vista. We'll need to figure out precisely how to wiggle the class definition so the in-memory representation of a default-constructed mutex looks exactly like the current result of dynamic-initialization on a Windows-7-or-later box.

@AlexGuteniev
Copy link
Contributor

I've tried to look into it. So we need to implement Concurrency::details::stl_critical_section_win7 and _Mtx_internal_imp_t which contains it in a header. These are not _Ugly. Should we:

  • Implement _Ugly version that does not match existing names?
  • Make non-_Ugly names visible in headers?

AlexGuteniev added a commit to AlexGuteniev/STL that referenced this issue Oct 30, 2021
@BillyONeal
Copy link
Member

Per the discussion in #2286, we believe mutex's default constructor can feasibly be made constexpr absent the need to support Vista. We'll need to figure out precisely how to wiggle the class definition so the in-memory representation of a default-constructed mutex looks exactly like the current result of dynamic-initialization on a Windows-7-or-later box.

I don't believe this is true, at least for std::mutex's current guarantees, because the vtbl must live in the STL's DLL. Otherwise, mutexes created by a user DLL become radioactive if that user DLL is unloaded. (There's a similar problem in system_error for generic_category and system_category, but error_codes tend not to be super long-lived objects likely to trigger the problem vs. mutexes)

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 31, 2021

@BillyONeal suggested a possible workaround for his concern: use __builtin_isconstant_evaluated() and do constant initialization to a new implementation only in constexpr context, while keeping old initialization for runtime.

Still there are problems in getting vtable class into mutex:

  • mutex subobjects can't contain a vtable directly or via an union member due to is_standard_layout requirement
  • vtable cannot be copied via __builtin_bit_cast due to requirement of trivially_copyable
  • vtable cannot be set with placement new, as only allocating new is valid in constexpr context
  • construct_at does not help either as there should be a pointer to vtable class, and reinterpreting casts won't work in constexpr
  • fake vtable could have worked. That is a structure with pointers to functions. __thiscall methods can be emulated with __fastcall function. But this all is way too scary

@Fedr
Copy link

Fedr commented Oct 30, 2022

There is a question on Stackoverflow about this issue.

@pdimov
Copy link

pdimov commented Jan 18, 2023

FWIW, I just got a bug report against Boost.System caused by std::mutex's default constructor not using static initialization: boostorg/system#102.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 16, 2023

I think this can definitely be fixed for /MT and /MTd - I've successfully tried the way that forward-declares a dummy variable and then let the linker alias it with Concurrency::details::stl_critical_section_win7's vtable. The address is a constant expression and works for constexpr constructor.

But it didn't work for /MD or /MDd to merely do so. Perhaps we need to export the vtable.

Edit: I see. I'll create a fake vtable when _DLL is defined.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 17, 2023

Possible approches (IMO) for /MD & /MDd:

  1. (suggested by @AlexGuteniev on Discord) Move Concurrency::details::stl_critical_section_win7's vtable to an import lib. I don't know how to do this, should we move _Mtx_init_in_situ to import lib?
  2. Additionally export ??_7stl_critical_section_win7@details@Concurrency@@6B@ (the mangled name of Concurrency::details::stl_critical_section_win7's vtable) for DLL. Nope. This seems ABI breaking.
  3. Use a fake vtable for static initialization, and use _Mtx_init_in_situ for dynamic initialization. Presumably all statically initialized mutexes that are created within a user DLL are destroyed when the DLL is unloaded. I think I've made a successful attempt by "scary" emulation with __fastcall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants