Enable CF runloop support for linux #101

dgrove-oss · 2016-07-07T21:56:00Z

Here's a first cut at this. It compiles, but I haven't actually tested it with foundation yet. Implementation mostly based on the code from experimental/foundation, but I decided to more closely mirror the mach code and stash the eventfd in dq->do_ctxt instead of introducing a static variable just for the main queue.

I'm not that happy with the HANDLE (windows) vs. int (linux) aspect of the patch. I partially introduced dispatch_handle_t, but I also should introduce a symbolic constant for 0/NULL and be more careful in the linux-specific code to always use dispatch_handle_t instead of int. Do we want dispatch_handle_t or similar? On as similar note, we could define dispatch_handle_t to be mach_port_t if TARGET_OS_MAC and if we could change the name of _dispatch_get_main_queue_port_4CF to _dispatch_get_main_queue_handle_4CF we could avoid duplicating code.

Also, I used TARGET_OS_MAC in queue.c instead of HAVE_MACH. This is different than other places in queue.c where we guard mach specific code. Is this correct?

MadCoder · 2016-07-07T22:01:51Z

src/queue.c

+#elif defined(__linux__)
+	int efd = (int)dq->do_ctxt;
+
+	if (efd != 0) {


0 is a valid fd, and you will definitely get it if you close STDIN which lots of daemons do, which makes it a bit of an issue. A very gross way to fix that is to store (eventfd + 1) in here

MadCoder · 2016-07-07T22:09:42Z

I don't think we care much about windows, the port is clearly not working, and I would just get rid of that.

Edit: I don't think we care much about trying to maintain that non working windows code and are better off ripping it out so that it gets reported cleanly.

maybe the right thing to do is to have a dispatch_runloop_handle_t rather than dispatch_handle_t which is a bit too generic that abstracts port & fd on linux.

and internally I would have a:

DISPATCH_ALWAYS_INLINE
static inline bool
_dispatch_runloop_handle_is_valid(dispatch_runloop_handle_t handle)
{
#if TARGET_OS_MAC
    return MACH_PORT_VALID(handle);
#elif defined(__linux__)
    return handle >= 0
#else
#error
#endif
}

DISPATCH_ALWAYS_INLINE
static inline dispatch_runloop_handle_t
_dispatch_runloop_queue_get_handle(dispatch_queue_t dq)
{
#if TARGET_OS_MAC
    return ((dispatch_runloop_handle_t)(uintptr_t)dq->dq_ctxt);
#elif defined(__linux__)
    return ((dispatch_runloop_handle_t)(uintptr_t)dq->dq_ctxt) - 1;
#else
#error
#endif
}

DISPATCH_ALWAYS_INLINE
static inline void
_dispatch_runloop_queue_set_handle(dispatch_queue_t dq, dispatch_runloop_handle_t handle)
{
#if TARGET_OS_MAC
    dq->do_ctxt = (void *)(uintptr_t)handle;
#elif defined(__linux__)
    dq->do_ctxt = (void *)(uintptr_t)(handle + 1);
#else
#error
#endif
}

That way the actual mangling of the handle is abstracted away and should allow you to write clean code whether it's a mach port, a file descriptor, a HANDLE, or maybe even something else in the future.

I think you need the +1/-1 trick for fd's because do_ctxt is initialized to 0 where you would want it to be -1, and that is easier to do that mangling above than to try to initialize runloop queues differently.

do you agree?

MadCoder · 2016-07-07T22:10:53Z

src/queue.c

@@ -23,6 +23,10 @@
 #include "protocol.h"
 #endif

+#if defined(__linux__)


this kind of #include should be in src/internal.h with other dependencies that dispatch needs for most of its source.

dgrove-oss · 2016-07-07T22:16:35Z

Thanks for the quick review!
The suggestion for dispatch_runloop_handle_t and setters/getters makes sense to me. I'll update the patch later tonight.

parkera · 2016-07-07T22:19:57Z

Not a blocker for this, but we will care about Windows in the long run - both because we actually do use dispatch on windows today, and also because Swift will get ported there soon enough (if it hasn't quite made it already).

MadCoder · 2016-07-07T22:33:50Z

@parkera I completely agree, what I meant is that @dgrove-oss was trying to maintain code that is not wroking at all. the _get_handle() function is actually a prototype only and has no corresponding function in queue.c, and in my experience, it's best to delete half-assed code so that porters aren't misled into thinking this may work.

I wasn't meaning that I don't care about windows, HTH.

EDIT: I updated my poorly worded comment above to clarify this.

parkera · 2016-07-08T00:01:15Z

Ok, sounds good.

MadCoder · 2016-07-08T02:25:56Z

private/private.h

+#elif defined(__linux__)
+typedef int dispatch_runloop_handle_t;
+#else
+#error


#error should have an argument, my example code was just the layout, but an actual error string is good :)

MadCoder · 2016-07-08T02:28:33Z

ok from a cursory glance this looks clean and nice, but I'm too fried to do a proper review now.
I'd also like @das to look at it

dgrove-oss · 2016-07-08T15:53:39Z

Squashed to a single commit.

Confirmed that combined with the changes in swiftlang/swift-corelibs-foundation#434 plus the temporary hack in #100, Foundation links with Dispatch support on Linux and testOperationCount in TestNSOperationQueue passes (however testOperationPriorities hangs). Did not investigate hang since I don't know if the Foundation side of the support is there for that test case yet.

Would be nice to enable some of the dispatch tests that require CoreFoundation. Would it make sense to try to reach into swift-corelibs-foundation from the dispatch build to find CoreFoundation to attempt to build them? Or do you want to defer to testing in the context of swift-corelibs-foundation?

parkera · 2016-07-08T15:54:22Z

I really want CF to remain a totally private implementation detail of Foundation.

parkera · 2016-07-08T15:55:08Z

Would it be possible to make these tests use Foundation instead? I guess that would require reimplementing them.

das · 2016-07-08T18:11:48Z

i'll take a look asap

MadCoder · 2016-07-08T18:15:08Z

LGTM, @das to rubber stamp too as it affects headers

das · 2016-07-12T02:11:37Z

private/private.h

@@ -166,31 +166,45 @@ void _dispatch_prohibit_transition_to_multithreaded(bool prohibit);
 * SPI for CoreFoundation/Foundation ONLY
 */

-#define DISPATCH_COCOA_COMPAT (TARGET_OS_MAC || TARGET_OS_WIN32)
+#if (TARGET_OS_MAC || TARGET_OS_WIN32)


I think we should remove the WIN32 here, it is just going to #error below anyway

das · 2016-07-12T02:25:26Z

src/queue.c

 _dispatch_main_queue_callback_4CF(mach_msg_header_t *msg DISPATCH_UNUSED)
+#else
+_dispatch_main_queue_callback_4CF(void)


this seems like an unnecessary difference if we made the argument void* and just pass in NULL (given that it is not used)

das · 2016-07-12T02:33:55Z

one bug and various small style/convention comments but lgtm otherwise, back to you

dgrove-oss · 2016-07-12T17:27:11Z

I'd already made changes to private/private.h based on my interpretation of @das comments. I can undo or do differently, just let me know what you'd like. Please be sure to look at the availability macro I put on _dispatch_get_main_queue_handle_4CF. Since this is a new function on MAC_OS, I think it should get these versions, but I'm only guessing.

Once we finalize this, I can update my pull request to foundation to match the final API.

parkera · 2016-07-12T17:30:47Z

Ok, we can deal with any lockstep changes later probably.

das · 2016-07-12T18:00:05Z

Thanks @dgrove-oss that looks exactly like what I had in mind. The new availability annotation should be __OSX_AVAILABLE(10.12) __IOS_AVAILABLE(10.0) __TVOS_AVAILABLE(10.0) __WATCHOS_AVAILABLE(3.0) I think
Given the above discussion, it would probably also be useful to add a #define DISPATCH_CF_SPI_VERSION 20160712 define above this section, that CF can use to switch to the new call without revlock.

das · 2016-07-12T18:02:00Z

private/private.h

-_dispatch_main_queue_callback_4CF(mach_msg_header_t *_Null_unspecified msg);
-#elif TARGET_OS_WIN32
-__OSX_AVAILABLE_STARTING(__MAC_10_6,__IPHONE_4_0)
+__OSX_AVAILABLE_STARTING(__MAC_10_12,__IPHONE_10_0)


__OSX_AVAILABLE(10.12) __IOS_AVAILABLE(10.0) __TVOS_AVAILABLE(10.0) __WATCHOS_AVAILABLE(3.0)

dgrove-oss · 2016-07-12T18:20:01Z

availability macros updated and DISPATCH_CF_SPI_VERSION #define added.

das · 2016-07-12T18:21:21Z

perfect, thanks

Enable CF runloop support for linux Signed-off-by: Daniel A. Steffen <[email protected]>

MadCoder reviewed Jul 7, 2016
View reviewed changes

MadCoder mentioned this pull request Jul 7, 2016

Add support for CFRunLoop integration APIs #46

Closed

MadCoder reviewed Jul 8, 2016
View reviewed changes

MadCoder added the under review label Jul 8, 2016

MadCoder assigned das and MadCoder Jul 8, 2016

dgrove-oss force-pushed the cf-runloop-hooks branch from 303e431 to 2e73a01 Compare July 8, 2016 15:45

dgrove-oss changed the title ~~first cut at updating CF runloop changes for linux~~ Enable CF runloop support for linux Jul 8, 2016

MadCoder removed their assignment Jul 8, 2016

das reviewed Jul 12, 2016
View reviewed changes

das assigned dgrove-oss and unassigned das Jul 12, 2016

dgrove-oss force-pushed the cf-runloop-hooks branch from 07384ca to 72f68c3 Compare July 12, 2016 17:48

das reviewed Jul 12, 2016
View reviewed changes

Enable CF runloop support for linux

0ad1c28

dgrove-oss force-pushed the cf-runloop-hooks branch from 72f68c3 to 0ad1c28 Compare July 12, 2016 18:18

das merged commit 2c0e5ee into swiftlang:master Jul 12, 2016

das added darwin pending and removed under review labels Jul 13, 2016

dgrove-oss deleted the cf-runloop-hooks branch July 21, 2016 14:42

MadCoder removed the darwin pending label Jul 30, 2016

das added a commit that referenced this pull request Aug 11, 2016

Merge pull request #101 from dgrove-oss/cf-runloop-hooks

56eb6b5

Enable CF runloop support for linux Signed-off-by: Daniel A. Steffen <[email protected]>

Enable CF runloop support for linux #101

Enable CF runloop support for linux #101

Uh oh!

Conversation

dgrove-oss commented Jul 7, 2016

Uh oh!

MadCoder Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

MadCoder commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MadCoder Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 7, 2016

Uh oh!

parkera commented Jul 7, 2016

Uh oh!

MadCoder commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parkera commented Jul 8, 2016

Uh oh!

MadCoder Jul 8, 2016

Choose a reason for hiding this comment

Uh oh!

MadCoder commented Jul 8, 2016

Uh oh!

dgrove-oss commented Jul 8, 2016

Uh oh!

parkera commented Jul 8, 2016

Uh oh!

parkera commented Jul 8, 2016

Uh oh!

das commented Jul 8, 2016

Uh oh!

MadCoder commented Jul 8, 2016

Uh oh!

das Jul 12, 2016

Choose a reason for hiding this comment

Uh oh!

das Jul 12, 2016

Choose a reason for hiding this comment

Uh oh!

das commented Jul 12, 2016

Uh oh!

dgrove-oss commented Jul 12, 2016

Uh oh!

parkera commented Jul 12, 2016

Uh oh!

das commented Jul 12, 2016

Uh oh!

das Jul 12, 2016

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 12, 2016

Uh oh!

das commented Jul 12, 2016

Uh oh!

Uh oh!

MadCoder commented Jul 7, 2016 •

edited

Loading

MadCoder commented Jul 7, 2016 •

edited

Loading