1209260 - iOS/A8: Crash due to unaligned access in js::math_sincos_impl

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect, P5)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

I frequently get the following crash on iOS. Surely we've run into these alignment issues before on Android/B2G? Or maybe A8 doesn't tolerate any unaligned accesses, and other processors do? Am I missing some build flag to avoid this?

* thread #1: tid = 0x6d6c0, 0x021f241e XUL`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] __sincos(__x=<unavailable>, __sinp=<unavailable>, __cosp=<unavailable>) at math.h:668, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_ARM_DA_ALIGN, address=0x149703)
    frame #0: 0x021f241e XUL`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] __sincos(__x=<unavailable>, __sinp=<unavailable>, __cosp=<unavailable>) at math.h:668
  * frame #1: 0x021f241e XUL`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] js::math_sincos_uncached(double, double*, double*) at jsmath.cpp:962
    frame #2: 0x021f241e XUL`js::math_sincos_impl(mathCache=<unavailable>, x=2.9570096233817572E-308, sin=0x00149703, cos=0x00000000) + 194 at jsmath.cpp:977

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't get any crash if I disable baselinejit and ion. At least with my use case: http://hexgl.bkcore.com/play

Comment 2

[Lars T Hansen [:lth]]

The "sin" pointer in the __sincos call looks badly misaligned (last line of the Description).

I'm guessing bug 984018 is part of the puzzle, hg blame points to that patch.

Comment 3

[Victor Carlquist]

I'll investigate it, thanks.

Comment 4

[Victor Carlquist]

For some reason, the problem seems to be in the memory alignment.
The Android/BG2 runs correctly on try server...

Nicolas, could I align the memory before calling the math_sincos_impl?

thanks.

Comment 5

[Lars T Hansen [:lth]]

Do you know what instruction it is that crashes?  I ask because I'm wondering if instructions are being generated on the A8 that we don't generate on our other test rigs, and if those instructions have more stringent alignment requirements.

Comment 6

[Victor Carlquist]

No, I don't.(In reply to Lars T Hansen [:lth] from comment #5)
> Do you know what instruction it is that crashes?  
No,

James, could you post the stack/instructions for us, please?

Comment 7

[Nicolas B. Pierron [:nbp]]

I do not see any issue in the CodeGenerator code added in Bug 984018.

The stack alignment is handled in CallWithABIPre [1] and is based on the ABI definition of ARM64 documentation [2] and on the expected stack alignment of the system [3].

Sean, are you able to reproduce similar issues on ARM64 devices?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.cpp#502,507-509
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm64/Assembler-arm64.cpp#38,49
[3] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm64/Architecture-arm64.h#301

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

The crash once again with the disassembly is below:


* thread #5: tid = 0x237d59, 0x01c8ccae GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] __sincos(__x=<unavailable>, __sinp=<unavailable>, __cosp=<unavailable>) at math.h:668, stop reason = EXC_BAD_ACCESS (code=EXC_ARM_DA_ALIGN, address=0x2bde03)
  * frame #0: 0x01c8ccae GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] __sincos(__x=<unavailable>, __sinp=<unavailable>, __cosp=<unavailable>) at math.h:668
    frame #1: 0x01c8ccae GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) [inlined] js::math_sincos_uncached(double, double*, double*) at jsmath.cpp:962
    frame #2: 0x01c8ccae GeckoKit`js::math_sincos_impl(mathCache=<unavailable>, x=6.8816537873078332, sin=0x002bde03, cos=0x00000000) + 194 at jsmath.cpp:977



GeckoKit`js::math_sincos_impl:
    0x1c8cbec <+0>:   push   {r4, r5, r6, r7, lr}
    0x1c8cbee <+2>:   add    r7, sp, #0xc
    0x1c8cbf0 <+4>:   push.w {r8, r10}
    0x1c8cbf4 <+8>:   vpush  {d8}
    0x1c8cbf8 <+12>:  sub    sp, #0x10
    0x1c8cbfa <+14>:  eor.w  r6, r2, r1
    0x1c8cbfe <+18>:  mov    r10, r3
    0x1c8cc00 <+20>:  add.w  r3, r6, #0x100
    0x1c8cc04 <+24>:  vmov   d8, r1, r2
    0x1c8cc08 <+28>:  uxth   r5, r3
    0x1c8cc0a <+30>:  eor.w  r5, r5, r3, lsr #16
    0x1c8cc0e <+34>:  mov    r3, r5
    0x1c8cc10 <+36>:  bfc    r3, #12, #20
    0x1c8cc14 <+40>:  eor.w  r3, r3, r5, lsr #4
    0x1c8cc18 <+44>:  add.w  r3, r3, r3, lsl #2
    0x1c8cc1c <+48>:  add.w  r5, r0, r3, lsl #2
    0x1c8cc20 <+52>:  vldr   d16, [r5]
    0x1c8cc24 <+56>:  vcmpe.f64 d16, d8
    0x1c8cc28 <+60>:  vmrs   APSR_nzcv, fpscr
    0x1c8cc2c <+64>:  bne    0x1c8cc40                 ; <+84> at jsmath.h:87
    0x1c8cc2e <+66>:  ldr    r3, [r5, #0x8]
    0x1c8cc30 <+68>:  cmp    r3, #0x1
    0x1c8cc32 <+70>:  bne    0x1c8cc44                 ; <+88> at jsmath.h:87
    0x1c8cc34 <+72>:  vldr   d16, [r5, #12]
    0x1c8cc38 <+76>:  movs   r3, #0x1
    0x1c8cc3a <+78>:  vstr   d16, [r10]
    0x1c8cc3e <+82>:  b      0x1c8cc46                 ; <+90> [inlined] js::MathCache::hash(double, js::MathCache::MathFuncId) at jsmath.h:84
    0x1c8cc40 <+84>:  movs   r3, #0x0
    0x1c8cc42 <+86>:  b      0x1c8cc46                 ; <+90> [inlined] js::MathCache::hash(double, js::MathCache::MathFuncId) at jsmath.h:84
    0x1c8cc44 <+88>:  movs   r3, #0x0
    0x1c8cc46 <+90>:  add.w  r6, r6, #0x200
    0x1c8cc4a <+94>:  ldr.w  r8, [r7, #0x8]
    0x1c8cc4e <+98>:  uxth   r4, r6
    0x1c8cc50 <+100>: eor.w  r4, r4, r6, lsr #16
    0x1c8cc54 <+104>: mov    r6, r4
    0x1c8cc56 <+106>: bfc    r6, #12, #20
    0x1c8cc5a <+110>: eor.w  r6, r6, r4, lsr #4
    0x1c8cc5e <+114>: add.w  r6, r6, r6, lsl #2
    0x1c8cc62 <+118>: add.w  r6, r0, r6, lsl #2
    0x1c8cc66 <+122>: vldr   d16, [r6]
    0x1c8cc6a <+126>: vcmpe.f64 d16, d8
    0x1c8cc6e <+130>: vmrs   APSR_nzcv, fpscr
    0x1c8cc72 <+134>: itt    eq
    0x1c8cc74 <+136>: ldreq  r0, [r6, #0x8]
    0x1c8cc76 <+138>: cmpeq  r0, #0x2
    0x1c8cc78 <+140>: beq    0x1c8cd08                 ; <+284> [inlined] js::MathCache::isCached(double, js::MathCache::MathFuncId, double*, unsigned int*) at jsmath.cpp:975
    0x1c8cc7a <+142>: cmp    r3, #0x0
    0x1c8cc7c <+144>: beq    0x1c8cca0                 ; <+180> at jsmath.cpp:987
    0x1c8cc7e <+146>: vldr   d16, [r6]
    0x1c8cc82 <+150>: add.w  r0, r6, #0x8
    0x1c8cc86 <+154>: vcmpe.f64 d16, d8
    0x1c8cc8a <+158>: vmrs   APSR_nzcv, fpscr
    0x1c8cc8e <+162>: itt    eq
    0x1c8cc90 <+164>: ldreq  r3, [r0]
    0x1c8cc92 <+166>: cmpeq  r3, #0x2
    0x1c8cc94 <+168>: bne    0x1c8cd40                 ; <+340> at jsmath.cpp:988
    0x1c8cc96 <+170>: vldr   d16, [r6, #12]
    0x1c8cc9a <+174>: vstr   d16, [r8]
    0x1c8cc9e <+178>: b      0x1c8cd34                 ; <+328> at jsmath.cpp:988
    0x1c8cca0 <+180>: mov    r0, sp
    0x1c8cca2 <+182>: blx    0x1ff25c8                 ; symbol stub for: __sincos_stret
    0x1c8cca6 <+186>: vldr   d16, [sp]
    0x1c8ccaa <+190>: add.w  r0, r5, #0x8
->  0x1c8ccae <+194>: vstr   d16, [r10]
    0x1c8ccb2 <+198>: vldr   d16, [sp, #8]
    0x1c8ccb6 <+202>: vstr   d16, [r8]
    0x1c8ccba <+206>: vldr   d20, [r5]
    0x1c8ccbe <+210>: vldr   d18, [r10]
    0x1c8ccc2 <+214>: vcmpe.f64 d20, d8
    0x1c8ccc6 <+218>: vmrs   APSR_nzcv, fpscr
    0x1c8ccca <+222>: itt    eq
    0x1c8cccc <+224>: ldreq  r1, [r0]
    0x1c8ccce <+226>: cmpeq  r1, #0x1
    0x1c8ccd0 <+228>: beq    0x1c8cce2                 ; <+246> [inlined] js::MathCache::store(js::MathCache::MathFuncId, double, double, unsigned int) at jsmath.cpp:979
    0x1c8ccd2 <+230>: movs   r1, #0x1
    0x1c8ccd4 <+232>: vstr   d8, [r5]
    0x1c8ccd8 <+236>: str    r1, [r0]
    0x1c8ccda <+238>: vstr   d18, [r5, #12]
    0x1c8ccde <+242>: vldr   d16, [r8]
    0x1c8cce2 <+246>: vldr   d18, [r6]
    0x1c8cce6 <+250>: add.w  r0, r6, #0x8
    0x1c8ccea <+254>: vcmpe.f64 d18, d8
    0x1c8ccee <+258>: vmrs   APSR_nzcv, fpscr
    0x1c8ccf2 <+262>: itt    eq
    0x1c8ccf4 <+264>: ldreq  r1, [r0]
    0x1c8ccf6 <+266>: cmpeq  r1, #0x2
    0x1c8ccf8 <+268>: beq    0x1c8cd34                 ; <+328> at jsmath.cpp:988
    0x1c8ccfa <+270>: movs   r1, #0x2
    0x1c8ccfc <+272>: vstr   d8, [r6]
    0x1c8cd00 <+276>: str    r1, [r0]
    0x1c8cd02 <+278>: vstr   d16, [r6, #12]
    0x1c8cd06 <+282>: b      0x1c8cd34                 ; <+328> at jsmath.cpp:988
    0x1c8cd08 <+284>: vldr   d16, [r6, #12]
    0x1c8cd0c <+288>: cmp    r3, #0x0
    0x1c8cd0e <+290>: vstr   d16, [r8]
    0x1c8cd12 <+294>: bne    0x1c8cd34                 ; <+328> at jsmath.cpp:988
    0x1c8cd14 <+296>: vldr   d16, [r5]
    0x1c8cd18 <+300>: add.w  r0, r5, #0x8
    0x1c8cd1c <+304>: vcmpe.f64 d16, d8
    0x1c8cd20 <+308>: vmrs   APSR_nzcv, fpscr
    0x1c8cd24 <+312>: itt    eq
    0x1c8cd26 <+314>: ldreq  r3, [r0]
    0x1c8cd28 <+316>: cmpeq  r3, #0x1
    0x1c8cd2a <+318>: bne    0x1c8cd5a                 ; <+366> at jsmath.h:80
    0x1c8cd2c <+320>: vldr   d16, [r5, #12]
    0x1c8cd30 <+324>: vstr   d16, [r10]
    0x1c8cd34 <+328>: add    sp, #0x10
    0x1c8cd36 <+330>: vpop   {d8}
    0x1c8cd3a <+334>: pop.w  {r8, r10}
    0x1c8cd3e <+338>: pop    {r4, r5, r6, r7, pc}
    0x1c8cd40 <+340>: movs   r3, #0x2
    0x1c8cd42 <+342>: vstr   d8, [r6]
    0x1c8cd46 <+346>: str    r3, [r0]
    0x1c8cd48 <+348>: mov    r0, r1
    0x1c8cd4a <+350>: mov    r1, r2
    0x1c8cd4c <+352>: blx    0x1ff28b8                 ; symbol stub for: cos
    0x1c8cd50 <+356>: vmov   d16, r0, r1
    0x1c8cd54 <+360>: vstr   d16, [r6, #12]
    0x1c8cd58 <+364>: b      0x1c8cc9a                 ; <+174> at jsmath.cpp:987
    0x1c8cd5a <+366>: movs   r3, #0x1
    0x1c8cd5c <+368>: vstr   d8, [r5]
    0x1c8cd60 <+372>: str    r3, [r0]
    0x1c8cd62 <+374>: mov    r0, r1
    0x1c8cd64 <+376>: mov    r1, r2
    0x1c8cd66 <+378>: blx    0x1ff3428                 ; symbol stub for: sin
    0x1c8cd6a <+382>: vmov   d16, r0, r1
    0x1c8cd6e <+386>: vstr   d16, [r5, #12]
    0x1c8cd72 <+390>: vstr   d16, [r10]
    0x1c8cd76 <+394>: b      0x1c8cd34                 ; <+328> at jsmath.cpp:988

And the contents of the registers:

General Purpose Registers:
        r0 = 0x13bfd5e8
        r1 = 0x00000000
        r2 = 0x401b86d0
        r3 = 0x00000000
        r4 = 0x00000208
        r5 = 0x13bfd5e0
        r6 = 0x13bfeb20
        r7 = 0x401b86c0
        r8 = 0x00000000
        r9 = 0x00000000
       r10 = 0x002bde03  GeckoKit`mozilla::net::CacheStorageService::RecordMemoryOnlyEntry(mozilla::net::CacheEntry*, bool, bool) + 627 [inlined] mozilla::detail::log_test(PRLogModuleInfo const*, mozilla::LogLevel) + 5 at CacheStorageService.cpp:1050
  GeckoKit`mozilla::net::CacheStorageService::RecordMemoryOnlyEntry(mozilla::net::CacheEntry*, bool, bool) + 622 at CacheStorageService.cpp:1050
       r11 = 0x401b8788
       r12 = 0x02dcdb18  (void *)0x3859c3f9: __sincos_stret + 1
        sp = 0x401b8694
        lr = 0x01c8cca7  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 187 [inlined] __sincos + 5 at jsmath.cpp:962
  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 182 [inlined] js::math_sincos_uncached(double, double*, double*) at jsmath.cpp:977
  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 182 at jsmath.cpp:977
        pc = 0x01c8ccae  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 194 [inlined] __sincos at jsmath.cpp:962
  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 194 [inlined] js::math_sincos_uncached(double, double*, double*) at jsmath.cpp:977
  GeckoKit`js::math_sincos_impl(js::MathCache*, double, double*, double*) + 194 at jsmath.cpp:977
      cpsr = 0x80000030

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hmmm, lldb thinks that address is some code in CacheStorageService? Is this just some garbage pointer?

Comment 10

[Victor Carlquist]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> Hmmm, lldb thinks that address is some code in CacheStorageService? Is this
> just some garbage pointer?

I don't know... 
I enabled the sincos on all platforms and built on my android (ARM) phone, but it doesn't crash, so probably, it only fails on iOS (or A8).

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: disable use of sincos in on iOS, NPOTB

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

The 'sin' pointer does appear to be total garbage somehow. We do have some function calling changes for iOS, so maybe something is wrong there. The attached patch works around it for now, however.

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8679681 [details] [diff] [review]
disable use of sincos in on iOS, NPOTB

Review of attachment 8679681 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, if we don't have a better alternatives, or way to test, we should probably disable the feature in the mean time.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> Comment on attachment 8679681 [details] [diff] [review]
> disable use of sincos in on iOS, NPOTB
> 
> Review of attachment 8679681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, if we don't have a better alternatives, or way to test, we
> should probably disable the feature in the mean time.

Thanks. Yeah, it looks like we only enable this on darwin right now, which only enabled it on iOS by accident anyway.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e868fb4f30

Comment 16

[Victor Carlquist]

Comment on attachment 8679681 [details] [diff] [review]
disable use of sincos in on iOS, NPOTB

Thanks James and Nicolas (:
Anyway, we have a problem somewhere in iOS.
The problem *seems* to be on ABI Call, I'll try investigate it.

Thanks!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6e868fb4f30

Comment 18

[Victor Carlquist]

All seems right, except the stack alignment.

The ARM alignment stack is 8-bytes [1], but the iOS requires 4-bytes [2].

Is it a problem Nicolas?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.h#187
[2] https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Victor Carlquist from comment #18)
> All seems right, except the stack alignment.
> 
> The ARM alignment stack is 8-bytes [1], but the iOS requires 4-bytes [2].
> 
> Is it a problem Nicolas?

No, this is not a problem for old iPhone, but the problem here is not for ARMv6 / ARMv7.

The ABI convention for ARMv8 (ARM64) is different than the default one suggested by ARM:

https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW1

Comment 20

[Victor Carlquist]

Thanks,
but the Ion is disabled on ARM64, right?

https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.h#157

Comment 21

[Nicolas B. Pierron [:nbp]]

This means that we have to rewrite ABIArgs::next [1], to account for the fact that Apple operating system probably have tons of booleans given as argument of functions.

You can also notice that based on the way the function decodes its arguments (this is the code of the hash function which hash the content of the double):

    0x1c8cbfa <+14>:  eor.w  r6, r2, r1

Also, there might be something wrong in the way we give arguments to the functions, as the double value 6.8816537873078332 is equal to  0x401b86d0 0x401b86d8, which feels a lot like 2 successive pointer to the stack. (sp = 0x401b8694)

I wonder if the function in comment 8 is really ARM64 architecture, because this calling convention and the naming/number of register definitely looks like ARMv7 with soft-float.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm64/Assembler-arm64.cpp#32

Comment 22

[Nicolas B. Pierron [:nbp]]

James, are you sure this code is compiled for ARMv8 (ARM64), because the assembly in comment 8 looks like ARMv7 from my point of view.

Comment 23

[Lars T Hansen [:lth]]

On an entirely pedantic note, "ARMv8" does not imply 64-bit.  ARMv8 has a 32-bit aspect known as AArch32, this is compatible with ARMv7 but has a few new instructions.  ARMv8 also has a 64-bit aspect known as AArch64, this is completely new.  Please use the AArch32 / AArch64 designators (or ARM32 / ARM64) rather than "ARMv8", which does not actually say anything about word size.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

The crash occurs when building armv7. I haven't tried 64bit yet, as my test device doesn't support it.

Comment 25

[Victor Carlquist]

Attachment: Patch

Hi,
I think that I have a good guess about what is happening here.

As the iOS has a different stack alignment, when we are reserving space in stack to pass the sin and cos variables by reference, probably, the esp register is not aligned, so we are passing two pointer unaligned to the math_sincos_impl function.

What do you think?

I can't test this patch because I don't have an iphone.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can test this when I get home.

Comment 27

[Victor Carlquist]

Thanks James!

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8689863 [details] [diff] [review]
Patch

Review of attachment 8689863 [details] [diff] [review]:
-----------------------------------------------------------------

The frame depth is statically known, as well as the JitFrameAlignment which is in general larger or equal to the ABIStackAlignment.  Thus if there is any need for an offset, then this can be handled statically.

I suggest you read more the details explained in comment 21. If you look at (comment 8),

    frame #2: 0x01c8ccae GeckoKit`js::math_sincos_impl(mathCache=<unavailable>, x=6.8816537873078332, sin=0x002bde03, cos=0x00000000) + 194 at jsmath.cpp:977

You will notice that the arguments locations expected by the debugger does not match the argument location provided by IonMonkey.

Thus, the problem is part of the ABIArgGenerator [1], the hard-float ABI maps it to (r0, d0, r1, r2) while the ABI used on iOS (based on comment 8) expects (r0, (r1, r2), r3, (sp+?)), which is not the soft-float ABI either.

Sounds like we would need a iOS ABI.  A work-around would be to change the function signature such that the ABI are both giving the same allocations.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#47,92

Comment 29

[Victor Carlquist]

Thanks for the nice review again!

As the iOS does not seem to support hard-float as argument[1][2], 
maybe we could disable the float-**** iOS or we could change the signature of the math_sincos_impl to
(Mathchache *mathcache, double *sin, double *cos, double x) -> (r0, r1, r2, sp+?). 

Should we disable the float-**** iOS?

[1] "For floating-point arguments, the Base Standard variant of the Procedure Call Standard is used. In this variant, floating-point (and vector) arguments are passed in general purpose registers (GPRs) instead of in VFP registers)"


[2]https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Victor Carlquist from comment #29)
> As the iOS does not seem to support hard-float as argument[1][2], 
> maybe we could disable the float-**** iOS or we could change the signature
> of the math_sincos_impl to
> (Mathchache *mathcache, double *sin, double *cos, double x) -> (r0, r1, r2,
> sp+?). 

I fear that it will be compiled to (r0, r1, r2, (r3, sp+?))
This looks likes the soft-float implementation without the alignment constraints on even registers for double values.

> Should we disable the float-**** iOS?

What surprises me is that we had no issue so far with functions starting with a double as a first argument, but this would only make sense if we were compiling these function with a soft-float ABI, which would use even aligned register couples for double.

So maybe the following would work well:
  (double x, Mathchache *mathcache, double *sin, double *cos) -> ((r0, 1), r2, r3, sp+?)

but this implies that we are already using soft-float, which contradicts what I explained in comment 28.

Comment 31

[Victor Carlquist]

(In reply to Nicolas B. Pierron [:nbp] from comment #30)
> What surprises me is that we had no issue so far with functions starting
> with a double as a first argument, but this would only make sense if we were
> compiling these function with a soft-float ABI, which would use even aligned
> register couples for double.

I think we should ask James if we are using hard-float or soft-float [1]

Because the MathFunctionD [2] is starting with (MathCache*, double) and it doesn't show any problem...  

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#141
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#5322

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

It looks like we are using -mfloat-abi=toolchain-default, so it's not clear, but I would expect that to be softvp. We should probably be explicit here like we do for Android.

Comment 33

[Victor Carlquist]

Attachment: Patch.

I changed the math_sincos_impl's signature as Nicolas commented.
 
What puzzles me is that the MathFunction isn't crashing, 
because it starts with an int argument (MathCache *) followed by a double argument.

It's pretty weird...

Comment 34

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8693374 [details] [diff] [review]
Patch.

Review of attachment 8693374 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds good to me.
Please wait for feedback before landing, as this patch enabled iOS again, or make a separated patch to enable iOS.

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8693374 [details] [diff] [review]
Patch.

Review of attachment 8693374 [details] [diff] [review]:
-----------------------------------------------------------------

I never got a chance try this, but will if I ever get back to iOS stuff.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 37

[Steven DeTar [:sdetar]]

Nicolas, what should we do with this bug?  Close? Remove leave-open flag?

Comment 38

[Nicolas B. Pierron [:nbp]]

As far as I know, except for James' builds, we have no support for iOS devices at the moment.

The patch landed in comment 17 disabled the specific optimization on iOS, and therefore should fix this issue at the cost of slower performances.  While the rest of the comments since seems to indicate potential issues for other malformed calls on iOS, we have no way to reproduce this issue.

Until we get feedback from James, I think we should just close this issue for the lack of reproducible path.
(James feel free to clear this need-info if you do not intent to work on it)

Comment 39

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, nothing is happening on iOS at the moment. Closing seems fine.