Can you separate the whitespace changes into a separate commit?

sure thing :)

I split the commit into two changes now

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

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

sential => sentinel ?

fixed.

Should this be "Fuschia" ?

Yes. rebase mistake.

fixed.

This should be "ld.lld".

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

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).

yeah it's for -fuse-ld

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.

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.

"Fuchsia"

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.

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).

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.

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

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

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

Yeah, that is the easiest to pull out.

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

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

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

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.

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

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.

https://bugs.swift.org/browse/SR-328

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

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.

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

Can you separate this out as well.

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.

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.

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.

fixed.

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?

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.

"-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.

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.

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 :)

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.

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.

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

Indentation seems off.

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.

Missing indentation?

woops. yep.

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

Extra blank line?

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

Sure

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.

awesome!

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.

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

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.

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.

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.

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.