Skip to content

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Jul 3, 2019

Unfortunately, std::atomic in msvcprt as of 14.21.27702 is broken for
double-width atomics on ARM64. This has been reported to Microsoft and
is going to be fixed in VC++ 2019u3. For the time being, add a partial
template specialisation for the two double-word sized types temporarily
as a workaround. This allows the standard library build to get further.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

Unfortunately, `std::atomic` in msvcprt as of 14.21.27702 is broken for
double-width atomics on ARM64.  This has been reported to Microsoft and
is going to be fixed in VC++ 2019u3.  For the time being, add a partial
template specialisation for the two double-word sized types temporarily
as a workaround.  This allows the standard library build to get further.
@compnerd
Copy link
Member Author

compnerd commented Jul 3, 2019

CC: @jckarter @mikeash

@compnerd
Copy link
Member Author

compnerd commented Jul 3, 2019

@swift-ci please smoke test

@mikeash
Copy link
Contributor

mikeash commented Jul 3, 2019

This is mildly terrifying but looks like the right solution until VC++ fixes it.

@compnerd
Copy link
Member Author

compnerd commented Jul 3, 2019

@mikeash - it seems that I have a tendency to come up with somewhat terrifying nuggets of code :-D

@mikeash
Copy link
Contributor

mikeash commented Jul 3, 2019

It's an interesting talent to have.

Just curious, does the broken implementation fail spectacularly or is it just silently non-atomic or what?

@compnerd
Copy link
Member Author

compnerd commented Jul 3, 2019

A bit of both really. So, the thing is that there is a compilation failure, but the true cause of the failure is obscured by the fact that SFINAE hides the fact that the there is a type mismatch on an intrinsic which causes _Unlock to fail to compile and thus get ignored - eventually, a reference to the _Unlock causes the compilation to fail as no definition could be synthesized.

This just works around it by providing the explicit template specialization which being a partial template specialization is given higher precedence and will suffice. The inline ensures that the definition is only emitted where used preventing the multiple definitions (as it is in a header).

@mikeash
Copy link
Contributor

mikeash commented Jul 3, 2019

Interesting, thanks for explaining and for the fix.

@jfbastien
Copy link
Contributor

The checks seem to capture non-MSVC compilers one ARM64 Windows. Does Swift care about e.g. clang-cl?

@compnerd
Copy link
Member Author

compnerd commented Jul 3, 2019

@jfbastien yes, Swift cares about clang-cl as that is the compiler used to build Swift :-). This is working around msvcprt, not a cl issue.

@jckarter
Copy link
Contributor

jckarter commented Jul 3, 2019

@jfbastien Only clang (and specifically Swift's branch of clang) can build the runtime.

@compnerd compnerd merged commit 8221c67 into swiftlang:master Jul 3, 2019
@compnerd compnerd deleted the nuclear-atomic-hammer branch July 3, 2019 18:27
@jfbastien
Copy link
Contributor

OK, maybe I'm confused... This:

#if defined(_WIN32) && defined(_M_ARM64)

Looks like it'll trigger on cl as well as clang-cl, and that seems wrong since you only want it to trigger on cl?

@jckarter
Copy link
Contributor

jckarter commented Jul 4, 2019

We're building with clang-cl, which still uses Microsoft's STL headers and C++ runtime, AIUI.

@jfbastien
Copy link
Contributor

Ah OK, so this is working around an STL header issue, which is why #if defined(_WIN32) && defined(_M_ARM64) is required.

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

Successfully merging this pull request may close these issues.

4 participants