Added -toolchain option to swiftc #2912

karwa · 2016-06-06T03:34:40Z

What's in this pull request?

Added a -toolchain swiftc option. This works like clang/gcc's -B flag (which allows specifying an alternate toolchain location). We need to be able to cross-compile executables in order to cross-compile the LLDB swift REPL. Since I believe it's only relevant for cross-compilers, I've made it a swiftc-only option (are there any use-cases for running the interpreter with an alternate toolchain?)

Currently, you can cross-compile with swift (assuming you've got a copy of the standard library compiled for the target), like so:

swiftc -target armv7-unknown-linux-gnueabihf -sdk ${sysroot} -resource-dir ${sdk-location} main.swift

However, this only works for modules (swiftmodules), not for executables. The host swift generates a link command to the system clang++ with -fuse-ld=gold (or whatever is the correct linker for the target platform), but there is currently no way to tell it where to find the cross-linker. Normally, you would put this information behind the -B switch, so we provide the same kind of functionality here.

Adding -toolchain ${gcc-toolchain-path} to the above command allows swift to cross-compile an executable.

This was previously suggested as -Blinker (#2483) and I thought it was resolved by the update to clang accepting absolute linker paths, however there are two reasons why that isn't true:

Swift's link commands go through the system clang, which might not support absolute linker paths
You might not know which linker to use. We recently switched all Linux targets to use gold. Maybe there are different options for different hosts. The Swift compiler already knows which linker to search for in the toolchain, so it's convenient not to have to say this again.

So in short a toolchain flag is more flexible, and more compatible.

Resolved bug number: (SR-)

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
All supported platforms	@swift-ci Please smoke test and merge
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
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

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

gribozavr · 2016-06-06T04:03:59Z

Looks reasonable to me. The change needs tests though.

CC @jrose-apple.

Swift's link commands go through the system clang, which might not support absolute linker paths

With this change, we are still going through the system Clang, right? This does not sound right to me because the system Clang might not even know about our target. (For example, if the toolchain for the target uses a fork of Clang.)

karwa · 2016-06-06T04:10:32Z

We're still going through system Clang, just passing the -B option as well. Nobody is shipping a system Clang that supports absolute paths in the --fuse-ld option yet, so we have to make to with that.

Without this change, this is what happens when trying to cross-compile any executable:

echo 'print("hello, world")' | ./swiftc -target armv7-linux-gnueabihf -sdk /Spring/Projects/3rdParty/OpenSource/Apple/cross-compile/sysroots/linux-armv7 -resource-dir ../../swift-linux-armv7/lib/swift -v  -     
Swift version 3.0-dev (LLVM cb08d1dbbd, Clang 383859a9c4, Swift 767443ddc1)
Target: armv7--linux-gnueabihf
/Spring/Projects/3rdParty/OpenSource/Apple/build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64/bin/swift -frontend -c -primary-file - -target armv7--linux-gnueabihf -disable-objc-interop -sdk /Spring/Projects/3rdParty/OpenSource/Apple/cross-compile/sysroots/linux-armv7 -resource-dir ../../swift-linux-armv7/lib/swift -color-diagnostics -module-name main -o /var/folders/rc/b9h98gpx74j1hqp82wv7sd0h0000gn/T/--4f1b14.o
/Spring/Projects/3rdParty/OpenSource/Apple/build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64/bin/swift-autolink-extract /var/folders/rc/b9h98gpx74j1hqp82wv7sd0h0000gn/T/--4f1b14.o -o /var/folders/rc/b9h98gpx74j1hqp82wv7sd0h0000gn/T/--2df8d3.autolink
/usr/bin/clang++ -fuse-ld=gold -target armv7--linux-gnueabihf -Xlinker -rpath -Xlinker ../../swift-linux-armv7/lib/swift/linux ../../swift-linux-armv7/lib/swift/linux/armv7/swift_begin.o /var/folders/rc/b9h98gpx74j1hqp82wv7sd0h0000gn/T/--4f1b14.o --sysroot /Spring/Projects/3rdParty/OpenSource/Apple/cross-compile/sysroots/linux-armv7 -L ../../swift-linux-armv7/lib/swift/linux --target=armv7--linux-gnueabihf -lswiftCore @/var/folders/rc/b9h98gpx74j1hqp82wv7sd0h0000gn/T/--2df8d3.autolink ../../swift-linux-armv7/lib/swift/linux/armv7/swift_end.o -o main
clang: error: invalid linker name in argument '-fuse-ld=gold'
<unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)

Note the call to /usr/bin/clang++ (yes, I know the rpath is wrong)

gribozavr · 2016-06-06T04:26:34Z

I understand. What I am saying is that the system Clang might not even know about the target, so we should prefer the Clang from the toolchain, if it exists.

karwa · 2016-06-06T10:25:24Z

You mean like this? (needs tests of course)

gribozavr · 2016-06-06T15:59:39Z

@karwa Yes, exactly! Thank you!

gribozavr · 2016-06-06T15:59:50Z

test/Driver/toolchain.swift

+// BINUTILS-DAG: [[OBJECTFILE]]
+// BINUTILS-DAG: @[[AUTOLINKFILE]]
+// BINUTILS-DAG: -B {{[^ ]+}}/Inputs/fake-toolchain
+// BINUTILS: -o toolchain


Could you add a newline here?

gribozavr · 2016-06-06T16:18:46Z

@swift-ci Please test

jrose-apple · 2016-06-06T17:11:16Z

Haven't had a chance to look at the code yet, but I'm concerned about overloading the term "toolchain". There's the classic compiler tools meaning, and then there's the Xcode meaning, which we've adopted on swift.org. They're related but refer to very different levels in the directory structure.

jrose-apple · 2016-06-06T17:19:59Z

include/swift/Driver/ToolChain.h

@@ -87,12 +87,12 @@ class ToolChain {

  /// Packs together information chosen by toolchains to create jobs.
  struct InvocationInfo {
-    const char *ExecutableName;
+    StringRef ExecutableName;


This was deliberately a char * to show that it was null-terminated. If you're removing that guarantee, you need to update the uses of it.

@jrose-apple If I'm not removing that guarantee, would a comment and some assertions suffice?

No, we've gotten into trouble here before. No StringRef should ever be assumed null-terminated. What's wrong with char *?

jrose-apple · 2016-06-06T17:21:34Z

I know it's not a good long-term solution, but is there anything wrong with setting PATH for now?

karwa · 2016-06-06T19:53:08Z

Fixed test.
@jrose-apple Why set PATH instead of doing this? Why is this not a good long-term solution? If Swift depends on a collection of tools from the host system, it makes sense to give an option to override them.

We can rename it from toolchain; maybe -host-tools or --override-tools or something.

jrose-apple · 2016-06-06T21:04:02Z

Sorry, I meant "I know setting PATH is not a good long-term solution". I just wanted to know how blocking this is.

karwa · 2016-06-06T21:28:55Z

Well, I only really only need it for LLDB (maybe swiftpm, too), and I'm only cross-compiling for armv7, where all breakpoints get cleared as you run, so LLDB is basically pointless. So I suppose I could technically live without it.

Since the only bit of binutils we need is the linker, and LLD seems to be making such great progress that we can imagine a day when we won't even need that, the clang lookup is probably the most long-term useful part. If even that isn't all that useful, I can drop it and keep the -B flag, maybe under some inconspicuous name where it won't disturb anybody. At least until out machines are shipping with newer versions of Clang.

jrose-apple · 2016-06-07T23:16:45Z

I'm wondering about other binutils. dsymutil seems like it would also come from the toolchain, if it were more commonly used on Linux systems. Even if we do have a dedicated command-line flag, I'm not sure why we wouldn't want to affect every tool looked up by the driver, which is basically equivalent to setting PATH.

karwa · 2016-06-08T06:36:08Z

dsymutil isn't part of GNU binutils. It's not needed on Linux platforms as the debug symbols are embedded, so the only time you would need it is if you're cross-compiling to Darwin. In that case, the only supported build machine is OSX which has a dsymutil to handle everything (do we support Linux->OSX in any sense?). We don't really support a 'toolchain' on OSX because everything's hardcoded. That seems to be by design.

Speaking of dsymutil on Linux: dsymutil was recently (10 months ago, http://llvm.org/viewvc/llvm-project?view=revision&revision=244270) added to LLVM, but it's called llvm-dsymutil. We could stop embedding debug information in the binaries on linux and standardise on dSYMs for all platforms. In any case, that would be out of scope of this PR.

EDIT: Oh no, llvm-dsymutil only supports Mach-O :(

Sure, we could add lookup support for dsymutil, and we should also look for llvm-dsymutil if the unprefixed one is not found.

jrose-apple · 2016-06-08T17:14:21Z

I guess my point is that if this is really just about the linker and it doesn't affect any other tools, maybe we should name it as such. Do we expect it to stay just the linker, or potentially grow to other tools in the future?

(BTW, the code comment about char * is still relevant.)

lattner · 2016-06-21T05:18:09Z

Random question, but did you consider having -toolchain point to a JSON file that describes the toolchain, rather than a path? That seems like a more generally extensible and customizable design.

lattner · 2016-06-21T05:21:26Z

For example, I think it could be cool to be able to use something like:
swiftc --toolchain beagleboard ....

and have the compiler look for "beagleboard.toolchain" in a directory next to the swift executable (or some system location), then in your home directory, etc. Of course an absolute path to a toolchain file should also work. The toolchain file could have paths to all of the components that make up the toolchain as well as specify configuration options to specify the "kind" of component each is.

@ddunbar since he probably also has thoughts on this from the SwiftPM / llbuild perspective.

jrose-apple · 2016-06-21T16:02:40Z

I think that's a bit overengineering. In the common case, people do organize their toolchains by directory; in the uncommon case, you can fake it with symlinks.

ddunbar · 2016-06-21T16:27:24Z

I agree... if it turns out we need more metadata about a toolchain to do nice things automatically, I think it would make more sense to establish a convention for embedding the metadata in a file inside the toolchain, so users can still refer to the toolchain by path. This is essentially what we already do on Darwin platforms (and then that metadata has identifiers we have convenient access to), and I could imagine Swift defining a convention for that for Linux along with associated tools to use them.

I agree with @lattner that the general features this enables are useful in the long run. For example, a current desired feature for SwiftPM that our users want/need is easy access to "cross compiling" to other toolchains, so they can use a newer swift build with sources designed for older Swift versions.

karwa · 2016-06-21T17:46:58Z

@lattner @ddunbar

Comprehensive cross-compiling support is something that is a (not-so-secret) desire of mine for Swift. We live in a multiple-device world - my laptop is Intel, my phone and tablet are ARM, the server I'm deploying to could conceivably be either, and the IoT platform I'm targeting may be something completely different (but probably ARM as well).

This was always a kind of expert feature in the past, but the proliferation of computing and rich, embedded, accessible devices makes it something the average user might desire to do. Cross-compiling for a Raspberry Pi (linux-armv7) is no more an "advanced" feature than cross-compiling for iOS; it's just easier because of Apple's well-structured toolchain and the fact you can only do it from OSX.

When it comes to "toolchain bundles" specifically, it is something we really should move to. I have actually been planning to introduce something like that for the build system, but of course the same challenge could be met with the same solution for cross-compiling other Swift code besides the standard library. Currently we have some... Undesirable things in CMake as a result - for example, we have include paths for Android hardcoded in AddSwift.cmake. This kind of stuff should go in a toolchain file (indeed, that's what I'm using so far for my own cross-compiling patch - a CMake toolchain file):

  if("${CFLAGS_SDK}" STREQUAL "ANDROID")
    list(APPEND result
        "-I${SWIFT_ANDROID_NDK_PATH}/sources/cxx-stl/llvm-libc++/libcxx/include"
        "-I${SWIFT_ANDROID_NDK_PATH}/sources/cxx-stl/llvm-libc++abi/libcxxabi/include"
        "-I${SWIFT_ANDROID_NDK_PATH}/sources/android/support/include")
  Endif()

The biggest challenges we have for cross-compiling right now:

[build-script]: Configuration is based on SDK, so you can't cross-compile for another architecture variant of the Host SDK. Cross-compiling the stdlib for linux on a linux host, for example, is not possible. I have some work to address this in [CMake] Replace SWIFT_SDKS with SWIFT_CROSS_COMPILE_DEPLOYMENT_TARGETS #2835
[stdlib]: The standard library is not relocatable. Specifically, module maps contain absolute paths and must be hand-edited before redeploying. @rintaro had an interesting idea to address this using the recently-introduced VFS system in clang -- although, if we're in one of these "toolchain bundles", we'll have a stable relative path to the sysroot, we can possibly skirt around it.

jrose-apple · 2016-08-23T17:43:48Z

lib/Driver/ToolChains.cpp

@@ -951,7 +951,21 @@ toolchains::Darwin::constructInvocation(const LinkJobAction &job,
  const Driver &D = getDriver();
  const llvm::Triple &Triple = getTriple();

-  InvocationInfo II{"ld"};
+  // Configure the toolchain


Style nitpick: please end sentences in comments with periods.

jrose-apple · 2016-08-23T17:46:55Z

Okay, looking good! I have a few last comments (mostly about the tests) but I think we're ready to merge. @modocache, @compnerd, @ddunbar?

compnerd · 2016-08-24T00:48:40Z

include/swift/Option/Options.td

@@ -133,6 +133,10 @@ def j : JoinedOrSeparate<["-"], "j">, Flags<[DoesNotAffectIncrementalBuild]>,
 def sdk : Separate<["-"], "sdk">, Flags<[FrontendOption]>,
  HelpText<"Compile against <sdk>">, MetaVarName<"<sdk>">;

+def tools_directory : Separate<["-"], "tools-directory">,
+  Flags<[FrontendOption, NoInteractiveOption, DoesNotAffectIncrementalBuild]>,
+  HelpText<"Look for external executables (ld, clang, binutils) in <toolchain>">, MetaVarName<"<toolchain>">;


Would be nice to change the MetaVarName to directory.

jrose-apple · 2016-08-24T03:04:20Z

And for the heck of it:

@swift-ci Please test

jrose-apple · 2016-08-24T03:05:32Z

Since there's still some feedback this won't be the final run of the tests, but we can probably just do smoke tests after this. Linux, OS X.

…d (Darwin), or clang/binutils (Unix)

karwa · 2016-08-24T14:12:23Z

@jrose-apple Feedback addressed. -### is awesome, thanks for the tip - even though I typically prefer tests which execute something, this way we can test the toolchain substitutions on all targets on all hosts (which makes more sense, since this is envisioned for cross-compiling support).

jrose-apple · 2016-08-24T15:49:40Z

@swift-ci Please smoke test

jrose-apple · 2016-08-24T15:50:03Z

@swift-ci Please Python lint

jrose-apple · 2016-08-24T15:52:37Z

Linux, OS X, Python.

karwa · 2016-08-25T12:30:10Z

@jrose-apple tests seem good (but swiftci didn't report back here for some reason). We all clear, then?

jrose-apple · 2016-08-25T15:57:49Z

The Linux tests failed, even though it's probably not related. Let's just run again.

@swift-ci Please smoke test

jrose-apple · 2016-08-25T19:48:11Z

Merged. Thanks for sticking with this one, @karwa!

Provides a path to look for ld (Darwin), or clang/binutils (Unix).

[swift-3.0] Added -tools-directory option. (#2912)

karwa force-pushed the linker-search-path branch 4 times, most recently from 427b49c to 3bfa8d8 Compare June 6, 2016 14:26

gribozavr reviewed Jun 6, 2016
View reviewed changes

jrose-apple reviewed Jun 6, 2016
View reviewed changes

karwa force-pushed the linker-search-path branch from 3bfa8d8 to 99a4f72 Compare June 6, 2016 19:51

gribozavr mentioned this pull request Jun 7, 2016

[Android] Add testing support #1714

Merged

2 tasks

karwa force-pushed the linker-search-path branch 2 times, most recently from e062bbd to 99a4f72 Compare June 8, 2016 18:49

karwa force-pushed the linker-search-path branch from c02a6a0 to 95a454b Compare August 22, 2016 23:08

jrose-apple reviewed Aug 23, 2016
View reviewed changes

compnerd reviewed Aug 24, 2016
View reviewed changes

karwa force-pushed the linker-search-path branch 5 times, most recently from d6481a5 to 7f6fa34 Compare August 24, 2016 13:24

[Driver] Added -tools-directory option. Provides a path to look for l…

0e32d15

…d (Darwin), or clang/binutils (Unix)

karwa force-pushed the linker-search-path branch from 7f6fa34 to 0e32d15 Compare August 24, 2016 13:52

jrose-apple merged commit 20d9b96 into swiftlang:master Aug 25, 2016

karwa deleted the linker-search-path branch August 25, 2016 21:52

karwa added a commit to karwa/swift that referenced this pull request Aug 29, 2016

[Driver] Added -tools-directory option. (swiftlang#2912)

df77f80

Provides a path to look for ld (Darwin), or clang/binutils (Unix).

tkremenek added a commit that referenced this pull request Aug 29, 2016

Merge pull request #4543 from karwa/toolchain-3.0

39a4362

[swift-3.0] Added -tools-directory option. (#2912)

Added -toolchain option to swiftc #2912

Added -toolchain option to swiftc #2912

Conversation

karwa commented Jun 6, 2016

What's in this pull request?

Resolved bug number: (SR-)

gribozavr commented Jun 6, 2016

karwa commented Jun 6, 2016 • edited Loading

gribozavr commented Jun 6, 2016

karwa commented Jun 6, 2016

gribozavr commented Jun 6, 2016

gribozavr Jun 6, 2016

Choose a reason for hiding this comment

gribozavr commented Jun 6, 2016

jrose-apple commented Jun 6, 2016

jrose-apple Jun 6, 2016

Choose a reason for hiding this comment

karwa Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

jrose-apple Aug 9, 2016

Choose a reason for hiding this comment

jrose-apple commented Jun 6, 2016

karwa commented Jun 6, 2016 • edited Loading

jrose-apple commented Jun 6, 2016

karwa commented Jun 6, 2016 • edited Loading

jrose-apple commented Jun 7, 2016

karwa commented Jun 8, 2016 • edited Loading

jrose-apple commented Jun 8, 2016 • edited Loading

lattner commented Jun 21, 2016

lattner commented Jun 21, 2016

jrose-apple commented Jun 21, 2016

ddunbar commented Jun 21, 2016

karwa commented Jun 21, 2016 • edited Loading

jrose-apple Aug 23, 2016

Choose a reason for hiding this comment

jrose-apple commented Aug 23, 2016

compnerd Aug 24, 2016

Choose a reason for hiding this comment

jrose-apple commented Aug 24, 2016

jrose-apple commented Aug 24, 2016

karwa commented Aug 24, 2016

jrose-apple commented Aug 24, 2016

jrose-apple commented Aug 24, 2016

jrose-apple commented Aug 24, 2016

karwa commented Aug 25, 2016

jrose-apple commented Aug 25, 2016

jrose-apple commented Aug 25, 2016

karwa commented Jun 6, 2016 •

edited

Loading

karwa Aug 9, 2016 •

edited

Loading

karwa commented Jun 6, 2016 •

edited

Loading

karwa commented Jun 6, 2016 •

edited

Loading

karwa commented Jun 8, 2016 •

edited

Loading

jrose-apple commented Jun 8, 2016 •

edited

Loading

karwa commented Jun 21, 2016 •

edited

Loading