Port to Android #1442

Merged
merged 1 commit into from Apr 13, 2016

Projects

None yet
@modocache
Collaborator

What's in this pull request?

This adds an Android target for the stdlib. It is also the first example of cross-compiling outside of Darwin: a Linux host machine builds for an Android target.

Relevant mailing list discussions:

  1. https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151207/000171.html
  2. https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000492.html

The Android variant of Swift may be built and tested using the following build-script invocation:

$ utils/build-script \
  -R \                                           # Build in ReleaseAssert mode.
  -T \                                           # Run all tests.
  --android \                                    # Build for Android.
  --android-deploy-device-path /data/local/tmp \ # Temporary directory on the device where Android tests are run.
  --android-ndk ~/android-ndk-r10e \             # Path to an Android NDK.
  --android-ndk-version 21 \
  --android-icu-uc ~/libicu-android/armeabi-v7a/libicuuc.so \
  --android-icu-uc-include ~/libicu-android/armeabi-v7a/icu/source/common \
  --android-icu-i18n ~/libicu-android/armeabi-v7a/libicui18n.so \
  --android-icu-i18n-include ~/libicu-android/armeabi-v7a/icu/source/i18n/

Android builds have the following dependencies, as can be seen in the build script invocation:

  1. An Android NDK of version 21 or greater, available to download here: http://developer.android.com/ndk/downloads/index.html.
  2. A libicu compatible with android-armv7. You may build your own by cloning https://github.com/SwiftAndroid/libiconv-libicu-android and running the build.sh script in that repository.

What's worth discussing about this pull request?

Continuous integration: I'd be thrilled to have this merged into the main Swift repository, but I don't want to dump a bunch of code that no one can maintain. I think CI is a very important part of maintaining a healthy build, but:

  1. An Android-compatible build of libicu is necessary to build for Android.
  2. An Android device (or possibly emulator) is necessary to run the Android tests.

Do either of those sound like something CI servers could be configured with? Or can someone help me think of alternatives--perhaps building libicu as part of the Swift build, as we do with ninja? #1398 includes some pkg-config logic that may be useful here.

FIXMEs: There are quite a few spots labeled "FIXME" that I could use some help with. Feedback welcome!


Thanks a ton to @zhuowei, who deserves the vast majority of the credit. I just tweaked a few things. 😊

@weissi weissi and 2 others commented on an outdated diff Feb 25, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
+ } else if pid == 0 {
+ // pid of 0 means we are now in the child process.
+ // Capture the output and execute the program.
+ dup2(childStdout.writeFD, STDOUT_FILENO)
+ dup2(childStdin.readFD, STDIN_FILENO)
+ dup2(childStderr.writeFD, STDERR_FILENO)
+
+ var result: Int32 = 0
+ withArrayOfCStrings([Process.arguments[0]] + args) {
+ result = execve(Process.arguments[0], $0, _getEnviron())
+ }
+
+ if result != 0 {
+ preconditionFailure("execve() failed. errno: \(errno)")
+ }
+ _exit(126)
weissi
weissi Feb 25, 2016

this has different semantics to posix_spawn. posix_spawn will give you a proper errno value if the spawn failed, this just exits the child process. I.e. spawning ["/bin/zsh", "-c", "exit 126;"] will be indistinguishable for the caller, right? It will trigger the preconditionFailure, no idea what that actually means.

I'd propose to open a separate pipe, called child2parent or so which will be fcntld to FD_CLOEXEC. That means if execve succeeds, it will be closed automatically and that means the parent will read EOF. If execve fails, we just write execve's errno to that pipe so the parent can read it and learns what went wrong in the child. So the parent waits until it can read, if it reads EOF (read returns 0) it knows, the child spawned successfully, if it reads anything else, it knows that this is execve's errno value and can return that to the caller. Does that make sense?

Here some pseudo-C code which will hopefully make clear what I mean:

[...]
} else if (pid == 0) {
    int err = fcntl(child2parent[1], F_SETFD, FD_CLOEXEC); /* This fd will be closed on exec */
    int errno_save = errno;
    if (err) {
       preconditionFailure("fcntl failed, errno: %d", errno_save);
       abort();
    }
    execve(...);
    errno_save = errno;
    /* execve returned, this is an error that we need to communicate back to the parent */
    ssize_t suc_write = write(child2parent[1], &errno_save, sizeof(typeof(errno)));
    errno_save = errno;
    if (suc_write > 0 && suc_write < sizeof(typeof(errno)));
        /* implement write retry ;) */
    } else if (suc_write == 0) {
        /* preconditionFailure("EOF?!"); */
        abort();
    } else if (suc_write < 0) {
        preconditionFailure("write failed, errno %d", errno_save);
        abort();
    }
    close(child2parent[1]);
}
[...]
gribozavr
gribozavr Apr 6, 2016 Collaborator

@modocache This great comment by @weissi is still valid, I think.

modocache
modocache Apr 11, 2016 Collaborator

Finally getting around to this, sorry for the wait. @weissi, could you explain child2parent in greater detail? Would the child process file descriptors be duped to write to the parent pipe in some way? I think I'm missing something.

weissi
weissi Apr 11, 2016

Don't worry about the wait, great work on this PR! So the reason we need child2parent is to communicate from child to parent.

There's basically two possible outcomes

  1. everything went fine
  2. there was an error in the child (for example file to execute not found, no permission or what not)

In both cases, it's crucial that the child can tell the parent. The easiest way on UNIX is to just open a pipe (child2parent) in the parent before the fork. Because file descriptors are forked as well, we will have a communication channel from the child to the parent after the fork. child2parent[0] is the reading end, so that's the file descriptor the parent keeps open. And child2parent[1] is the writing end, that's the fd the child keeps open. In other words, if the child writes something to child2parent[1], the parent will receive it on child2parent[0]. So far so good, now we have a communication facility.

If there was an error execveing in the child, we can just write the errno value that it got into child2parent[1] and the parent will know what exactly went wrong. For example if the file to be executed wasn't found, a ENOENT will be transmitted and the parent can fail appropriately.

The seemingly easier case is actually the harder: What to do if there was no error in the child, i.e. the child executed another process. Then execve succeeds and we replaced the process with some other binary. The file descriptor child2parent[1] will now still be open so we could put something in there which tells the parent that everything went fine. However, the program we executed probably doesn't know it's supposed to do that. The standard trick to get around this issue is to set the file descriptor child2parent[1] as FD_CLOEXEC. If FD_CLOEXEC is set, the file descriptor is closed by the OS kernel automatically if execve succeeds. That's perfect for us: If child2parent[1] is closed without anything ever being written to the pipe, the parent can now learn that execve has succeeded which is exactly the missing big.

So the logic in the parent will be

  1. try to read on parent2child[0]
    2a. if read returns 0 (meaning EOF), the parent knows everything went fine, the child successfully executed another program
    2b. if read returns some bytes, we know that something went wrong and the value that we read is the errno of the child that it got when trying to execute the other program. That's awesome as we now know in the parent what went wrong in the child :).

Hope this makes any sense, if not or if there are any questions left, feel free to ask.

modocache
modocache Apr 12, 2016 Collaborator

Thank you @weissi, your explanation was incredibly helpful! Thanks for so clearly explaining the problem, it made writing the code much easier. I have a working implementation that I will amend to this commit soon; I'm running the Android test suite from #1714 to confirm it all still works (looks good so far!).

(For those that are interested, there are seven failing tests when the stdlib is compiled for Android. Most are related to assumptions in the test expectations themselves.)

modocache
modocache Apr 12, 2016 Collaborator

OK, I've amended changes as per your comments, @weissi! Again, thanks a ton!

1ace commented Feb 25, 2016

First off, thank you for this awesome work!
I haven't read much of it yet, but I just wanted to mention one thing: I think you should try to separate changes in multiple commits as much as possible to make it more digestible. That will help reviewers by letting them focus on related changes before moving on to the next commit.

My personal rule is: if you make sense to be between those two changes (ie. have one but not the other), put them in separate commits. You should obviously order those commits in a way that ensures you don't use features before adding them. For instance, the last commit should be the one adding the new target to utils/build-script once everything else is ready.

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
stdlib/public/Bionic/Glibc.swift
@@ -0,0 +1,215 @@
+//===----------------------------------------------------------------------===//
gribozavr
gribozavr Feb 25, 2016 Collaborator

Please don't duplicate these files. Can we find some way of reusing the Glibc overlay here, adding some #ifs into its source?

modocache
modocache Feb 25, 2016 Collaborator

Yes, I'll take another look at stdlib/public/Glibc/CMakeLists.txt. I think this should be possible. The difficult part is that since we're building for both Linux and Android at once, I'll need to use variables other than CMAKE_SYSTEM_NAME to determine which modulemap to copy. I'll try working on it some more, thanks!

modocache
modocache Mar 11, 2016 Collaborator

@gribozavr I've been trying to combine stdlib/public/Bionic and stdlib/public/Glibc.

stdlib/public/Glibc/CMakeLists.txt uses the host system name to either use a Linux or FreeBSD modulemap. I figure I can do something similar to use either a Linux or Android modulemap--but instead of using the host system name (which is always Linux when cross-compiling from Linux to Android), I produce a modulemap for each requested SDK and architecture:

# Unabbreviated source code here: https://gist.github.com/modocache/08e7aefd6c220d7750ac
foreach(SDK ${SWIFT_SDKS})
  foreach(arch ${SWIFT_SDK_${SDK}_ARCHITECTURES})
  string(TOLOWER "${SDK}" sdk)
  set(output_dir "${SWIFTLIB_DIR}/${sdk}/${arch}/glibc")

  # ...

  # Configure the modulemap based on the target. Each platform needs to
  # reference different headers, based on what's available in their glibc.
  set(modulemap_path "${CMAKE_CURRENT_BINARY_DIR}/${sdk}/${arch}/module.map")
  if("${SDK}" STREQUAL "FREEBSD")
    configure_file(module.freebsd.map.in "${modulemap_path}" @ONLY)
  elseif("${SDK}" STREQUAL "ANDROID")
    configure_file(module.android.map.in "${modulemap_path}" @ONLY)
  else()
    configure_file(module.map.in "${modulemap_path}" @ONLY)
  endif()

  # Create the lib/swift SDK/arch subdirectory and copy the configured
  # modulemap there.
  add_custom_command_target(unused_var
    COMMAND
        "${CMAKE_COMMAND}" "-E" "make_directory" "${output_dir}"
    COMMAND
        "${CMAKE_COMMAND}" "-E" "copy_if_different"
        "${modulemap_path}"
        "${output_dir}/module.map"
    CUSTOM_TARGET_NAME "copy_${sdk}_${arch}_glibc_module"
    OUTPUT "${output_dir}/module.map" "${output_dir}"
    DEPENDS "${modulemap_path}"
    COMMENT "Copying Glibc module to ${output_dir}")

  # Install the modulemap. When compiling Glibc, make sure
  # we use the correct module map for the current SDK.
  swift_install_in_component(stdlib
    FILES "${output_dir}/module.map"
    DESTINATION "${output_dir}")
  add_swift_library(swiftGlibc IS_SDK_OVERLAY
    ${sources}
    FILE_DEPENDS "copy_${sdk}_${arch}_glibc_module" "${output_dir}"
    TARGET_SDKS "${SDK}"
    SWIFT_COMPILE_FLAGS "-Xcc" "-fmodule-map-file=${modulemap_path}"
    INSTALL_IN_COMPONENT stdlib-experimental)
  endforeach()
endforeach()

The problem with this approach is that swiftc appears to expect that a Glibc modulemap exists at the following path:

/path/to/built/swift-linux-x86_64/lib/swift/glibc/module.map

Whereas the above CMake file places the module maps at two different paths, in order to avoid them overwriting one another:

/path/to/built/swift-linux-x86_64/lib/swift/linux/x86_64/glibc/module.map
/path/to/built/swift-linux-x86_64/lib/swift/android/armv7/glibc/module.map

This causes the following failure to occur when running tests that import Glibc:

<unknown>:0: error: missing required module 'SwiftGlibc'

Placing either one of these modulemaps at the expected path lib/swift/glibc/module.map gets the tests to pass for that target. That is, copying the Linux modulemap ensures the Linux build succeeds (and Android fails), and copying the Android modulemap causes Android to succeed and Linux to fail.

I'm not sure what the significance of the /path/to/built/swift-linux-x86_64/lib/swift/glibc/module.map path is, or how to hook up the compiler to use the SDK/arch subdirectory path lib/swift/android/armv7/glibc/module.map instead.

Any suggestions? I've been banging my head against this for a while, so any help would be greatly appreciated! 🙇

modocache
modocache Mar 14, 2016 Collaborator

OK, managed to figure this one out: #1679 -- I'd definitely appreciate feedback on that approach! 🙇

@gribozavr gribozavr commented on an outdated diff Feb 25, 2016
stdlib/public/runtime/Errors.cpp
@@ -132,10 +134,18 @@ reportBacktrace(int *count)
if (count) *count = 0;
return NULL;
}
+#if defined(__ANDROID__)
gribozavr
gribozavr Feb 25, 2016 Collaborator

Why not push the #if all the way up, to where #if !defined(__CYGWIN__) is?

@gribozavr gribozavr and 2 others commented on an outdated diff Feb 25, 2016
stdlib/public/stubs/Stubs.cpp
@@ -128,6 +128,26 @@ static locale_t getCLocale() {
}
#endif
+#ifdef __ANDROID__
+#define uselocale(a) (nullptr)
+static double swift_strtod_l(const char* a, char** b, locale_t c) {
+ return strtod(a, b);
+}
+static float swift_strtof_l(const char* a, char** b, locale_t c) {
+ return strtof(a, b);
gribozavr
gribozavr Feb 25, 2016 Collaborator

This won't implement the right semantics -- the current locale will affect the output on Android.

Also, please use spaces for indentation.

zhuowei
zhuowei Feb 26, 2016 Contributor

@gribozavr Android's Bionic libc doesn't have support for locales: the only supported locale is the "C" locale. (See https://github.com/android/platform_bionic/blob/master/libc/bionic/locale.cpp#L40)

gribozavr
gribozavr Feb 26, 2016 Collaborator

It says "// We currently support a single locale", so it can change.

You can do the same thing as Cygwin does.

zhuowei
zhuowei Feb 27, 2016 Contributor

@gribozavr On api 21 (Android 5.0) uselocale is present, so the real uselocale can be used. However, strtod_l and strtof_l are not present on Android even in the latest version (6.0), and strtold_l is also missing in 5.0 (the version we're targetting). So I don't think locale support is possible at this time.

modocache
modocache Mar 10, 2016 Collaborator

Here I've decided to:

  1. Remove the #define uselocale(a), since that function is available on API 21 (which this work is now based upon).
  2. Convert tabs to spaces.
  3. Preserve the definitions of strtod_l and friends here, since they're unavailable on Android.

Does that sound reasonable? Of course we can always improve upon this in the future.

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
test/IRGen/report_dead_method_call.swift
@@ -5,6 +5,9 @@
// RUN: %target-run %t/a.out p1 p2 2> %t/err3.log; FileCheck %s < %t/err3.log
// REQUIRES: executable_test
+// This test correctly outputs "fatal error" on an Android device, but
+// the Android test runner interprets the abort as a test failure.
gribozavr
gribozavr Feb 25, 2016 Collaborator

Could you rewrite it using StdlibUnitest then? It has special provisions for detecting crashes.

modocache
modocache Feb 25, 2016 Collaborator

Oh, good call! Can do. I'll submit this change as a separate pull request.

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
utils/android/adb/commands.py
@@ -0,0 +1,134 @@
+from __future__ import print_function
gribozavr
gribozavr Feb 25, 2016 Collaborator

License header?

modocache
modocache Feb 25, 2016 Collaborator

Yes of course, I knew I had forgotten something. I'll add one to all the new Python files.

Collaborator

Thanks for the feedback, @1ace! I agree: this work depends on #1396, #1410, #1426, #1395, #1334, which I've issued separately to make them easier to review. Unfortunately, GitHub falls apart with a dependent stack of commits, such as this pull request would end up being. I think you'll find other ports, such as Cygwin #1108, end up being similarly large.

The problem is that all of these changes are directly related to Android. Take the os(Android) definition, for example: I don't think it would make sense for the Swift maintainers to merge a pull request that added #if os(Android) ... #endif before Swift can even be built for Android. Phabricator would allow users to specify that the commit adding os(Android) is dependent upon the commit that adds Android options to CMakeLists.txt, and so forth. GitHub has no such system of specifying dependencies, and encourages large pull requests like this one.

Of course, if the review unearths more pieces I can split out into separate pull requests, I'll definitely do so! 👍

@gribozavr gribozavr commented on an outdated diff Feb 25, 2016
validation-test/stdlib/CollectionType.swift.gyb
@@ -8,6 +8,9 @@
// <rdar://problem/24357067> The compiler never finishes compiling validation-test/stdlib/CollectionType.swift.gyb in -O
// REQUIRES: swift_test_mode_optimize_none
+// FIXME: This test takes too long to complete on Android.
+// UNSUPPORTED: OS=linux-androideabi
gribozavr
gribozavr Feb 25, 2016 Collaborator

Is there a way we could split it up? Maybe something like validation-test/stdlib/Slice?

@gribozavr gribozavr commented on an outdated diff Feb 25, 2016
validation-test/stdlib/String.swift
import Glibc
#endif
StringTests.test("lowercaseString") {
// Use setlocale so tolower() is correct on ASCII.
- setlocale(LC_ALL, "C")
+ setlocale(Int32(LC_ALL), "C")
gribozavr
gribozavr Feb 25, 2016 Collaborator

I think the right way to fix this is to add an overlay for var LC_ALL or setlocale or both.

@modocache @zhuowei @ephemer Awesome job! I've been watching this since possibly day one. 👍 :shipit: 😄

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
utils/android/adb_test_runner/main.py
+ # on device.
+ program_name = os.path.basename(args.pop(0))
+
+ if len(args) == 1 and args[0] in ['-h', '--help']:
+ print(_help(program_name))
+ return 0
+
+ try:
+ executable_path, executable_arguments = args[0], args[1:]
+ except IndexError:
+ print(_usage(program_name))
+ print('{}: error: argument "executable_path" is required'.format(
+ program_name))
+ return 1
+
+ return execute_on_device(executable_path, executable_arguments, True)
gribozavr
gribozavr Feb 25, 2016 Collaborator

In our experience, executing tests on device without limiting concurrency will usually oversubscribe the device and will lead to spurious failures (out of memory issues etc.) That's because the number of device cores is typically much smaller than the number of cores on the host where lit is running.

modocache
modocache Feb 25, 2016 Collaborator

Ah yes, very true! One hacky solution would be to limit the number of parallel lit tests on Android. I'm sure I could find a way to do this in utils/build-script-impl or test/lit.cfg. I may even be able to query the number of device cores. Is this close to what you had in mind?

gribozavr
gribozavr Feb 25, 2016 Collaborator

That would unnecessarily limit the parallelism for running host (compiler) tests. What I was thinking about is adding a persistent server that would be responsible for device communication, and have %target-run be a thin client that just asks the server to run a certain command. The server would throttle (or possibly load balance over multiple devices!) as appropriate.

modocache
modocache Feb 25, 2016 Collaborator

This would be awesome. I assume we may use it to handle iOS/tvOS host tests as well some day?

One thought: should I make two pull requests? One to get Android building, and another to get its tests running and passing?

gribozavr
gribozavr Feb 25, 2016 Collaborator

Yes, that'd likely make things easier.

@gribozavr gribozavr and 2 others commented on an outdated diff Feb 25, 2016
stdlib/public/runtime/ProtocolConformance.cpp
+ if (!conformances) {
+ // if there are no conformances, don't hold this handle open.
+ dlclose(handle);
+ return;
+ }
+
+ // Extract the size of the conformances block from the head of the section
+ auto conformancesSize = *reinterpret_cast<const uint64_t*>(conformances);
+ conformances += sizeof(conformancesSize);
+
+ _addImageProtocolConformancesBlock(conformances, conformancesSize);
+
+ dlclose(handle);
+}
+
+static void android_iterate_libs(void (*callback)(const char*)) {
gribozavr
gribozavr Feb 25, 2016 Collaborator

Could we factor this code duplication?

modocache
modocache Feb 25, 2016 Collaborator

Absolutely! Will do.

tinysun212
tinysun212 Feb 27, 2016 Contributor

Just I made #1466, which implemented the idea I mentioned you several days ago in #1108 (Cygwin).
It is factored only for Linux/Cygwin (except APPLE), but I think many lines could be shared with Android.

@gribozavr gribozavr commented on an outdated diff Feb 25, 2016
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb
@@ -1089,6 +1092,8 @@ func _getOSVersion() -> OSVersion {
return .Linux
#elseif os(FreeBSD)
return .FreeBSD
+#elseif os(Android)
+ return .Android
gribozavr
gribozavr Feb 25, 2016 Collaborator

This needs tests like validation-test/stdlib/StdlibUnittestFreeBSD.swift.

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
lib/Driver/ToolChains.cpp
@@ -1211,12 +1211,33 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
Arguments.push_back(context.Args.MakeArgString(LibProfile));
}
- // FIXME: We probably shouldn't be adding an rpath here unless we know ahead
- // of time the standard library won't be copied.
- Arguments.push_back("-Xlinker");
- Arguments.push_back("-rpath");
- Arguments.push_back("-Xlinker");
- Arguments.push_back(context.Args.MakeArgString(RuntimeLibPath));
+ if (getTriple().isAndroid()) {
+ // FIXME: These should be set in CMake.
+ Arguments.push_back("-target");
+ Arguments.push_back("armv7-none-linux-androideabi");
+
+ const char* ndkhome = getenv("ANDROID_NDK_HOME");
gribozavr
gribozavr Feb 25, 2016 Collaborator

Is using this environment variable customary for targeting Android? What does gcc and clang do?

dcow
dcow Feb 26, 2016

This is customary with respect to the Android ndk build scripts. You set this and the build scripts handle constructing the paths given to gcc/clang while compiling. To my knowledge gcc/clang don't handle this themselves in any way.

@gribozavr gribozavr and 1 other commented on an outdated diff Feb 25, 2016
lib/Driver/ToolChains.cpp
@@ -1211,12 +1211,33 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
Arguments.push_back(context.Args.MakeArgString(LibProfile));
}
- // FIXME: We probably shouldn't be adding an rpath here unless we know ahead
- // of time the standard library won't be copied.
- Arguments.push_back("-Xlinker");
- Arguments.push_back("-rpath");
- Arguments.push_back("-Xlinker");
- Arguments.push_back(context.Args.MakeArgString(RuntimeLibPath));
+ if (getTriple().isAndroid()) {
gribozavr
gribozavr Feb 25, 2016 Collaborator

I'm not sure this condition is appropriate in a GenericUnix toolchain...

modocache
modocache Mar 15, 2016 Collaborator

Sorry for the slow turnaround on this one! I'm beginning to better understand the relationship between the Driver and the ToolChain. I'm thinking of modifying Driver::getToolChain such that it returns a toolchains::Android for Android targets. That should help isolate this Android-specific logic.

@gribozavr gribozavr and 3 others commented on an outdated diff Feb 25, 2016
lib/Driver/ToolChains.cpp
+ "arm-linux-androideabi-4.8/prebuilt/linux-x86_64/"
+ "lib/gcc/arm-linux-androideabi/4.8";
+ Arguments.push_back("-L");
+ Arguments.push_back(context.Args.MakeArgString(libgccpath));
+
+ auto libcxxpath = Twine(ndkhome) + "/sources/"
+ "cxx-stl/llvm-libc++/libs/armeabi-v7a";
+ Arguments.push_back("-L");
+ Arguments.push_back(context.Args.MakeArgString(libcxxpath));
+ } else {
+ // FIXME: We probably shouldn't be adding an rpath here unless we know ahead
+ // of time the standard library won't be copied.
+ Arguments.push_back("-Xlinker");
+ Arguments.push_back("-rpath");
+ Arguments.push_back("-Xlinker");
+ Arguments.push_back(context.Args.MakeArgString(RuntimeLibPath));
gribozavr
gribozavr Feb 25, 2016 Collaborator

Changes to the driver need to be accompanied by a driver test.

modocache
modocache Feb 25, 2016 Collaborator

Can do. Do you have a suggestion on how I could set these driver flags from within CMake? Is that possible? I'm not thrilled with the getenv() approach here.

gribozavr
gribozavr Feb 25, 2016 Collaborator

Typically, clang uses -sysroot to specify the path to a copy of the target filesystem. But, I'd recommend looking into what clang actually does for Android, what is the recommended way to invoke it, and trying to replicate that, as long as it makes sense.

gribozavr
gribozavr Feb 25, 2016 Collaborator

The CMake function is _add_variant_c_compile_link_flags in cmake/modules/AddSwift.cmake.

modocache
modocache Feb 25, 2016 Collaborator

Fantastic. I believe I tried that with limited success before, but I'll give it another go and ask questions if it can't get it working. Thanks!

grp
grp Feb 25, 2016

It looks like Android does use --sysroot= for this. I found that referenced here: http://developer.android.com/ndk/guides/standalone_toolchain.html (under "Simple method").

gribozavr
gribozavr Feb 25, 2016 Collaborator

OK. I think --sysroot corresponds to -sdk in the Swift driver. CC @jrose-apple

@gribozavr gribozavr commented on the diff Feb 25, 2016
lib/Basic/LangOptions.cpp
@@ -105,6 +106,8 @@ std::pair<bool, bool> LangOptions::setTarget(llvm::Triple triple) {
addPlatformConditionValue("os", "watchOS");
else if (triple.isiOS())
addPlatformConditionValue("os", "iOS");
+ else if (triple.isAndroid())
+ addPlatformConditionValue("os", "Android");
gribozavr
gribozavr Feb 25, 2016 Collaborator

This change needs a test like test/Parse/ConditionalCompilation/x64FreeBSDTarget.swift for each supported triple.

modocache
modocache Mar 14, 2016 Collaborator

Done! I added test/Parse/ConditionalCompilation/armAndroidTarget.swift.

Collaborator

@swift-ci Please smoke test

Collaborator

@modocache There seem to be build issues on Linux x86_64, please take a look when you have a moment.

Member

Looks like <mutex> is missing from the target C++ standard library:

In file included from /home/buildnode/jenkins/workspace/swift-PR-Linux/swift/stdlib/public/runtime/HeapObject.cpp:17:
/home/buildnode/jenkins/workspace/swift-PR-Linux/swift/include/swift/Basic/Lazy.h:19:10: fatal error: 'mutex' file not found
#include <mutex>
         ^

this is really cool, I've never seen a 52 file change commit though, good lord!

Collaborator

I'm working on addressing the feedback--thanks so much for the review, @gribozavr!

In the meantime, if anyone out there wants to help with this pull request, here's the output from the test suite: https://gist.github.com/modocache/48babfa768d495806116. Six tests are failing.

Some of those failures, like IRGen/objc_simd.sil, could be addressed outside of this pull request (for that test specifically, check out some of my notes in SwiftAndroid#16).

Collaborator
lattner commented Feb 25, 2016

@modocache This is really awesome to see, thank you for working on it. Per the previous comment, if you can split this up into smaller PR's, that will make it easier to review and merge piecemeal.

Collaborator

@modocache Excellent stuff! Keep those great contributions coming 👍

Excellent work, I'm looking forword to use it o.o

b3ll commented Feb 26, 2016

🎉

Keep up the great work @modocache! :D

pbassut commented Feb 26, 2016

Great work! @modocache

Good work @modocache

Collaborator

For those following this pull request:

  • I'm busy with other stuff this week, but will address feedback (like splitting this up) next week.
  • In the meantime, I'll rebase this work onto apple/swift master each morning (US time), and confirm it all still works for me.
  • If you'd like to help me address feedback, send pull requests to SwiftAndroid/swift's master branch.
  • I don't mind +1 comments, but not everyone likes them. Let's hold off on those! 😅

Enough congratulations people! I'm as excited about this as anyone, but either offer to help or stop spamming this issue with 👍 's ...

Collaborator

Here I've decided to:

  1. Remove the #define uselocale(a), since that function is available on API 21 (which this work is now based upon).
  2. Convert tabs to spaces.
  3. Preserve the definitions of strtod_l and friends here, since they're unavailable on Android.

Does that sound reasonable? Of course we can always improve upon this in the future.

SGTM! Thanks, @modocache!

@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 14, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).

`lldb-import-test` does not appear to pass a valid target triple to
ClangImporter, so we disable tests that use it outside of OS X.
2766687
@modocache modocache referenced this pull request Mar 14, 2016
Merged

[Glibc] Configure modulemap for target, not host #1679

0 of 1 task complete
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 14, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).

`lldb-import-test` does not appear to pass a valid target triple to
ClangImporter, so we disable tests that use it outside of OS X.
b62940d
@modocache modocache added a commit to SwiftAndroid/swift that referenced this pull request Mar 14, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).

`lldb-import-test` does not appear to pass a valid target triple to
ClangImporter, so we disable tests that use it outside of OS X.
03a8623
@modocache modocache added a commit to SwiftAndroid/swift that referenced this pull request Mar 14, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).

`lldb-import-test` does not appear to pass a valid target triple to
ClangImporter, so we disable tests that use it outside of OS X.
85d41ec
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 14, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
b4ebbe7
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 15, 2016
@modocache @modocache modocache + modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
826e0dc
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 15, 2016
@modocache @modocache modocache + modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
83cefbf
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 15, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
e3ddc84
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 15, 2016
@modocache @modocache modocache + modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
097c9be
@modocache modocache added a commit to modocache/swift that referenced this pull request Mar 15, 2016
@modocache @modocache modocache + modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
04e1cd5
@modocache modocache added a commit to SwiftAndroid/swift that referenced this pull request Mar 15, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
f97fead
@modocache modocache added a commit to SwiftAndroid/swift that referenced this pull request Mar 15, 2016
@modocache modocache [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
afe0e33
Collaborator

@gribozavr @jckarter First of all, thank you both for the early feedback! I've addressed much of it, and have also split this pull request in two. This one now only covers building the stdlib for Android, not testing it.

At 917 insertions and 40 deletions, it's still larger than the Cygwin port in #1108. I could split it up further, for example by sending a pull request to add --android flags to the build script first, before having them do anything at all. But I think that would add overhead in reviewing and merging this work, so I'm holding off on that unless you think I should.

One note: the Android ToolChain requires that the ANDROID_NDK_HOME environment variable be set. It's set by the Swift build script when building with --android. This is unfortunate, and I've received feedback that it makes the Driver logic more complicated. I agree, but for the time being have isolated the ugliness in an Android-specific ToolChain, which I hope is acceptable for now. I suppose an alternative is asking users to link libc++ and libgcc manually when invoking swiftc -target armv7-none-linux-androideabi.

Any and all feedback appreciated! 🙌


You may build Linux as usual, and to cross-compile to Android you may use the following command:

$ utils/build-script \
  -R \                                # Build in ReleaseAssert mode.
  --android \                         # Build for Android.
  --android-ndk ~/android-ndk-r10e \  # Path to an Android NDK.
  --android-ndk-version 21 \
  --android-icu-uc ~/libicu-android/armeabi-v7a/libicuuc.so \
  --android-icu-uc-include ~/libicu-android/armeabi-v7a/icu/source/common \
  --android-icu-i18n ~/libicu-android/armeabi-v7a/libicui18n.so \
  --android-icu-i18n-include ~/libicu-android/armeabi-v7a/icu/source/i18n/

Android builds have the following dependencies, as can be seen in the build script invocation:

  1. An Android NDK of version 21 or greater, available to download here: http://developer.android.com/ndk/downloads/index.html.
  2. A libicu compatible with android-armv7. You may build your own by cloning https://github.com/SwiftAndroid/libiconv-libicu-android and running the build.sh script in that repository.

To give the built stdlib a whirl, you'll need to push the build products to the device. #1714 includes details on how to do so (and automates the process for the purposes of the test suite).

@modocache modocache changed the title from [RFC] Port to Android to Port to Android Mar 16, 2016
@modocache modocache referenced this pull request Mar 16, 2016
Merged

[Android] Add testing support #1714

1 of 2 tasks complete
@jrose-apple jrose-apple and 1 other commented on an outdated diff Mar 18, 2016
lib/Driver/ToolChains.h
@@ -50,6 +50,20 @@ class LLVM_LIBRARY_VISIBILITY GenericUnix : public ToolChain {
~GenericUnix() = default;
};
+class LLVM_LIBRARY_VISIBILITY Android : public ToolChain {
jrose-apple
jrose-apple Mar 18, 2016 Owner

I think it would make more sense to inherit from GenericUnix. It seems fair to even make an overload point for LinkJobAction if the Android customization is easy to subset out.

modocache
modocache Mar 18, 2016 Collaborator

Good call. There's a lot of duplication here! toolchains::Windows is also very similar--do you think that should inherit from some more generic ToolChain as well?

@compnerd compnerd commented on the diff Mar 21, 2016
lib/Driver/Driver.cpp
@@ -2031,6 +2031,12 @@ const ToolChain *Driver::getToolChain(const ArgList &Args) const {
TC = new toolchains::Darwin(*this, Target);
break;
case llvm::Triple::Linux:
+ if (Target.isAndroid()) {
compnerd
compnerd Mar 21, 2016 Collaborator

I think that we should specialize GenericUnix into a GNUToolchain or LinuxToolchain and sink the android check into the toolchain. Android is an environment of linux, and the toolchain really is identical across the two. Creating two toolchains feels wrong. This has previously been a problem for modeling windows in clang, and it was a fair amount of effort to clean up afterwards. I think we should draw on our experiences there to improve our choices here.

modocache
modocache Mar 21, 2016 Collaborator

@gribozavr originally commented that an Android check may not be appropriate in a GenericUnix toolchain. I interpreted that to mean I should split Android out into its own toolchain, but I agree with you here, @compnerd. I'll rename GenericUnix to Linux, then include the isAndroid() check. Does that sound reasonable, @gribozavr?

jrose-apple
jrose-apple Mar 21, 2016 Owner

@compnerd What was the problem in Clang with a hierarchy of toolchains? It seems to me that littering branches everywhere creates very fiddly code, whereas a hierarchy with proper extension points makes it clear which parts might be different.

modocache
modocache Mar 23, 2016 Collaborator

@jrose-apple As you suggested, I'm going to go ahead and create a class hierarchy here. There's a ton of duplication across GenericUnix and Windows, so I'll begin by submitting a separate pull request that establishes the following hierarchy:

GenenricUnix
    Windows

Then this pull request could add an additional subclass:

GenenricUnix
    Windows
    Android

Does that seem reasonable? (Admittedly, it seems strange to have Windows inherit from GenericUnix...)

grp
grp Mar 23, 2016

toolchains::Windows came from the Cygwin port, so if you rename it toolchains::Cygwin, that's a little less strange to inherit from toolchains::GenericUnix.

modocache
modocache Mar 23, 2016 Collaborator

Totally agree. One potential wart is the fact that other parts of the codebase will reference things like os(Windows), whereas the toolchain will be named Cygwin. Even with that in mind, I still prefer naming the toolchain Cygwin, though.

jrose-apple
jrose-apple Mar 23, 2016 Owner

I expect os(Windows) to be true for both Cygwin and a hypothetical port to the MSVC runtime and toolchains. Naming the toolchain Cygwin seems fine to me.

tinysun212
tinysun212 Mar 24, 2016 Contributor

I am also agree renaming the toolchain Cygwin. I thought rename it if I add toolchain MSVC or Mingw someday.
and...
How about renaming GenericUnix to GNUToolchain?
The heirarchy will be

GNUToolchain
    Android
    Cygwin
    Mingw

Cygwin uses the GNU toolchain and has Unix compatible libraries, but it is not UNIX OS.

jrose-apple
jrose-apple Mar 26, 2016 Owner

We're using it for FreeBSD too, though. I think it's fair to say Cygwin is a Unix-like toolchain even if it's not a Unix-like target.

jrose-apple
jrose-apple Mar 26, 2016 Owner

(Maybe it is a Unix-like target too, and then not a Unix-like OS. But that's not about the Driver.)

@compnerd compnerd and 1 other commented on an outdated diff Mar 21, 2016
lib/Driver/ToolChains.cpp
+ llvm::sys::path::remove_filename(LibProfile); // remove platform name
+ llvm::sys::path::append(LibProfile, "clang", "lib");
+
+ llvm::sys::path::append(LibProfile, getTriple().getOSName(),
+ Twine("libclang_rt.profile-") +
+ getTriple().getArchName() +
+ ".a");
+ Arguments.push_back(context.Args.MakeArgString(LibProfile));
+ }
+
+
+ // Explicitly set the linker target to "androideabi", as opposed to the
+ // llvm::Triple representation of "armv7-none-linux-android".
+ // This is the only ABI we currently support for Android.
+ Arguments.push_back("-target");
+ Arguments.push_back("armv7-none-linux-androideabi");
compnerd
compnerd Mar 21, 2016 Collaborator

I think that the default vendor is spelt "unknown".

modocache
modocache Apr 20, 2016 Collaborator

Sorry, I thought I had left a reply for this, but it seems like it had slipped somehow. I took this target from the Android guides; see the section "Invoking the Compiler" here. There's a table on that page of architectures and corresponding values for -target:

Architecture Value
armeabi -target armv5te-none-linux-androideabi
armeabi-v7a -target armv7-none-linux-androideabi
arm64-v8a -target aarch64-none-linux-android
x86 -target i686-none-linux-android
x86_64 -target x86_64-none-linux-android
mips -target mipsel-none-linux-android
@gregomni gregomni added a commit to gregomni/swift that referenced this pull request Mar 22, 2016
@modocache @gregomni modocache + gregomni [Glibc] Configure modulemap for target, not host
The current Glibc CMakeLists.txt uses the host machine to determine
which modulemap to use. The same modulemap can't be used for all
platforms because headers are available in different locations on
different platforms.

Using the host machine to determine which modulemap to configure and
place at a specific path in the resource dir is fine, so long as:

1. Only one Glibc is being compiled in a single CMake invocation.
2. The target machine needs the same modulemap as the host.

apple#1442 violates both of these
assumptions: the Glibc module for both Linux and Android is compiled
at the same time, and the Android target can't use the Linux modulemap.

This commit instead uses the target(s) to determine which
modulemap to use. The modulemap is configured and placed in an OS-
and architecture-specific directory in the resource dir. The path to
that modulemap is referenced by the ClangImporter (since it is no
longer at a path that is automatically discovered as an implicit
modulemap).
6f3df98
@ephemer ephemer and 1 other commented on an outdated diff Apr 5, 2016
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb
@@ -1363,6 +1370,9 @@ public enum TestRunPredicate : CustomStringConvertible {
case linuxAny(reason: let reason):
return "linuxAny(*, reason: \(reason))"
+ case androidAny(reason: let reason):
+ return "AndroidAny(*, reason: \(reason))"
ephemer
ephemer Apr 5, 2016

Any reason the string returned here is upper-cased and the two around it are lower-cased?

modocache
modocache Apr 6, 2016 Collaborator

Good catch! These were originally LinuxAny, etc., before they were converted to lowercase linuxAny. I missed this spot when rebasing these changes. Thanks for pointing this out!

Collaborator

Thanks to the Driver changes in #1908, I think this is looking pretty good! And at +794 additions, −39 deletions, it's probably not the largest pull request ever merged.

@gribozavr @jrose-apple Could this get another set of reviews?

@gribozavr gribozavr and 1 other commented on an outdated diff Apr 6, 2016
lib/Driver/ToolChains.cpp
+}
+
+bool toolchains::Android::shouldSpecifyTargetTripleToLinker() const {
+ return false;
+}
+
+bool toolchains::Android::shouldProvideRPathToLinker() const {
+ return false;
+}
+
+std::vector<std::string> toolchains::Android::getAdditionalLinkerPaths() const {
+ // "ANDROID_NDK_HOME" is set by the Swift build script. Still, emit a nice
+ // assertion message here for people building via CMake directly (even though
+ // this is not something we support).
+ const char *ndkhome = getenv("ANDROID_NDK_HOME");
+ assert(ndkhome && "Environment variable ANDROID_NDK_HOME needs to be set "
gribozavr
gribozavr Apr 6, 2016 Collaborator

Please produce a proper error message (through DiagnosticEngine) if this is an issue that the user needs to fix. Not only asserts provide a bad user experience, but they are also turned off in release builds.

jrose-apple
jrose-apple Apr 8, 2016 Owner

Agreed. It's too bad that this logic needs to show up in multiple places, but that's where we are today. validateArgs in Driver.cpp would be a good place to do this, despite the name.

gribozavr
gribozavr Apr 11, 2016 Collaborator

But given the other discussion, we shouldn't be using an environment variable anyway.

@gribozavr gribozavr and 1 other commented on an outdated diff Apr 6, 2016
stdlib/public/Platform/glibc.android.modulemap.in
@@ -0,0 +1,373 @@
+//===--- module.map -------------------------------------------------------===//
gribozavr
gribozavr Apr 6, 2016 Collaborator

Please factor out the duplication between the new module map and existing ones. gyb seems to be a good tool for the job.

modocache
modocache Apr 6, 2016 Collaborator

Good idea! I did so in #2081.

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/public/stubs/LibcShims.cpp
@@ -18,8 +18,17 @@
#include <string.h>
#include "../SwiftShims/LibcShims.h"
+#ifdef __ANDROID__
+extern "C" {
+extern size_t dlmalloc_usable_size(void*);
+// arc4random_random is missing in Android headers, but does exist.
+extern unsigned int arc4random_uniform(unsigned int upper_bound);
gribozavr
gribozavr Apr 6, 2016 Collaborator

We don't use arc4random_uniform anymore, please remove this declaration.

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/public/stubs/LibcShims.cpp
@@ -18,8 +18,17 @@
#include <string.h>
#include "../SwiftShims/LibcShims.h"
+#ifdef __ANDROID__
+extern "C" {
+extern size_t dlmalloc_usable_size(void*);
+// arc4random_random is missing in Android headers, but does exist.
+extern unsigned int arc4random_uniform(unsigned int upper_bound);
+}
+#else
+// Android's ssize_t is an int, which is the same size as long int.
gribozavr
gribozavr Apr 6, 2016 Collaborator

Please adjust the definition of __swift_ssize_t instead of commenting out the sanity check.

This is not the first time people misunderstand the purpose of this static_assert, can you think of a better way to word the error message below?

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/public/stubs/LibcShims.cpp
@@ -18,8 +18,17 @@
#include <string.h>
#include "../SwiftShims/LibcShims.h"
+#ifdef __ANDROID__
+extern "C" {
+extern size_t dlmalloc_usable_size(void*);
gribozavr
gribozavr Apr 6, 2016 Collaborator

I would recommend sinking this forward declaration down to its only point of use below. That will reduce the number of #ifs and will keep the code focused.

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/public/stubs/Stubs.cpp
@@ -33,6 +33,20 @@
#include <sstream>
#include <cmath>
#define fmodl(lhs, rhs) std::fmod(lhs, rhs)
+#elif defined(__ANDROID__)
+#include <locale.h>
+static double swift_strtod_l(const char *nptr, char **endptr, locale_t loc) {
+ return strtod(nptr, endptr);
+}
+static float swift_strtof_l(const char *nptr, char **endptr, locale_t loc) {
+ return strtof(nptr, endptr);
+}
+static long double swift_strtold_l(const char *nptr, char **endptr, locale_t loc) {
gribozavr
gribozavr Apr 6, 2016 Collaborator

80 columns.

@gribozavr gribozavr and 1 other commented on an outdated diff Apr 6, 2016
stdlib/public/stubs/Stubs.cpp
@@ -33,6 +33,20 @@
#include <sstream>
#include <cmath>
#define fmodl(lhs, rhs) std::fmod(lhs, rhs)
+#elif defined(__ANDROID__)
+#include <locale.h>
+static double swift_strtod_l(const char *nptr, char **endptr, locale_t loc) {
gribozavr
gribozavr Apr 6, 2016 Collaborator

Please add a comment that this is only valid on Android because it does not support locales other than C. (Is that actually true?)

modocache
modocache Apr 11, 2016 Collaborator

Added a comment. As far as I know from others' comments and from the source code, I think it is true that Android does not support locales other than C. Of course, the Android SDK allows applications to be localized, but this support does not exist at the libc (Bionic) level.

@gribozavr gribozavr and 3 others commented on an outdated diff Apr 6, 2016
utils/build-script
@@ -740,6 +780,23 @@ the number of parallel build jobs to use""",
"--symbols-package.")
return 1
+ if args.android:
+ if args.android_ndk is None or \
+ args.android_ndk_version is None or \
+ args.android_icu_uc is None or \
+ args.android_icu_uc_include is None or \
+ args.android_icu_i18n is None or \
+ args.android_icu_i18n_include is None:
+ print_with_argv0("When building for Android, --android-ndk, "
+ "--android-ndk-version, --android-icu-uc, "
+ "--android-icu-uc-include, --android-icu-i18n, "
+ "and --android-icu-i18n-include must be "
+ "specified.")
+ return 1
+ # lib/Driver/ToolChains.cpp depends on this environment variable being
gribozavr
gribozavr Apr 6, 2016 Collaborator

If this is the compiler interface, then you don't need to be that specific. Interested engineers can grep the compiler source. Just say something like "Swift driver for targeting Android requires this variable".

gribozavr
gribozavr Apr 6, 2016 Collaborator

I feel like we had this discussion before, but I don't see this variable being consumed by clang. What does it do? Why do we need to differ?

ephemer
ephemer Apr 7, 2016

The variable is consumed within the CMAKE invocations as a base path to set the android sysroot, its toolchain, and things like the android c++ stdlib path (probably other things I've forgotten).

We could also pass these paths in individually from build-swift or elsewhere, but given the NDK is bundled and installed as it is consumed here, this seems to be the cleanest option

gribozavr
gribozavr Apr 7, 2016 Collaborator

The variable is consumed within the CMAKE invocations

I don't think so. We pass the CMake variable separately on its command line.

We could also pass these paths in individually from build-swift or elsewhere

I would very much prefer that, assuming that's what Clang does. Having the compiler behavior depend on environment variables seems very fragile to me, and I can see how it will lead to hard-to-debug situations for our users. Say, you see a line in the build log. You copy it into the terminal, and it does not work.

this seems to be the cleanest option

"Fewer changes to existing systems" is not always the cleanest and maintainable option.

compnerd
compnerd Apr 7, 2016 Collaborator

A very emphatic +1 on the passing the paths explicitly. clang already expects users to do that rather than set some obscure environment variable.

ephemer
ephemer Apr 7, 2016

I agree that it shouldn't have to be an environment variable. My comment about being cleaner relates to the fact that all of the above are just subpaths of ANDROID_NDK_HOME, so it seems unnecessary to pass each of them in individually.

I'd support a change to pass NDK_HOME itself into build-script as a variable, and only compile android if it is present. Not sure how this fits in with the logic of deciding which other platforms to build for though

gribozavr
gribozavr Apr 7, 2016 Collaborator

My comment about being cleaner relates to the fact that all of the above are just subpaths of ANDROID_NDK_HOME, so it seems unnecessary to pass each of them in individually.

Oh, if it is possible to reliably infer other paths from the base one, then by all means we should do it.

ephemer
ephemer Apr 7, 2016

My language about Cmake invocations was imprecise. We don't invoke Cmake with those variables, but there are a number of Cmake variables that build on top of this bath path and are used throughout the project

modocache
modocache Apr 12, 2016 Collaborator

It sounds like the consensus here is to require users to pass -L /path/to/libgcc -L /path/to/libcxx when invoking swiftc -- is that right?

@gribozavr gribozavr and 2 others commented on an outdated diff Apr 6, 2016
CMakeLists.txt
@@ -137,6 +137,20 @@ option(SWIFT_ENABLE_LTO
# The following only works with the Ninja generator in CMake >= 3.0.
set(SWIFT_PARALLEL_LINK_JOBS "" CACHE STRING
"Define the maximum number of linker jobs for swift.")
+set(SWIFT_ANDROID_NDK_PATH "" CACHE STRING
+ "Path to the directory that contains the Android NDK tools that are executable on the build machine")
+set(SWIFT_ANDROID_NDK_TOOLCHAIN_VERSION "" CACHE STRING
+ "A version of the toolchain to use when building for Android. Use 4.8 for 32-bit builds, 4.9 for 64-bit builds")
+set(SWIFT_ANDROID_SDK_PATH "" CACHE STRING
+ "Path to the directory that contains the Android SDK tools that will be linked to the swiftc frontend")
gribozavr
gribozavr Apr 6, 2016 Collaborator

"linked to the swiftc frontend" sounds like we will link some of Android libraries into the swiftc compiler. Is this true?

ephemer
ephemer Apr 7, 2016

I think the word "passed" would be more appropriate here than "linked"

modocache
modocache Apr 11, 2016 Collaborator

Updated to "passed". Indeed, we do not link any additional libraries to the swiftc compiler.

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
cmake/modules/AddSwift.cmake
@@ -220,11 +235,20 @@ function(_add_variant_link_flags)
RESULT_VAR_NAME result)
if("${LFLAGS_SDK}" STREQUAL "LINUX")
- list(APPEND result "-lpthread" "-ldl")
+ list(APPEND result "-lpthread" "-ldl"
+ # Include libbsd, which includes dependencies needed by Android stdlib
+ # targets, such as `arc4random_uniform`.
+ "${BSD_LIBRARIES}")
gribozavr
gribozavr Apr 6, 2016 Collaborator

We don't use libbsd anymore.

@gribozavr gribozavr commented on the diff Apr 6, 2016
cmake/modules/AddSwift.cmake
@@ -997,7 +1021,7 @@ function(_add_swift_library_single target name)
set_target_properties("${target}"
PROPERTIES
INSTALL_NAME_DIR "${install_name_dir}")
- elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
+ elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux" AND NOT "${SWIFTLIB_SINGLE_SDK}" STREQUAL "ANDROID")
gribozavr
gribozavr Apr 6, 2016 Collaborator

How do you handle library embedding on Android? Just curious.

ephemer
ephemer Apr 7, 2016

I may be misunderstanding the question but if you're referring to swift's dynamic library dependencies on android, they currently need to be added to the android project by hand alongside libswiftCore.so.

Then you use java's dlib api, i.e. System.loadLibrary("swiftCore")

gribozavr
gribozavr Apr 7, 2016 Collaborator

Can you use rpath (enabled by this if statement) so that you don't have to load them manually?

dant3
dant3 Apr 7, 2016

The Bionic linker-loader ignores RPATH afaicr

gribozavr
gribozavr Apr 7, 2016 Collaborator

Interesting. How do apps typically bundle libraries? Do they have to call System.loadLibrary()? Is that the recommended approach?

I'm just concerned about putting in a hack that would cause issues later. For example, how would an app with a Swift main() work? It won't be able to call loadLibrary() before running Swift code.

dant3
dant3 Apr 7, 2016

It's up to app developer how it will be loaded. Between other solutions one of the popular approaches it to link everything statically into libmyapp.so and then load it once with System.loadLibrary("myapp").

Usually you can't invoke something like main() in native code in Android directly (there are exceptions which usually depend on rooting your phone). Then your app starts it always starts in Java code, which then can instantly load native code and call C/Swift function of interest. Developers who don't want to use any Java code for their app can use NativeActivity as a shortcut, which comes from Android SDK and does exactly this - loads your library of choice and calls function android_main.

gribozavr
gribozavr Apr 7, 2016 Collaborator

I see. Thank you for the explanation. Given this platform choice, the current approach looks reasonable.

ephemer
ephemer Apr 7, 2016

Yes, in my use case it suffices to just run System.loadLibrary("mySwiftLibrary") at an appropriately early point in the app's lifecycle - the runtime linker finds the rest of the shared objects provided they're also in the APK bundle. Given that Android is basically a Java-first/-only platform (except for command line tools on rooted devices), this is the "best practices" approach as far as I can tell.

@gribozavr gribozavr commented on the diff Apr 6, 2016
stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb
@@ -1288,6 +1293,8 @@ public enum TestRunPredicate : CustomStringConvertible {
case freeBSDAny(reason: String)
+ case androidAny(reason: String)
gribozavr
gribozavr Apr 6, 2016 Collaborator

This change needs a test like validation-test/StdlibUnittest/FreeBSD.swift.

modocache
modocache Apr 11, 2016 Collaborator

Done! I added validation-test/StdlibUnittest/Android.swift.

@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
@@ -64,12 +67,38 @@ func posixPipe() -> (readFD: CInt, writeFD: CInt) {
/// stderr.
public func spawnChild(args: [String])
-> (pid: pid_t, stdinFD: CInt, stdoutFD: CInt, stderrFD: CInt) {
+ let childStdout = posixPipe()
+ let childStdin = posixPipe()
+ let childStderr = posixPipe()
+
+#if os(Android)
+ // posix_spawn isn't available on Android. Instead, we fork and exec.
+ let pid = fork()
+ if pid < 0 {
+ preconditionFailure("fork() failed")
gribozavr
gribozavr Apr 6, 2016 Collaborator
let pid = fork()
precondition(pid >= 0, "fork() failed")
if pid == 0 { ...
@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
+ let childStderr = posixPipe()
+
+#if os(Android)
+ // posix_spawn isn't available on Android. Instead, we fork and exec.
+ let pid = fork()
+ if pid < 0 {
+ preconditionFailure("fork() failed")
+ } else if pid == 0 {
+ // pid of 0 means we are now in the child process.
+ // Capture the output and execute the program.
+ dup2(childStdout.writeFD, STDOUT_FILENO)
+ dup2(childStdin.readFD, STDIN_FILENO)
+ dup2(childStderr.writeFD, STDERR_FILENO)
+
+ var result: Int32 = 0
+ withArrayOfCStrings([Process.arguments[0]] + args) {
gribozavr
gribozavr Apr 6, 2016 Collaborator
let result = withArrayOfCStrings(...) { execve(...) }
@gribozavr gribozavr commented on an outdated diff Apr 6, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
+ if pid < 0 {
+ preconditionFailure("fork() failed")
+ } else if pid == 0 {
+ // pid of 0 means we are now in the child process.
+ // Capture the output and execute the program.
+ dup2(childStdout.writeFD, STDOUT_FILENO)
+ dup2(childStdin.readFD, STDIN_FILENO)
+ dup2(childStderr.writeFD, STDERR_FILENO)
+
+ var result: Int32 = 0
+ withArrayOfCStrings([Process.arguments[0]] + args) {
+ result = execve(Process.arguments[0], $0, _getEnviron())
+ }
+
+ if result != 0 {
+ preconditionFailure("execve() failed. errno: \(errno)")
gribozavr
gribozavr Apr 6, 2016 Collaborator
precondition(result == 0, "...")
Collaborator

@modocache Reviewed. I'd be happy to review and merge smaller pull requests, if you can find a way to slice it further.

@modocache modocache referenced this pull request Apr 6, 2016
Merged

[Platform] Unify Linux/FreeBSD modulemaps with gyb #2081

0 of 1 task complete
Collaborator

Rebased onto #2081, we're now at +522 additions and −128 deletions. I'll address the feedback soon, but on the topic of splitting this up, I could do the following:

  1. Make all the CMake changes (although they never take effect because build-script does not pass any Android-related options at this point)
  2. Make the C++ and Swift source code changes (theoretically one could invoke CMake directly to build Android at this point)
  3. Add the build-script options to actually enable building Android

Would that make it easier to review these changes? @gribozavr

Collaborator

@modocache Thank you! Could you look at the comments I left? https://github.com/apple/swift/pull/1442/files

@jrose-apple jrose-apple commented on an outdated diff Apr 8, 2016
lib/Driver/ToolChains.cpp
@@ -1210,6 +1210,10 @@ std::string toolchains::GenericUnix::getDefaultLinker() const {
}
}
+std::string toolchains::GenericUnix::getTarget() const {
jrose-apple
jrose-apple Apr 8, 2016 Owner

Can you name this something like "getOverridingTarget" or "getTargetForLinker"?

Also, does it make sense to fold this into "shouldSpecifyTargetTripleToLinker", now that we have that? Maybe there's just one "getTargetForLinker" that returns the current target by default, an override on Android, and no target on Cygwin (or wherever).

@jrose-apple jrose-apple commented on an outdated diff Apr 8, 2016
lib/Driver/ToolChains.cpp
+
+std::vector<std::string> toolchains::Android::getAdditionalLinkerPaths() const {
+ // "ANDROID_NDK_HOME" is set by the Swift build script. Still, emit a nice
+ // assertion message here for people building via CMake directly (even though
+ // this is not something we support).
+ const char *ndkhome = getenv("ANDROID_NDK_HOME");
+ assert(ndkhome && "Environment variable ANDROID_NDK_HOME needs to be set "
+ "when building Swift to target Android. It should point to the directory "
+ "where the Android NDK has been installed, such as "
+ "/usr/local/opt/android-ndk-r10e.");
+
+ // FIXME: This ties the Android driver to the tree layout of Android's NDK,
+ // which is undesirable. It may be better to allow the build script
+ // to specify a path to these instead. This would also allow a
+ // different libc++ from within the Android NDK to be specified.
+ auto libgcc = Twine(ndkhome) + "/toolchains/arm-linux-androideabi-4.8/"
jrose-apple
jrose-apple Apr 8, 2016 Owner

Twines are temporary objects, unfortunately; they're not guaranteed to work correctly if stored in local variables. You'll have to do this all in one expression.

@jrose-apple jrose-apple and 1 other commented on an outdated diff Apr 8, 2016
lib/Driver/ToolChains.h
@@ -45,10 +45,14 @@ class LLVM_LIBRARY_VISIBILITY GenericUnix : public ToolChain {
virtual std::string getDefaultLinker() const;
+ virtual std::string getTarget() const;
jrose-apple
jrose-apple Apr 8, 2016 Owner

I probably should have required this on the last commit, but all of these need doc comments describing the default behavior.

modocache
modocache Apr 11, 2016 Collaborator

Sure thing! Here's a pull request for the already committed methods: #2134

@jrose-apple jrose-apple commented on an outdated diff Apr 8, 2016
...tPrivatePthreadExtras/SwiftPrivatePthreadExtras.swift
@@ -85,7 +85,7 @@ internal func _make_pthread_t() -> pthread_t {
#if os(Linux)
return pthread_t()
#else
- return nil
+ return 0
jrose-apple
jrose-apple Apr 8, 2016 Owner

This is not correct. Android should be in the Linux path here. Darwin and FreeBSD's pthread_ts are pointers.

@modocache modocache referenced this pull request Apr 11, 2016
Merged

[Toolchains] Document virtual methods #2134

0 of 1 task complete
@gribozavr gribozavr and 1 other commented on an outdated diff Apr 11, 2016
stdlib/public/Platform/glibc.modulemap.in.gyb
+ module sem {
+ header "@GLIBC_ARCH_INCLUDE_PATH@/sys/sem.h"
+ export *
+ }
+ module shm {
+ header "@GLIBC_ARCH_INCLUDE_PATH@/sys/shm.h"
+ export *
+ }
+ module statvfs {
+ header "@GLIBC_ARCH_INCLUDE_PATH@/sys/statvfs.h"
+ export *
+ }
+ module syslog {
+ header "@GLIBC_INCLUDE_PATH@/syslog.h"
+ export *
+ }
gribozavr
gribozavr Apr 11, 2016 Collaborator

syslog and other submodules weren't used to be a part of the sys submodule.

modocache
modocache Apr 11, 2016 Collaborator

Ack, this was a bad cut-and-paste job. Thanks for catching it!

Collaborator

OK, I think I've addressed all of the feedback so far. Most notably, the ugliness with storing ANDROID_NDK_HOME in an environment variable is gone. Instead, users are expected to link libgcc and libcxx when invoking swiftc. The test/lit.cfg in the Android testing pull request #1714 demonstrates. All but 8 tests pass.

We now stand at +540 additions and −146 deletions. As I mentioned before, I could theoretically break this pull request up even more. Should I do so? Or is this pull request good to review/merge as-is? @gribozavr

@gribozavr gribozavr commented on the diff Apr 12, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
+
+ let writeErrno = errno
+ if writtenBytes > 0 && writtenBytes < errnoSize {
+ // We were able to write some of our error, but not all of it.
+ // FIXME: Retry in this case.
+ preconditionFailure("Unable to write entire error to child-to-parent " +
+ "pipe.")
+ } else if writtenBytes == 0 {
+ preconditionFailure("Unable to write error to child-to-parent pipe.")
+ } else if writtenBytes < 0 {
+ preconditionFailure("An error occurred when writing error to " +
+ "child-to-parent pipe; errno: \(writeErrno)")
+ }
+
+ // Close the pipe when we're done writing the error.
+ close(childToParentPipe.writeFD)
gribozavr
gribozavr Apr 12, 2016 Collaborator

Does the parent read from the pipe?

modocache
modocache Apr 12, 2016 Collaborator

Ah, good point. I'm working on figuring out how StdlibUnittest works at the moment. I can see _ParentProcess._runTestInChild() reads from the file descriptors passed from spawnChild(), and I'm trying to figure out how to incorporate this new pipe in there. I should be able to figure this out eventually... 😅

Also, silly question, but is it just me or is runChild() not actually used anywhere?

modocache@ubuntu:~/GitHub/apple/swift$ git grep runChild
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift:public func runChild(_ args: [String])
modocache
modocache Apr 12, 2016 Collaborator

Still stumped by this. This is what I have locally, but the test suite stalls out at test/1_stdlib/Map.swift -- that test never finishes executing.

@gribozavr I'm tempted to ask whether we can punt this work to #1714, since the tests aren't even capable of being run for Android with just the changes in this pull request. At this point, anything that compiles would theoretically be acceptable.

modocache
modocache Apr 13, 2016 Collaborator

OK, I have an implementation that reads from the child process. However, this implementation fails for any StdlibUnittest that expects a crash. I think the reason why is because of how execve behaves when the program it runs crashes, but I'm not sure.

Documentation states that it "on success, execve() does not return". But in cases where a StdlibUnittest subprocess fails, I think it does return. This means that it writes errno to the parent pipe. When we read the errno out of the pipe, I don't think we have a way to tell whether the error has to do with execve() being unable to run a child process, or whether the child process crashed.

I might be missing something here, so feedback would be appreciated. Also, I still think we could hold off on this work for now. Once this pull request is merged, I can work on this more as part of #1714.

gribozavr
gribozavr Apr 13, 2016 Collaborator

I think it is OK to punt it to #1714.

weissi
weissi Apr 13, 2016

@modocache I'm currently looking from my phone so it's likely that it's there and I don't see it. After the fork, the parent and the child have both opened the read and the writeFD of the pipe. It's important (before doing anything else) to close the writeFD in the parent and the readFD in the child.

Because what we're looking for is a unidirectional comm channel from child to parent. The close will only be seen by the parent when the last writer closes the pipe. If writeFD is also open in the parent, the child closing the writeFD is not the last writer.

Code should be

if pid == 0 {
    close(pipe.readFD)
    fcntl(pipe.writeFD, close_on_exec)
    execve()
    Write(pipe.writeFD, errno
     Close(pipe.writeFD
} else if pid >= 0 {
    Close(pipe.writeFD) // very important 
    bytes = read(pipe.readFD, buffer)
    analyse bytes and buffer...
}

Excuse the horrible pseudo code, not fun to type on phone.

Does that make sense?

Collaborator

Modulo the comment about reading from the pipe, CMake, runtime, library and corresponding test changes LGTM. I defer to @jrose-apple for Driver changes.

@ephemer ephemer commented on the diff Apr 12, 2016
utils/build-script
@@ -700,6 +705,41 @@ the number of parallel build jobs to use""",
a '\'"""),
action="append", dest="extra_swift_args", default=[])
+ android_group = parser.add_argument_group(
+ title="Build settings for Android")
+ android_group.add_argument(
+ "--android-ndk",
+ help="An absolute path to the NDK that will be used as a libc "
+ "implementation for Android builds",
ephemer
ephemer Apr 12, 2016

This would suggest to me that I should look for a subpath of the NDK root / to manually find the libc/bionic headers within. Is that true or is this expecting the NDK root?

As an aside - ndk11b is now out (initial work was done on 10e), from what I understand the android team has now deprecated gcc in favour of clang. I've kind of lost track of what's happening with the swift android toolchain- is the current paradigm still sustainable?

modocache
modocache Apr 12, 2016 Collaborator

I'll have to try out the latest NDK, thanks for the tip! If the paths have changed then I'll make sure we support both.

Not sure about paradigms, but if you're asking whether this works, I would say it does! You should be able to run the test suite yourself by following the instructions in #1714.

modocache
modocache Apr 19, 2016 Collaborator

@ephemer Good call on the NDK version update. Support to the latest version is in #2244.

@jrose-apple jrose-apple commented on the diff Apr 12, 2016
lib/Driver/ToolChains.cpp
@@ -1340,15 +1341,23 @@ toolchains::GenericUnix::constructInvocation(const LinkJobAction &job,
return {"clang++", Arguments};
}
-std::string toolchains::Cygwin::getDefaultLinker() const {
- // Cygwin uses the default BFD linker, even on ARM.
- return "";
+std::string
+toolchains::Android::getTargetForLinker() const {
+ // Explicitly set the linker target to "androideabi", as opposed to the
+ // llvm::Triple representation of "armv7-none-linux-android".
+ // This is the only ABI we currently support for Android.
jrose-apple
jrose-apple Apr 12, 2016 Owner

Just to double-check, there's no arm64/aarch64 Android Swift yet?

modocache
modocache Apr 12, 2016 Collaborator

You're correct, we only support armv7.

jrose-apple
jrose-apple Apr 12, 2016 Owner

Let's actually assert that then. That way, we'll know very loudly when it changes.

assert(getTriple().getArch() == llvm::Triple::arm && "No other architectures supported for Android");

or something.

@jrose-apple jrose-apple commented on the diff Apr 12, 2016
lib/Driver/ToolChains.h
class LLVM_LIBRARY_VISIBILITY Cygwin : public GenericUnix {
protected:
std::string getDefaultLinker() const override;
- bool shouldSpecifyTargetTripleToLinker() const override;
jrose-apple
jrose-apple Apr 12, 2016 Owner

Cygwin should need to return "" from getTargetForLinker now, correct?

We should be able to write platform-independent tests for these, like those in test/Driver/linker.swift.

modocache
modocache Apr 12, 2016 Collaborator

Ah, nice catch, thanks! (I should really get a Cygwin environment so that I can catch these myself...)

Looking into adding tests now.

modocache
modocache Apr 12, 2016 Collaborator

Added tests to test/Driver/linker.swift 👍

jrose-apple
jrose-apple Apr 12, 2016 Owner

Ah, we probably want to include the negative check on Cygwin. For that you'll need to run FileCheck again on the same input but with a different prefix, something like

// RUN: FileCheck -check-prefix CYGWIN-x86_64-NEGATIVE %s < %t.cygwin.txt

// CYGWIN-x86_64-NEGATIVE-NOT: -target
modocache
modocache Apr 12, 2016 Collaborator

Neat trick! I'll add an ANDROID-armv7-NEGATIVE-NOT: -Xlinker -rpath. However I think the Cygwin negative expectation won't work here, since we explicitly invoke %swiftc_driver -target x86_64-unknown-windows-cygnus in the first place. So -target does appear in the output...

jrose-apple
jrose-apple Apr 12, 2016 Owner

Ah, you can mix CHECK and CHECK-NOT lines, in which case the -NOT line is checking that the output doesn't appear since the previous CHECK (or the beginning of the input) and before the next CHECK (or the end of the input).

Owner

The Driver changes being so small make me feel like we've finally got a good design. :-) Still a few minor comments, but the general plan looks good.

@modocache modocache and 1 other commented on an outdated diff Apr 12, 2016
stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift
+ // close the pipe if the execve() below successfully executes a child
+ // process.
+ let closeResult = fcntl(childToParentPipe.writeFD, F_SETFD, FD_CLOEXEC)
+ let closeErrno = errno
+ precondition(
+ closeResult == 0,
+ "Could not set the close behavior of the child-to-parent pipe; " +
+ "errno: \(closeErrno)")
+
+ // Start the executable. If execve() does not encounter an error, the
+ // code after this block will never be executed, and the parent write pipe
+ // will be closed.
+ withArrayOfCStrings([Process.arguments[0]] + args) { argumentCStrings in
+ execve(
+ Process.arguments[0],
+ argumentCStrings.map { UnsafeMutablePointer<Int8>($0) },
modocache
modocache Apr 12, 2016 Collaborator

I made this change after rebasing onto bc83940. Previously I could simply use $0 to pass the C strings directly to execve(). Not sure if this is the best way to do it -- thoughts? 🙌

jrose-apple
jrose-apple Apr 12, 2016 Owner

Hm. I changed withArrayOfCStrings to use UnsafePointer instead of UnsafeMutablePointer, but the only other client is swift_posix_spawn, whose declaration doesn't match the real posix_spawn. I'll change it back.

modocache
modocache Apr 12, 2016 Collaborator

Excellent, changed this back.

Owner

Driver changes all look good to me at this point.

@zhuowei @modocache zhuowei Port to Android
This adds an Android target for the stdlib. It is also the first
example of cross-compiling outside of Darwin.

Mailing list discussions:

1. https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151207/000171.html
2. https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000492.html

The Android variant of Swift may be built using the following `build-script`
invocation:

```
$ utils/build-script \
  -R \                                           # Build in ReleaseAssert mode.
  --android \                                    # Build for Android.
  --android-ndk ~/android-ndk-r10e \             # Path to an Android NDK.
  --android-ndk-version 21 \
  --android-icu-uc ~/libicu-android/armeabi-v7a/libicuuc.so \
  --android-icu-uc-include ~/libicu-android/armeabi-v7a/icu/source/common \
  --android-icu-i18n ~/libicu-android/armeabi-v7a/libicui18n.so \
  --android-icu-i18n-include ~/libicu-android/armeabi-v7a/icu/source/i18n/
```

Android builds have the following dependencies, as can be seen in
the build script invocation:

1. An Android NDK of version 21 or greater, available to download
   here: http://developer.android.com/ndk/downloads/index.html.
2. A libicu compatible with android-armv7.
7c502b6
Collaborator

@swift-ci Please test and merge

@swift-ci swift-ci merged commit 728d21a into apple:master Apr 13, 2016

1 check was pending

Test and Merge Build started.
Details
Siprah commented Apr 13, 2016

<3

: -|

zky001 commented Apr 13, 2016

great,perfect job,I will be more “choiceble”

so fast 0-0

au5ton commented Apr 13, 2016

Congratulations

just 'wow'

very promising 👍

congrats! This is exciting stuff!

ephemer commented Apr 13, 2016

@modocache @zhuowei @jrose-apple @gribozavr thank you all for your awesome work, looking forward to continuing with this

Amazing!!

Absolutely terrific work from everyone involved!

Incredible effort from all! Beautiful work! Looking forward to seeing this move further.

Wow, we just witnessed history in the making.

@kleinlieu true! Watch the media/news pick this up in a few days and milk it for weeks 😄

Anticipated

lovemo commented Apr 14, 2016

great

kiban18 commented Apr 14, 2016

Amazing!!

vapor99 commented Apr 14, 2016

Whoah! Very nice!

Ir1d commented Apr 14, 2016

Congrats!

I'm out?

kewang commented Apr 14, 2016

Today's Big Thing !!!

  1. Kobe last game
  2. Warriors 73 wins
  3. Swift accepted "Port to Android" PR

This is exciting!

Unsubscribbling, because spam. Can someone close, to pipe comments into, comfortably silent, reaction emojis? Also, fantastic work!

OMG, I think I must to learn Swift.

Exciting!

nolim1t commented Apr 14, 2016

Good work @modocache

Congratulations!

Exciting !

66666666666

Awesome

Great news 👍

Good job!

Nice work!

Excellent!

dodola commented Apr 15, 2016

👍 Excellent

@icepy icepy referenced this pull request in icepy/we-writing Apr 16, 2016
Open

Swift的期待 #35

Good job!

ray26 commented Apr 16, 2016

6666

😎 Nice

Great work.

otymartin commented Apr 17, 2016 edited

I just want to put my mark on this😎

istx25 commented Apr 18, 2016

Wow! Fantastic news. I'm excited to play around with this. Congrats to everyone involved. Your work is appreciated. 👏🏼👏🏼

chenp-fjnu commented Apr 19, 2016 edited

It's one very good new for developers, for many companies, they have IOS and Android version for every application, it will reduce the effort. Xamarin is another option, but Linux based system should support swift well. I love Swift. Wish it will become usable for real case soon.

Good Job !!!

@jrose-apple jrose-apple locked and limited conversation to collaborators Apr 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.