Make Swift work on Linux/armv7 #608
Conversation
|
That's really cool. It seems like this forces linux arm to use thumb. Is that true? What would be the advantage (other than presumably smaller code size) to using thumb? Does it integrate seamlessly with Glibc, and does the swiftc compiler complain about architecture triples? |
|
There aren't many drawbacks to using thumb-2 on armv7, since it can automatically fall back to full-width instructions for things that don't have a 2-byte encoding. |
| @@ -36,7 +36,8 @@ const std::vector<std::string> LangOptions::SupportedArchBuildConfigArguments = | |||
| "arm", | |||
| "arm64", | |||
| "i386", | |||
| "x86_64" | |||
| "thumb", | |||
There was a problem hiding this comment.
Why do we need to support it as a separate architecture build configuration?
|
@hpux735 Yes, in my personal experience, all Linux/armv7 userland code I saw was always compiled Thumb-2, so it was nice to have Swift generate Thumb-2 code instead of full width ARM. |
|
Out of curiosity, why aren't the relative references across sections not supported? Does ld.so load and relocate ELF sections independently? |
|
I ask because part of the goal of the relative references is to reduce the initial dynamic linking cost by minimizing relocations. If ELF .sos end up with tons of relocations anyway that would be counterproductive. |
|
@gribozavr Targeting armv7-unknown-linux-gnueabihf produces full-width ARM, LLVM wants thumbv7-*, at least on Linux, and not having that, Swift complains to not know anything about it. This was the only clean solution I could come up, the other involved adding feature flags during the creation of the target machine, but that would break support for other classes of ARM processors, and honestly it didn't look right. |
|
@jckarter Cross-section relocations work, but not in shared objects, the linker will emit R_ARM_REL32, but that kind of relocation is forbidden in shared objects. |
|
@tienex I understand. I was pointing at the Swift-level name of the build configuration, any reason we can't just use |
|
@tienex If the sections are always mapped at the same relative offset to each other (as happens with Mach-O files), though, then there shouldn't need to be a relocation in the .so at all, should there? |
|
@jckarter You're right, but both BFD and Gold produce the same R_ARM_REL32 relocation in the cross-section case, I tried all possible switches to no avail, the net result is that you will receive an error message from ld.so stating: : unexpected reloc type 0x03. |
|
|
||
| /// A function that produces the witness table given an instance of the | ||
| /// type. The function may return null if a specific instance does not | ||
| /// conform to the protocol. | ||
| RelativeDirectPointer<WitnessTableAccessorFn> WitnessTableAccessor; | ||
| RelativeIndirectablePointer<WitnessTableAccessorFn> WitnessTableAccessor; |
There was a problem hiding this comment.
Can we conditionalize this so that platforms without this problem don't pay for the indirection check? It could also be slightly more efficient on ARM-ELF to have a RelativeIndirectPointer that's always treated as indirect.
There was a problem hiding this comment.
Sure, what would be the best way to make it conditional? Checking for ARM is easy, but for ELF?
There was a problem hiding this comment.
Not sure. __armv7__ && !__APPLE__ might be close enough.
There was a problem hiding this comment.
OK, will try first what @hpux735 suggested and see if it still needs to be done.
There was a problem hiding this comment.
Let me know when you make that change and commit to your fork. I'm getting my build system ready to try out your changes.
|
@tienex Interesting, I wonder if that's a bug. I don't want to hold up your progress on the port, but it'd be worth further investigation to see whether those relocations are really necessary. |
|
I was able to get around the R_ARM_REL32 problem using -Wl,-Bsymbolic https://github.com/hpux735/swift/commit/4b56718ff433163431b86b62323a39cac24afbf1#diff-72a829713c96d210e727174a26ea85beR161 (note that the BSD_LIBRARIES isn't necessary, and has since been removed.) |
|
@hpux735 Ah I thought to have tried that, good to know! I will try again. My bad, I must be more careful in my testings, if that works I will happily revert the AlwaysIndirect code. |
|
@gribozavr I can remove "thumb" safely, I remember it complained but probably was related to some other changes I did while experimenting, so that can go away. |
|
While waiting for the build and tests to complete, I should have specified in my commit log (I will amend with the changes), that the following change is needed for Thumb on Linux for swift-clang: apple/swift-clang@b071a9a, I don't know if it has been already merged, it was the only missing bits in clang to support thumb targets on Linux, everything else was already in place but for include paths, I kindly asked Saleem Abdulrasool (@compnerd) to add the missing bits. |
Adds -Bsymbolic at link time when producing shared objects so that the linker will not produce the forbidden relocation R_ARM_REL32. Do the same also when linking Swift standard libraries.
tienex
force-pushed
the
linux-arm
branch
3 times, most recently
from
20068a8 to
b0a6b20
Compare
|
Thanks everyone for the feedback, I updated the change, it is much more slim and has no workarounds. Thanks especially to @hpux735, you are indeed correct, my mistake was not using the symbolic switch for shared objects produced by the build process. |
|
Very glad I could help! I spent many days chasing that one down. I certainly learned a lot. Incidentally, I also learned from your alternate method of addressing it. |
|
Does this (compiling for thumb) add a new dependency on multilib? |
|
@hpux735 No, it does not |
| @@ -160,8 +160,14 @@ function(_add_variant_link_flags | |||
|
|
|||
| if("${sdk}" STREQUAL "LINUX") | |||
| list(APPEND result "-lpthread" "-ldl") | |||
| if("${arch}" MATCHES "^arm") | |||
There was a problem hiding this comment.
Except we don't need this for arm64, do we?
There was a problem hiding this comment.
Isn't arm64 aarch64, and therefore wouldn't match?
There was a problem hiding this comment.
It it sometimes one and sometimes the other. I would prefer to hardcode the exact list here to make maintenance easier. Many people need to modify this file, but very few can test on more rare platforms.
There was a problem hiding this comment.
@gribozavr Will fix.
|
@tienex We discussed this with Tim Northover, and we think that the right thing to do is have the Swift compiler canonicalize triples, The directory name for the runtime libraries should be Would you mind making these changes in this PR or would you like to have this accepted (since it is an incremental improvement), and start a new one? |
|
@gribozavr I can do either, in the case of canonicalization what's the best place to do that? I think making it armv7 is the right choice, I wrote that foreach() in the cmake file as a bandaid to solve issues running the test suite. |
|
@tienex I think canonicalization should be done in |
|
@gribozavr About the idea of using armv7-gnueabihf as the ld script directory, what if we just use the triple passed to configure_sdk_unix() ? Do you see any issue with that? |
|
@gribozavr Sorry, I meant the libraries directory, the alternative is to rename the architecture to include the environment. |
|
@tienex There would be some duplication, because the OS is already mangled into the directory one level above, |
|
So finally I finished all the tests with -Bsymbolic and the results are not that good. I got 33 tests failing with -Bsymbolic vs 25 with the AlwaysIndirect, and some of them are very basic tests. The -Bsymbolic has hideous side effects, which Intel has well described at this link: https://software.intel.com/en-us/articles/performance-tools-for-software-developers-bsymbolic-can-cause-dangerous-side-effects The list of failing tests with -Bsymbolic: Failing Tests (33): Expected Passes : 1598 And the ones with AlwaysIndirect: Failing Tests (25): Expected Passes : 1606 @gribozavr any thoughts? |
|
I think |
|
@jckarter OK, but the descriptors are ignored, for example, in the ArrayCore case the first test result is <Tracked.something Tracked.something ... (i don't have the test at hand right now, but I distinctly remember the output was something like that), while using indirection the results are what expected. The same happens in the others which are not common to the set of failures. |
|
Do the offsets in the conformance table look correct, if you step through swift_conformsToProtocol? |
|
I can check that, but the addresses of symbols are different in the -Bsymbolic case (that's why runtime/SwiftRuntimeTests/MetadataTest.getExistentialMetadata fail). They're the same for libraries but not for the final executable (even if linked with -Bsymbolic). It will require a couple of hours to rebuild, I will report my findings. |
|
-Bsymbolic shouldn't change the address of anything IIUC, it only statically resolves references to the symbol within the .so, which should be fine since we either export and link to unique symbols where we can or have the Swift runtime handle uniquing when we can't. Those failures might be due to other problems, or binutils bugs. I tried building with |
|
@jckarter I hope it's not a binutils bug, that would be extremely annoying. :-) |
|
You might double-check that you backed out your IRGen and runtime changes in sync. If the runtime expects |
|
Yes, that was done :-) |
|
OK, just making sure. |
|
Going back to the discussion between @jckarter and @gribozavr regarding the canonicalization of arm architectures in the triple. I've been working in Foundation while this work has been ongoing. Foundation's build system is independent from most of the swift compiler, and therefore canonicalization during the build process needs to be implemented separately. What was the final determination here? Are we using thumbv7-unknown-linux-gnueabihf? Is that triple comprehensible by the swift compiler when provided in the -target option? |
|
@tienex Howdy. I've been following along as well and was working with @hpux735 to help bring to bear a BeagleBoard X15 I have to build everything. I am currently building your linux-arm branch along with stable/master of other projects. A question for you, do you have a complete set of instructions/scripts that you're using to git clone and then compile everything, along with all of the various apt-get libraries you've had to pull in. For those working on the ARM port, I can generally get through a ground-up build in a few hours, and would be more than happy to lend some CPU power to test things. |
|
I reduced the test case, and this is what I found with -Bsymbolic, the failure happens during the pointer comparison, as you can see the ProtocolDescriptor pointers are different but both point at the same thing: 2706 if (protocol != P) |
|
In the case of indirection they are the same thing. |
|
So, I investigated further. The BFD linker is generating R_ARM_COPY relocations in the executable for the symbols, that's why the pointers differ, while using Gold (with the approach suggested by Nick Clifton at https://bugzilla.redhat.com/show_bug.cgi?id=927573) works, the linker is generating R_ARM_ABS32 for those symbols, thus pointer comparison works. |
|
I completed the tests using -Bsymbolic + gold and the results look quite good, even better than using indirection. At this point the only issue is that gold can't grok the linker script. Testing Time: 1220.47s Failing Tests (16): Expected Passes : 1615 |
|
That's great @tienex ! |
|
@tienex Could you check is there anything in this pull request not already on master? |
|
@gribozavr I am working with @hpux735, we were waiting for his PR to get in before continuing this one. Now they're in, real soon I'll update this. |
|
Yep, I've got @tienex's latest changes almost ready to go. Expect a new PR from me soon. |
|
Hi @tienex and @gribozavr . There's a new PR that superseeds this one. Please see #1157 |
|
OK, let's close this one then. |
…urce-compat Add README entry how to only XFail the Source Comapat CI job
This change completes the work started by William Dillon in order to support ARM (and Thumb) on Linux and make it work. The change also enables support for other architectures that do not support direct references between distinct sections, actually I have a working Swift on Linux/PowerPC as well in order to verify the indirection works as good as on ARM, but that requires some more changes being a big endian target. I'll eventually provide a patch for that if there's interest.