1314172 - Assertion failure: usesBalance <= 0 (More use checks than operand checks), at js/src/jit/IonAnalysis.cpp:2755

Status: RESOLVED FIXED

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

Description

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

The following testcase crashes on mozilla-central revision 37ab1d54a08e (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

oomTest(function() {
    Array(1[new Uint8Array()])
})

Backtrace:

0   js-dbg-64-dm-clang-darwin-37ab1d54a08e	0x000000010df664a6 js::jit::AssertBasicGraphCoherency(js::jit::MIRGraph&) + 5510 (IonAnalysis.cpp:2755)
1   js-dbg-64-dm-clang-darwin-37ab1d54a08e	0x000000010df61de6 js::jit::OptimizeMIR(js::jit::MIRGenerator*) + 230 (Ion.cpp:1513)
2   js-dbg-64-dm-clang-darwin-37ab1d54a08e	0x000000010df6ee3a js::jit::CompileBackEnd(js::jit::MIRGenerator*) + 74 (Ion.cpp:2021)
3   js-dbg-64-dm-clang-darwin-37ab1d54a08e	0x000000010df70c9a js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool) + 3834 (Ion.cpp:2302)
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: OOM_VERBOSE=1 stack from m-c rev 37ab1d54a08e

Comment 3

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

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160830082723" and the hash "1a6361b000fcb97f941e4091001e88be0e46927f".
The "bad" changeset has the timestamp "20160830085122" and the hash "633c05b48792f4d55a13e43ad12034a53006797d".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1a6361b000fcb97f941e4091001e88be0e46927f&tochange=633c05b48792f4d55a13e43ad12034a53006797d

Andre/Waldo, is bug 1121938 a likely regressor?

Comment 4

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

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20161028054458" and the hash "36deeea122f9f5898cfb8360cd7012e3d4a3fe03".
The "bad" changeset has the timestamp "20161028054558" and the hash "726ec9c83018397c0617eb25f71dde6e383b5ec3".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=36deeea122f9f5898cfb8360cd7012e3d4a3fe03&tochange=726ec9c83018397c0617eb25f71dde6e383b5ec3

Oops, probably the wrong bug for comment 3.

Nicolas, is bug 1303399 a likely regressor?

Comment 5

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/47e4fb57325d
user:        Nicolas B. Pierron
date:        Fri Oct 28 12:45:31 2016 +0000
summary:     Bug 1303399 part 2 - IonMonkey: Fallback when we fail to inline an uninlinable function. r=h4writer

Comment 6

[Nicolas B. Pierron [:nbp]]

Yes, this error was never seen before and sounds like something that could have been introduced by an incomplete clean-up.  I will look at it tomorrow.

Comment 7

[Nicolas B. Pierron [:nbp]]

This issue arise because IonBuilder fails to properly reports AbortReason_Alloc, and by default reports them as AbortReason_NoAbort.  This should be fixed by Bug 1286505, as mentioned in Bug 1303399 comment 9.

Comment 8

[Hannes Verschore [:h4writer]]

Merge has happened. Moving the bugs from P2 to P1. Bug 1286505 is also P1.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: WIP

This patch fixes the first issue, but apparently, this just push the issue
to another allocation nearby, which causes the same failure.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: WIP

Fixes both issues, all of the same kind which is that we are trying to
recover from an allocation failure, which leave dangling uses.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Add an option to toggle backtracking on inlining failures.

This re-add the AbortReason_Inlining previously removed by Bug 1303399, and
add a JitOption flag to toggle the backupPoint.restore().

This patch disables it by default in order to prevent fuzzers/users from
hitting this issue while Bug 1286505 is not fixed.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8813715 [details] [diff] [review]
Add an option to toggle backtracking on inlining failures.

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

Better approach than just backing out! Thanks!

::: js/src/jit/IonBuilder.cpp
@@ +5267,2 @@
>              current = backup.restore();
>              return InliningStatus_NotInlined;

Might be easier to reverse it. That way you don't have to put "return InliningStatus_Error";

if (!JitOptions.disableInlineBacktracking) {
    current = backup.restore();
    return InliningStatus_NotInlined;
}

abortReason_ = AbortReason_Inlining;

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cbfe9c9060
Add an option to toggle backtracking on inlining failures. r=h4writer

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08cbfe9c9060

Comment 15

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

Nicolas, it would be nice to backport this for 52 please (future ESR).

Comment 16

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

Also it is showing quite a bit on 52 as current Aurora branch.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8813715 [details] [diff] [review]
Add an option to toggle backtracking on inlining failures.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1303399 (not safe until Bug 1286505 is resolved)
[User impact if declined]: Bad reporting of OOM => bad crash addresses.
[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]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This patch add a flag to disable the feature added in Bug 1303399.
[String changes made/needed]: N/A

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8813715 [details] [diff] [review]
Add an option to toggle backtracking on inlining failures.

JS jit fix for aurora52

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d11354d4580

Comment 21

[Jan de Mooij [:jandem]]

Nicolas, is there anything blocking us still from enabling this option? I noticed some Inlining aborts on Speedometer.

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #21)
> Nicolas, is there anything blocking us still from enabling this option? I
> noticed some Inlining aborts on Speedometer.

No, all should have been fixed by Bug 1286505.
We should no longer have any blocker, I will keep this needinfo to recall to enable & test it later this week.

Comment 23

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #21)
> Nicolas, is there anything blocking us still from enabling this option? I
> noticed some Inlining aborts on Speedometer.

This got enabled by default in Bug 1373323.