-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[FreeBSD] Adding FreeBSD support #77836
base: main
Are you sure you want to change the base?
Conversation
It's my early Christmas gift. Thanks for working on this. |
unfortunately this PR alone here is not enough. I do have patches to other swift components to get it fully working and going to PR them to coming weeks. But even with those, you'll need to have a functional you also need a patch to the base libc++ to workaround this issue |
it's very impressive work especially the bootstrap part! |
@@ -65,6 +65,7 @@ headers = [ | |||
'spawn.h', | |||
'strings.h', | |||
'sys/event.h', | |||
'sys/extattr.h', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this header available universally? I think that this might need to be under a FreeBSD condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gyb expands '${header}' to an include guarded by __has_include
, which test if the header can be included. similarly, the Linux specific sys/inotify.h
header is also in the list.
#if __has_include(<${header}>)
#include <${header}>
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, any headers added to this file are just ignored if they don't exist, as there are already some OpenBSD-specific headers.
Have you considered adding a FreeBSD overlay instead? You could take a look at the recent Musl, WASI, and Android overlays for examples.
I know that's more work, but the WASI one isn't so bad, and FreeBSD may not be either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely on my radar! I'm not sure yet if I have to bandwidth to include that change in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shoulda just called them all import C
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah, the implementation is actually very difficult, especially when C++ gets involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the feedback! Sorry for the late reply I was extremely sick for the past 2 weeks.
I certainly agree FreeBSD shouldn't use Glibc. In fact I have been working on the overlay for quite a while now but never gotten into a state I'm perfectly happy about it. More specifically I'd love to follow the Darwin modulemap and make certain C headers replaceable by clang, however it doesn't seems to be possible without making changes to the FreeBSD source tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan is to put the overlay changes into a separate PR, due to size of diff is quite big and this is already a rather large PR (as you can imagine all instance of where import Glibc
occurs needs to be patched).
I have a working implementation ready to go, either committing to this PR, or as a separate PR, depending on what will be more comfortable for you to review. Wdyt @al45tair @ian-twilightcoder @grynspan @finagolfin
This also helps keep my head sane as I've been to maintaining quite a few separate trees of all the swift sub-projects locally to implement various feature for FreeBSD and it's getting quite messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no opinion either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overlay PR created. #79261
Nice, there is some work to get Swift 6 bootstrapping again, #77815, that you'll want to follow. Pinging @mhjacobson and @3405691582, who added BSD support before and may be able to help review. |
As a heads up, the code uses LLVM target triples and the build system has a few places where it assumes that the platform calls the architecture with the same name as LLVM does. This isn't a problem for FreeBSD on what LLVM calls In the past, I've tried to keep some meaningful separation between platform spelling and LLVM spelling, but I'm reconsidering that in #77879. You probably won't need to do anything right now for this pr if you're just targeting |
Which is not correct for the FreeBSD part. On FreeBSD itself, arm64 is the TARGET, meanwhile aarch64 is the TARGET_ARCH and MACHINE_ARCH. Below is what I got from a build of the FreeBSD tree on my side:
For reference how we build the -target tuple in the tree:
|
@swift-ci please smoke test |
@swift-ci please test |
8c4f0d4
to
65db70b
Compare
65db70b
to
0f1000b
Compare
This commit adds required conditional compilation blocks to enable bulding on FreeBSD (tested on x86_64 FreeBSD 14.1-RELEASE-p6). Also implements FreeBSD synchronization shims using `_umtx_op(2)`
0f1000b
to
c22dacb
Compare
@@ -422,7 +422,14 @@ class DerivativeFunctionTypeError | |||
Kind kind; | |||
|
|||
/// The type and index of a differentiability parameter or result. | |||
using TypeAndIndex = std::pair<Type, unsigned>; | |||
/// std::pair does not have a trivial copy constructor on FreeBSD <= 14 for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just change the call sites that are using the constructor? That would simplify the diff.
@@ -2562,6 +2562,10 @@ PlatformAvailability::PlatformAvailability(const LangOptions &langOpts) | |||
case PlatformKind::visionOSApplicationExtension: | |||
break; | |||
|
|||
case PlatformKind::FreeBSD: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we coalesce the non-Apple cases?
@@ -2680,6 +2687,10 @@ bool PlatformAvailability::treatDeprecatedAsUnavailable( | |||
// No deprecation filter on xrOS | |||
return false; | |||
|
|||
case PlatformKind::FreeBSD: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we coalesce?
@@ -244,6 +244,8 @@ getLinkerPlatformId(OriginallyDefinedInAttr::ActiveVersion Ver, | |||
llvm_unreachable("cannot find platform kind"); | |||
case swift::PlatformKind::OpenBSD: | |||
llvm_unreachable("not used for this platform"); | |||
case swift::PlatformKind::FreeBSD: | |||
llvm_unreachable("not used for this platform"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we coalesce? (Also, nitpick, the order of FreeBSD relative to OpenBSD is inconsistent. It's not important in any practical sense, but consistency is a good thing.)
@@ -338,12 +338,12 @@ public var SIG_DFL: sig_t? { return nil } | |||
public var SIG_IGN: sig_t { return unsafeBitCast(1, to: sig_t.self) } | |||
public var SIG_ERR: sig_t { return unsafeBitCast(-1, to: sig_t.self) } | |||
public var SIG_HOLD: sig_t { return unsafeBitCast(5, to: sig_t.self) } | |||
#elseif os(OpenBSD) | |||
#elseif os(OpenBSD) || os(FreeBSD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: platform order
@@ -19,7 +19,7 @@ | |||
/// It's not named just Glibc so that it doesn't conflict in the event of a | |||
/// future official glibc modulemap. | |||
module SwiftGlibc [system] { | |||
% if CMAKE_SDK in ["LINUX", "OPENBSD"]: | |||
% if CMAKE_SDK in ["LINUX", "OPENBSD", "FREEBSD"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: platform order
@@ -24,7 +24,7 @@ | |||
|
|||
// Clang has been defining __INTxx_TYPE__ macros for a long time. | |||
// __UINTxx_TYPE__ are defined only since Clang 3.5. | |||
#if !defined(__APPLE__) && !defined(__linux__) && !defined(__OpenBSD__) && !defined(__wasi__) | |||
#if !defined(__APPLE__) && !defined(__linux__) && !defined(__OpenBSD__) && !defined(__FreeBSD__) && !defined(__wasi__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: platf—okay you get the idea
@available(SwiftStdlib 6.0, *) | ||
@frozen | ||
@_staticExclusiveOnly | ||
public struct _MutexHandle: ~Copyable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually need to be public?
@@ -16,7 +16,7 @@ StdOptionalTestSuite.test("pointee") { | |||
let pointee = nonNilOpt.pointee | |||
expectEqual(123, pointee) | |||
|
|||
#if !os(Linux) // crashes on Ubuntu 18.04 (rdar://113414160) | |||
#if !os(Linux) && !os(FreeBSD) // crashes on Ubuntu 18.04 (rdar://113414160) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is crashing on FreeBSD as well, can you file a GitHub issue or Radar as appropriate and reference it here too?
@@ -27,14 +27,22 @@ if #available(OSX 10.10, iOS 8.0, *) { | |||
// dispatch/source.h | |||
_ = DispatchSource.makeUserDataAddSource() | |||
_ = DispatchSource.makeUserDataOrSource() | |||
#if !os(FreeBSD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this compiling on Linux right now? Surely whatever guards Linux should guard FreeBSD too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DispatchSource is basically kqueue wrapper, which exists on FreeBSD and Darwin but not on Linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to commit to supporting it at this time, or does it make more sense to leverage future improvements aimed at Linux?
I'm considering this a blocker for swiftlang/swift-build#12 since that requires the header file providing access to the xattr functions to be in the modulemap in order to enable xattr support in Swift Build's filesystem implementation. |
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing! I have just one comment about a C++ interop test.
@@ -1,4 +1,4 @@ | |||
// RUN: %target-swift-ide-test -print-module -module-to-print=FakeToolchain -tools-directory %S/Inputs/fake-toolchain/bin -source-filename=x -enable-experimental-cxx-interop -Xcc -stdlib=libc++ | %FileCheck %s | |||
// RUN: %target-swift-ide-test -print-module -module-to-print=FakeToolchain -tools-directory %S/Inputs/fake-toolchain/bin -source-filename=x -enable-experimental-cxx-interop -Xcc -stdlib=libc++ -Xcc -I%S/Inputs/fake-toolchain/include/c++/v1 | %FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right – this test basically checks that passing libc++ include paths via -I
or -isystem
is not required when using -stdlib=libc++
.
I wonder if this -I
flag turns out to be needed on FreeBSD because of the lack of support for -stdlib=libc++
in Clang Driver on this specific platform?
Either way, could you please disable the test on FreeBSD instead?
This commit adds required conditional compilation blocks to enable building and running
on FreeBSD (tested on x86_64 FreeBSD 14.1-RELEASE-p6).
Also implements FreeBSD synchronization shims using
_umtx_op(2)