revive the Windows port #344

compnerd · 2018-02-22T01:51:35Z

The DISPATCH_EXPORT macro added an extraneous extern on the
declaration. This was pointed out by a newer clang. Adjust the
declaration accordingly.

Add build rules to improve Windows builds. These additions help
cross-compile to Windows with both clang-cl as well as the GNU clang
driver.

Use the newer Windows APIs to detect the windows CPU state. This allows
us to collect all of the CPU configuration information to properly
affinitise work to the preferred CPU.

Use the FLS API to create thread local data storage for libdispatch's
queues.

compnerd · 2018-02-22T01:51:53Z

CC: @das @MadCoder @jrose-apple @parkera

compnerd · 2018-02-22T01:54:35Z

This is a partial patch, it still needs more work. But, it sets up a lot of the work that is needed. It also fixes cross-compilation on Linux to Windows. The assumption here is that you are using clang-cl or clang to build libdispatch. I figure that putting this up here earlier is better as it allows others to take a look as well as possibly solve some of the remaining issues.

MadCoder · 2018-02-22T01:59:03Z

src/shims/tsd.h

+	void *dispatch_deferred_items_key;
+};
+
+extern __declspec(thread) struct dispatch_tsd __dispatch_tsd;


can't you try to reuse DISPATCH_USE_THREAD_LOCAL_STORAGE it's supposed to be the same trick, no? instead of duplicating the whole struct

Sure, its actually the same exact structure too ... just didn't move things around to make it easier. I can do that.

MadCoder · 2018-02-22T02:00:00Z

I have to look at the patch more, and it's more than I can do in a timely fashion, but I've seen nothing shocking in there overall.

jrose-apple

I'm an expert in neither Windows nor GCD, so I can't really comment. Nothing jumped out at me as wrong, but I also didn't understand a lot of it, like which things need to be marked stdcall and which don't.

jrose-apple · 2018-02-22T17:05:38Z

os/generic_win_base.h

+#define MAX(a,b) (((a)>(b))?(a):(b))
+// Unicies provide `MIN` via sys/param.h
+#define MIN(a,b) (((a)<(b))?(a):(b))
+// Unicies provide `howmany` via sys/param.h


Nitpick: inconsistent between Unices and Unicies.

MadCoder · 2018-02-27T07:18:54Z

src/block.cpp

@@ -69,7 +69,8 @@ struct dispatch_block_private_data_s {
 	{
 		// copy constructor, create copy with retained references
 		if (dbpd_voucher) voucher_retain(dbpd_voucher);
-		if (o.dbpd_block) dbpd_block = _dispatch_Block_copy(o.dbpd_block);
+		if (o.dbpd_block) dbpd_block =


please use braces

MadCoder · 2018-02-27T07:22:28Z

private/private.h

@@ -32,7 +32,9 @@
 #include <os/availability.h>
 #include <TargetConditionals.h>
 #include <os/base.h>
-#elif defined(__linux__) || defined(__FreeBSD__)
+#elif defined(_WIN32)


this should use TARGET_OS_WIN32 to be consistent with the rest of the tests I think

MadCoder · 2018-02-27T07:23:19Z

src/introspection.c

@@ -137,7 +137,7 @@ _dispatch_introspection_thread_add(void)
 	_dispatch_unfair_lock_unlock(&_dispatch_introspection.threads_lock);
 }

-static void
+static __stdcall void


is __stdcall defined by clang on darwin or linux?

I get

<stdin>:1:6: warning: calling convention '__stdcall' ignored for this target [-Wignored-attributes] void __stdcall test(); ^

So yeah, it might be something you want to guard.

Sure, I can control this by a macro

compnerd · 2018-03-02T01:35:56Z

Updated to address the comments. This is further along now, with a lot more of the port formalized.

MadCoder

the review makes perfect sense to me, however I see a problem, where do you get TARGET_OS_WIN32 from? is there someone that defines it that I missed? os/object uses _WIN32 as a guard but not the rest.

MadCoder · 2018-03-09T02:47:47Z

src/internal.h


-#if __GNUC__
+#if __GNUC__ || defined(__clang__)


#if defined(__GNUC__) || defined(__clang__) would look better ;)

compnerd · 2018-03-12T16:28:30Z

Oh, TARGET_OS_WIN32 comes from the build system. If there is a better way, Id love to know about that. https://github.com/apple/swift-corelibs-libdispatch/blob/master/CMakeLists.txt#L266

Good call on the style in src/internal.h.

compnerd · 2018-03-12T16:34:57Z

There is a small amount of porting left: src/io.c (this previously was completely disabled).

MadCoder · 2018-03-12T17:07:32Z

if TARGET_OS_WIN32 comes from the build system you can't have it in private.h or any header, you should use _WIN32 there, and then it is a question whether we need TARGET_OS_WIN32 at all, it seems confusing. TARGET_OS* comes from <TargetConditionals.h> which is a thing that Apple has when we build things on Windows platforms that's why it's this way. I don't think it should stay this way.

have you ported all of event/event.c too? ;) because that part is the one that needs basically full reimplementation, unless windows supports kevent :P (not that it should block anything, but between having dispatch sources working and io.c I know what should be done first :P)

MadCoder · 2018-03-12T17:08:30Z

so please get rid of TARGET_OS_WIN32 consistently, we use __linux__ and __freebsd__ and such for other platforms, there's no reason for windows to be in between IMO. Once you've done that I'll merge.

compnerd · 2018-03-12T17:31:24Z

Hah, kevent.c is huge (and as you pointed out, will require a full re-implementaiton), and I think that doing that as a separate change is best -- this is a pretty big change as is. While I agree that kevent.c is crucial, having this part means that we can at least ensure that build regressions don't occur while that is ported over. Replacing TARGET_OS_WIN32 with defined(_WIN32) sounds completely fine to me.

MadCoder · 2018-03-12T17:38:34Z

as far as kevent.c is, it's huge, but you should instead model it after event_epoll.c which is way smaller, I have abstracted platform specific code this way way more than it used to be.

compnerd · 2018-03-12T23:07:46Z

So, the build should be complete at this point, the top patch is the only thing which I don't know how to formalize. But, the entire build now completes successfully :-)

MadCoder · 2018-03-14T20:11:32Z

src/object_internal.h

@@ -340,7 +340,7 @@ DISPATCH_ENUM(dispatch_invoke_flags, uint32_t,
 #define _DISPATCH_INVOKE_AUTORELEASE_MASK	  0x00300000u
 );

-enum {
+enum : unsigned long {


this syntax is not supported by C mode on clang I think, can you use DISPATCH_ENUM() instead?

Silence the enum assignment warning from clang. It is triggered by use of constants in macros that are not part of an enumeration when the parameter is of an enumerated type. This shows up when building for Windows. Rather than clutter the codebase with compiler specific macros to silence the warning, disable it.

The `DISPATCH_EXPORT` macro added an extraneous `extern` on the declaration. This was pointed out by a newer clang. Adjust the declaration accordingly. Add build rules to improve Windows builds. These additions help cross-compile to Windows with both clang-cl as well as the GNU clang driver. Use the newer Windows APIs to detect the windows CPU state. This allows us to collect all of the CPU configuration information to properly affinitise work to the preferred CPU. Use the FLS API to create thread local data storage for libdispatch's queues.

Windows uses signed enumerations by default rather than unsigned. This results in the values triggering warnings on Windows.

MadCoder · 2018-03-19T01:28:12Z

@swift-ci please test

revive the Windows port Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

MadCoder reviewed Feb 22, 2018

View reviewed changes

MadCoder self-assigned this Feb 22, 2018

compnerd force-pushed the windows branch from 236649a to 1ad1ab4 Compare February 22, 2018 03:15

jrose-apple reviewed Feb 22, 2018

View reviewed changes

MadCoder suggested changes Feb 27, 2018

View reviewed changes

compnerd force-pushed the windows branch from 1ad1ab4 to d6ccfaf Compare March 2, 2018 01:34

MadCoder reviewed Mar 9, 2018

View reviewed changes

compnerd force-pushed the windows branch from d6ccfaf to d5a542c Compare March 12, 2018 23:07

MadCoder suggested changes Mar 14, 2018

View reviewed changes

compnerd added 3 commits March 18, 2018 17:07

unformalized windows patches for enumerations

6fc5d79

Windows uses signed enumerations by default rather than unsigned. This results in the values triggering warnings on Windows.

compnerd force-pushed the windows branch from d5a542c to 6fc5d79 Compare March 19, 2018 00:14

MadCoder approved these changes Mar 19, 2018

View reviewed changes

MadCoder merged commit 8d3aa22 into swiftlang:master Mar 19, 2018

MadCoder added the darwin pending label Mar 19, 2018

compnerd deleted the windows branch March 19, 2018 21:39

ktopley-apple pushed a commit that referenced this pull request Dec 6, 2018

Merge pull request #344 from compnerd/windows

dab5a65

revive the Windows port Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

ktopley-apple removed the darwin pending label Apr 5, 2019

revive the Windows port #344

revive the Windows port #344

Uh oh!

Conversation

compnerd commented Feb 22, 2018

Uh oh!

compnerd commented Feb 22, 2018

Uh oh!

compnerd commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadCoder commented Feb 22, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Mar 2, 2018

Uh oh!

MadCoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Mar 12, 2018

Uh oh!

compnerd commented Mar 12, 2018

Uh oh!

MadCoder commented Mar 12, 2018

Uh oh!

MadCoder commented Mar 12, 2018

Uh oh!

compnerd commented Mar 12, 2018

Uh oh!

MadCoder commented Mar 12, 2018

Uh oh!

compnerd commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadCoder commented Mar 19, 2018

Uh oh!

Uh oh!