1518565 - ARM64 IonMonkey: Assertion failure: hasIonScript(), at js/src/vm/JSScript.h:2223

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

The following issue appears much more frequently on Softiron ARM64 devices than seen on the ARM64 simulator, and might be intermittent.

Thread 1 "js-bugzil.la-ar" received signal SIGSEGV, Segmentation fault.
0x0000aaaaab5414a8 in AnnotateMozCrashReason (reason=0xaaaaabe9b512 "MOZ_RELEASE_ASSERT(hasIonScript())") at /home/nicolas/mozilla/_build/js/bugzil.la/arm64/wip/arm64/clang/pro/dist/include/mozilla/Assertions.h:38
38        gMozCrashReason = reason;
(gdb) bt
#0  0x0000aaaaab5414a8 in AnnotateMozCrashReason (reason=0xaaaaabe9b512 "MOZ_RELEASE_ASSERT(hasIonScript())") at /home/nicolas/mozilla/_build/js/bugzil.la/arm64/wip/arm64/clang/pro/dist/include/mozilla/Assertions.h:38
#1  js::jit::GetCalleeTokenTag (token=<optimized out>) at /home/nicolas/mozilla/wksp-0/js/src/jit/JitFrames.h:31
#2  js::jit::ScriptFromCalleeToken (token=<optimized out>) at /home/nicolas/mozilla/wksp-0/js/src/jit/JitFrames.h:63
#3  js::jit::BailoutFrameInfo::BailoutFrameInfo (this=<optimized out>, activations=..., bailout=0xfffffffe0aa0) at /home/nicolas/mozilla/wksp-0/js/src/jit/arm64/Bailouts-arm64.cpp:43
#4  0x0000aaaaaba78bcc in js::jit::Bailout (sp=0xfffffffe0aa0, bailoutInfo=0xfffffffe0a98) at /home/nicolas/mozilla/wksp-0/js/src/jit/Bailouts.cpp:41
#5  0x00000e832ffaf494 in ?? ()

Comment 1

[Sean Stangl [:sstangl]]

The simulator is intended to simulate the ICache... that's really the obvious culprit, but you'd think that Baseline would crash too if that were the problem.

Comment 2

[Nicolas B. Pierron [:nbp]]

Ok, it took me some time to figure out the issue, and my latest understanding is that we do not trash the instruction cache after patching it during an Invalidation. This causes us to enter a Bailout (or worse) instead of the InvalidationBailout.

The bailout code path expect the IonScript to still be present on the JSScript and to be found from the calleeToken, whereas after an invalidation, we lookup the IonScript differently.

I will work on a patch an attach it here after double checking if it works well on ARM64 hardware.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Flush the instruction cache when patching OSIPoints.

This patch add AutoFlushICache::flush call to Assembler::PatchWrite_NearCall,
and also adds extra release assertions for the encoding of the 26 bits
immediate.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 9035921 [details] [diff] [review]
Flush the instruction cache when patching OSIPoints.

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

Awesome.

MacroAssembler::patchCall() also looks like it's missing ICache flushing after patching a BL instruction; might as well fix that here too.

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +387,5 @@
> +                                    CodeLocationLabel toCall) {
> +  Instruction* dest = (Instruction*)start.raw();
> +  ptrdiff_t relTarget = (Instruction*)toCall.raw() - dest;
> +  ptrdiff_t relTarget00 = relTarget >> 2;
> +  MOZ_RELEASE_ASSERT((relTarget & 0x3) == 0);

Why is this true? We `>>2` for every BL, but I don't know why.

@@ +389,5 @@
> +  ptrdiff_t relTarget = (Instruction*)toCall.raw() - dest;
> +  ptrdiff_t relTarget00 = relTarget >> 2;
> +  MOZ_RELEASE_ASSERT((relTarget & 0x3) == 0);
> +  ImmNTest test;
> +  test.imm26 = relTarget00;

VIXL Utils (js/src/jit/arm64/vixl/Utils.h) has a helper function `is_int26(int64_t)` that could be used instead of using the struct. That's the same function that `bl()` uses internally (via `ImmUncondBranch`).

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Sean Stangl [:sstangl] from comment #4)

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +387,5 @@

•                                CodeLocationLabel toCall) {
  

• Instruction* dest = (Instruction*)start.raw();
• ptrdiff_t relTarget = (Instruction*)toCall.raw() - dest;
• ptrdiff_t relTarget00 = relTarget >> 2;
• MOZ_RELEASE_ASSERT((relTarget & 0x3) == 0);

Why is this true? We >>2 for every BL, but I don't know why.

The ARM64 documentation explicit that the branch target is the imm26 immediate to which it appends '00' bits. (see C5.6.20 in the Aarch64 reference manual)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c3af3df20e
Flush the instruction cache when patching OSIPoints. r=sstangl

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/68c3af3df20e