Add support for building libdispatch on FreeBSD #319

Rogiel · 2017-10-24T18:07:20Z

This mostly adds defined(__FreeBSD__) conditionals where necessary. I am 99% sure this will break builds on Darwin, specially on kevent stuff. I tried to build the Xcode project, but there are a LOT of files missing which probably are internal Apple headers and sources.

Please note that the library is not yet completely working on FreeBSD. It compiles, few tests are working, but most are still failing.

Test project /home/rogiel/Swift/swift-corelibs-libdispatch/build
      Start  1: dispatch_apply
 1/21 Test  #1: dispatch_apply ...................   Passed    0.15 sec
      Start  2: dispatch_api
 2/21 Test  #2: dispatch_api .....................***Failed    0.13 sec
      Start  3: dispatch_debug
 3/21 Test  #3: dispatch_debug ...................   Passed    0.12 sec
      Start  4: dispatch_queue_finalizer
 4/21 Test  #4: dispatch_queue_finalizer .........***Timeout  30.27 sec
      Start  5: dispatch_group
 5/21 Test  #5: dispatch_group ...................***Failed    0.13 sec
      Start  6: dispatch_overcommit
 6/21 Test  #6: dispatch_overcommit ..............***Timeout  30.01 sec
      Start  7: dispatch_context_for_key
 7/21 Test  #7: dispatch_context_for_key .........***Failed    0.13 sec
      Start  8: dispatch_after
 8/21 Test  #8: dispatch_after ...................***Failed    0.13 sec
      Start  9: dispatch_timer
 9/21 Test  #9: dispatch_timer ...................***Timeout  30.01 sec
      Start 10: dispatch_timer_short
10/21 Test #10: dispatch_timer_short .............***Timeout  30.01 sec
      Start 11: dispatch_timer_timeout
11/21 Test #11: dispatch_timer_timeout ...........***Failed    6.15 sec
      Start 12: dispatch_sema
12/21 Test #12: dispatch_sema ....................   Passed    0.12 sec
      Start 13: dispatch_timer_bit31
13/21 Test #13: dispatch_timer_bit31 .............***Timeout  30.01 sec
      Start 14: dispatch_timer_bit63
14/21 Test #14: dispatch_timer_bit63 .............***Timeout  30.02 sec
      Start 15: dispatch_timer_set_time
15/21 Test #15: dispatch_timer_set_time ..........***Timeout  30.02 sec
      Start 16: dispatch_starfish
16/21 Test #16: dispatch_starfish ................***Timeout  30.01 sec
      Start 17: dispatch_data
17/21 Test #17: dispatch_data ....................***Failed    0.13 sec
      Start 18: dispatch_io_net
18/21 Test #18: dispatch_io_net ..................***Failed   25.30 sec
      Start 19: dispatch_select
19/21 Test #19: dispatch_select ..................***Failed    0.12 sec
      Start 20: dispatch_c99
20/21 Test #20: dispatch_c99 .....................***Failed    0.13 sec
      Start 21: dispatch_plusplus
21/21 Test #21: dispatch_plusplus ................***Failed    0.13 sec

Rogiel · 2017-10-24T18:25:53Z

For those now following swift-corelibs-dev@:

kevent is very (very, very, very!) ugly right now and 99% sure broken on Darwin. FreeBSD and Darwin diverge on a ton of type definitions on kevent structures. I had to add a lot of casting to get it working on FreeBSD which almost certainly broken it on Darwin. Maybe it should be split into a kevent_mach for Apple platforms and another for FreeBSD, but that would cause code duplication problems.

alblue · 2017-10-24T18:55:05Z

CMakeLists.txt

@@ -73,7 +75,8 @@ option(BUILD_SHARED_LIBS "build shared libraries" ON)
 option(ENABLE_TESTING "build libdispatch tests" ON)

 if(CMAKE_SYSTEM_NAME STREQUAL Linux OR
-   CMAKE_SYSTEM_NAME STREQUAL Android)
+   CMAKE_SYSTEM_NAME STREQUAL Android OR
+   CMAKE_SYSTEM_NAME STREQUAL FreeBSD)


This would be a less intrusive patch if you put the conditional above Android rather than below. If you had it above then you'd only generate one line of addition:

if(CMAKE_SYSTEM STREQUAL Linux OR + CMAKE_SYSTEM STREQUAL FreeBSD OR CMAKE_SYSTEM STREQUAL Android)

alblue · 2017-10-24T18:56:37Z

src/CMakeLists.txt

@@ -100,7 +111,8 @@ if(ENABLE_SWIFT)
                      -fmodule-map-file=${CMAKE_SOURCE_DIR}/dispatch/module.modulemap
                    SWIFT_FLAGS
                      -I ${CMAKE_SOURCE_DIR}
-                      ${swift_optimization_flags})
+                      -I/usr/include
+          ${swift_optimization_flags})


This looks like it's changed the indentation here - at least, as far as GitHub is concerned. Why do we need to add /usr/include here anyway?

I believe this is a issue with swiftc driver issue, for some reason it appears to not be appending /usr/include to path. Maybe I should make it a variable and mark it explicitly as a workaround.

I tried to find the root issue in the swift compiler but I couldn't.

And yes, indentation has changed... my IDE insists in reformatting after each line and messed on this and some other places too.

alblue · 2017-10-24T18:57:06Z

src/CMakeLists.txt

@@ -123,11 +135,13 @@ target_include_directories(dispatch
                             ${CMAKE_CURRENT_SOURCE_DIR}
                             ${CMAKE_CURRENT_BINARY_DIR}
                             ${CMAKE_SOURCE_DIR}/private)
+


I'd recommend against adding blank lines as it adds more noise to the patch than is necessary to get your changes through

alblue · 2017-10-24T18:57:27Z

src/block.cpp

@@ -28,6 +28,8 @@
 #error Must build without C++ exceptions
 #endif

+#define _Bool bool


This looks like it might cause problems for other builds

Indeed. This shouldn't even be there!

alblue · 2017-10-24T18:58:36Z

src/event/event_config.h

 #	else
 #		define DISPATCH_USE_KEVENT_QOS 0
 #	endif

+/// The flags here are set unconditionally (from DISPATCH_USE_KEVENT_QOS).


This also looks like an odd change - why has the indentation changed?

These flags were being used everywhere and unconditionally. Originally they were only defined if defined(EV_SET_QOS), which is not true on FreeBSD. The indentation didn't change per se, it was moved one level down and indentation corrected accordingly.

In epoll KEVENT_FLAG_IMMEDIATE is emulated with a timeout, the same must be done on kevent (this is not done yet -- this PR is only to get the library building and submitting things in small batches)

KEVENT_FLAG_ERROR_EVENTS already has a emulation (line 611)

alblue · 2017-10-24T19:00:11Z

src/event/event_kevent.c

@@ -601,7 +614,7 @@ _dispatch_kq_poll(dispatch_wlh_t wlh, dispatch_kevent_t ke, int n,
 		for (r = 0; r < n; r++) {
 			ke[r].flags |= EV_RECEIPT;
 		}
-		out_n = n;
+		n_out = n;


Is this a bug in your change, or is it a bug in the existing code?

Bug in the existing code. This is probably due to a refactoring and the fact that code with !DISPATCH_USE_KEVENT_QOS not being exercised recently.

alblue · 2017-10-24T19:01:17Z

src/event/event_kevent.c

@@ -1129,7 +1152,7 @@ _dispatch_event_loop_poke(dispatch_wlh_t wlh, uint64_t dq_state, uint32_t flags)
 			.ident  = 1,
 			.filter = EVFILT_USER,
 			.fflags = NOTE_TRIGGER,
-			.udata = (uintptr_t)DISPATCH_WLH_MANAGER,
+			.udata = (dispatch_kqueue_udata)DISPATCH_WLH_MANAGER,


There seems to be a lot of these kind of changes here. It might make sense to break this out (uintptr_t -> dispatch_kqueue_udata) into its own change first, which can then be merged ahead of the rest of the changes.

Yeah... I think kevent is a horrible mess with these changes, I hate it, but I don't see it could be made much better than this.

I think it would make sense to split into separate changes.

alblue · 2017-10-24T19:01:33Z

src/event/event_kevent.c

@@ -1271,6 +1294,8 @@ _dispatch_event_loop_timer_program(uint32_t tidx,
 #endif
 	};

+	(void) leeway; // if DISPATCH_HAVE_TIMER_COALESCING == 0


This change seems odd ...

This is for -Werror compliance. Maybe it should be:

DISPATCH_NOINLINE static void _dispatch_event_loop_timer_program(uint32_t tidx, uint64_t target, uint64_t leeway, uint16_t action) { dispatch_kevent_s ke = { .ident = DISPATCH_KEVENT_TIMEOUT_IDENT_MASK | tidx, .filter = EVFILT_TIMER, .flags = action | EV_ONESHOT, .fflags = _dispatch_timer_index_to_fflags[tidx], .data = (int64_t)target, .udata = (dispatch_kqueue_udata)&_dispatch_timers_heap[tidx], #if DISPATCH_HAVE_TIMER_COALESCING .ext[1] = leeway, #endif #if DISPATCH_USE_KEVENT_QOS .qos = _PTHREAD_PRIORITY_EVENT_MANAGER_FLAG, #endif }; #if !DISPATCH_HAVE_TIMER_COALESCING (void) leeway; #endif _dispatch_kq_deferred_update(DISPATCH_WLH_ANON, &ke); }

(added the whole function for more context)

no the change isn't odd, this is because leeway is unused, and it's fine to do it in an unguarded part. (void)foo; to avoid an unused warning is idiomatic in libdispatch.

MadCoder

In general, a lot is about not being darwin and looks like the linux_* headers.

When the diffs are so small, I think it says we should have a:

generic_unix_port.h or generic_unix_base.h header and have small localized ifdefs for unix variants (linux, android, FreeBSD, *BSD, ...).

generic_unix allows us to leave out a namespace for windows which will definitely not look similar presumably ;)

In general, I think we should check in code that is correct, and not put stubs for things that are obviously non functional: leaving a lock implementation as "no ops" is IMO criminal ;) a build failure is a way better outcome.

ALso for the part about locks, having a single pull request just about that is much better anyway, I agree all the rest makes sense in a single initial porting pull request.

Last, please adhere to the coding style for libdispatch which is:

no space for casts
80 columns wrapping with tabs for indent, 2 incremental tabs for rejects, but no more than 1 level of rejects.

It is almost similar to what vim does with:

set cinoptions=
set cinoptions+=Ls
set cinoptions+=:0,=1s
set cinoptions+=t0
set cinoptions+=+2s,(2s,u2s,W2s
set cinoptions+=j1,J1,m1

MadCoder · 2017-10-24T18:18:46Z

os/freebsd_base.h

@@ -0,0 +1,125 @@
+/*


this file is mostly a copy of linux_base I'd rather have a non_darwin_base or similar that has a few defines inside for the rare differences between the two

I agree; a generic_unix_base with a few small #ifdef clauses will be easier to maintain.

MadCoder · 2017-10-24T18:19:27Z

os/voucher_activity_private.h

@@ -26,7 +26,7 @@
 #include <mach/mach_time.h>
 #include <firehose/tracepoint_private.h>
 #endif
-#ifndef __linux__
+#if !defined(__linux__) && !defined(__FreeBSD__)


#if __APPLE__ clearly makes more sense here IMO

MadCoder · 2017-10-24T18:19:37Z

os/voucher_private.h

@@ -21,7 +21,7 @@
 #ifndef __OS_VOUCHER_PRIVATE__
 #define __OS_VOUCHER_PRIVATE__

-#ifndef __linux__
+#if !defined(__linux__) && !defined(__FreeBSD__)


#if __APPLE__

MadCoder · 2017-10-24T18:20:13Z

private/queue_private.h

@@ -227,7 +227,7 @@ DISPATCH_EXPORT DISPATCH_MALLOC DISPATCH_RETURNS_RETAINED DISPATCH_WARN_RESULT
 DISPATCH_NOTHROW
 dispatch_queue_t
 dispatch_pthread_root_queue_create(const char *_Nullable label,
-	unsigned long flags, const pthread_attr_t *_Nullable attr,
+	unsigned long flags, _Nullable const pthread_attr_t *_Nullable attr,


bogus, please revert this hunk

This must be a Clang bug then?

/home/rogiel/Swift/swift-corelibs-libdispatch/private/queue_private.h:230:29: error: pointer is missing a nullability type specifier
(_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]

maybe not, if pthread_attr_t is a pointer type on FreeBSD, but it's not on Darwin.

Yeah... thats absolutely the case then.

Not sure where I should and and #ifdef for this... the entire function signature? only that line? define a #define NULLBLE_PTHREAD_ATTR _Nullable on FreeBSD and #define NULLBLE_PTHREAD_ATTR on other platforms?

MadCoder · 2017-10-24T18:20:52Z

src/CMakeLists.txt

@@ -100,7 +111,8 @@ if(ENABLE_SWIFT)
                      -fmodule-map-file=${CMAKE_SOURCE_DIR}/dispatch/module.modulemap
                    SWIFT_FLAGS
                      -I ${CMAKE_SOURCE_DIR}
-                      ${swift_optimization_flags})
+                      -I/usr/include
+          ${swift_optimization_flags})


looks like you broke the indenting

MadCoder · 2017-10-24T19:29:09Z

src/swift/Wrapper.swift

@@ -180,7 +180,7 @@ extension DispatchSource : DispatchSourceMachSend,
 }
 #endif

-#if !os(Linux) && !os(Android)
+#if !os(Linux) && !os(Android) && !os(FreeBSD)


revert all in this, FreeBSD has all the underlying filters

MadCoder · 2017-10-24T19:29:20Z

src/transform.c

@@ -22,14 +22,22 @@

 #ifdef __APPLE__
 #include <libkern/OSByteOrder.h>
-#elif __linux__
+#elif defined(__linux__)


gratuitous change, please revert

MadCoder · 2017-10-24T19:30:05Z

tests/bsdtests.h

@@ -29,6 +29,10 @@
 #include <CoreFoundation/CoreFoundation.h>
 #endif

+#if defined(__linux__) || defined(__FreeBSD__)
+#include <unistd.h>


just always #include it it will not harm, and remove the empty lines

MadCoder · 2017-10-24T19:30:46Z

tests/dispatch_test.c

 		.ident = fd,
+#else
+		.ident = (uintptr_t) fd,


always do the cast it will not harm

MadCoder · 2017-10-24T19:31:26Z

tests/freebsd_port.h

@@ -0,0 +1,54 @@
+#include <limits.h>


seems a lot is like the linux port header, same as other comments, should be more of a "non_darwin_port.h"

Rogiel · 2017-10-24T22:34:51Z

What I have done:

Reverted all swift changes: those weren't supposed to be there anyway and had no testing whatsoever, just some find and replace.
Reverted lock stubs
Reverted most changes to queue

What still remains to be done:

Fix to nullability in attr argument of dispatch_pthread_root_queue_create (pthread_attr_t is a pointer on FreeBSD but not on Darwin)

Rogiel · 2018-01-20T01:48:02Z

All right @MadCoder, I have fixed the pthread_attr_t issue with a conditionally defined macro. I find this solution pretty ugly, but I don't know what do to here.

I guess the ugly macro is better than redefining the whole signature? It a private header anyways, maybe it's not that bad.

Just to recap, these changes makes the library almost compile on FreeBSD. There are still 4 files with issues:

queue.c (mostly runloop implementation)
lock.c/lock.h shims
bsdtestharness.c (this is a new issue, wasn't there in the first patch)

MadCoder

This is quite better, I like the patch thanks for this, there are a few minor nits to address and I'll test + merge.

MadCoder · 2018-01-20T21:26:09Z

os/generic_unix_base.h


 #if __has_include(<sys/sysmacros.h>)
 #include <sys/sysmacros.h>
+#if defined(__FreeBSD)


shouldn't this be __FreeBSD__ ?

also it's weird it's inside the __has_include(<sys/sysmacros.h>), I'd move it outside.

MadCoder · 2018-01-20T21:28:36Z

src/event/event_config.h

 #	else
 #		define DISPATCH_USE_KEVENT_QOS 0
 #	endif

+#	ifndef KEVENT_FLAG_ERROR_EVENTS
+#	 define KEVENT_FLAG_ERROR_EVENTS 0x002


the indent is wrong here

MadCoder · 2018-01-20T21:29:08Z

src/event/event_config.h

@@ -106,6 +104,14 @@
 #	define NOTE_FUNLOCK 0x00000100
 #	endif

+/// FreeBSD's kevent does not support those


double // triple is for headerdoc.

MadCoder · 2018-01-20T21:29:57Z

src/event/event_config.h

+// translated back by the various backends to similar semantics
+// hence must be defined even on non Darwin platforms
+#ifndef KEVENT_FLAG_IMMEDIATE
+# define KEVENT_FLAG_IMMEDIATE 0x001


remove the leading space

All other #defines are with the leading space on this file... should I still remove them?

the big hunk of configuration has a lot of nesting so it is pseudo indented for readability, but once it's at top level we don't use leading indent.

ALso I think it is tab-indented for the big fat one at the top not space ;)

@MadCoder changed to a tab AND fixed my tab size... now I can see them 😝

I kept a tab for consistency with the previous defines.

MadCoder · 2018-01-20T21:31:04Z

src/event/event_kevent.c

@@ -614,6 +631,8 @@ _dispatch_kq_poll(dispatch_wlh_t wlh, dispatch_kevent_t ke, int n,
 		}
 		r = kevent_qos(kqfd, ke, n, ke_out, n_out, buf, avail, flags);
 #else
+		(void)buf; // buf is unused here


remove the comment, (void)variable means it's unused throughout the repository already

MadCoder · 2018-01-20T21:31:46Z

src/event/event_kevent.c

@@ -961,7 +983,8 @@ _dispatch_unote_register_muxed(dispatch_unote_t du, dispatch_wlh_t wlh)
 #if DISPATCH_USE_KEVENT_QOS
 		dmn->dmn_kev.qos = _PTHREAD_PRIORITY_EVENT_MANAGER_FLAG;
 #endif
-		dmn->dmn_kev.udata = (uintptr_t)dmn | DISPATCH_KEVENT_MUXED_MARKER;
+		dmn->dmn_kev.udata = (dispatch_kevent_udata_t)((uintptr_t)dmn |
+		  DISPATCH_KEVENT_MUXED_MARKER);


this indent looks wrong, it should be led by 4 tabs (the 2 from the line above + 2 because it's a split line)

MadCoder · 2018-01-20T21:32:33Z

src/io.c

@@ -2165,7 +2171,7 @@ _dispatch_operation_advise(dispatch_operation_t op, size_t chunk_size)
 	}
 	advise.ra_offset = op->advise_offset;
 	op->advise_offset += advise.ra_count;
-#ifdef __linux__
+#if __linux__


I think this change is wrong, #if defined() is what you use elswhere, please revert

MadCoder · 2018-01-20T21:33:10Z

src/shims/atomic.h

@@ -31,6 +31,10 @@
 #error libdispatch requires C11 with <stdatomic.h>
 #endif

+// FreeBSD only defines _Bool in C mode. In C++ mode _Bool is not being defined.
+#if defined(__cplusplus) && defined(__FreeBSD__)
+# define _Bool bool


please remove the leading space.

MadCoder · 2018-01-20T21:34:01Z

tests/dispatch_test.c

@@ -56,7 +56,7 @@ dispatch_test_check_evfilt_read_for_fd(int fd)
 	int kq = kqueue();
 	assert(kq != -1);
 	struct kevent ke = {
-		.ident = fd,
+		.ident = (uintptr_t) fd,


remove space after cast

MadCoder · 2018-01-20T21:36:27Z

Btw @Rogiel it seems your patch has been stuck for a long time if I read history right. For some reason, when someone pushes a commit on a branch I'm reviewing I get no notification. Do not hesitate to ping me explicitly in the comments with an @MadCoder, that definitely sends me an email that I react to (which is how I looked at it again).

I'm really sorry it felt like I forgot you for months, I just don't poll pull requests for lack of time.

Rogiel · 2018-01-20T21:49:35Z

@MadCoder

No problem, I just got busy too and my FreeBSD machine died 😕

Okay, I made the changes (except the one about the leading space in the a #define, see the inline comment). I will wait until CI is done in case anything else comes up.

This is the first commit of several to bring libdispatch support up to date on FreeBSD

MadCoder · 2018-01-20T22:09:26Z

@swift-ci please test

Add support for building libdispatch on FreeBSD Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

alblue reviewed Oct 24, 2017

View reviewed changes

MadCoder suggested changes Oct 24, 2017

View reviewed changes

MadCoder added the changes requested label Oct 24, 2017

MadCoder suggested changes Jan 20, 2018

View reviewed changes

Fixes most compilation issues on FreeBSD

6436a89

This is the first commit of several to bring libdispatch support up to date on FreeBSD

MadCoder merged commit f3531a2 into swiftlang:master Jan 21, 2018

MadCoder added darwin pending and removed changes requested labels Jan 21, 2018

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

Merge pull request #319 from Rogiel/feature/freebsd-support

a7c6359

Add support for building libdispatch on FreeBSD Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

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

Add support for building libdispatch on FreeBSD #319

Add support for building libdispatch on FreeBSD #319

Uh oh!

Conversation

Rogiel commented Oct 24, 2017

Uh oh!

Rogiel commented Oct 24, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Rogiel Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadCoder 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rogiel commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rogiel commented Jan 20, 2018

Uh oh!

MadCoder left a comment

Choose a reason for hiding this comment

Rogiel Oct 24, 2017 •

edited

Loading

Rogiel commented Oct 24, 2017 •

edited

Loading