Skip to content

Fix runloop integration for libdispatch from swift #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 6, 2019

Conversation

ngrewe
Copy link
Member

@ngrewe ngrewe commented Nov 26, 2019

It seems like the runloop integration for the libdispatch main queue broke down a bit – unfortunately in a quite symmetrical way so that both the implementation and the tests would never be built, even if a compatible version of libdispatch existed (it was checking for HAVE_LIBDISPATCH_RUNLOOP where it should have been using GS_USE_LIBDISPATCH_RUNLOOP).

That uncovered a few problems with libdispatch from swift-corelibs. Our side of that is fixed here (simple typos, mostly), the libdispatch side of things is addressed in swiftlang/swift-corelibs-libdispatch#534.

I've also freshened up the Travis CI job a bit in the process so that this is continuously tested again.

@ngrewe ngrewe requested a review from rfm as a code owner November 26, 2019 13:17
@triplef
Copy link
Member

triplef commented Nov 26, 2019

The initial integration of the Swift libdispatch version was my work in #41, but it sounds like while it worked for us in the context of the Android toolchain it wasn’t quite up to par. Thank you for fixing it and especially also for the CI integration! Unlike the other libdispatch versions, the Swift one is being quite actively maintained, so this is great to see.

I was wondering if you could clarify swiftlang/swift-corelibs-libdispatch#534 when using the libobjc2 runtime. We’ve been using libBlocksRuntime from libdispatch, and as far as I can see libobjc2 does not include its own libBlocksRuntime. Where is the custom libBlocksRuntime defined and how does it differ from the one in libdispatch?

The one thing we noticed about ObjC interoperability was that libdispatch objects such as queues are not declared as ObjC types, so they e.g. can’t partake in ARC (OS_OBJECT_HAVE_OBJC_SUPPORT for us is 0).

@ngrewe
Copy link
Member Author

ngrewe commented Nov 26, 2019

far as I can see libobjc2 does not include its own libBlocksRuntime.

libobjc2 is the blocks runtime (even the first one, as David likes to say, since the relevant libobjc2 release was earlier than Apple managed to ship theirs, IIRC). So you just link libobjc2 and already get the runtime hooks required by the blocks ABI.

how does it differ from the one in libdispatch

Since blocks are also Objective-C objects they need some level of interoperability with Objective-C. The BlocksRuntime in libdispatch only interoperates with the Apple flavour of Objective-C (or not at all, unless you're building on Darwin, I suppose). The blocks runtime in libobjc2 (obviously) supports the GNUstep ABI.

The one thing we noticed about ObjC interoperability was that libdispatch objects such as queues are not declared as ObjC types, so they e.g. can’t partake in ARC (OS_OBJECT_HAVE_OBJC_SUPPORT for us is 0).

That mostly controls whether dispatch queues etc. also behave like ObjC objects and whether dispatch_data_t is toll-free bridged to NSData (nice!).
Unfortunately, looking at the implementation set off a brief psychotic episode for me. It's very tightly coupled with Apple's Objective-C ABI. It does fun things like manually creating vtables for classes and placing symbols into specific sections of the binary. There's also mention of some linker magic being required… I haven't yet given much sort to how to untangle all that.

@triplef
Copy link
Member

triplef commented Nov 26, 2019

Thank you @ngrewe, that’s very helpful info.

Copy link
Member

@fredkiefer fredkiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

.travis.yml Outdated
@@ -1,5 +1,5 @@
language: cpp
dist: trusty
dist: xenial
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update even further?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, that makes sense. I was living in the past… and forgot that bionic is the newest Ubuntu LTS version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, that gives us some unrelated test failures for NSURL-related things 🤔. I guess I'll revert to xenial for the time being and investigate those independently....

@@ -8,14 +8,12 @@

const NSTimeInterval kDelay = 0.01;

#if HAVE_LIBDISPATCH_RUNLOOP && __has_feature(blocks)
#if GS_USE_LIBDISPATCH_RUNLOOP && __has_feature(blocks)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to keep this code here identical to the one in NSRunLoop.m?

Copy link
Member Author

@ngrewe ngrewe Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the implementation, we only need to call C functions from libdispatch, but in the test, we also need to schedule blocks onto the main queue to verify that running the runloop will drain the queue as well. Hence the subtle difference: The test code depends on a blocks capable compiler, the implementation code doesn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was rather aiming at the lines below, not that one by itself.

Copy link
Member Author

@ngrewe ngrewe Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it: The HAVE_DISPATCH_H defines etc. come from Source/config.h, so it doesn't seem like they were supposed to be exposed to the test framework (otherwise it should be in a Headers/ subdirectory -- probably one of the reasons why this code path broke down with time). And since this is effectively a clang only code path, it made sense to do it using __has_include() here.

This is very defensive anyways: I haven't seen a build of libdispatch that didn't place its headers in subdirectory for a long time.

@ngrewe ngrewe force-pushed the swift-corelibs-libdispatch branch from f2f6a1f to 4550779 Compare November 26, 2019 21:28
@ngrewe ngrewe force-pushed the swift-corelibs-libdispatch branch from 4550779 to ecd2d85 Compare November 28, 2019 18:13
@fredkiefer
Copy link
Member

I just merged your other pull request. If you update this one to the new master, I will merge it as well.

@ngrewe ngrewe merged commit c77f40f into master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants