Skip to content
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

Update aarch64-linux-android target to match android abi. #33500

Merged

Conversation

Nercury
Copy link
Contributor

@Nercury Nercury commented May 8, 2016

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Argh target_env: "gnu" was a typo, these are intended to have an empty env as they don't currently have one listed. Perhaps this could update the others instead?

Added eliminate_frame_pointer: false because the same option is also used by android's LLVM

Seems reasonable to me to follow what LLVM does here, but would be good to leave a comment as to why the option is set.

Added features +neon,+fp-armv8 because they

Is there some "official" documentation stating this as well that we could point to?

@Nercury
Copy link
Contributor Author

Nercury commented May 8, 2016

Is there some "official" documentation stating this as well that we could point to?

Updated doc link to http://developer.android.com/ndk/guides/cpu-features.html, the section "64-bit ARM CPU family" mentions that FP and ASIMD (which is NEON) must be available.

@Nercury Nercury force-pushed the update-aarch64-android-target-to-match-abi branch from 6851e9c to 1b64a24 Compare May 8, 2016 18:31
@alexcrichton
Copy link
Member

Could you add that link to the source where we specify those features? Also if you've got a link to the frame pointer business as well that'd be useful too!

@Nercury
Copy link
Contributor Author

Nercury commented May 9, 2016

@alexcrichton Link added.

Also if you've got a link to the frame pointer business as well that'd be useful too!

The frame pointer business so far is just a guess. Guess that works.

@alexcrichton
Copy link
Member

Thanks! Could you squash the commits down, and if the frame pointer elimination is still up in the air perhaps we could leave that option as the default?

@bors
Copy link
Contributor

bors commented May 9, 2016

☔ The latest upstream changes (presumably #33048) made this pull request unmergeable. Please resolve the merge conflicts.

@Nercury
Copy link
Contributor Author

Nercury commented May 10, 2016

leave that option as the default

what do you mean? I only saw it set for aarch64, presumably because there are problems with stack unwinding otherwise, but I might be mistaken

squash

a commit that updated liblibc got in by mistake, but do you want me to update libc as well?

@Nercury Nercury closed this May 10, 2016
@Nercury Nercury reopened this May 10, 2016
@alexcrichton
Copy link
Member

Have you observed crashes or otherwise incorrect results if frame pointers elimination is left on? I'm just thinking that if we don't have a concrete reason to turn it off we probably shouldn't, but if there's one then it's fine to turn off and we should just comment to that effect.

Ah throwing in a libc update is fine if necessary, but you can also rebase it out if you want.

@Nercury Nercury force-pushed the update-aarch64-android-target-to-match-abi branch from 64146e4 to 87c90da Compare May 14, 2016 22:09
@Nercury
Copy link
Contributor Author

Nercury commented May 14, 2016

@alexcrichton

  • Rebased on latest master, noticed that max_atomic_width was added for arm target but not armv7, so added a fix for that.
  • I have checked a version of my app with and without frame pointer elimination, and there was no observable difference. This change is now excluded from the PR.
  • Updating libc to latest master.

@alexcrichton
Copy link
Member

@bors: r+ 87c90da89e9683abd50a671bbdd4e041310b2192

@bors
Copy link
Contributor

bors commented May 15, 2016

⌛ Testing commit 87c90da with merge 81f9134...

@bors
Copy link
Contributor

bors commented May 15, 2016

💔 Test failed - auto-linux-64-cross-netbsd

@Nercury Nercury force-pushed the update-aarch64-android-target-to-match-abi branch from 87c90da to e512abd Compare May 15, 2016 08:58
@Nercury
Copy link
Contributor Author

Nercury commented May 15, 2016

Excluding latest libc commit for now because it removed libc::HW_NCPU for some reason. We will probably need to resolve it in libc.

@alexcrichton
Copy link
Member

Hm yes that's an accident that it was removed, but we can land this in the meantime.

@bors: r+ e512abd

@bors
Copy link
Contributor

bors commented May 16, 2016

⌛ Testing commit e512abd with merge 4fdf2c4...

bors added a commit that referenced this pull request May 16, 2016
…-abi, r=alexcrichton

Update aarch64-linux-android target to match android abi.

- Changed `target_env` to "gnu" to empty "" for all android targets because it does not matter for android.
- The PR #33048 added "max_atomic_width" for arm-android but missed recently added armv7-android. Add it there too.
- Added features `+neon,+fp-armv8` because they [must exist on `aarch64` android](http://developer.android.com/ndk/guides/cpu-features.html).
- Update libc to include rust-lang/libc#282 so that rust's std lib works on android's aarch64 (the main issue there was incorrect structure alignment on 64-bit arm).

r? @alexcrichton
@bors bors merged commit e512abd into rust-lang:master May 16, 2016
@Nercury Nercury deleted the update-aarch64-android-target-to-match-abi branch May 17, 2016 06:03
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.

4 participants