1108413 - Assertion failure: isLowered(), at js/src/jit/MIR.h:716 or Crash [@ js::jit::LiveInterval::addRangeAtHead]

Status: REOPENED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 035a951fc24a (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-debug, run with --fuzzing-safe --thread-count=2):

var globalinc = 0;
function testincops(n) {
    var i = 0,
        o = {
            p: 0
        },
        a = [0];
    n = 100;
    for (o.p = 0; o.p < n; o.p++) globalinc++;
    while (o.p-- > 0) valueOf ? this : this;
    for (a[i] = 0;
        'foo'; a[i] ++);
}
assertEq(testincops(), "0,0,0");


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff7f5d700 (LWP 41672)]
js::jit::LiveInterval::addRangeAtHead (this=this@entry=0x0, from=..., to=to@entry=...) at js/src/jit/LiveRangeAllocator.cpp:157
157	    if (ranges_.empty())
#0  js::jit::LiveInterval::addRangeAtHead (this=this@entry=0x0, from=..., to=to@entry=...) at js/src/jit/LiveRangeAllocator.cpp:157
#1  0x00000000006745a0 in js::jit::LiveRangeAllocator<js::jit::LinearScanVirtualRegister, true>::buildLivenessInfo (this=this@entry=0x7ffff7f5c7f0) at js/src/jit/LiveRangeAllocator.cpp:851
#2  0x00000000006597fe in js::jit::LinearScanAllocator::go (this=this@entry=0x7ffff7f5c7f0) at js/src/jit/LinearScan.cpp:1288
#3  0x0000000000659d77 in js::jit::GenerateLIR (mir=mir@entry=0x1813a38) at js/src/jit/Ion.cpp:1634
#4  0x000000000065a205 in js::jit::CompileBackEnd (mir=0x1813a38) at js/src/jit/Ion.cpp:1722
#5  0x00000000008284a1 in js::HelperThread::handleIonWorkload (this=this@entry=0x1708970) at js/src/vm/HelperThreads.cpp:1063
#6  0x0000000000828a28 in js::HelperThread::threadLoop (this=0x1708970) at js/src/vm/HelperThreads.cpp:1361
#7  0x0000000000881469 in nspr::Thread::ThreadRoutine (arg=0x1709e60) at js/src/vm/PosixNSPR.cpp:45
#8  0x00007ffff7bc4182 in start_thread (arg=0x7ffff7f5d700) at pthread_create.c:312
#9  0x00007ffff6cb3fbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
rax	0x7e	126
rbx	0x0	0
rcx	0x7fffe8002a00	140737085712896
rdx	0x81	129
rsi	0x7e	126
rdi	0x0	0
rbp	0x7e	126
rsp	0x7ffff7f5c120	140737353466144
r8	0x7fffe8007a20	140737085733408
r9	0x7f60	32608
r10	0x7fffe80089e0	140737085737440
r11	0x7ffff7f5c0e0	140737353466080
r12	0x7fffe8002b38	140737085713208
r13	0x81	129
r14	0x7ffff7f5c7f0	140737353467888
r15	0x0	0
rip	0x63f00b <js::jit::LiveInterval::addRangeAtHead(js::jit::CodePosition, js::jit::CodePosition)+11>
=> 0x63f00b <js::jit::LiveInterval::addRangeAtHead(js::jit::CodePosition, js::jit::CodePosition)+11>:	mov    0x20(%rdi),%rax
   0x63f00f <js::jit::LiveInterval::addRangeAtHead(js::jit::CodePosition, js::jit::CodePosition)+15>:	test   %rax,%rax

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9188c8b7962b
user:        Nicolas B. Pierron
date:        Mon Nov 24 16:11:32 2014 +0100
summary:     Bug 1093674 - IonMonkey: Add Sink for instruction which can be recovered on bailout. r=sunfish

This iteration took 289.141 seconds to run.

Comment 2

[Christian Holler (:decoder)]

Needinfo from nbp based on comment 1 :)

Comment 3

[Nicolas B. Pierron [:nbp]]

This should be fixed, as the Sink is now disabled on inbound & aurora (Bug 1093674)
I am keeping this bug open, as a blocker for enabling Sink in nightly.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 671726d017c2).

Comment 5

[Nicolas B. Pierron [:nbp]]

Fixing this issue is not trivial if we want to guarantee that once we have computed the actual value, then the recovered value is no longer used in dominated blocks.  Being capable ensure such property would be sane and easier to reason about, but unfortunately this is working against the approach taken for registering stores on resume points (Bug 991720).

My priority is to get Bug 1073033 (and Bug 1044202) done, which implies taking changes from Bug 991720.  So, in the mean time, the solution would not be optimal, as we would have to prevent any operand replacement on any recovered instruction which is part of the computation of the entry resume point.  This implies that the entry resume point, as well as any of its store (side-effect) instructions, and thus potentially even other resume points within the block, can capture the operands of the computed instruction.  Doing so might be bad for the register allocation within the branch in which the computation is moved into, but this might still reduce the number of instruction computed if this branch is not-taken.

The reason why this is against Bug 991720, is that the stores are registered in a stack which are not aware of its uses.  This implies that we are unable to fork the stack of store instructions (side-effects) for all dominated/reachable resume points.

Comment 6

[Nicolas B. Pierron [:nbp]]

I will leave enabling Sink algorithm with recovered-on-bailout instructions for later, there is one corner case which would be solved by having a sane approach.  The corner case is when a recovered-on-bailout instruction is by both the entry resume point and another, and that the resume point hold the executed copy of the instruction.

Doing so would be fine for any instruction which can safely be reproduced, but this would not be for instruction which have a distinguishable result after the execution, such as object allocations / Math.random.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Skip Sink optimizations if the instruction is used by other recover instructions.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8535087 [details] [diff] [review]
Skip Sink optimizations if the instruction is used by other recover instructions.

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

::: js/src/jit/Sink.cpp
@@ +108,5 @@
> +                if (consumer->isRecoveredOnBailout()) {
> +                    // if the instruction was not sunk previously, then do not
> +                    // attempt to optimize it.
> +                    hasDeadUses = !consumer->isInWorklist();
> +                    hasSinkUses = consumer->isInWorklist();

These can set hasDeadUses and hasSinkUses to false, even if they were true for a prior use, so these variables only reflect the last use with a recovered-on-bailout consumer. That doesn't look right, unless I'm misunderstanding what's going on here.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8535087 [details] [diff] [review]
Skip Sink optimizations if the instruction is used by other recover instructions.

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

::: js/src/jit/Sink.cpp
@@ +108,5 @@
> +                if (consumer->isRecoveredOnBailout()) {
> +                    // if the instruction was not sunk previously, then do not
> +                    // attempt to optimize it.
> +                    hasDeadUses = !consumer->isInWorklist();
> +                    hasSinkUses = consumer->isInWorklist();

Indeed, I fixed it locally, these are supposed to be  " |= " instead of " = ".

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8535087 [details] [diff] [review]
Skip Sink optimizations if the instruction is used by other recover instructions.

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

::: js/src/jit/Sink.cpp
@@ +108,5 @@
> +                if (consumer->isRecoveredOnBailout()) {
> +                    // if the instruction was not sunk previously, then do not
> +                    // attempt to optimize it.
> +                    hasDeadUses = !consumer->isInWorklist();
> +                    hasSinkUses = consumer->isInWorklist();

Ok, r+ with this fixed.

Comment 11

[Nicolas B. Pierron [:nbp]]

P3 based on comment 3 and comment 4.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Reopening because crash bugs **with testcases** should not be resolved **as WONTFIX** based on queries of crash-stats.  Other resolutions may be appropriate for other reasons.

(Crash signatures are not the same as bug identity; they're merely a search aid to find and group similar crashes.  The bug may still be present, but the signature may have changed slightly, or the bug may even still be present with the same signature but there are simply no recent reports of crashes in that function.)