utils: require gold on Linux targets #2609
Conversation
|
@swift-ci please test |
|
@swift-ci Please test |
|
CC @bryanpkc @antonblanchard How does this affect s390x and powerpc64le? |
|
@gribozavr Thanks for the heads-up. gold support on s390x is included in the latest version of binutils, but is not yet shipped by most Linux distributions. Let me build binutils and test the Swift build. |
|
Both OS X and Linux failed with two separate failing tests, and both are unrelated. (Also, @shahmishal, I'm still unable to trigger CI. Hasn't worked for about a week now.) |
|
@modocache Are you able to only trigger on your own PR? |
|
@modocache Can you please try again? It should let you trigger CI on others PR. |
|
@shahmishal Thanks!! Whatever you did worked, I was able to trigger CI on #2549 (comment) :) |
|
@modocache Sorry for the trouble! Thanks! |
|
The test failure seem unrelated to this change. |
|
Does this change the driver to use the gold linker too? If it doesn't then I think applications compiled by Swift will use the BFD linker instead of gold. |
Given the discussion on swift-dev, and no opposition, swift the linux targets to using gold by default.
|
@RLovelett I believe it does support the explicit linker specification, but updated the patch to default x86_64, ppc64, ppc64le to gold as well. |
| case llvm::Triple::ppc64: | ||
| case llvm::Triple::ppc64le: | ||
| // BFD linker has issues wrt relocations against protected symbols. | ||
| return "gold"; |
There was a problem hiding this comment.
In light of what this change is proposing to do, switch to the gold linker by default on Linux, it might be worthwhile to wholly re-write this code.
What I mean is the default should be "gold" and then any thing that wants different should opt-out. Something more like:
std::string toolchains::GenericUnix::getDefaultLinker() const {
switch(getTriple().getArch()) {
default:
// Otherwise, use the gold linker.
return "gold";
}
}
Also something that I noticed when changing the patch is that the toolchain is for GenericUnix does that mean this changes things for BSD? Is that OK?
There was a problem hiding this comment.
Yes, its an ELF thing rather than a Linux thing, so I think that the current change as proposed makes sense. However, Id rather leave the structure as is (I explicitly split up the switch as the reasoning is different). That said, we could change the structure in a follow up change as well if others feel that is a good idea.
There was a problem hiding this comment.
+1 for considering this in a follow-up pull request.
|
Thanks @gribozavr ppc64le has gold support. I built and tested with this commit and it looks good. |
|
|
|
@modocache I built gold from binutils-2.26 on s390x and tested this commit (adapting it for s390x). No obvious problem was found. |
|
Great! Thanks everyone. :) |
|
Seems like, this change introduced some link error in XCTest test. With my local tests, reverting this change fixes the problem. |
|
@rintaro my personal nightly builds started failing with this same issue. The integration tests catch it for me on my system. I have not yet had time to track down the problem to get a fix. |
|
Gah, sorry about that. I'm looking into the XCTest failure now. @swift-ci wouldn't have caught this one, for two reasons:
|
|
No problem. I'm glad it is being looked into. Can you just put a link in this thread whenever you put up a fix? That way I can test it locally too. |
swiftlang/swift#2609 introduced a change to the default behavior of the Swift driver, which now uses the gold linker unless specified otherwise. This caused a linker error in swift-corelibs-xctest's test suite, which this commit fixes.
|
Fix for XCTest here: swiftlang/swift-corelibs-xctest#118. Sorry for the inconvenience everyone! 🙇 |
|
@RLovelett @rintaro I merged swiftlang/swift-corelibs-xctest#118, which should fix the problem. Sorry again! |
|
Sadly the build still is not working again. Though that issue is resolved. Thanks again @modocache! |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
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
Validation Testing
Note: Only members of the Apple organization can trigger swift-ci.
Given the discussion on swift-dev, and no opposition, swift the linux targets to
using gold by default.