Skip to content

[stdlib][SR-2239]: add support for AArch64 variadics #20708

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

Merged
merged 1 commit into from
Feb 20, 2019
Merged

[stdlib][SR-2239]: add support for AArch64 variadics #20708

merged 1 commit into from
Feb 20, 2019

Conversation

futurejones
Copy link
Contributor

Adds support for AArch64 variadics.
This PR along with #19462 will allow successful builds of swift-4.2-branch on Linux AArch64 Ubuntu 16.04.

This is an updated version of #15174

Resolves SR-2239.

@jrose-apple @compnerd can you push #19462 to the swift-4.2-branch

With these changes we can finally see successful builds on https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-aarch64/
Then we should be able to have an official release of Swift 4.2 for Ubuntu 16.04 AArch64.

[stdlib][SR-2239]: add support for AArch64 variadics
@jrose-apple
Copy link
Contributor

Like #20709, we don't really take additive changes on release branches, especially when we're already up to working on the next release. Let's put this on master instead, and we can cherry-pick to 5.0.

@futurejones
Copy link
Contributor Author

@jrose-apple are you refusing to merge this PR?

@jrose-apple
Copy link
Contributor

@bob-wilson already explained our release process. The final authority would be the 4.2 release manager, @tkremenek.

@jrose-apple
Copy link
Contributor

Aside: we especially aren't going to take any changes that haven't been integrated into master yet.

@futurejones
Copy link
Contributor Author

@jrose-apple

Aside: we especially aren't going to take any changes that haven't been integrated into master yet.

This is simply not true. There are numerous examples of changes to release branches that are not integrated in the master.
As there are also numerous examples of additions to the swift-4.2-branch after the release of Swift 4.2 on Sept 17.

These changes only affect AArch64 on Linux and have no impact on any other systems.
Adding these changes to the swift-4.2-branch also means they have no effect on the release of Swift 5.0 or add any obligation that Swift 5.0 must support AArch64 on Linux.

The purpose is to enable successful builds of 4.2 on the Swift Community CI.

Achieving this is extremely important for motivation and moral of all involved in bringing Open Source Swift to Arm devices. After looking at 1000's of build:failing for the past few months in would really nice to see build:passing for a change.

We can then have an "Official Swift Community-Hosted" release of Swift 4.2 for AArch64 on Linux.
This will lead to greater use of Swift on AArch64, increased community engagement, more users, more bug reports and hopefully more contributors to the Swift source code.

Then we can look at integrating these changes and others into the master and moving toward having AArch64 on Linux officially supported in future releases.

@tkremenek can we get a decision on this please.

@hpux735
Copy link
Contributor

hpux735 commented Nov 27, 2018

The problem is, @jrose-apple that master has diverged too far to allow this to be cherry-picked into it.

This puts the Swift Arm community in a position again to maintain parallel repos. We've tried to work with the core team about this over the years, and they've expressed interest in helping us stay on mainline. However, not being willing to work with us pushes us away again.

Swift 5 has not been released, and it's not usable for any user in production settings, let alone on Arm64. Saying that we should accept the fact that Swift 4.2 doesn't work on Arm64, and never will because we need to wait until some indeterminate point in the future (WWDC 2019?), when Swift 5 is released is pretty painful.

If you look at the changes proposed, they are incredibly minor. In fact, all but one change is gated in #if blocks to have no effect outside of AArch64 and Linux

@jrose-apple
Copy link
Contributor

I'm not in control of the policy here; I'm just reporting what we've done in the past. But we don't take changes on release branches precisely because master changes enough that things get out of date between releases. If the change goes in here, and you never get it into the 5.0 branch, then Swift 4.2.x will work with AArch64 but Swift 5.0 won't. That's not a state the official project releases should be in.

@bob-wilson
Copy link
Contributor

Can we start with the question of getting this into master? We really do want the Swift 4.2 branch to be frozen at this point, but perhaps we could consider an exception. Ted would need to approve that. But, regardless, it is a very important part of the development process for Swift that all work should first be done on the master branch. If the code has diverged too far for a simple cherry-pick to work, then you may have to reimplement the changes. You should get the community-hosted CI working for both the master and Swift 5.0 branches before we consider pulling any changes back to the 4.2 branch.

@hpux735
Copy link
Contributor

hpux735 commented Nov 27, 2018

It's worth pointing out that the velocity of work in mainline swift is such that we're always catching up.

There's a chicken-and-egg issue where the lack of community CI means that we deal with regressions introduced between the months it takes our "team" of 1-3 volunteers to get through struggles. Morale in this team is so low that we may be down to 1 member before too long, as this PR push is one of those member's last attempt to move forward.

We face the very real possibility that Swift on arm will no longer be supported on anything beyond Swift 4 by this fact alone.

@futurejones
Copy link
Contributor Author

@bob-wilson
Copy link
Contributor

Thanks! That's great. I'm sympathetic to the challenge of keeping up with the velocity of changes in Swift. As long as your changes don't affect other platforms, we should try to be as flexible as possible in getting your changes merged. Can you create a PR to get this change (or the equivalent) on the master branch?

@futurejones
Copy link
Contributor Author

@bob-wilson I am working on getting this done in the near future. Unfortunately the swift-5.0-branch and the master no longer build on AArch64. I don't like to submit a PR without first achieving successful builds and testing first.
This PR and the others I have uploaded have all been thoroughly tested before uploading.
You can check the build logs and download the artifacts here - http://futurejones.xyz:8080/job/swift-4.2-aarch64-RELEASE/

By the time we fix all the new issues with 5.0 we will probably be in the same situation as we are now in with trying to get changes added to 4.2.

@tkremenek
Copy link
Member

There will likely be more 4.2 releases for Linux, so I'm open to taking this PR. However, the platform support should be in master and swift-5.0-branch as well as those represent the next active releases. How hard is it to get this working for master as a separate PR?

@futurejones
Copy link
Contributor Author

@tkremenek Thanks for looking at this.
I have an updated PR nearly ready to go for swift-5.0-branch but builds are currently failing due to other reasons. I haven't looked at the master yet but it hopefully it will be just a case realigning the patch and updating some syntax.
The plan always has been to add these changes to the master and 5.0. and as soon as they are ready they will uploaded. The reason for wanting the updates to the swift-4.2-branch first is so we can achieve this milestone and give some motivation to everyone for moving forward.

@tkremenek
Copy link
Member

@swift-ci clean test

@futurejones
Copy link
Contributor Author

@tkremenek @bob-wilson As it turns out the numerous issues we were having trying get swift-5.0-branch and the master built were all related to the need to default to the "gold" linker. This has been addressed in PR's #20845 and #20846.

As a result we have been able to move forward a lot sooner than I was expecting.

PR's for AArch64 variadics support have now been uploaded for swift-5.0-branch and the master
#20862 and #20863

@hpux735
Copy link
Contributor

hpux735 commented Nov 29, 2018

Fantastic work! Thanks so much!

@tkremenek
Copy link
Member

@futurejones this is great. We'll get these reviewed and merged as soon as possible into the respective branches.

We all appreciate the efforts that have gone into making this port happen!

One thing that came out of this discussion is that we should have clearer documentation guidance for folks working on new platform support on how that work should proceed. Specifically:

  • All work should be on master, like the regular core development of the project, as that is the branch that continues to "live on" for later releases
  • Port work can be cherry-picked into release branches as appropriate, but ports should target master first

I know it is tempting to first work on a previous release branch, but that doesn't match with the development model of the project and can lead to work not continuing to live on going forward.

@compnerd
Copy link
Member

compnerd commented Dec 1, 2018

FWIW, I've had pretty good luck with keeping swift master working on android aarch64. That really is close enough to Linux aarch64 that there isn't much broken I suspect. Now, I don't know if that is due to my local patchset or not, so, having a set of SRs on JIRA to track the issues that you are seeing would be useful. One "undocumented" trick that I have though is that I am using lld and not bfd (which is unusable) nor gold (which does have a few issues). The only thing that I hadn't actually gotten around to fixing properly was the variadics which @tkremenek did push into master (with follow up comments from @rjmccall and myself).

@futurejones
Copy link
Contributor Author

@weissi @kevints is it possible to get this into the 4.2.3 Release?
These changes have been merged with the master (#20862)(#21237) and 5.0 (#21885) branches.
Thanks.

@tkremenek
Copy link
Member

@rjmccall any concerns with landing this change as is for 4.2.3?

@rjmccall
Copy link
Contributor

No, this looks great; sorry for being slow to respond.

@tkremenek tkremenek merged commit 283cddb into swiftlang:swift-4.2-branch Feb 20, 2019
@futurejones
Copy link
Contributor Author

@tkremenek @rjmccall Thank you!
Would it also be possible to get #20709 merged into 4.2.3 well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants