Skip to content

Conversation

@StephanTLavavej
Copy link
Member

Followup to #4294.

In theory, this #pragma detect_mismatch was a good thing. In practice, it causes ~200 directories to fail with linker errors in the Windows build. That's too many to feasibly deal with on short notice, even if we added an escape hatch, so we should simply drop this attempt to detect mismatch. (The change was originally tested in Windows in December without detect_mismatch.)

The net effect of #4294 and this PR is to make _Critical_section_size additionally sensitive to UNDOCKED_WINDOWS_UCRT. This should be a strict improvement, reducing mismatches between the headers and separately compiled code (we know for a fact that this was causing problems). If these ~200 directories are mixing TUs with varying settings of UNDOCKED_WINDOWS_UCRT, then depending on linker behavior that might prevent this fix from taking effect, but it shouldn't make things worse compared to the state before #4294.

If we continue to have problems in this area, we may need to consider removing the Windows special case for mutex entirely (according to my understanding, we could do that because Windows is always built fresh with its LKG toolset, so it doesn't actually care about ABI stability here). That would increase mutex size, of course, but we've ripped out the other inefficiencies (virtual calls etc.), so the potential performance impact would be less than it previously would have been.

@StephanTLavavej StephanTLavavej added bug Something isn't working high priority Important! labels Jan 8, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner January 8, 2024 20:17
@CaseyCarter
Copy link
Contributor

Roll a d20 for ODR violation.

@StephanTLavavej StephanTLavavej self-assigned this Jan 8, 2024
@StephanTLavavej
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working high priority Important!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants