Build and testing for iOS

[pietro]

My work on iOS is on commits: pietro/ring@b4a0141, pietro/ring@db3e647 and pietro/ring@7aec786

I don't know much about testing static libraries on iOS. But with my changes all tests pass for i386-apple-ios and x86_64-apple-ios. And building works for aarch64-apple-ios.

Both armv7-apple-ios and armv7s-apple-ios fail due to differences on the syntax of crypto/curve25519/asm/x25519-asm-arm.S and what the version of clang distributed by apple support.

FYI:

$ clang --version
Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[pietro]

The issues I had with crypto/curve25519/asm/x25519-asm-arm.S and clang were reported by google and fixed upstream https://llvm.org/bugs/show_bug.cgi?id=25722 and https://llvm.org/bugs/show_bug.cgi?id=25720

clang 3.9 snapshot has issues with the code generated by crypto/sha/asm/sha256-armv4.pl. It uses the adrl pseudo-instruction and llvm won't implement it: https://llvm.org/bugs/show_bug.cgi?id=24350 The current openssl version of that file has some changes and it at least compiles.

[briansmith]

Both armv7-apple-ios and armv7s-apple-ios fail due to differences on the syntax of crypto/curve25519/asm/x25519-asm-arm.S

armv7*-applie-ios is a dying platform, so I'm OK with just using the C implementation instead of the asm implementation for those targets. I don't want to spend extra effort on 32-bit ARM iPhones which clearly Apple is going all-Aarch64.

clang 3.9 snapshot has issues with the code generated by crypto/sha/asm/sha256-armv4.pl. It uses the adrl pseudo-instruction and llvm won't implement it: https://llvm.org/bugs/show_bug.cgi?id=24350 The current openssl version of that file has some changes and it at least compiles.

I'll try merging in the latest OpenSSL versions.

[briansmith]

I committed f185fe5, 7d6e282, and 0e4abb4. Thanks!

[briansmith]

I'll try merging in the latest OpenSSL versions.

My first attempt at this is in my ios branch: https://github.com/briansmith/ring/tree/ios. Let me know if/how it helps.

[briansmith]

I've updated my ios branch with an attempt to merge all the seemingly-relevant changes to OpenSSL's asm code (on all ring platforms) into ring.

[briansmith]

I've now pushed all those asm changes to master.

[pietro]

Cool, I'll test them out. I was worried ring wouldn't build for android when using the clang based tooling but it uses gcc as assembler.

[briansmith]

Cool, I'll test them out. I was worried ring wouldn't build for android when using the clang based tooling but it uses gcc as assembler.

Any update? If you're too busy to finish this, then could you share your partially-completed work? Your contributions have been amazing and I'm really looking forward to getting the iOS stuff done.

[luser]

FYI, I looked into automated testing on-device with iOS when working on my Gecko port, and it's a nightmare. The only real supported scenario for unit tests is XCTest run from XCode:
https://developer.apple.com/library/ios/documentation/ToolsLanguages/Conceptual/Xcode_Overview/UnitTesting.html

There is ios-deploy which can run arbitrary apps on an un-jailbroken phone, but I found it very fiddly to use:
https://github.com/phonegap/ios-deploy

There is this undocumented project I found from some brief googling which looks promising:
https://github.com/SSheldon/rust-test-ios

[briansmith]

According to @kali, his PR #315 fixes the build. It would be good to get confirmation of that. If so, then "just" the test automation is left here.

[kali]

OK, #315 may not work as I thought it was, but we made a cargo extension to help testing "stuff" library on device.

https://github.com/snipsco/dinghy

[briansmith]

IIUC, @kali's PR made armv7-apple-ios (and armv7s-apple-ios?; see #426) build, and I think aarch64-apple-ios is building. Thus I think we “just” need to do the automation!

[kali]

I have added support for external files on iOS in dinghy starting 0.1.12. With a tiny set of changes to tests #429, we have x86_64 ios simulator and aarch64 ios devices passing!

Both armv7 variant are not working. They are actually crashing without much information. Maybe dinghy eats it somewhere, I'm investigating.

I also gave a quick shot at running the tests on an android phone (despite not having a strategy for copying the support files yet), but it looks like dinghy does not even manage to link the test runner. More investigations.

[kali]

mmm... back in GFp_x25519_NEON :/

* thread #2: tid = 0x4b316c, 0x0013bb72 Dinghy`GFp_x25519_NEON + 2, name = 'agreement::tests::test_agreement_agree_ephemeral', stop reason = EXC_BAD_INSTRUCTION (code=EXC_ARM_UNDEFINED, subcode=0xc00ded2d)
  * frame #0: 0x0013bb72 Dinghy`GFp_x25519_NEON + 2
    frame #1: 0x00131818 Dinghy`GFp_x25519_public_from_private(out_public_value="", private_key="�F�k�R|\x9d;\x16\x15K\x82F^�b\x14L\n��Z\x18Pj\"D�D\x9a\U00000080") + 52 at curve25519.c:4809
    frame #2: 0x000d76f4 Dinghy`ring::ec::x25519::x25519_public_from_private::h9c732748788dd2d2 + 296 at x25519.rs:60
    frame #3: 0x00119590 Dinghy`ring::agreement::tests::test_agreement_agree_ephemeral::_$u7b$$u7b$closure$u7d$$u7d$::h0d7fdaf1e8201348 + 1512 at ec.rs:73
    frame #4: 0x001127fc Dinghy`ring::test::from_file::_$u7b$$u7b$closure$u7d$$u7d$::h28d5b6a9b5a08798 + 100 at test.rs:256

[briansmith]

mmm... back in GFp_x25519_NEON :/

Do you think this could be a THUMB2 issue? Maybe the iOS machine requires THUMB2? Otherwise, can we isolate the bad instruction?

[kali]

I'm not sure where this is going, but at least it's prompting me to extends Dinghy's capabilities :) Now it can run lldb interactively...

Here is a lldb dump of the begin of the problematic function. TBH, I'm not even sure it makes any sense at all. WDYT ?

Process 364 stopped
* thread #2: tid = 0x60ea, 0x0011fb72 Dinghy`GFp_x25519_NEON + 2, name = 'agreement::tests::test_agreement_agree_ephemeral', stop reason = EXC_BAD_INSTRUCTION (code=EXC_ARM_UNDEFINED, subcode=0xc00ded2d)
    frame #0: 0x0011fb72 Dinghy`GFp_x25519_NEON + 2
Dinghy`GFp_x25519_NEON:
->  0x11fb72 <+2>:  stc    p0, c12, [sp, #-52]!
    0x11fb76 <+6>:  b      0x11feba                  ; ._mainloop + 42
    0x11fb78 <+8>:  udf    #0x2e
    0x11fb7a <+10>: b      0x120018                  ; ._mainloop + 392
(lldb) di
Dinghy`GFp_x25519_NEON:
    0x11fb70 <+0>:   ldrh   r0, [r2, #0x18]
->  0x11fb72 <+2>:   stc    p0, c12, [sp, #-52]!
    0x11fb76 <+6>:   b      0x11feba                  ; ._mainloop + 42
    0x11fb78 <+8>:   udf    #0x2e
    0x11fb7a <+10>:  b      0x120018                  ; ._mainloop + 392
    0x11fb7c <+12>:  beq    0x11fbbe                  ; <+78>
    0x11fb7e <+14>:  b      0x12031c                  ; ._mainloop + 1164
    0x11fb80 <+16>:  lsrs   r0, r6
    0x11fb82 <+18>:  b      0x11ff20                  ; ._mainloop + 144
    0x11fb84 <+20>:  str    r0, [r7, #0xc]
    0x11fb86 <+22>:  b      0x11ff24                  ; ._mainloop + 148
    0x11fb88 <+24>:  strh   r0, [r6, #0xe]
    0x11fb8a <+26>:  b      0x11ff28                  ; ._mainloop + 152
    0x11fb8c <+28>:  adr    r1, #992
    0x11fb8e <+30>:  b      0x11ff2c                  ; ._mainloop + 156
    0x11fb90 <+32>:  stm    r1!, {r5, r6, r7}
    0x11fb92 <+34>:  b      0x11f6b0                  ; ChaCha20_neon + 848
    0x11fb94 <+36>:  b      0x11ff60                  ; ._mainloop + 208
    0x11fb96 <+38>:  b      0x11f6b4                  ; ChaCha20_neon + 852
    0x11fb98 <+40>:  movs   r0, r0
    0x11fb9a <+42>:  b      0x11fede                  ; ._mainloop + 78
    0x11fb9c <+44>:  asrs   r1, r0, #0x20
    0x11fb9e <+46>:  b      0x11fee2                  ; ._mainloop + 82
    0x11fba0 <+48>:  movs   r0, #0x2
    0x11fba2 <+50>:  b      0x11fee6                  ; ._mainloop + 86
    0x11fba4 <+52>:  adds   r0, #0x20
    0x11fba6 <+54>:  b      0x1200c4                  ; ._mainloop + 564
    0x11fba8 <+56>:  ands   r0, r0
    0x11fbaa <+58>:  b      0x1202ee                  ; ._mainloop + 1118
    0x11fbac <+60>:  str    r6, [r7, r3]
    0x11fbae <+62>:  b      0x1202f2                  ; ._mainloop + 1122
    0x11fbb0 <+64>:  lsls   r1, r2, #0x1
    0x11fbb2 <+66>:  .long  0x20d0f280                ; unknown opcode
    0x11fbb6 <+70>:  .long  0x00d0f3b9                ; unknown opcode
    0x11fbba <+74>:  .long  0x4013f3b8                ; unknown opcode
    0x11fbbe <+78>:  .long  0x5016f281                ; unknown opcode
    0x11fbc2 <+82>:  .long  0x6c02f282                ; unknown opcode
    0x11fbc6 <+86>:  b      0x1200e4                  ; ._mainloop + 596

[briansmith]

That disassembly doesn't look like it makes sense to me. I'm guessing that we have a mismatch in expectations between thumb and non-thumb mode. I think maybe the problem is the .thumb_func? Maybe try removing that and/or adding an .arm or .syntax unified or whatever.at the top of the file. (Sorry, I'm not sure.)

[kali]

It does work without the thumb_func, good call. PR coming.

[kali]

With that we're down to two failing test on armv7, i'm having a look:

---- c::test_i64_metrics stdout ----
	thread 'c::test_i64_metrics' panicked at 'assertion failed: `(left == right)` (left: `(4, 8)`, right: `(8, 8)`)', src/c.rs:162
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- c::test_u64_metrics stdout ----
	thread 'c::test_u64_metrics' panicked at 'assertion failed: `(left == right)` (left: `(4, 8)`, right: `(8, 8)`)', src/c.rs:164

[kali]

ok, once again, I'm not too sure about what is happening. Could you enlight me again ? what are we trying to check there ?

           let rust_align =
                if $expected_align_factor != 1 &&
                   mem::align_of::<$name>() != c_align as usize {
                   mem::align_of::<$name>() * $expected_align_factor
                } else {
                    mem::align_of::<$name>()
                };

            assert_eq!((rust_align, mem::size_of::<$name>()),
                       (c_align as usize, c_size as usize));

[briansmith]

That test is verifying that our understanding of the alignment of C's uint64_t and int64_t is correct. Unfortunately, Rust has different beliefs about how to align u64 and i64 on some platforms, so we have to do this in src/c.rs:

#[cfg(all(test,
          not(all(target_arch = "x86",
                  any(target_os = "linux",
                      target_os = "macos",
                      target_os = "ios")))))]
const SIXTY_FOUR_BIT_ALIGNMENT_FACTOR: usize = 1;

#[cfg(all(test, target_arch = "x86",
                  any(target_os = "linux",
                      target_os = "macos",
                      target_os = "ios")))]
const SIXTY_FOUR_BIT_ALIGNMENT_FACTOR: usize = 2;

It seems like we need to switch the alignment factor that applies for 32-bit ARM iOS in a similar way.

Note that this won't have any effect other than making the test pass. The reason for this test is to ensure we don't get surprised by alignment requirements on new architectures that are different than what we've already run into.

[briansmith]

“An important difference between iOS ABI and EABI is alignment of 64-bit arguments: iOS uses 4-byte alignment, while EABI uses 8.” - http://stackoverflow.com/questions/2795360/iphone-arm-calling-convention#comment17867468_13131328

[briansmith]

It seems like maybe we should be preserving the r7 register and maybe also r11 in addition to other registers being preserved? See https://developer.apple.com/library/content/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html. So more work may be required.

[briansmith]

I filed #439 about enabling Thumb-2.

[briansmith]

See also http://shervinemami.info/armAssembly.html.

[kali]

With this latest PR, I could make ring compile and pass test on the four iOS arch I have at my disposal (armv7, v7s, aarch64, and the x86_64 sim). I am not in a position to test the x86 simulator. It may need the same kind of fix than #440, but I'm not sure we care that much.

[briansmith]

Awesome work!

I am not in a position to test the x86 simulator. It may need the same kind of fix than #440, but I'm not sure we care that much.

I think it was already fixed earlier, since it was already handled the same way:

            all(target_arch = "x86", target_os = "ios"),
            all(target_arch = "arm", target_os = "ios")

What about the potential register preservation issues mentioned in #184 (comment)? Are you or is anybody you know able to research and correct that? See https://bugs.chromium.org/p/boringssl/issues/detail?id=176 in addition to what I pointed out above, which shows that this code likely isn't perfect in that respect.

[kali]

Yeah, I think you're right. From my understanding, r11 must be saved and restored, r7 must at least be saved and restored, and it would be better to set it to the FP value so that debugger would not get lost.

I'll try to have a look one of these days.

[kali]

About the ARM code, looking at the prolog and epilog, I notice that r11 and r7 are saved and restored (the strd and ldrd store/load two registers at a time).
Setting r7 to FP is not possible without refactoring the whole function, as it makes use of all the registers. From my understanding, the only impact here is to break the stack dump in the debugger while stopped in the function.

[briansmith]

32-bit iOS is no longer a thing so i386- and armv7- don't matter any more, IIUC.

aarch64-apple-ios is now built in CI/CD but the tests are not run. This is blocked on having M1 Mac runners. My understanding is that on M1 Macs, Xcode's iOS emulator will run ARM iOS executables.

We should add x86_64-apple-ios builds to CI/CD as well, I suppose, to make sure we stay compatible with the emulator on x86_64 Macs.

https://medium.com/snips-ai/dinghy-painless-rust-tests-and-benches-on-ios-and-android-c9f94f81d305 notes that (prior to ring 0.16.17), cargo dinghy had trouble building ring. I think that was because they were building ring from GitHub and so the warnings that were fixed in PR #1071 were probably breaking their build. So, while the aforementioned CI/CD job will probably help ensure ring keeps working with cargo dinghy, it might be worth having a new CI/CD job for that.

[briansmith]

I think this is probably as complete as we can get it.