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

modocache · 2016-03-14T18:04:41Z

What's in this pull request?

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:

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

#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).
#1442 will build upon the work here by adding an Android module map:

diff --git a/stdlib/public/Glibc/CMakeLists.txt b/stdlib/public/Glibc/CMakeLists.txt
index c152ab5..f898636 100644
--- a/stdlib/public/Glibc/CMakeLists.txt
+++ b/stdlib/public/Glibc/CMakeLists.txt
@@ -18,6 +18,8 @@ foreach(SDK ${SWIFT_SDKS})
     else()
       set(GLIBC_ARCH_INCLUDE_PATH "${GLIBC_INCLUDE_PATH}")
     endif()
+  elseif("${SDK}" STREQUAL "ANDROID")
+    set(GLIBC_INCLUDE_PATH "${SWIFT_ANDROID_SDK_PATH}/usr/include")
   endif()

   # Verify that the location is valid.
@@ -30,6 +32,8 @@ foreach(SDK ${SWIFT_SDKS})
   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()
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
index 135f575..1098d51 100644
--- a/lib/ClangImporter/ClangImporter.cpp
+++ b/lib/ClangImporter/ClangImporter.cpp
@@ -351,12 +351,15 @@ getNormalInvocationArguments(std::vector<std::string> &invocationArgStrs,
       });
     }
   } else {
    // The module map used for Glibc depends on the target we're compiling for,
    // and is not included in the resource directory with the other implicit
-    // module maps. It's at {freebsd|linux}/{arch}/glibc/module.map.
+    // module maps. It's at {android|freebsd|linux}/{arch}/glibc/module.map.
     auto GlibcModuleMapPath =
       llvm::SmallString<128>(searchPathOpts.RuntimeResourcePath);
-    if (triple.isOSFreeBSD()) {
+    if (triple.isAndroid()) {
+      llvm::sys::path::append(GlibcModuleMapPath, "android");
+    } else if (triple.isOSFreeBSD()) {
       llvm::sys::path::append(GlibcModuleMapPath, "freebsd");
     } else {
       llvm::sys::path::append(GlibcModuleMapPath, "linux");

Resolved bug number: None

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

tkremenek · 2016-03-14T18:30:40Z

@swift-ci Please smoke test

modocache · 2016-03-14T18:48:22Z

Hmm, looks like a legitimate failure when running the swift-package-manager tests on Linux. I'll check it out, thanks!

Any comments on the overall approach would be very much appreciated!! 🙇

gribozavr · 2016-03-14T20:08:17Z

I haven't reviewed CMake code line-by-line, but this looks like the right direction to me.

modocache · 2016-03-14T20:22:20Z

Excellent, thanks @gribozavr! That's reassuring.

modocache · 2016-03-14T21:39:15Z

@tkremenek @gribozavr Updated! The tests should now pass, if one of you could do me the favor of asking @swift-ci to please test.

gribozavr · 2016-03-14T22:04:08Z

@swift-ci Please test

modocache · 2016-03-14T23:54:59Z

Hmm, looks like some OS X errors this time. Strange, since this shouldn't affect OS X at all. I'll look into it more--thanks!

modocache · 2016-03-15T04:07:11Z

Updated once again. I added a check such that -fmodule-map-file= is only specified if the Glibc module map actually exists. This fixes the test failures on OS X, which were caused because swiftc -target x86_64-unknown-linux-gnu -emit-ir was being invoked, even though the compiler was not built to support that target (and so the module map did not exist at the specified path).

This got all my tests passing on both OS X and Linux locally--fingers crossed it also works on CI!

@gribozavr, is it alright that I left a FIXME in there? Also, an alternative approach would have been to modify the tests such that swiftc -target x86_64-unknown-linux-gnu -emit-ir is only tested on Linux hosts. Would this be preferable?

gribozavr · 2016-03-15T04:10:39Z

It would be best to keep the current extended coverage.

modocache · 2016-03-15T04:16:02Z

Excellent. Well, this pull request has all the same test coverage, but can also handle the Android changes in #1442 -- whaddya think? Also, could you kick off a test?

gribozavr · 2016-03-15T04:17:04Z

@swift-ci Please test

gribozavr · 2016-03-15T04:18:44Z

CC @jrose-apple for Clang importer changes.

gribozavr · 2016-03-15T04:19:10Z

stdlib/public/Glibc/CMakeLists.txt

-    Glibc.swift
-    Misc.c
+  Glibc.swift
+  Misc.c


CMake code uses 4 spaces for indentation on continuation lines.

gribozavr · 2016-03-15T04:26:27Z

CMake changes LGTM modulo comments.

jrose-apple · 2016-03-15T16:50:44Z

lib/ClangImporter/ClangImporter.cpp

+        llvm::SmallString<128>(searchPathOpts.RuntimeResourcePath);
+    }
+    if (!GlibcModuleMapPath.empty()) {
+      if (triple.isOSFreeBSD()) {


Please use swift::getPlatformNameForTriple instead of hardcoding names here.

modocache · 2016-03-15T17:38:34Z

@jrose-apple Thanks for the review!! I hope I've addressed all of your comments--I'm very new to C++, let alone llvm/ADT, so I can't be sure I did. 😅

modocache · 2016-03-15T21:42:37Z

@gribozavr All tests pass locally for me. Could you ask @swift-ci to test? Also, any additional feedback here?

jrose-apple · 2016-03-15T22:28:11Z

lib/ClangImporter/ClangImporter.cpp

+    // a Swift compiler not built for Linux targets.
+    if (llvm::sys::fs::exists(GlibcModuleMapPath)) {
+      invocationArgStrs.insert(invocationArgStrs.end(), {
+        (SmallString<128>("-fmodule-map-file=") + GlibcModuleMapPath).str(),


I think this is still going to go through std::string, but maybe that's all right. I forgot this is building up a vector of std::strings anyway. Let's go back to the Twine version.

Actually, you can simplify it a bit with push_back. insert is just a trick for adding multiple arguments at once.

invocationArgStrs.push_back( (Twine("-fmodule-map-file=") + GlibcModuleMapPath).str());

Thanks for the tips! Updated to use push_back() and Twine.

jrose-apple · 2016-03-15T22:29:51Z

@swift-ci Please test

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

modocache · 2016-03-15T22:42:35Z

I wonder if amending the commit to use Twine caused CI to abort the test run...?

jrose-apple · 2016-03-15T22:52:06Z

It's probably still running, but I guess GitHub can't report status for it. I'll trigger it again.

jrose-apple · 2016-03-15T22:52:11Z

@swift-ci Please test

modocache · 2016-03-16T02:17:09Z

@jrose-apple @gribozavr Hooray, everything passed! Does this look good to merge?

jrose-apple · 2016-03-16T03:25:54Z

Importer changes look good to me. 👍

gribozavr · 2016-03-16T03:32:10Z

CMake LGTM!

modocache · 2016-03-16T05:08:12Z

Well, if everything looks fine to everyone and the CI says it's alright, I guess I've got no choice but to merge this! 😁

Thanks again for the reviews! This will be a big help for #1442.

[Glibc] Configure modulemap for target, not host

jrose-apple · 2016-03-16T16:38:45Z

stdlib/public/Glibc/CMakeLists.txt

-swift_install_in_component(stdlib
-    FILES "${output_dir}/module.map"
-    DESTINATION "lib/swift/glibc")
+    swift_install_in_component(stdlib


It looks like we've got a problem: this shows up as part of the install component even when we're not building for Linux or FreeBSD. Maybe this just needs to go in the if below? (Maybe everything needs to be guarded by that if?)

Good call, I believe I may have misinterpreted some of the comments in the pull request review. I could place an if("${SDK}" STREQUAL "LINUX" OR "${SDK}" STREQUAL "${FREEBSD}") at the very beginning of the inner loop--does that sound right?

Maybe. Why would "${FREEBSD}" be a variable?

I copy-pasted from five lines down, but you're absolutely right. I'll change that part, too. 😅

modocache force-pushed the target-specific-glibc branch from 2766687 to b62940d Compare March 14, 2016 18:19

modocache mentioned this pull request Mar 14, 2016

Port to Android #1442

Merged

modocache force-pushed the target-specific-glibc branch from b62940d to b4ebbe7 Compare March 14, 2016 21:38

modocache force-pushed the target-specific-glibc branch from b4ebbe7 to 826e0dc Compare March 15, 2016 02:11

gribozavr reviewed Mar 15, 2016
View reviewed changes

jrose-apple reviewed Mar 15, 2016
View reviewed changes

modocache force-pushed the target-specific-glibc branch from 826e0dc to 83cefbf Compare March 15, 2016 17:36

modocache force-pushed the target-specific-glibc branch 3 times, most recently from e3ddc84 to 097c9be Compare March 15, 2016 21:41

jrose-apple reviewed Mar 15, 2016
View reviewed changes

modocache force-pushed the target-specific-glibc branch from 097c9be to 04e1cd5 Compare March 15, 2016 22:40

modocache added a commit that referenced this pull request Mar 16, 2016

Merge pull request #1679 from modocache/target-specific-glibc

5e24af7

[Glibc] Configure modulemap for target, not host

modocache merged commit 5e24af7 into swiftlang:master Mar 16, 2016

modocache deleted the target-specific-glibc branch March 16, 2016 05:08

jrose-apple reviewed Mar 16, 2016
View reviewed changes

milseman mentioned this pull request Mar 16, 2016

Revert "[Glibc] Configure modulemap for target, not host" #1703

Merged

modocache mentioned this pull request Mar 16, 2016

[Un-revert][Glibc] Configure modulemap for target, not host #1704

Merged

1 task

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

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

Uh oh!

Conversation

modocache commented Mar 14, 2016

What's in this pull request?

Resolved bug number: None

Uh oh!

tkremenek commented Mar 14, 2016

Uh oh!

modocache commented Mar 14, 2016

Uh oh!

gribozavr commented Mar 14, 2016

Uh oh!

modocache commented Mar 14, 2016

Uh oh!

modocache commented Mar 14, 2016

Uh oh!

gribozavr commented Mar 14, 2016

Uh oh!

modocache commented Mar 14, 2016

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

gribozavr commented Mar 15, 2016

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

gribozavr commented Mar 15, 2016

Uh oh!

gribozavr commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 15, 2016

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

jrose-apple commented Mar 15, 2016

Uh oh!

jrose-apple commented Mar 15, 2016

Uh oh!

modocache commented Mar 16, 2016

Uh oh!

jrose-apple commented Mar 16, 2016

Uh oh!

gribozavr commented Mar 16, 2016

Uh oh!

modocache commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!