mutex.cpp, cond.cpp: Use static dispatch

[cpplearner]

After #2317 removed stl_critical_section_vista and stl_condition_variable_vista, only the Win7 versions inherit from the corresponding interfaces. Thus the functions in mutex.cpp and cond.cpp could directly use the Win7 classes, without going through the interfaces. This improves performance, and makes it easier to implement constexpr mutex.

The first commit (24922f3) changes the getters in _Mtx_internal_imp_t and _Cnd_internal_imp_t to return the Win7 classes. Since these classes are final, apparently the compiler knows to use static dispatch for them.

Subsequent commits change create_stl_critical_section and create_stl_condition_variable to create non-polymorphic classes, with the "vptr" set to nullptr.

I can imagine that these cause trouble if a mutex object is created by new code and used by old code, and old code uses the old function definitions (because it links to an old static lib?). IIUC this scenario is unsupported, and it will be a noticeable breakage due to null dereference.

[frederick-vs-ja]

Should we still declare the native_handle_type even if native_handle is not usable? I guess this is technically conforming, but looks weird.

[StephanTLavavej]

Yeah, it's an open question.

[CaseyCarter]

[thread.req.native]/1:

Several classes described in this Clause have members native_handle_type and native_handle. The presence of these members and their semantics is implementation-defined."

We can do whatever we like here, if we document it. I'd prefer for native_handle_type to be undefined or denote void, but we can debate it after merging this change.

Whatever we do, we need to update https://learn.microsoft.com/en-us/cpp/standard-library/mutex-class-stl?view=msvc-170#native_handle.

[AlexGuteniev]

makes it easier to implement constexpr mutex.

I don't see it.
We need to support old DLL version, which will want vtable.

(I previously attempted a PR with fake vtable instead, that was rejected)

[cpplearner]

makes it easier to implement constexpr mutex.

I don't see it. We need to support old DLL version, which will want vtable.

(I previously attempted a PR with fake vtable instead, that was rejected)

Yes, new code will not work with old DLL/LIB, but IIUC that's unsupported anyway.

[AlexGuteniev]

Yes, new code will not work with old DLL/LIB, but IIUC that's unsupported anyway.

I believe it is unsupported for the LIB, but for the DLL it is supported due to "printer driver problem"

[strega-nil-ms]

makes it easier to implement constexpr mutex.

I don't see it. We need to support old DLL version, which will want vtable.

(I previously attempted a PR with fake vtable instead, that was rejected)

@AlexGuteniev makes a very good point. If we're working towards constexpr mutex, we would need to allow the user code to create a stl_critical_section_win7 with the correct vtable, rather than no vtable. It should theoretically be constexpr to do this if we declare stl_critical_section_win7 in a public header (although suitably uglified).

[AlexGuteniev]

It should theoretically be constexpr to do this if we declare stl_critical_section_win7 in a public header (although suitably uglified).

Let's work towards this then!

And then we won't even need manual devirtualization as in this PR: due to final keyword the compiler would devirtualize calls.

https://godbolt.org/z/6PM87qvYP

[AlexGuteniev]

It should theoretically be constexpr to do this if we declare stl_critical_section_win7 in a public header (although suitably uglified).

Let's work towards this then!

And then we won't even need manual devirtualization as in this PR: due to final keyword the compiler would devirtualize calls.

https://godbolt.org/z/6PM87qvYP

Oh, forgot about [thread.mutex.class]/3:

[...] It is a standard-layout class ([class.prop]).

It can't be made constexpr I thnk.

[strega-nil-ms]

@AlexGuteniev

ah. That is unfortunate.

edit: given that we never call typeid, we could theoretically just create a vtable manually :>

[StephanTLavavej]

Actually, I believe I have convinced myself that we don't even need to retain the void* unused null placeholder for the vptr. The headers don't actually care how much of the aligned storage space is used, and (aside from native_handle() which I assert no user could feasibly have been calling), nothing cares about the internal layout.

I was led to this by thinking about whether stl_condition_variable_win7 needed the vptr, given that condition variables have no mirrored _Count to worry about. Then I realized that _Mtx_internal_imp_mirror / _Mtx_internal_imp_t separate the aligned storage (where the stl_critical_section_win7 lives) from the _Count / count.

To repeat/clarify, because ABI issues are unfamiliar to reason about, there is only one copy of the STL's DLL or static LIB that we ever need to worry about. (It is possible for multiple user DLLs to be loaded into a single process with separate STL DLLs or statically linked LIBs, but they cannot communicate CRT/STL objects between themselves.) So, the STL's DLL/LIB is always internally consistent and we don't need to worry about mix-and-match the way that we do for headers built by users. The flat C API (with the _Count and native_handle() "punch through" exceptions as noted) ensures that the user-built headers are insulated from what's happening in the STL's DLL/LIB. As long as native_handle() is unused, and _Count at the mirrored layer outside the aligned storage appears at the expected location, we are fine.

I will push a change. If anyone believes that my analysis is incorrect, I welcome corrections, but please check with me before beginning an "edit war" of commits. 😺

[StephanTLavavej]

One more thought: for a constexpr mutex followup PR, we could have an escape hatch, so that "print driver" code could give up constexpr mutex and continue calling the separately compiled initialization functions. That would be my preferred path forward, since it would conform by default for the vast majority of code that doesn't care (even among code that takes advantage of bincompat guarantees).

[AlexGuteniev]

I am also not concerned about the dreaded "print driver" scenario that we may or may not actually need to support (it is unclear whether it is actually guaranteed to work according to our documented restrictions on MSDN). Because this isn't adding new separately compiled APIs (which is the sort of thing that would cause immediate failure on load), and instead would only be detectable upon use of native_handle(), I think it falls into the same "effectively unobservable" category.

If we don't call the constructor from DLL, and don't initialize vtable by other means, I'm afraid "print driver" scenario would be observable by crash on trying to lock the mutex.

[AlexGuteniev]

One more thought: for a constexpr mutex followup PR, we could have an escape hatch, so that "print driver" code could give up constexpr mutex and continue calling the separately compiled initialization functions.

My understanding that escape hatch is not good here, unlike, say, atomic alignment escape hatch.
Because the determination whether you affected by "print driver" problem or not is made not when creating app, but by user environment.
Even if technically not claimed supported, I think this scenario should be maximally supported, just because failure to support it would lead to random failures. And in this particular case, when export surface is intact, the failures are more subtle.

[StephanTLavavej]

I've pushed a revert, to restore the null vptrs (in advance of the weekly maintainer meeting). @strega-nil-ms noted that a guaranteed crash was safer and better than treating data as a vptr.

We will still need to talk about the print driver scenario for a followup constexpr mutex PR - I see your point, @AlexGuteniev, but maximally supporting that scenario is in conflict with many users who have expected and requested constexpr mutex conformance. If the scenario does exist in the wild, and people are using mutex in it, I don't think the failures will be "random" (like race conditions); they would happen immediately upon attempting to use the mutex, as @strega-nil-ms has envisioned by restoring the null vptr.

[CaseyCarter]

From Discord discussion, STL plans to address my comments, so I'll go ahead and approve.

[CaseyCarter]

[thread.req.native]/1:

Several classes described in this Clause have members native_handle_type and native_handle. The presence of these members and their semantics is implementation-defined."

We can do whatever we like here, if we document it. I'd prefer for native_handle_type to be undefined or denote void, but we can debate it after merging this change.

Whatever we do, we need to update https://learn.microsoft.com/en-us/cpp/standard-library/mutex-class-stl?view=msvc-170#native_handle.

[StephanTLavavej]

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

[StephanTLavavej]

Thanks for this major and long-awaited simplification! 🎉 😻 ✨