Failure to compile to armv7-apple-ios #312
Comments
|
See also #184. For this specific issue, we can probably use some |
|
Fixed by #315. Thanks! |
|
All right. So this is still not building. I must admit I don't understand what lead me thing it was building at the time of the #315 PR. Something may have changed in XCode side, but most likely I was just confused. So. The issue is still here, and exactly the same. My understanding is that x25519-asm-arm.S uses gcc/gas directives and so compiles well with the android toolchain, but clang choke on it. As far as I can tell, it's the only file we have in this form in the code, others are generated from perl scripts. |
|
[Note: I currently don't have a working Mac to develop changes to fix this issue, so contributions from others are highly encouraged.] Background information: Clang's integrated assembler doesn't support everything that gas supports. Recently the Chromium project has motivated a lot of improvements in this area that are relevant to ring since ring uses the same assembly code as BoringSSL, which is a component of Chromium. Can you try the newest version of Apple's iOS toolchain that you can reasonably use in production to see if it has incorporated the aforementioned clang fixes? Clang's assembler added
If I understand your pull request #315, we pass crypto/perlasm/arm-xlate.pl implies that we need to use Similarly, crypto/perlasm/arm-xlate.pl contains this: Presumably we need to do a similar substitution for the |
|
All right, here are some findings. I'm running current/latest xcode version. So, first, I think part of the problem is that Then, as far as I can tell, I could not find any combination of options that would make clang accept a .s input with .fpu neon. So I'm not sure about the hint in the LLVM discussion (https://llvm.org/bugs/show_bug.cgi?id=20700) Could it be something Apple chose not to include in its clang version ? I also tried to compile the My best assumption at this point is that Apple decided to enable neon support all the time (as their devices all have it). They ignore the command line switch, and just choke on the directive. |
Yes, that's probably why only this file has the problem. No, we shouldn't make it PerlAsm just to solve this problem. There are other non-PerlAsm assembly language sources in ring and BoringSSL for other platforms, and we want to keep this in sync with the upstream.
It seems then that we can just use Thanks for your help with this! |
|
I'm glad to help, it's just... I may be a bit over my head around here :) Anyway. Progress. I managed to get the thing to compile AND link its tests. Here is what I had to change. I used whatever I could grab from perlasm as you suggest, but had to add leading underscore to symbols, and transform ldr ops into mov as mentioned here: https://llvm.org/bugs/show_bug.cgi?id=25722. Also, I had to throw in a GFp_cpuid_setup and did not think twice about where to do it. diff --git a/crypto/cpu-arm.c b/crypto/cpu-arm.c
index c364348..3e2b759 100644
--- a/crypto/cpu-arm.c
+++ b/crypto/cpu-arm.c
@@ -21,6 +21,9 @@
extern uint32_t GFp_armcap_P;
+void GFp_cpuid_setup(void) {
+ GFp_armcap_P |= ARMV7_NEON;
+}
char GFp_is_NEON_capable_at_runtime(void) {
return (GFp_armcap_P & ARMV7_NEON) != 0;
diff --git a/crypto/curve25519/asm/x25519-asm-arm.S b/crypto/curve25519/asm/x25519-asm-arm.S
index c382202..3f7d268 100644
--- a/crypto/curve25519/asm/x25519-asm-arm.S
+++ b/crypto/curve25519/asm/x25519-asm-arm.S
@@ -20,14 +20,25 @@
#if !defined(OPENSSL_NO_ASM)
#if defined(__arm__)
+#if !defined(__APPLE__)
.fpu neon
+#endif
.text
.align 4
.global GFp_x25519_NEON
-.hidden GFp_x25519_NEON
-.type GFp_x25519_NEON, %function
-GFp_x25519_NEON:
+
+#if defined(__APPLE__)
+ .private_extern _GFp_x25519_NEON
+ #ifdef __thumb2__
+ .thumb_func _GFp_x25519_NEON
+ #endif
+#else
+ .hidden GFp_x25519_NEON
+ .type GFp_x25519_NEON, %function
+#endif
+
+_GFp_x25519_NEON:
vpush {q4,q5,q6,q7}
mov r12,sp
sub sp,sp,#736
@@ -42,8 +53,13 @@ mov r0,r0
mov r1,r1
mov r2,r2
add r3,sp,#32
-ldr r4,=0
-ldr r5,=254
+#if defined(__APPLE__)
+ mov r4, #0
+ mov r5, #254
+#else
+ ldr r4,=0
+ ldr r5,=254
+#endif
vmov.i32 q0,#1
vshr.u64 q1,q0,#7
vshr.u64 q0,q0,#8
@@ -61,7 +77,11 @@ vst1.8 {d4-d5},[r6,: 128]!
vst1.8 {d4-d5},[r6,: 128]!
vst1.8 d4,[r6,: 64]
add r6,r3,#0
-ldr r7,=960
+#if defined(__APPLE__)
+ mov r7, #960
+#else
+ ldr r7,=960
+#endif
sub r7,r7,#2
neg r7,r7
sub r7,r7,r7,LSL #7
diff --git a/src/init.rs b/src/init.rs
index c4d470b..b2e476b 100644
--- a/src/init.rs
+++ b/src/init.rs
@@ -14,7 +14,7 @@
#[inline(always)]
pub fn init_once() {
- #[cfg(not(all(target_arch = "aarch64", target_os = "ios")))]
+ #[cfg(not(all(target_os = "ios")))]
{
extern crate std;
extern { fn GFp_cpuid_setup(); }
diff --git a/src/rand.rs b/src/rand.rs
index a4c0879..1d20d96 100644
--- a/src/rand.rs
+++ b/src/rand.rs
@@ -26,7 +26,6 @@
//! documentation for more details.
#[cfg(any(target_os = "linux",
- target_os = "ios",
windows,
test))]
use c;This is obviously not intended as a PR, but feel free to have a look. Many tests are failing. I'm using dinghy for testing, and it does not propagate the source files so any test trying to open a file fails miserably. I think I should probably fix that on dinghy side instead of rewritting half of the tests from ring (and from the whole rust ecosystem). I'll try to get that done quickly so we can move forward. |
|
Yes, that looks right to me. I'm not sure if all assemblers accept the indented directives so it would be better if they weren't indented. I suggest that we do it this way:
WDYT? |
|
I forgot to mention: The benefit of submitting it to BoringSSL is that they probably have iOS testing infrastructure that works already, so they may be be able to test the code on iOS before we can. |
In these cases, I think we can always just use |
|
No objection with that plan. |
|
It is now building and tests are passing, but may not be 100% correct yet. It looks like the rest of the work is being tracked in #184 now, so closing this. Thanks for the awesome contributions! |
The text was updated successfully, but these errors were encountered: