-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support ARMv6 (original Raspberry Pi), ARMv7 bugfixes #901
Conversation
I'm not really the right person to review (just trying to get past the easy stuff), but what tests currently are and aren't passing? |
ARMv7 test results: Testing Time: 754.18s Failing Tests (28): Expected Passes : 1611 |
ARMv6 results: Testing Time: 7199.76s Failing Tests (30): Expected Passes : 1609 |
@@ -159,7 +159,7 @@ function(_add_variant_link_flags | |||
result) | |||
|
|||
if("${sdk}" STREQUAL "LINUX") | |||
list(APPEND result "-lpthread" "-ldl") | |||
list(APPEND result "-lpthread" "-ldl" "-Wl,-Bsymbolic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Bsymbolic
seems to be a big hammer (and it might be just a workaround for what is an LLVM or a linker bug). Could you scope it as narrowly as possible, just to armv6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. After 2.2 is forked, I'm going to work with @tienex on a different (probably better) solution based upon the gold linker. I'll restrict this to arm-linux.
@hpux735 The general approach is to run |
// LINUX-armv6-DAG: [[OBJECTFILE]] | ||
// LINUX-armv6-DAG: -lswiftCore | ||
// LINUX-armv6-DAG: -L [[STDLIB_PATH:[^ ]+/lib/swift]] | ||
// LINUX-armv6-DAG: --target=junk-unknown-linux-gnueabihf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While attempting to add testing for whether --target is added to the clang++ invocation, this seems like the correct approach. However, this does not cause a failure as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When lit is running a test on a file with more than one test (such as linker.swift), it looks like it bails when it finds the first failure, right? I deleted the tests relating to macos and the relative linker and verified that the tests for providing --target to clang functions as intended.
I'm almost done with a patch that will do away with the swift.ld linker script, but in the spirit of small changes, can we get this moved forward so I can open up a new PR? In your opinion (@gribozavr )? what are the remaining issues that need to be addressed? Thanks!! |
Sure! Could you rebase the patch? (there are conflicts) It also seems that @jckarter added |
triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6) { | ||
triple.setArchName("armv6"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick: Perhaps combine some of these conditionals, and use else if
? For example:
if (triple.getOS() == llvm::Triple::Linux && triple.getArch() == llvm::Triple::arm) {
if (triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v7) {
triple.setArchName("armv7");
} else if (triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6) {
triple.setArchName("armv6");
}
}
Personally I find this more readable, but that's just my two cents. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree.
I has conflicts with apple/master and resulting regressions. I'm working on resolving them at the moment. |
I believe that all the regression issues have been resolved. I think this is ready to go, finally. |
@hpux735 Fantastic! Are all tests passing? If not, could you post a list of which aren't? I'd like to help if possible. Also, I have a feeling someone on the core team will ask you to squash these commits--or at least rebase onto But I'm not sure, so don't take my word for it. 😛 |
// LINUX-armv6-DAG: [[OBJECTFILE]] | ||
// LINUX-armv6-DAG: -lswiftCore | ||
// LINUX-armv6-DAG: -L [[STDLIB_PATH:[^ ]+/lib/swift]] | ||
// LINUX-armv6-DAG: --target=armv6-unknown-linux-gnueabihf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the test you asked for, @gribozavr
@swift-ci Please test |
@hpux735 Sorry, this failed on OS X:
Do you have access to an OS X machine to investigate? |
Sorry about that! I just fixed an issue and ran it on my OSX machine. I think it will pass this time, it did for me. |
@swift-ci Please test |
Is there a list of machines/platforms used for CI and the arguments used? I definitely don't want to waste people's time with these problems, if it can be avoided. I just tried it again on my mac and the tests completed without error. |
@hpux735 It failed because of infrastructure reasons. Linux passed, we don't know about OS X. We'll get it fixed and I will re-run. |
@swift-ci Please test |
It failed with:
which I fixed. |
Support ARMv6 (original Raspberry Pi), ARMv7 bugfixes
@hpux735 Thank you for working on the Linux arm port! |
Awesome!!! Thanks so much Dmitri! |
@hpux735 you are my hero. |
LOL, Glad I could help, @peterdan |
The target was added for Unix toolchains in swiftlang#901, but a later pull swiftlang#1891 added it again. Since clang only uses the last target flag that's passed in, all customization done for the first one was unused these last 4+ years, so remove it and change tests that look for custom strings passed by the first one.
this pull request adds support for ARMv6 (including the original Raspberry pi), and fixes several bugs encountered on ARMv7. I've been using this code for a few weeks, and is has been rather stable. My hope is that AMRv6 and ARMv7 on linux can be included in the Swift 2.2 release.