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

Compilation/link fixes for iOS/arm #423

Closed
wants to merge 3 commits into from
Closed

Conversation

kali
Copy link
Contributor

@kali kali commented Jan 24, 2017

As discussed in #312.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Hopefully #425 will allow us to get rid of all the non-assembly-language changes in this PR. Otherwise, please split the PR into two commits: The changes to the assembly language source file, and the rest. Also, please add the statement at https://github.com/briansmith/ring#contributing to the end of your commit messages.

Thanks again!

.fpu neon
#endif
.text
.align 4

.global GFp_x25519_NEON
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the .global directive need to have an underscore prefix added too?

extern uint32_t GFp_armcap_P;

void GFp_cpuid_setup(void) {
GFp_armcap_P |= ARMV7_NEON;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to implement GFp_cpuid_setup for NEON on non-Android targets that have a C compiler with NEON enabled. Please rebase your assembly language changes on top of #425 to see if that PR fixes the NEON detection problem.

This should allow armv7-apple-ios targets to use NEON without any
runtime checks.
@coveralls
Copy link

coveralls commented Jan 25, 2017

Coverage Status

Coverage increased (+0.1%) to 93.607% when pulling 6659e01531c5133b58d0dc79dbf691783615b589 on kali:master into 9e194a3 on briansmith:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.607% when pulling 6659e01531c5133b58d0dc79dbf691783615b589 on kali:master into 9e194a3 on briansmith:master.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.
I agree to license my contributions to each file under the terms given
at the top of each file I changed.
@kali
Copy link
Contributor Author

kali commented Jan 25, 2017

Rebased again #425, but still need two conditional compilation adjutements to link sucessfully.

@briansmith
Copy link
Owner

Thanks! I reordered these changes, adjusted the commit messages a bit, and landed them as 68c262c and 6215cef.

I appreciate your help here! I won't have my Mac until next week!

@briansmith briansmith closed this Jan 25, 2017
@briansmith briansmith added this to the 0.6.3 milestone Jan 25, 2017
@briansmith
Copy link
Owner

By the way, I am surprised to see people trying to target armv7-apple-ios. I had thought everybody had moved to aarch64-apple-ios exclusively. Are you building for aarch64-apple-ios too?

@briansmith briansmith reopened this Jan 25, 2017
@briansmith briansmith closed this Jan 25, 2017
@kali
Copy link
Contributor Author

kali commented Jan 25, 2017

I'm targetting both. The hard reality is Apple pushes developpers to move to bitcode, but rust is not there yet (and I'm not sure how assembly bits could fit in). In the meantime, I make my iOS colleagues less grumpy by providing 5-arch binaries... but maybe it's bit of a waste of time.

@briansmith
Copy link
Owner

OK, understood. Again, I appreciate your effort here. If you have any suggestions for how to add CI for iOS (armv7 and/or aarch64), I'd love to get them—preferably in PR form!

Also, I am curious: It seems like there's not much point in us optimizing 32-bit ARM binaries on 64-bit iOS, is there? In particular, right now ring has this:

#if defined(OPENSSL_ARM) || defined(OPENSSL_AARCH64)

#if defined(OPENSSL_APPLE)
/* iOS builds use the static ARM configuration. */
#define OPENSSL_STATIC_ARMCAP

#if defined(OPENSSL_AARCH64)
#define OPENSSL_STATIC_ARMCAP_AES
#define OPENSSL_STATIC_ARMCAP_SHA1
#define OPENSSL_STATIC_ARMCAP_SHA256
#define OPENSSL_STATIC_ARMCAP_PMULL
#endif

#endif

So, we'll never try to detect the ARMv8 crypto extensions in 32-bit mode, and we'll always assume they're present in 64-bit mode. I think this is the right thing to do because every iOS application should always include an 64-bit binary and only the 32-bit binary is optional now (effectively). But if I'm wrong, let me know.

@kali
Copy link
Contributor Author

kali commented Jan 25, 2017

I think this is correct. The only unlikely situation where anybody would ship an armv7 only app is if you have some external dependency that for some reason you can not get in aarch64.

For iOS CI, there are a few things that can probably be done, by somewhat increasing level of difficulty and risk.

  • Continuous build of library using travis for the 5 ios platforms
  • Continuous build of tests, on travis for all platforms (this finds missing symbols errors, that the library-only build typically misses). Dinghy helps getting the linker right (cargo alone will do it wrong).
  • Continuous test for ios/x86 and ios/x86_64 simulator on travis with Dinghy. I'm trying to get Dinghy to deploy test files alongside tests, as this is a very common issue.
  • Continuous test on iOS devices requires some kind of infrastructure travis does not provides. It could be as simple as a phone plugged on a mac mini somewhere. There are probably enough iphones with broken screens around to manage that at a single project scale, but even that would probably require some human attention for maintenance.

I have the same issues with ramp (minus the supporting test files), and other projects in my company, so I will spend some time on these issues. I'll PR whatever make sense for ring.

@kali
Copy link
Contributor Author

kali commented Jan 25, 2017

http://iossupportmatrix.com/

My iOS colleagues send me this link. So armv7 is not supported, but armv7s still is and will probably stay suported for about one more year. Differences are the FP unit and a hardware integer division. Not sure it is worth the trouble optimizing ring specifically for this. The compiler will do it right for rust code, only manual/generated assembly could benefit from work here...

@briansmith
Copy link
Owner

@kali I submitted your changes here to BoringSSL: https://boringssl-review.googlesource.com/c/13385/

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.

None yet

3 participants