Skip to content
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

Add Fuchsia OS support #12955

Closed
wants to merge 12 commits into from
Closed

Add Fuchsia OS support #12955

wants to merge 12 commits into from

Conversation

@zbowling
Copy link
Contributor

zbowling commented Nov 16, 2017

Adds initial support for Fuchsia OS to the compiler and adds support for building for Fuchsia in the stdlib.

This change also introduces lld linker support to the build system and fixes a number of issues related to cross compiling Swift to platforms that do not support universal binaries. As part of this change, non-Darwin libraries in the standard library are now stored in lib/swift/<platform>/<arch> instead of lib/swift/<platform>.

@@ -68,7 +68,7 @@ Driver::Driver(StringRef DriverExecutable,
: Opts(createSwiftOptTable()), Diags(Diags),
Name(Name), DriverExecutable(DriverExecutable),
DefaultTargetTriple(llvm::sys::getDefaultTargetTriple()) {

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 16, 2017

Member

Can you separate the whitespace changes into a separate commit?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

sure thing :)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

I split the commit into two changes now

This comment has been minimized.

Copy link
@AfridiShahriar

AfridiShahriar Nov 20, 2017

Hey what's ur plan with the fushciaOS? Will it replace Android? Or, be another platform?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 21, 2017

Author Contributor

Sorry, I'm not qualified to speak to that and it's little off-topic for this PR.

@zbowling zbowling force-pushed the google:fuchsia_pr branch Nov 16, 2017
@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 16, 2017

FYI, in the pipeline after this we will have some PRs related to:

  • adding ARM64 support for the Fuchsia SDK
  • fixing cross-compiling issues for targeting BSD, Linux and Fuchsia targets from a Darwin toolchain
  • adding support for using lld for linking specific SDK stdlibs (part of getting a Darwin toolchain capable of cross compiling to other targets)
  • supporting unit tests on Fuchsia
@zbowling zbowling force-pushed the google:fuchsia_pr branch to 0c253b7 Nov 16, 2017
@@ -1530,20 +1553,16 @@ bool toolchains::GenericUnix::shouldProvideRPathToLinker() const {

std::string toolchains::GenericUnix::getPreInputObjectPath(
StringRef RuntimeLibraryPath) const {
// On Linux and FreeBSD and Haiku (really, ELF binaries) we need to add objects
// to provide markers and size for the metadata sections.
// On Linux, FreeBSD, Fuchsia, and Haiku we need to add sential objects to

This comment has been minimized.

Copy link
@gparker42

gparker42 Nov 16, 2017

Contributor

sential => sentinel ?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

fixed.

@@ -1462,6 +1463,8 @@ public enum OSVersion : CustomStringConvertible {
return "Cygwin"
case .windows:
return "Windows"
case .fuchsia:
return "Windows"

This comment has been minimized.

Copy link
@gparker42

gparker42 Nov 16, 2017

Contributor

Should this be "Fuschia" ?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yes. rebase mistake.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

fixed.

@@ -1781,6 +1803,14 @@ bool toolchains::Android::shouldProvideRPathToLinker() const {
return false;
}

std::string toolchains::Fuchsia::getDefaultLinker() const {
return "lld";

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

This should be "ld.lld".

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Fix in flight. I thought "-fuse-ld=ld.lld" looked funnier than "-fuse-ld=lld" that swift invokes on clang.

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

Ah, if this is what Swift passes to -fuse-ld= then "lld" is correct, if this is the name of the binary that Swift will invoke then it should be "ld.lld" (I haven't looked at how this is wired up internally).

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

yeah it's for -fuse-ld

@@ -111,6 +111,16 @@ class LLVM_LIBRARY_VISIBILITY Android : public GenericUnix {
~Android() = default;
};

class LLVM_LIBRARY_VISIBILITY Fuchsia : public GenericUnix {

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

This is what we originally did in Clang as well, but it was causing more issues than solving since Fuchsia really isn't UNIX, so we since moved to inheriting directly from the ToolChain class, that's something to consider.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yeah, right now there isn't too much friction pretending Fuchsia is a type of "Unix" for at least the args used by Swift to drive clang/ld (Fuchsia is an elf platform but not a Unix, even though it has some posix-y looking things).

It might make sense to make a base class that both GenericUnix and Fuchsia's toolchain class inherit called ElfPlatform or something maybe and move things around.

We have a similar friction with the name of the Glibc module (we aren't using "GNU Glibc" but it's the name that existing code assumes the libc module is called outside of Darwin). I'm hoping people will use our platform libc module map instead there even though the Glibc module works right now too.

@@ -1462,6 +1463,8 @@ public enum OSVersion : CustomStringConvertible {
return "Cygwin"
case .windows:
return "Windows"
case .fuchsia:
return "Windows"

This comment has been minimized.

Copy link
@petrhosek
@@ -64,6 +64,10 @@ static long double swift_strtold_l(const char *nptr,
#define strtod_l swift_strtod_l
#define strtof_l swift_strtof_l
#define strtold_l swift_strtold_l
#elif defined(__Fuchsia__)
// this might be for all MUSL libc implementations

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

Fuchsia's libc has already diverged sufficiently from musl to the point where they're not interchangeable, this came up in libc++ where we originally just used musl support but since then we introduced Fuchsia support because of the differences.

is_sdk_requested(FUCHSIA swift_build_fuchsia)
if(swift_build_fuchsia AND NOT "${SWIFT_FUCHSIA_BUILD_PATH}" STREQUAL "")
# HACK: support arm64 fuchsia builds
set(SWIFT_FUCHSIA_SYSROOT

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

Can we please make this settable externally instead of hardcoding the path within Fuchsia checkout? The name should also contain architecture so you can add aarch64 in the future (e.g. in Clang we expect FUCHSIA_x86_64_SYSROOT and FUCHSIA_aarch64_SYSROOT to be set).

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yeah I can do that. I will also need have a setting for specifying the shared libs directory from Fuchsia since I need to link ICU (given that it's outside of the zircon generated sysroot). Originally I had it so that you just passed just the sysroot for zircon into the build-util but I was doing really brittle path transversal to our "x86-shared" folder for fuchsia so that I could find and link ICU libs.

Need to think of a clean way to do that on the build-util args so I'm not adding a new arg for each arch.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

Fixed in the latest build. Removed all hardcoded paths.

You can now set everything as args:

utils/build-script --fuchsia -j500 \
--extra-stdlib-deployment-targets=fuchsia-aarch64,fuchsia-x86_64 \
--fuchsia-toolchain-path $FUCHSIA_DIR/buildtools/linux-x64/clang \
--fuchsia-icu-uc-include $FUCHSIA_DIR/third_party/icu/source/common \
--fuchsia-icu-i18n-include $FUCHSIA_DIR/third_party/icu/source/i18n \
--fuchsia-x86_64-sysroot $FUCHSIA_DIR/out/build-zircon/build-zircon-pc-x86-64/sysroot \
--fuchsia-aarch64-sysroot $FUCHSIA_DIR/out/build-zircon/build-zircon-qemu-arm64/sysroot \
--fuchsia-x86_64-libs $FUCHSIA_DIR/out/debug-x86-64/x64-shared \
--fuchsia-aarch64-libs $FUCHSIA_DIR/out/debug-aarch64/arm64-shared  \
--export-compile-commands true --lld --build-runtime-with-host-compiler true \
--skip-test-fuchsia-host --skip-test-linux --build-swift-static-stdlib true \
--build-swift-dynamic-sdk-overlay true
set(SWIFT_FUCHSIA_SYSROOT
"${SWIFT_FUCHSIA_BUILD_PATH}/out/build-zircon/build-zircon-pc-x86-64/sysroot")

set(SWIFT_SDK_FUCHSIA_ARCH_x86_64_LINKER

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 16, 2017

The same here, this should be set externally, not hardcoded.

Copy link
Member

gottesmm left a comment

This PR has a bunch of really large changes in it. It really should be split up at least into multiple commits, but perhaps into multiple PRs. That will make it easier to review. I provided several examples of places that I think that you could split off into separate commits/PRs.

@@ -1968,6 +1980,17 @@ for host in "${ALL_HOSTS[@]}"; do
-DCMAKE_SHARED_LINKER_FLAGS:STRING="-fuse-ld=gold"
)
fi
elif [[ "${USE_LLD_LINKER}" ]]; then

This comment has been minimized.

Copy link
@gottesmm

gottesmm Nov 16, 2017

Member

Can you split the LLD change into a separate PR. That should be able to stand on its own.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yeah, that is the easiest to pull out.

@@ -436,7 +440,10 @@ function set_build_options_for_host() {
linux-*)
SWIFT_HOST_VARIANT="linux"
SWIFT_HOST_VARIANT_SDK="LINUX"
USE_GOLD_LINKER=1
if [[ -z "${USE_LLD_LINKER}" ]] ; then

This comment has been minimized.

Copy link
@gottesmm

gottesmm Nov 16, 2017

Member

Can you split the build-script changes into a separate commit.

@@ -184,7 +184,7 @@ set(SWIFT_ANDROID_DEPLOY_DEVICE_PATH "" CACHE STRING
# User-configurable ICU specific options for Android, FreeBSD, Linux and Haiku.
#

foreach(sdk ANDROID;FREEBSD;LINUX;WINDOWS;HAIKU)
foreach(sdk ANDROID;FREEBSD;LINUX;WINDOWS;FUCHSIA;HAIKU)

This comment has been minimized.

Copy link
@gottesmm

gottesmm Nov 16, 2017

Member

Can you split the cmake changes into a separate commit (preferably a separate PR).

@@ -533,6 +533,12 @@ getNormalInvocationArguments(std::vector<std::string> &invocationArgStrs,
});
}
} else {
invocationArgStrs.insert(invocationArgStrs.end(), {

This comment has been minimized.

Copy link
@gottesmm

gottesmm Nov 16, 2017

Member

This seems really random. @jrose-apple your thoughts?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

I would be ok with wrapping this in a if tripple.isFuchsia() here but it seemed benign on glibc libs. glibc and musl (and consequently our libc which is heavily forked from musl) hides or defines things in different locations depending on if BSD_SOURCE, GNU_SOURCE, etc are set because of features.h. If nothing is set then some pretty common functions go missing in musl libcs that are used in some of the platform folder code for the glibc module. Without some x_SOURCE define, glibc behavior is not exactly sync'd up with musl (or even bionic on android). Setting GNU_SOURCE seems to gets glibc and musl similar enough so that they end up defining types and declaring libc functions in the same places in the same named header files which makes the clang importer happy and things consistent with the glibc module.

There was thread about making this change on the list in 2016 by someone else and that someone would ok it if they sent a PR but nothing came of it.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Consequently this change also gets arch-linux closer to working with swift since they use musl instead of glibc.

This comment has been minimized.

Copy link
@gparker42

gparker42 Nov 16, 2017

Contributor

I assume this is the previous thread:
https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151221/000541.html

Pierre and Jordan were fine with it for Linux then.

This comment has been minimized.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 16, 2017

Contributor

Yeah, I don't love it, but it seems reasonable. 👍 from me.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

One issue I still need to solve is that Swift bakes in absolute paths for the glibc module map, which for cross compiling where the sysroot of the platform you are targeting can be at a different location on each developer's system, is big non-starter. It basically means I can't make any binary cross-compile capable toolchains that are reusable. Having a "-sdk"/sysroot relative module map path lookup would be a good solution but it would require some deeper changes to the clang importer and module code in clang to support that concept.

There was a discussion about this last year on the list and an abandoned PR that was approaching fixing this but the only use case that was fixed was for for canadian cross-compiling and not for direct cross compiling.

Right now I've just made a platform libc module map in our Fuchsia sysroot that lives relative to all our headers which our internal swift code is using, but the glibc module doesn't work for anyone that I share my swift toolchain builds with.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Let's tackle that later, in a separate PR.

@@ -14,7 +14,7 @@
//
//===----------------------------------------------------------------------===//

#if defined(__CYGWIN__) || defined(__ANDROID__) || defined(_WIN32) || defined(__HAIKU__)
#if defined(__CYGWIN__) || defined(__ANDROID__) || defined(_WIN32) || defined(__Fuchsia__) || defined(__HAIKU__)

This comment has been minimized.

Copy link
@gottesmm

gottesmm Nov 16, 2017

Member

Can you separate this out as well.

list(APPEND result
"-I${SWIFT_FUCHSIA_BUILD_PATH}/buildtools/linux-x64/clang/lib/x86_64-fuchsia/include/c++/v1/"
"-L${SWIFT_FUCHSIA_BUILD_PATH}/buildtools/linux-x64/clang/lib/x86_64-fuchsia/lib")
endif()

This comment has been minimized.

Copy link
@modocache

modocache Nov 16, 2017

Collaborator

One big regret I have with the way I merged the Swift/Android CMake is the presence of hardcoded paths, such as "${SWIFT_ANDROID_PREBUILT_PATH}/arm-linux-androideabi/lib/armv7-a" or "${SWIFT_ANDROID_PREBUILT_PATH}/lib/gcc/arm-linux-androideabi/${SWIFT_ANDROID_NDK_GCC_VERSION}.x". The problem is that the paths sometimes change, and so newer releases of the Android SDK/NDK break Swift's build.

Similarly, I think these would be better as externally settable values. If appropriate, the paths you use here could be provided as defaults, but I think allowing users to specify the paths themselves may prevent some build breakages later on.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yah previously I was debating between adding 8 different args to specify paths vs working off one path and baking in where things are in Fuchsia into the cmake files. I chose the latter originally because early on in the port I didn't know what paths swift would eventually need to finally compile.

Right now I'm baking in:

  • aarch64 sysroot path
  • aarch64 userland shared lib path (for libicu*.so)
  • x86_64 sysroot path
  • x86_64 userland shared lib path (for libicu*.so)
  • ICU's common include path
  • ICU's i18n include path
  • fuchsia's toolchain libc++ include path (at different path between linux/darwin toolchains)
  • fuchsia's toolchain lib path (at different path between linux/darwin toolchains)

All this I should clean up. I will likely need build-util args for each of these. I will also need an arg to specify which archs you want to build the stdlib for since your local build of fuchsia might only have built artifacts for one arch or you just want to make a leaner build locally.

This comment has been minimized.

Copy link
@compnerd

compnerd Nov 17, 2017

Collaborator

We really should consider upgrading the minimum CMake version for non-Darwin targets. That way we could take advantage of the cross-compiling toolchain definitions from CMake which really simplify handling the android NDK.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

fixed.

"-mcx16"
"-I${SWIFT_FUCHSIA_BUILD_PATH}/third_party/icu/source/common/"
"-I${SWIFT_FUCHSIA_BUILD_PATH}/third_party/icu/source/i18n/")
endif()

This comment has been minimized.

Copy link
@modocache

modocache Nov 16, 2017

Collaborator

I don't recall all the details, but I believe Swift/Android started out with some hardcoded paths like this, and those were later refactored into something that most other platforms could use. Maybe it was #3205? I wonder if you could use that mechanism as well?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yeah, I was going to move to the SDK defined include paths for ICU in the top level cmake. There isn't a built-util arg to set those per SDK though. The android target added their own one off to override that though. I need to fix it because I think it's possible for your system's ICU headers can get accidentally included if nothing is set instead of the target platform's ICU headers when compiling the stdlib.

Flinging other args and paths up through build-util, the build-util-impl, and finally cmake and adding tests for it can be a bit of chore so I decided that since this path is well known on Fuchsia (unlike the android build where the user could checkout the ICU they use anywhere) that it would would ok temporarily to add it relative to the directory I had already flung up the layers. Having hardcoded paths like this though is generally bad.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

"-mcx16" is also interesting here. Having it (or any target CPU that implies that flag) might speed up refcounting heap objects on Linux and it will likely fix an issue FreeBSD users had with building swift unless they installed gcc on their system (or really anyone that is using compiler-rt and instead of libgcc/atomic). We noticed the issue since we use compiler-rt which doesn't have an implementation of "_sync_val_compare_and_swap_16" like libgcc/libatomic does.

Since the swift cmake doesn't set any minimum target CPU, LLVM will assume it's ok in it's target lowering that you are on a CPU that doesn't support the CMPXCHG16B instruction and that it's ok to call out to that libgcc intrinsic. HeapObject.cpp is calling out to that function a lot on Linux which is probably a lot slower than the native instruction for that on x86 CPUs that support it.

See https://bugs.swift.org/browse/SR-5779

This might have also been partially fixed in LLVM recently.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

I'm reusing some of the settings from that change now (specifically just the include paths) so I can reuse some of the logic. The icu libraries in Fuchsia are in the same shared libs directory as number of our shared libraries that are not in our sysroot so I just use that path for search paths everywhere.

import Glibc
#endif


#if !os(Windows)
// posix_spawn is not available on Windows.
// posix_spawn is not available on Android.
// posix_spawn is not available on Fuchsia. (TODO: Fuchsia provides liblaunchpad for this)

This comment has been minimized.

Copy link
@modocache

modocache Nov 16, 2017

Collaborator

nit-pick: Maybe we could put all of these comments on a single line? posix_spawn is not available on Windows, Android, Haiku, or Fuchsia.? It just seems a little excessive to have four lines of comments here, IMHO :)

This comment has been minimized.

Copy link
@gparker42

gparker42 Nov 16, 2017

Contributor

There should be at least two lines, because the Windows case is unlike the others. If I'm reading it correctly this !Windows check covers the entire file, not just the spawn vs fork/exec distinction. In any case such cleanup shouldn't be the job of this change.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Yeah a lot of this is to just to get it compiling. Some of the functions in Subprocess.swift are defined in our libc today for compatibility but not not actually backed by a real implementation. Basically, how you create, manage, and communicate with a child process is something that Fuchsia really departs heavily from compared to how any other Unix clone out there works today. So for Subprocess.swift we will have to do something a lot different. I was going to land our implementation of this later as we start bringing up some of the unit tests and get them running on device though.

For background reference, on Fuchsia we don't have fork or posix_spawn equivalents and we don't use dup'd fds to communicate with child processes (although you can do something like that to work with your child's stdin/out). Instead you ask the system to create you a process via a syscall and then it's up to you load everything your child process needs into it's memory via shared memory object. Then when you are done you use another syscall to ask the system to start a thread on that process for you. We have a convenience library that makes this easier but it doesn't really end up looking anything like posix_spawn or fork/exec.

@@ -84,7 +85,7 @@ public func spawnChild(_ args: [String])
let childStdin = posixPipe()
let childStderr = posixPipe()

#if os(Android) || os(Haiku)
#if os(Android) || os(Haiku) || os(Fuchsia)

This comment has been minimized.

Copy link
@modocache

modocache Nov 16, 2017

Collaborator

The comment immediately below this could use some updating, since it's not just a codepath taken by Android anymore.

@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 16, 2017

Yeah sorry about the big PR. I squashed all my commits from before I got approval to upstream.

I think can split this PR into 3 different parts that would still be viable and testable on their own. I think it makes sense to break it into:

  • lld support (mostly build-util and cmake changes)
  • fixes for the mutli-arch cross compiling issues (mostly cmake changes and some compiler changes to change where it looks for libs)
  • adding the Fuchsia target support (still making changes all over but it should be a smaller patch)

After that I send up additional things as separate PRs that I listed perviously. That work?

@gottesmm

This comment has been minimized.

Copy link
Member

gottesmm commented Nov 16, 2017

SGTM

@gribozavr

This comment has been minimized.

Copy link
Collaborator

gribozavr commented Nov 16, 2017

Could you add tests like these:

test/Parse/ConditionalCompilation/x64FreeBSDTarget.swift
validation-test/StdlibUnittest/FreeBSD.swift

It would be also great to add some docs about running the build like docs/Android.md.

@gottesmm

This comment has been minimized.

Copy link
Member

gottesmm commented Nov 16, 2017

Whoah! Its a wild @gribozavr! Waves!

/// On Darwin we link fat libs in the root of the library path and on other OS
/// targets we look up libraries in the target arch's subfolder.
static void getRuntimeLibraryPathWithArch(SmallVectorImpl<char> &runtimeLibPath,
const llvm::opt::ArgList &args,

This comment has been minimized.

Copy link
@gribozavr

gribozavr Nov 16, 2017

Collaborator

Indentation seems off.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

yeah it's off on a few functions in this file it seems. Something a good clang-format/tidy pass might be good to do.

@@ -2028,6 +2051,12 @@ for host in "${ALL_HOSTS[@]}"; do
"${llvm_cmake_options[@]}"
)

if [[ "${USE_LLD_LINKER}" ]]; then
cmake_options+=(
-DLLVM_ENABLE_LLD:BOOL=YES

This comment has been minimized.

Copy link
@gribozavr

gribozavr Nov 16, 2017

Collaborator

Missing indentation?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

woops. yep.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 21, 2017

Author Contributor

looks like that bad indentation was used in other places too. I'll fix it everywhere in an upcoming CL.

static __swift_uint64_t randomUInt64() {
std::random_device randomDevice;
std::mt19937_64 twisterEngine(randomDevice());
std::uniform_int_distribution<__swift_uint64_t> distribution;
return distribution(twisterEngine);
}
#endif

This comment has been minimized.

Copy link
@gribozavr

gribozavr Nov 16, 2017

Collaborator

Extra blank line?


zx_status_t status = zx_cprng_draw(offset, read_len, &actual);

// decrement the remainder and update the pointer offset in the buffer

This comment has been minimized.

Copy link
@gribozavr

gribozavr Nov 16, 2017

Collaborator

Could you start comments with a capital letter and finish with a period? (Per LLVM style guide) (Please apply everywhere in this PR)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 16, 2017

Author Contributor

Sure

@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 16, 2017

Yeah, I may add a Fuchsia.md doc to docs folder. I think it makes sense to just document the current args on build-util and then link to Fuchsia's doc repo for how to use swift since things are constantly changing in Fuchsia. At some point I soon I would love to get things working with swiftpm.

@gribozavr

This comment has been minimized.

Copy link
Collaborator

gribozavr commented Nov 16, 2017

Right, it does not have to be perfect, but it would ideally contain end-to-end instructions on how to compile Swift for the platform: what are the prerequisites, what is the expected filesystem layout that you need to prepare, how to run build-script, how to run tests for the host side and the device side.

@zbowling zbowling force-pushed the google:fuchsia_pr branch from 0c253b7 to c3d8063 Nov 16, 2017
@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 16, 2017

I broke up the PR into 5 separate commits and fixed a number of things mentioned here. I will land another commit to fix where we are hardcoding paths in the Fuchsia tree and replace with them build-util configurable options shortly.

@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 16, 2017

I just realized one of the earlier commits I broke up makes a reference to a cmake variable that is introduced in a later commit so they are not perfectly hermetic.

@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 17, 2017

@swift-ci Please smoke test

@phausler

This comment has been minimized.

Copy link
Member

phausler commented Nov 17, 2017

@swift-ci please smoke test

SmallString<128> PreInputObjectPath = RuntimeLibraryPath;
llvm::sys::path::append(PreInputObjectPath,
swift::getMajorArchitectureName(getTriple()));

This comment has been minimized.

Copy link
@compnerd

compnerd Nov 17, 2017

Collaborator

Im working on removing the sentinels from the ELFish platforms in favor of the ELF linker tables. Im hoping that I can figure out the one last issue and get that merged soon.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 17, 2017

Author Contributor

awesome!

This comment has been minimized.

Copy link
@compnerd

compnerd Nov 23, 2017

Collaborator

FYI, I've gotten it mostly passing with the changes. It uncovered a latent issue with the swift reflection dump tool that needs to be accounted for, but the rest of it is working now. I think that testing with that is probably a good idea.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

Yeah, I'll pull it in locally and see how well it works after the 🦃 coma is over. Happy Giving Of Thanks!

@compnerd

This comment has been minimized.

Copy link
Collaborator

compnerd commented Nov 18, 2017

@zbowling I think it would be better to create separate PRs for thee commits. I dont see why we should wait for the whitespace cleanups. I think that the lld support change can probably go in earlier as well. Reducing the outstanding patches is probably a good idea.

@@ -106,12 +106,36 @@ swift::_SwiftEmptySetStorage swift::_swiftEmptySetStorage = {
0 // int entries; (zero'd bits)
};

#if defined(__Fuchsia__)
#include <zircon/syscalls.h>
static __swift_uint64_t randomUInt64() {

This comment has been minimized.

Copy link
@petrhosek

petrhosek Nov 23, 2017

This should be using getentropy which is provided by Fuchsia's libc, not zx_cprng_draw which is a syscall and considered to be implementation detail.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

Yeah, I wrote this before that was implemented and had our security guys give me an ok. Our getentropy function ends up doing nearly the same thing (except this has some protection if we actually read was less than requested). I was going to wait for TO-460 was finished and then we can just use std::random_device like every other arch in this ifdef.

@@ -122,6 +124,9 @@ macro(configure_sdk_darwin
set(SWIFT_SDK_${prefix}_OBJECT_FORMAT "MACHO")

foreach(arch ${architectures})
# On Darwin, all archs share the same SDK path.
set(SWIFT_SDK_${prefix}_ARCH_${arch}_PATH "${SWIFT_SDK_${prefix}_PATH}")

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

Having SWIFT_SDK_${prefix}_PATH as the only variable works for Darwin since the SDK has all the archs compiled and lipo'd together in a single directory but other platforms have arch specific sysroot paths. This assumption was limiting other Unix SDKs to only supporting one arch at time. I fixed it here. For Darwin each arch specific SDK path actually just points to the same path.

@zbowling zbowling force-pushed the google:fuchsia_pr branch to a499c41 Nov 23, 2017
help="list of additional targets to cross-compile the Swift standard "
"library for in addition to default",
action=arguments.action.concat, type=arguments.type.shell_split,
default=None)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 23, 2017

Author Contributor

I added this arg so that you can append to the default list instead of replacing what would be there normally for the current platform. For Darwin, it is macos/ios/watch/tv/etc SDKs for each supported arch (which is a very long list to recreate). For our build I just want to append "fuchsia-x86_64,fuchsia-aarch64" to whatever would be default for the platform.

Copy link
Contributor

jrose-apple left a comment

Assorted comments from me. I'd also like to be sure someone outside Fuchsia is willing to sign off on the CMake and build-script changes. @Rostepher and @modocache are good candidates.

echo "${product}: using lld linker"
if [[ "${product}" != "swift" ]]; then
# All other projects override the linker flags to add in
# lld linker support.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Does this include LLVM/Clang too? Seems like those should support LLVM_ENABLE_LLD.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

Those are getting by with the LLVM_ENABLE_LLD flag. This is doing the same logic we do for -fuse-ld=gold above.

@@ -972,6 +972,10 @@ def create_argument_parser():
help="the absolute path to libtool. Default is auto detected.",
type=arguments.type.executable,
metavar="PATH")
parser.add_argument(

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

I think build-script options are supposed to match build-script-impl whenever possible, so this should be --use-lld-linker. (That also makes sense because it's conceivable that the option would build lld, since it's an LLVM project.)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

that makes sense. I'll change that over.

I actually added another arg locally that I'm going to push soon as part of fixing things so that we can cross compile from a Darwin toolchain to Linux/Fuchsia/BSD to be able to use lld only for the stdlib for specific sdk targets. It's a bit funky how that maps though though the python/shell/cmake layers since we don't have any sdk specific targeting args from the buid-util anywhere else. Looks something like:

--use-lld-for-stdlib=linux-x86_64,linux-aarch64,fuchsia-x86_64,fuchsia-aarch64

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed. now is --use-lld-linker in frontend script.

@@ -1069,7 +1066,8 @@ function(_add_swift_library_single target name)
set(library_search_directories
"${SWIFTLIB_DIR}/${SWIFTLIB_SINGLE_SUBDIR}"
"${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFTLIB_SINGLE_SUBDIR}"
"${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}")
"${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}"
"${SWIFT_NATIVE_SWIFT_TOOLS_PATH}/../lib/swift/${SWIFT_SDK_${SWIFTLIB_SINGLE_SDK}_LIB_SUBDIR}/${SWIFTLIB_SINGLE_ARCHITECTURE}")

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

We should not search both of these; either the arch is part of the path or it isn't.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

yeah, I can split that out. I originally didn't have two code paths for darwin/non-darwin code paths for adding libs but it ended up going that way because the logic for setting up cmake dependencies to the lipo task got a bit convoluted.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Ah, that makes sense why you'd do it this way. I'd still like to go with the stricter way, though.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed. I test if you are targeting "elf" and search in an arch specific directory. I could flip it to test on mach instead if the windows PE logic wants to do something similar to what I'm doing for ELF.

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 28, 2017

Contributor

Yeah, I suspect Mach-O fat binaries are the weird ones here, not the other way around.

/// On Darwin we link fat libs in the root of the library path and on other OS
/// targets we look up libraries in the target arch's subfolder.
static void getRuntimeStaticLibraryPathWithArch(
SmallVectorImpl<char> &runtimeLibPath,

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Nitpick: rather than right-aligning, we just double-indent these.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed.

@@ -1689,6 +1708,7 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
Arguments.push_back(context.Args.MakeArgString(StaticRuntimeLibPath));

SmallString<128> linkFilePath = StaticRuntimeLibPath;
llvm::sys::path::remove_filename(linkFilePath); // remove arch name
llvm::sys::path::append(linkFilePath, "static-executable-args.lnk");

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Are these not arch-specific?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

They currently are not. I could see them possibly being arch specific but right now we use the same libraries everywhere.

static-executable and static-library are a little busted though and do some squirrely stuff. Some of the libs end up getting doubled in the linker script and with static-executable it ends up linking in all symbols and not allowing the linker to strip at all which ends up making huge binaries unnecessarily. Was going to poke this in a later commit because it doesn't seem to being doing anything too useful for Linux right now and it's something we will probably want on Fuchsia.

invocationArgStrs.insert(invocationArgStrs.end(), {
// Most of the good portable functions used in the Glibc module are hidden
// behind this.
"-D_GNU_SOURCE",

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Is it possible to write a test for this? (Not that the macro is defined, but that we actually get one of the "good portable functions" that was previously hidden.)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

sure yeah. getline is a really common one that goes missing without a feature define. Also could do dladdr, dlsym, lseek64, or getaddrinfo_a.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

done. added a LibcFeatureFlag.swift test in the last commit.

@@ -56,7 +57,11 @@ foreach(sdk ${SWIFT_SDKS})
set(GLIBC_SYSROOT_RELATIVE_INCLUDE_PATH "/system/develop/headers/")
else()
# Determine the location of glibc headers based on the target.
set(GLIBC_SYSROOT_RELATIVE_INCLUDE_PATH "/usr/include")
if(${sdk} STREQUAL "FUCHSIA")

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Nitpick: elseif?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

Yeah sorry. I wrote this before the Haiku port landed and just wrapped what it was doing in my version in the else in the merge. Will rethink this.

Part of it is borked anyways because the module is baking in absolute paths on the machine that compiled it (which is fine when you have absolute paths that reference well known paths like /usr/include but not fine when you have a sysroot for another machine in some directory). We need to get VFS mappable module importer working to support SDKs that are relocatable with glibc.modulemap properly. Different problem to solve though after this to get cross compiling working properly. For us we have a libc.modulemap in Fuchsia that people will use instead of the Glibc module map (we only use glibc module map for tests right now). Would like to get things working cleaner though everywhere.

@@ -89,7 +94,7 @@ foreach(sdk ${SWIFT_SDKS})

# If this SDK is a target for a non-native host, create a native modulemap
# without a sysroot prefix. This is the one we'll install instead.
if(NOT "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_PATH}" STREQUAL "/")
if(NOT "${SWIFT_SDK_${sdk}_PATH}" STREQUAL "/")

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

@modocache, is this correct? Is it supposed to be the target SDK, not the host SDK?

@@ -18,7 +18,7 @@
///
//===----------------------------------------------------------------------===//

#if (defined(__ELF__) || defined(__ANDROID__)) && !defined(__HAIKU__)
#if (defined(__ELF__) || defined(__ANDROID__) || defined(__Fuchsia__)) && !defined(__HAIKU__)

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

Out of curiosity: Fuchsia doesn't define __ELF__?

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

It does :) I didn't look closely and was adding defines wherever I seen tests for ANDROID early on. We can kill it for android and fuchsia.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed. also cleaned up the __ANDROID__ check and cleaned up #endif comment.

@@ -1486,7 +1486,7 @@ static bool ParseIRGenArgs(IRGenOptions &Opts, ArgList &Args,

if (Args.hasArg(OPT_use_jit))
Opts.UseJIT = true;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Nov 27, 2017

Contributor

More whitespace changes snuck into this commit. In general, please don't make whitespace changes at all (even good ones) if you're not touching that part of a file.

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 27, 2017

Author Contributor

oops yeah. I'll split that out. My editors are pretty crazy about killing trailing whitespace. :)

This comment has been minimized.

Copy link
@zbowling

zbowling Nov 28, 2017

Author Contributor

fixed. (removed this file from that commit. I'll do a white space PR cleanup later)

@zbowling zbowling force-pushed the google:fuchsia_pr branch from a499c41 Nov 28, 2017
@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 28, 2017

I separated this PR out into five separate PRs. This one specifically, adding Fuchsia support, is dependent on the other four for the Fuchsia port to function properly. The other PRs should work on their own and independently of each other.

I also cleaned up, squashed, split, and cleaned up the commits on this branch for the PR split.

@zbowling zbowling changed the title Add Fuchsia OS and lld linker support Add Fuchsia OS support Nov 28, 2017
zbowling added 3 commits Nov 16, 2017
Allows compiling swift, clang, llvm, etc with lld instead of gold on
Linux and cross compile targets.
On platforms without fat binaries (anything but Darwin), we now build
and install stdlibs for each arch in it's own folder. Namely
lib/swift/<platform>/<arch> instead of lib/swift/<platform>.
@zbowling zbowling force-pushed the google:fuchsia_pr branch Dec 5, 2017
@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Dec 5, 2017

This branch was getting stale waiting to merge all the other PRs so I rebased it on the progress in those our PRs and merged all my changes from master including porting over to the new build script driver arguments format.

@zbowling zbowling force-pushed the google:fuchsia_pr branch to 2836fc8 Dec 6, 2017
zbowling added 4 commits Nov 16, 2017
Adds Fuchsia target support to the compiler and builds the stdlib for
Fuchsia.
Some functions and types are hidden behind feature flags.

_GNU_SOURCE exposes the most symbols and methods in glibc
and musl. Currently only turning it on for the Fuchsia triple.

Also add a test for the missing extended C functions in the Glibc
module.
@Dev1an Dev1an mentioned this pull request Dec 10, 2017
@zbowling zbowling force-pushed the google:fuchsia_pr branch 2 times, most recently Dec 12, 2017
@zbowling zbowling force-pushed the google:fuchsia_pr branch to e3438e4 Dec 13, 2017
@zbowling zbowling force-pushed the google:fuchsia_pr branch Jan 11, 2018
@zbowling zbowling force-pushed the google:fuchsia_pr branch to 85bce8d Jan 11, 2018
@zakrid

This comment has been minimized.

Copy link

zakrid commented Apr 27, 2018

any updates to this PR? very interesting to see the work being done here

@IOOI-SqAR

This comment has been minimized.

Copy link

IOOI-SqAR commented Dec 14, 2018

Is this still being worked on? @zbowling ?

@CodaFi

This comment has been minimized.

Copy link
Collaborator

CodaFi commented Nov 17, 2019

@zbowling It pains me to have to close this since there's so much good work in here. But it's been long enough and this has accumulated enough merge conflicts that there isn't a way to take it in its current form. If there are still plans for Swift support in Fuchsia, even preliminary support, please rebase this patch and reopen this pull request or shoot us another one.

Again, I'm so sorry we couldn't resolve this at the time. It would have been incredible to have first-class support for Swift.

@CodaFi CodaFi closed this Nov 17, 2019
@zbowling

This comment has been minimized.

Copy link
Contributor Author

zbowling commented Nov 19, 2019

Sounds good. I still hope to be able to circle back on this some day but for now my internal priorities have changed. A massive amount has changed in both Fuchsia and Swift since I worked on this and I was having trouble in the moment when I was actively working on it full time keeping up with both. Things seem to have started to settle I think so giving it another attempt now may get much further. Fuchsia now has an actual SDK target that gets generated that has more stable ABI and layout that a Swift build could consume and build against instead of what I was doing here pulling in a full Fuchsia tree to get sysroot and build artifacts to link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.