1330667 - Assertion failure: opBlock->dominates(*block) (Instruction is not dominated by its operands), at js/src/jit/IonAnalysis.cpp:3050

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 97d6f7364394 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --ion-check-range-analysis):

function printStatus(msg) {}
function reportCompare(expected, actual) {
    if (expected != actual) 
      var testcase = new TestCase(expected, actual);
    return true;
}
printStatus(366396 < printStatus.length ? (yield) : (yield) = (null));
function h(k) {
    for (0; reportCompare(0, 0);
        (yield)) {
        var a = arguments;
    }
}
h(0);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff60fe700 (LWP 9597)]
js::jit::AssertExtendedGraphCoherency (graph=..., underValueNumberer=false) at js/src/jit/IonAnalysis.cpp:3049
#0  js::jit::AssertExtendedGraphCoherency (graph=..., underValueNumberer=false) at js/src/jit/IonAnalysis.cpp:3049
#1  0x000000000065a5ac in js::jit::OptimizeMIR (mir=mir@entry=0x7ffff69af278) at js/src/jit/Ion.cpp:1652
#2  0x000000000065bcea in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff69af278) at js/src/jit/Ion.cpp:2067
#3  0x0000000000b1470e in js::HelperThread::handleIonWorkload (this=this@entry=0x7ffff6947518, locked=...) at js/src/vm/HelperThreads.cpp:1511
#4  0x0000000000b15385 in js::HelperThread::threadLoop (this=this@entry=0x7ffff6947518) at js/src/vm/HelperThreads.cpp:1874
#5  0x0000000000b156c5 in js::HelperThread::ThreadMain (arg=0x7ffff6947518) at js/src/vm/HelperThreads.cpp:1407
#6  0x0000000000b1fe82 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul> (this=0x7ffff6920070) at js/src/threading/Thread.h:234
#7  js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0x7ffff6920070) at js/src/threading/Thread.h:227
#8  0x00007ffff7bc16fa in start_thread (arg=0x7ffff60fe700) at pthread_create.c:333
#9  0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x2061520	33953056
rbx	0x7ffff69b54b0	140737330762928
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x11f8b18	18844440
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7ffff60fd980	140737321621888
rsp	0x7ffff60fd920	140737321621792
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff60fe700	140737321625344
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff69b6ec0	140737330769600
r13	0x7ffff69af050	140737330737232
r14	0x0	0
r15	0x7ffff69b6e70	140737330769520
rip	0x61ce69 <js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&, bool)+2713>
=> 0x61ce69 <js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&, bool)+2713>:	movl   $0x0,0x0
   0x61ce74 <js::jit::AssertExtendedGraphCoherency(js::jit::MIRGraph&, bool)+2724>:	ud2

Comment 1

[Fuzzing Team]

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/c98510adc780
user:        Sean Stangl
date:        Mon Jan 09 23:10:00 2017 -0500
summary:     Bug 1329901 - Remove expensive isObservableOperand() loop guards. r=nbp

This iteration took 278.823 seconds to run.

Comment 2

[Sean Stangl [:sstangl]]

Taking a brief look, we seem to be generating invalid SSA. An operand that only shows up (invalidly) in MResumePoints seems to have made its way into a piece of code without a phi to handle control flow at an OSR point.

Weirdly, this strange graph seems to pass AssertExtendedGraphCoherency() even though it's clearly incorrect, although we later fail when inserting a nonsense MBox based on that incorrect value.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: mirgraph.png

Nicolas, this is the graph before the TypeAnalyzer runs. It looks like phi35's reference to constant10 is invalid. ResumePoints are the only things holding a reference to constant10, but I don't know much about them. Have you seen anything like this before?

Comment 4

[Sean Stangl [:sstangl]]

Ah, never mind: the graph is incorrect when it comes out of IonBuilder. Both operands of a phi refer to the same predecessory block. It looks like the patch in Bug 1329901 just uncovered some pre-existing bad behavior.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

Using constant from the first basic blocks used to be a hack to we used before the addition of AssertGraphCoherency, such as shown by the first operand of the entry resume point of block 2 (constant 10).

Comment 6

[Sean Stangl [:sstangl]]

After spending time trying to write an assertion to catch this case, I realized that we already *did* have this assertion, but that it was specifically ignored for the case of MagicOptimizedArguments.

https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonAnalysis.cpp#3035

This is the code that removes the SSA-form assertion for this kind of phi node:

> // We sometimes see a phi with a magic-optimized-arguments
> // operand defined in the normal entry block, while the phi is
> // also reachable from the OSR entry (auto-regress/bug779818.js)
> if (phi->getOperand(i)->type() == MIRType::MagicOptimizedArguments)
>     continue;

That code was added by Bug 1004363, the first patch that asserted the consistency of the dominator tree.

Hannes, is there a reason I don't know why the IonBuilder would intentionally generate SSA-invalid phis for MagicOptimizedArguments? Otherwise it looks like we should just fix it and remove the assertion dodge.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

This is the least ugly code I found. The issue is that we expect to not throw MagicValue(JS_OPTIMIZED_ARGUMENTS) around. In most cases it is deleted also. Though not always. This is such a case. Which still is not a problem. Though it triggers that it doesn't dominate the whole graph. (We have an osr block in between). To solve it I just add a new MConstant in the preheader (if we have osr). That way whatever construct we make the constant always dominates the uses.

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

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

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

::: js/src/jit/IonBuilder.cpp
@@ +2326,5 @@
>      loopDepth_ = header->loopDepth();
>  
> +    if (lazyArguments_ && lazyArguments_->isDiscarded()) {
> +        MOZ_ASSERT(!lazyArgumentsBeforeOsr_->isDiscarded());
> +        lazyArguments_ = lazyArgumentsBeforeOsr_;

If we were to restart a loop which does not have the OSR header, then we would take the lazyArgument coming from the main-entry block, and not the one from the pre-header.

@@ +6326,5 @@
>  
> +    // Make sure we have a lazyArguments that dominates the next instructions.
> +    if (lazyArguments_) {
> +        lazyArguments_ = MConstant::New(alloc(), MagicValue(JS_OPTIMIZED_ARGUMENTS));
> +        preheader->add(lazyArguments_);

Having a new constant at the OSR pre-header is not enough, because we could OSR in an inner loop.
Thus any "arguments" keyword will use the constant from the main-entry instead of the constant of the phi from the main-entry and the backedge of the outer loop.

Comment 9

[Jan de Mooij [:jandem]]

Why do we need lazyArguments_ exactly? If it's just to avoid creating MConstants I don't think it's worth spending complexity/time on that. If it's okay to push a fresh constant for JSOP_ZERO it should definitely be fine for lazy args.

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Patch

Like requested.

Comment 11

[Jan de Mooij [:jandem]]

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

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

Glad it works! Sorry for the delay.

::: js/src/jit/IonBuilder.cpp
@@ +157,5 @@
>      failedBoundsCheck_(info->script()->failedBoundsCheck()),
>      failedShapeGuard_(info->script()->failedShapeGuard()),
>      failedLexicalCheck_(info->script()->failedLexicalCheck()),
>      nonStringIteration_(false),
> +    lazyArguments_(false),

Nit: this should be in #ifdef DEBUG, also a few times below. Maybe rename to hasLazyArguments_?

@@ +9267,5 @@
>          current->push(current->argumentsObject());
>          return Ok();
>      }
> +
> +    printf("|\n");

Nit: remove

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Will this land soon?  Does this affect 52?  Thanks

Comment 13

[Hannes Verschore [:h4writer]]

This is FF53+. I'll look to land it.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41cb71937e92
IonMonkey - Create a new constant for every optimized arguments use, r=jandem

Comment 15

[Hannes Verschore [:h4writer]]

[Tracking Requested - why for this release]:
Fix a potential crash?

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41cb71937e92

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 18

[Marcia Knous [:marcia]]

Tracking 53/54+ for this regression.

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 20

[Hannes Verschore [:h4writer]]

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

Approval Request Comment

[Feature/Bug causing the regression]:
bug 1329901

[User impact if declined]:
In the easy case it seems like everything is going correctly. There might be harder cases where this is causing crashes, but should be low-volume. Another issue might be that fuzzers on aurora might not be able to find other issues because hitting these asserts.
Given the low risk and the potential to cause crashes I think this would be good to backport.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
/

[Is the change risky?]:
[Why is the change risky/not risky?]:
No, It creates a new constant instead of reusing the same one. This means in this case mostly the same thing. Except we work around the assert.

[String changes made/needed]:
/

Comment 21

[Gerry Chang [:gchang]]

Hi :decoder,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 22

[Christian Holler (:decoder)]

According to comment 20 the fix has already been verified on nightly.

Comment 23

[Gerry Chang [:gchang]]

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

Fix an assertion failure and was verified. Aurora53+.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/94debbbf7e5a