1529559 - Crash [@ js::jit::PatchJump] or Crash [@ ??] or Hit MOZ_CRASH(PatchJump target not reachable) at jit/arm64/Assembler-arm64.cpp:377

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision bf3951daded0+ (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-jemalloc --disable-debug, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --ion-warmup-threshold=1):

See attachment.

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  js::jit::PatchJump (jump_=..., label=...) at js/src/jit/arm64/Assembler-arm64.cpp:377
#1  0x0000aaaafcc0264c in js::jit::IonCacheIRCompiler::compile (this=0xffffe6f37de0) at js/src/jit/IonCacheIRCompiler.cpp:632
#2  0x0000aaaafcc0c228 in js::jit::IonIC::attachCacheIRStub (this=0xaaab1fc8f3c0, cx=<optimized out>, writer=..., kind=<optimized out>, ionScript=<optimized out>, attached=0xffffe6f391c4, typeCheckInfo=0x0) at js/src/jit/IonCacheIRCompiler.cpp:2570
#3  0x0000aaaafcc1a8b8 in js::jit::IonGetPropertyIC::update (cx=0xaaab1faa6ab0, outerScript=..., ic=<optimized out>, val=..., idVal=..., res=...) at js/src/jit/IonIC.cpp:149
#4  0x00003e14b9117540 in ?? ()
#5  0x0000ffffe6f396e0 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
x0	0x0	0
x1	0xb91587c8	68258725464008
x2	0x0	0
x3	0xc38b4be0	68258900954080
x4	0xe6f38329	281474556461865
x5	0xc38b4c41	68258900954177
x6	0xc8	200
x7	0xc8	200
x8	0x0	0
x9	0xfe05b440	187651382948928
x10	0xfd1b8ce0	187651367603424
x11	0x179	377
x12	0x0	0
x13	0xe6f38118	281474556461336
x14	0x0	0
x15	0x400	1024
x16	0xfd4f0358	187651370976088
x17	0xe74790	281462106965904
x18	0x1fff	8191
x19	0xb91587c8	68258725464008
x20	0xc38b4bd4	68258900954068
x21	0x1345a20	281462112016928
x22	0xe6f38ea8	281474556464808
x23	0xfd1c0aec	187651367635692
x24	0x14c	332
x25	0x1	274877906945
x26	0x1	1
x27	0x1345a20	281462112016928
x28	0xe6f39290	281474556465808
x29	0xe6f37cd0	281474556460240
x30	0xfcafa154	187651360530772
sp	0xe6f37cc0	281474556460224
pc	0xaaaafcafa188 <js::jit::PatchJump(js::jit::CodeLocationJump&, js::jit::CodeLocationLabel)+92>
cpsr	[ EL=0 C N ]
fpcsr	void
fpcr	0x0	0
=> 0xaaaafcafa188 <js::jit::PatchJump(js::jit::CodeLocationJump&, js::jit::CodeLocationLabel)+92>:	str	w11, [x8]
   0xaaaafcafa18c <js::jit::PatchJump(js::jit::CodeLocationJump&, js::jit::CodeLocationLabel)+96>:	bl	0xaaaafc5c3608 <abort()>

This requires native ARM64 and the patch from bug 1528869 to enable ARM64 Ion in the JS shell. Unfortunately, reducing the test makes it highly intermittent. Sean, if you could figure out where the intermittent behavior is coming from when fixing this, that would be a great bonus.

Marking s-s because this will likely be security-sensitive once we enable the ARM64 JIT.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Nicolas B. Pierron [:nbp]]

Thanks, this happens in the wild and this test case would definitely be helpful.
Either me or Sean will take in soon.

Comment 3

[Daniel Veditz [:dveditz]]

Could be two different crashes here: this signature spikes in 65.0.1 release on 32-bit ARM with wildptr-looking crashes. There's also a new Nightly ARM64-only null-deref crash

Comment 4

[Daniel Veditz [:dveditz]]

bug 1528621 seems to be a dupe of this?

Comment 5

[Christian Holler (:decoder)]

(In reply to Daniel Veditz [:dveditz] from comment #3)

Could be two different crashes here: this signature spikes in 65.0.1 release on 32-bit ARM with wildptr-looking crashes. There's also a new Nightly ARM64-only null-deref crash

I found this bug only with ARM64 Ion enabled, so I guess it can't be either of these two.

Comment 6

[Nicolas B. Pierron [:nbp]]

I let my laptop run this under the simulator with rr, and so far not luck over night.

Comment 7

[Christian Holler (:decoder)]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)

I let my laptop run this under the simulator with rr, and so far not luck over night.

This reproduces reliably for me (first try) on two different machines.

One is a Cortex A72 (what we have on AWS):

processor	: 0
BogoMIPS	: 166.66
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x0
CPU part	: 0xd08
CPU revision	: 3

The other is a Cortex A57:

processor       : 0
BogoMIPS        : 500.00                                                         
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x1
CPU part        : 0xd07
CPU revision    : 2

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Looks to be a null-ptr-deref store, so I'm opening it up.

Comment 9

[Nicolas B. Pierron [:nbp]]

For what I worth toogling IonMonkey on in about:config on Windows ARM64 makes gmail extremely crashy.
I will give it more chance on hardware, and otherwise try to fix it blindly or instrument the Windows ARM64 build to get more information.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Christian Holler (:decoder) from comment #7)

This reproduces reliably for me (first try) on two different machines.

The other is a Cortex A57:

processor       : 0
BogoMIPS        : 500.00                                                         
Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
CPU implementer : 0x41
CPU architecture: 8
CPU variant     : 0x1
CPU part        : 0xd07
CPU revision    : 2

I tested on the SoftIron device, which has exactly the same CPU, and I was still unable to reproduce this issue even after letting the loop run over the week-end.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 9050727 [details]
Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

Decoder as you were able to reproduce this issue quite reliably when building, can you try this patch and check if this crashes in a different manner?

In the mean time I will look on how to get a try build running with this patch to see if I can reproduce this issue on Windows ARM64, where I can reliably reproduce it.

Comment 13

[Nicolas B. Pierron [:nbp]]

I was not able to reproduce this crash on gmail.com with the Try build which include this fix:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=222419738&revision=a3a112c5e66f2d799fb0d9eea2b3a7ea9818616e

Thus, I am going to request a review now.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 9050727 [details]
Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

I don't think I'll be able to get to this anytime soon, unfortunately.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d77a06489c3e
ARM64: PatchJump change the branching schema if the target is out of range. r=sstangl

Comment 16

[Oana Pop-Rus]

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

Comment 17

[Christian Holler (:decoder)]

Comment on attachment 9050727 [details]
Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

I can confirm that this is fixed (doesn't reproduce anymore on newest builds) and it also stopped popping up in fuzzing while it was seen there with reliable frequency before.

Comment 18

[Sean Stangl [:sstangl]]

[Tracking Requested - why for this release]: We've been asked to enable Ion ARM64 in-browser for 67. Previously, we labeled the 67 tracking for this bug as "Disabled," since the feature was disabled, but now that we are going to enable it retroactively, this bug is in-scope.

Comment 19

[Sean Stangl [:sstangl]]

Comment on attachment 9050727 [details]
Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1536220
• User impact if declined: We'll land 1536220 on Beta, and then users will crash a lot.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It implements a path that was left as a FIXME, which finally got implemented in 68.
• String changes made/needed:

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9050727 [details]
Bug 1529559 - ARM64: PatchJump change the branching schema if the target is out of range.

arm64 crash fix, uplift accepted for 67 beta 8, thanks.

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4c5f31c23ab