Discard swift.ld in favor of portable solution and support gold linker in Swift #1157
Conversation
|
I think the FreeBSD port is also using the linker script, as does the Cygwin port. /cc @dcci @tinysun212 What do you think? |
| @@ -12,6 +12,9 @@ | |||
|
|
|||
| // REQUIRES: executable_test | |||
| // REQUIRES: objc_interop | |||
| // UNSUPPORTED: OS=linux-gnu | |||
| // UNSUPPORTED: OS=linux-gnueabihf | |||
| // UNSUPPORTED: OS=freebsd | |||
There was a problem hiding this comment.
Are these OS's not already covered by the REQUIRES: objc_interop requirement above? I believe #1032 disabled this test for all Linux variants.
There was a problem hiding this comment.
Yep, you're right. Removed.
|
By the way, the failing test count in arm-linux is down to 5 (from the 40-or-so prior to this patch) Testing Time: 601.26s Failing Tests (5): Expected Passes : 1699 |
|
@hpux735 That's great! Kudos to you and @tienex, this is fantastic work. I'm pulling it down now to give it a whirl. I've looked into the |
|
@modocache Yes, that's correct, 'char' is unsigned on Linux/arm, the very same issue you have, I see you did already some work on it! Cool. The other failure that can be fixed quickly is 'Driver/Dependencies/one-way-external.swift', it is failing because it tries to run touch with year 3000, which fails on Linux/arm because of 32-bits time_t, changing it to 2037 makes it pass. SDK seems just a matter of CHECK order. This leaves VarArgs and class_resiliance to be fixed. |
|
The following changes get diff --git a/test/IRGen/class_resilience.swift b/test/IRGen/class_resilience.swift
index 6868574..b9782fa 100644
--- a/test/IRGen/class_resilience.swift
+++ b/test/IRGen/class_resilience.swift
@@ -232,11 +232,11 @@ public class MyResilientChild : MyResilientParent {
// CHECK: [[SIZE_METADATA:%.*]] = call %swift.type* @_TMaV16resilient_struct4Size()
// CHECK: call void @swift_initClassMetadata_UniversalStrategy(
// CHECK-native: [[METADATA_PTR:%.*]] = bitcast %swift.type* [[METADATA]] to [[INT]]*
-// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 12
+// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 15
// CHECK-native-NEXT: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* [[FIELD_OFFSET_PTR]]
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @_TWvdvC16class_resilience26ClassWithResilientProperty1sV16resilient_struct4Size
// CHECK-native-NEXT: [[METADATA_PTR:%.*]] = bitcast %swift.type* [[METADATA]] to [[INT]]*
-// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 13
+// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 16
// CHECK-native-NEXT: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* [[FIELD_OFFSET_PTR]]
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @_TWvdvC16class_resilience26ClassWithResilientProperty5colorVs5Int32
// CHECK: ret %swift.type* [[METADATA]]
@@ -249,11 +249,11 @@ public class MyResilientChild : MyResilientParent {
// CHECK: [[RECTANGLE_METADATA:%.*]] = call %swift.type* @_TMaV16resilient_struct9Rectangle()
// CHECK: call void @swift_initClassMetadata_UniversalStrategy(
// CHECK-native: [[METADATA_PTR:%.*]] = bitcast %swift.type* [[METADATA]] to [[INT]]*
-// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 11
+// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 14
// CHECK-native-NEXT: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* [[FIELD_OFFSET_PTR]]
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @_TWvdvC16class_resilience33ClassWithResilientlySizedProperty1rV16resilient_struct9Rectangle
// CHECK-native-NEXT: [[METADATA_PTR:%.*]] = bitcast %swift.type* [[METADATA]] to [[INT]]*
-// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 12
+// CHECK-native-NEXT: [[FIELD_OFFSET_PTR:%.*]] = getelementptr inbounds [[INT]], [[INT]]* [[METADATA_PTR]], [[INT]] 15
// CHECK-native-NEXT: [[FIELD_OFFSET:%.*]] = load [[INT]], [[INT]]* [[FIELD_OFFSET_PTR]]
// CHECK-native-NEXT: store [[INT]] [[FIELD_OFFSET]], [[INT]]* @_TWvdvC16class_resilience33ClassWithResilientlySizedProperty5colorVs5Int32
// CHECK: ret %swift.type* [[METADATA]] |
|
CC @slavapestov for the |
|
(Sorry, missed that you are William Dillon.) We can avoid runtime cost by using |
|
@jckarter We tried, but the .ll approach resulted in a too complex expression, while the .s variant has to special case arm because of assembly syntax. In the end, the major problem is that using subtraction in the assembly produces PC-relative relocations, they work on ARM with -Bsymbolic, but will fail with gold on x86-64 because the relocation is unsupported in PIC model or results in being evaluated to zero using BFD linker. |
|
Which relocation is it that gold doesn't support—was it |
|
@modocache @tienex Perhaps the failing class_resilience test is caused by differences in alignment? Was it failing on Linux/ARM before this patch too? |
|
@tienex The which should produce a |
|
@slavapestov Yes, the failure occurs prior to this patch as well. |
|
@modocache Oh, the problem is obvious. Those CHECK lines only run when no ObjC runtime is present. We have CI for 32-bit and 64-bit Darwin platforms, but on Linux we're only testing 64-bit. So I think you can safely replace those offsets with |
|
@jckarter I'll perform more tests tomorrow, I'm on the GMT+1 timezone, no offense :-) We wanted to avoid to clutter the file with ifdefs supporting arm and little/big endian configurations, also we would need to change the top-level cmake file to enable support for assembly files, by default cmake doesn't know how to handle them, and we found some issues with cmake 2.8 which seems to be the minimum version supported by swift, @hpux735 can give you more information regarding this issue. |
|
@jckarter We were able to eventually get CMake to play nice with .S files, though the solution was a little hackish. CMake versions subsequent to 2.8 handle it more cleanly. |
|
I wouldn't worry too much about big endian for now; there are far deeper endianness sins in IRGen and the runtime. |
|
…though that's another thing an .ll file could abstract away. A |
|
@slavapestov I applied your fix for class_resilience. Thanks! |
|
Cygwin port is not using the linker script swift.ld. It is reading the size of the sections directly from the COFF section header structure which is loaded in memory. |
|
@jckarter I performed some tests, even in the case of R_X86_64_PC32 I get gold complain about it, unless I use -Bsymbolic, I could use .quad instead of .long and I would have the same result. BFD accepts it, but like R_X86_64_PC64 produces a zero value, should I assume -Bsymbolic is the default now? If so, we should make it also the default for non-ARM platforms when invoking clang++ from swift during linking shared objects, in this PR we specify it only for ARM/Thumb targets. |
|
Yeah, |
|
I had added it back in #1154, and everything seems to work with BFD using |
|
Longer-term, it might be a good idea to investigate alternatives to |
|
@jckarter that's why we ended up with the non-assembly version, it worked independently of -Bsymbolic |
|
@tienex Sure, but the behavior of |
|
@jrose-apple Does everything look good from your side? |
|
@hpux735 I have ran the tests on Linux/x86_64, and it looks like we would also need a companion PR at least for xctest, and possibly the package manager and possibly lldb (for the JIT'ed code). You can reproduce the buildbot configuration with: |
|
Thanks for spending the time looking into it, @gribozavr . I tried to reproduce your issues with both my branch and the apple/master branch. The errors pasted below occur on the apple master branches as well as mine. Here were my steps to verify (my branch is the only provided example, the apple branch is just using The results from testing are these two errors which seem to be lit being braindead, and have been present for quite a long time.: Looking at them with my (admittedly very permissive) expression parser, I don't understand why these tests fail in the first place. |
|
@hpux735 I think these issues are related to your local configuration. Something is adding |
|
Thanks @gribozavr . I was able to reproduce what I think was the error you're referencing: I'll look into it. With regard to the other two tests that were failing, it's probably not great that environment-defined variables can break them. |
|
@gribozavr My comments were all addressed, LGTM. :-) |
|
@hpux735 Lit filters out most environment variables but it can't filter out PATH because it expects to be able to use things like "cp" and "ls" unqualified. I'm not sure what to do about LD_LIBRARY_PATH because it's possible some systems depend on it just to run things. We could either:
But either way we can do that in a separate PR. |
|
@jrose-apple Thanks! I agree that it's inappropriate for this PR 👍 |
|
@gribozavr would you mind taking a look at the changes in the other PRs and the current version of this. I think we've addressed your comments, but there is some new weirdness with the Evolution validation tests. These are new since the last time we exchanged about this, and I don't know if they're supposed to fail. Either way, they're preventing the buildbot from finishing. In my testing, I deleted the tests and everything went though fine. |
|
CC @slavapestov for Evolution tests. |
|
I'm running tests on Ubuntu 14.04. |
|
Oops! Sorry @slavapestov I just saw that I was a couple hundred commits behind master. I rebased and ran it again and the evolution tests pass. Also, @gribozavr , the patch to swift package manager was just merged in (I works with or without this PR). Anyway, I just re-tested with your build script, and all the tests pass. The only exception is |
|
I tested a combination of:
on Ubuntu 14.04 x86_64 and found no issues. I couldn't reproduce Merging. |
Discard swift.ld in favor of portable solution and support gold linker in Swift
|
Wow. I want to sincerely thank @tienex @gribozavr @compnerd @modocache @jckarter and @jrose-apple for all your help! This was one heck of a PR. As far as I can tell, it has the most comments of any non-joke PR. |
|
@hpux735 This is huge, thank you! Could you add an item to the changelog? |
|
@gribozavr Of course, under the Swift3 header? |
|
@hpux735 Yes. |
[pull] swiftwasm-release/5.3 from release/5.3
This pull request addresses the conformances and metadata sections differently than the former Linux solution. Unlike the former solution, the size of the sections are not stored as part of the sections themselves. Rather, section start and end markers are stored in the binary, and are used to compute the size of these sections at load time.
This solution is more portable and allows the use of the gold linker, which is far more stable than BFD on ARM targets. Specifically, the gold linker does not generate
R_ARM_COPYrelocations, which break protocol conformances, though BFD does. Although gold does address some of these issues,-Bsymbolicis still required because swift still generatesR_ARM_REL32relocations, which are illegal. Finally, because the JIT images also contain the illegal relocations, RuntimeDyld refuses to process them, and as such the interpreter tests are disabled on ARM..In further support of the use of the gold linker on arm-linux, we added a switch to swift for the selection of linker, and under arm, the gold linker is selected as the default.
This work was the topic of an RFC on the swift-dev mailing list:
https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20160111/000822.html