stdlib/public fixes for Windows support #11110

troughton · 2017-07-21T23:33:34Z

This pull request contains a few changes to allow the stdlib to compile against the Windows/MSVC toolchain.

Of particular note:

There was a compile issue with the use of __cxa_demangle in Errors.cpp as that's unavailable on Windows. The containing method is actually not used on Windows, since it's conditional on SWIFT_SUPPORTS_BACKTRACE_REPORTING; however, I thought it better to provide an implementation for when Windows eventually does support that.
I'm using the Windows API for thread-local storage, but it's still implemented under the pthread_create method; the functionality should be the same, but a higher level of abstraction might be appropriate. The other caveat with this implementation is that TlsAlloc doesn't support a destructor; from what I can gather, it takes care of disposing its own memory, but if I'm incorrect in that or misunderstanding the usage this could cause a memory leak.

dabrahams · 2017-07-22T17:07:14Z

@milseman This touches TLS, so you should probably have a look

compnerd · 2017-07-22T18:10:52Z

libc++ at least has a complete abstraction over the threading model (it uses Win32 threading on Windows, and pthreads on Linux/Darwin). It would be nice if we could use that instead in the LibcShims.cpp if the TLS data is not exposed to others through the pthread_key_t.

troughton · 2017-07-22T22:15:47Z

From what I can tell, the only usage of those methods is in ThreadLocalStorage.swift, which uses it to cache the result of __swift_stdlib_ubrk_open per-thread. I don't think there's anything that's specifically tied to a pthread implementation, so if the consensus is that an abstraction over the libc++ implementation should be used (perhaps with names like _swift_stdlib_tls_setvalue or _swift_stdlib_tls_setspecific rather than _swift_stdlib_pthread_setspecific) then I can have a go at refactoring that.

milseman · 2017-08-14T22:21:45Z

Right, this key is for the standard library internal use only. I think that on Darwin, we will still want to call pthread for now (see https://bugs.swift.org/browse/SR-4915, CC @gparker42 and @atrick). Otherwise, I think @troughton's renaming the shims is a good idea to reflect that it's not literally pthread on all platforms anymore.

milseman

Rename _swift_stdlib_pthread_[get|set]specific to _swift_stdlib_tls_[get|set]specific and this looks great to me!

troughton · 2017-08-15T02:57:14Z

I've done the renaming. I also modified my original change to use the Fls* rather than Tls* functions on Windows, which should do the same thing (as per the comment in LibcShims.cpp) and ensures the destructor is correctly called.

dabrahams · 2017-08-15T03:20:09Z

The suggestion from @compnerd seems like an excellent one and should not be ignored. Using the C++ standard library as a portability layer, where applicable, is a technique we want to employ elsewhere, e.g. random number generation.

milseman · 2017-08-15T20:09:31Z

@dabrahams I am not ignoring his suggestions. I agree with you in general, but there are specific (pun intended!) concerns surrounding project policy and especially surrounding Darwin's implementation of thread local storage. (Also, as this PR was originally written, the semantics were potentially different, but I'm completely unfamiliar with "fibers" on Windows. @troughton's latest commit makes this a moot point.)

@compnerd, are you referring specifically to __libcpp_tls_get/set? If so, that addresses performance concerns at it results in the exact same pthread calls. But, using that would require a dependency on libc++ internals, which may not be something we can always assume (cc @dexonsmith). On the other hand, __threading_support is present in Xcode distributions, so I don't have a strong opinion on this. Do you know of any other places where we pretty much assume libc++ specifically is used and available to build the project?

Another approach would be to copy the definitions (or even the file itself) into the Swift project. I don't think that should block this PR, but if anyone thinks that's worth pursuing, please file an SR.

milseman

LGTM

milseman · 2017-08-15T22:07:56Z

@swift-ci please smoke test

compnerd · 2017-08-16T01:29:04Z

@milseman yeah, I was referring to the __threading_support. We could pull just that implementation from libc++. However, the thing is that those are what back the std::thread implementation. I don't expect that the overhead from the class on top of them would make any substantial performance difference. But, using the std::thread routines completely makes swift agnostic to the underlying threading semantics, which seems like a positive to me (in terms of portability).

compnerd · 2017-08-16T01:51:11Z

stdlib/public/runtime/Errors.cpp

+
+#if defined(_WIN32)
+    char demangled[1024];
+    DWORD length = UnDecorateSymbolName(syminfo.symbolName, demangled, 1024, 0);


Using sizeof(demanglled) / sizeof(*demangled) or defining a local array_size as follows would be nicer:

template <typename T, size_t N> size_t array_size(const T (&)[N]) { return N; }

compnerd · 2017-08-16T01:52:02Z

stdlib/public/stubs/LibcShims.cpp

@@ -15,10 +15,11 @@
 #include <cmath>
 #if defined(_WIN32)
 #include <io.h>
+#include <windows.h>


Probably a good idea to add:

#define WIN32_LEAN_AND_MEAN #define VC_EXTRA_LEAN #define NOMINMAX

before the windows.h include to avoid a ton of Windows.h cruft. It particularly helps clang.

compnerd · 2017-08-16T01:52:43Z

stdlib/public/stubs/LibcShims.cpp

+    // on Windows, "If no fiber switching occurs, FLS acts exactly the same 
+    // as thread local storage," and FlsAlloc takes a destructor while
+    // TlsAlloc doesn't.
+    // https://msdn.microsoft.com/en-us/library/windows/desktop/ms682661.aspx


This also makes the code more portable, as some Windows targets (e.g. Windows Store, Windows Phone) only support the FLS variant.

LuizZak · 2017-08-16T11:19:47Z

stdlib/public/stubs/LibcShims.cpp

@@ -15,6 +15,9 @@
 #include <cmath>
 #if defined(_WIN32)
 #include <io.h>
+#define WIN32_LEAN_AND_MEAN
+#define VC_EXTRA_LEAN


Mispelled VC_EXTRALEAN I think? ref

Fixed - thanks for the catch.

milseman · 2017-08-16T17:56:27Z

@swift-ci please smoke test

milseman · 2017-08-16T21:10:48Z

@compnerd, there are two constraints here. First, we want to manage the TLS allocation, initialization, and deinitialization on the Swift side for the stdlib. The second is that after ABI stability, we definitely want to be making pthread calls on Darwin for highly-Darwin-specific reasons.

Currently, the TLS only caches an ICU pointer, but we want the ability to add pure-Swift constructs onto it in the future. If std::thread can be used to implement some kind of tls_get|set, then I'm totally on board with that for all non-Darwin platforms.

troughton · 2017-09-06T10:02:23Z

I've rebased this onto master; the only modification is removing what I'd changed in UnicodeNormalization.cpp.

Is this pull request ready to be merged? I know there was some discussion about possibly moving to std::thread for non-Darwin, but I'd consider that slightly out of scope for this PR and best suited to a separate refactor. If preferred, I could remove the TLS additions for Windows and the rest of this could be merged in.

milseman · 2017-09-06T18:37:35Z

@swift-ci please smoke test

milseman · 2017-09-06T18:38:18Z

@troughton sorry, if tests pass I will merge.

CodaFi · 2017-09-19T14:06:59Z

This now requires rebasing

milseman · 2017-09-19T17:52:13Z

I'm sorry, I thought this got merged. @compnerd, you've been generalizing this area as well. Does this PR overlap with your work? I'm going to assign to you to help steward this through.

edit: Please feel free to assign to me if you're no longer working in this area.

troughton · 2017-09-19T20:37:09Z

I think everything here has now been covered by @compnerd’s separate PRs (#11949 and #11976), so I’m going to close this.

stdlib/public fixes for Windows support

9d4a932

dabrahams requested a review from milseman July 22, 2017 17:06

milseman requested changes Aug 14, 2017

View reviewed changes

Rename _swift_stdlib_pthread to _swift_stdlib_tls

02d822e

milseman approved these changes Aug 15, 2017

View reviewed changes

compnerd reviewed Aug 16, 2017

View reviewed changes

Add WIN32_LEAN_AND_MEAN et al.

4a379e0

LuizZak reviewed Aug 16, 2017

View reviewed changes

Fix VC_EXTRALEAN

627b42d

troughton added 2 commits September 6, 2017 21:53

Merge branch 'master' into windows-fixes

13e91e1

Update UnicodeNormalization.cpp

9f0ac24

milseman assigned compnerd Sep 19, 2017

troughton closed this Sep 19, 2017

stdlib/public fixes for Windows support #11110

stdlib/public fixes for Windows support #11110

Uh oh!

Conversation

troughton commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dabrahams commented Jul 22, 2017

Uh oh!

compnerd commented Jul 22, 2017

Uh oh!

troughton commented Jul 22, 2017

Uh oh!

milseman commented Aug 14, 2017

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

troughton commented Aug 15, 2017

Uh oh!

dabrahams commented Aug 15, 2017

Uh oh!

milseman commented Aug 15, 2017

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

milseman commented Aug 15, 2017

Uh oh!

compnerd commented Aug 16, 2017

Uh oh!

compnerd Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

LuizZak Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

troughton Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

milseman commented Aug 16, 2017

Uh oh!

milseman commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troughton commented Sep 6, 2017

Uh oh!

milseman commented Sep 6, 2017

Uh oh!

milseman commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Sep 19, 2017

Uh oh!

milseman commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troughton commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

troughton commented Jul 21, 2017 •

edited

Loading

milseman commented Aug 16, 2017 •

edited

Loading

milseman commented Sep 6, 2017 •

edited

Loading

milseman commented Sep 19, 2017 •

edited

Loading

troughton commented Sep 19, 2017 •

edited

Loading