1006301 - Assertion failure: ins->lhs()->type() == MIRType_Int32, at jit/Lowering.cpp or Assertion failure: lhs->type() == MIRType_Int32, at jit/Lowering.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

function f(x, y) {
    return (undefined >> 0) % y
}
for (var j = 0; j < 1; j++) {
    f(f(0, 1))
}

asserts js debug shell on m-c changeset 81651ad5e43c with --ion-eager --ion-parallel-compile=off at Assertion failure: ins->lhs()->type() == MIRType_Int32, at jit/Lowering.cpp

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/87feb441d562
user:        Dan Gohman
date:        Mon May 05 15:02:02 2014 -0700
summary:     Bug 998580 - IonMonkey: Generalize RangeAnalysis truncation to handle other kinds of paths to integer types. r=nbp

Dan, is bug 998580 a likely regressor?

Comment 1

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

This is a fuzzblocker for jsfunfuzz.

Comment 2

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

Attachment: stack for second assertion

for (var j = 0; j < 1; j++) {
    (function() {
        return -0 / true
    })()
}

Assertion failure: lhs->type() == MIRType_Int32, at jit/Lowering.cpp

I'm guessing this testcase is also related (run with "--ion-parallel-compile=off --ion-eager"), as autoBisect points to the same regressing changeset.

Comment 3

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

Let's lock this first because bug 1006561 is s-s (crash in the wild?) and is very likely related.

Comment 5

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

Attachment: Opt stack showing access of weird memory address

Exploitable testcase found:

function f(x) {
    return (undefined | 0) <= (null > 0) ? (x | 0) % 2147483647 | 0 : 0
}
f(-2147483647)
f(null)
f(null)

$ ./js-opt-64-dm-ts-linux-4e4e0f502969 --ion-eager --ion-parallel-compile=off w523-reduced.js 
Segmentation fault (core dumped)

(also asserts in Assertion failure: ins->lhs()->type() == MIRType_Int32, at jit/Lowering.cpp)

I'm sorry to say that, in addition to this exploitable testcase and the real-world crash in bug 1006561, plus that this is a fuzzblocker, I'm deciding to back bug 998580 out, unfortunately.

Comment 6

[Christian Holler (:decoder)]

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

Comment 7

[Dan Gohman [:sunfish]]

Attachment: fixed-truncate-kinds.patch

The underlying bug here is that the patch made MMod::truncate and MDiv::truncate set their specialization_ to MIRType_Int32 without providing an MMod::operandTruncateKind and MDiv::operandTruncateKind to arrange for their operands to be specialized to MIRType_Int32.

Attached is the same patch but with MMod::operandTruncateKind and MDiv::operandTruncateKind specializations added. With this patch, all three of the testcases in this bug pass.

Comment 9

[Christian Holler (:decoder)]

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

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/23d16c2f0f67
user:        Gary Kwong
date:        Tue May 06 22:57:59 2014 -0700
summary:     Backout 87feb441d562 for causing issues.

This iteration took 235.655 seconds to run.

Comment 10

[Dan Gohman [:sunfish]]

Comment on attachment 8418827 [details] [diff] [review]
fixed-truncate-kinds.patch

Requesting fuzzing :-). The patch is based on 7fffbe8dc9ec9f0dc124485c112419b5a2882674.

Comment 11

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

Comment on attachment 8418827 [details] [diff] [review]
fixed-truncate-kinds.patch

(function(stdlib, n, heap) {
    "use asm";
    var Uint16ArrayView = new stdlib.Uint16Array(heap)
    function f(i1) {
        i1 = i1 | 0;
        Uint16ArrayView[8] = (.0 > ((+(2 ^ i1)) % +2))
    }
})()

Crashes debug shell [@ js::jit::LRecoverInfo::appendResumePoint]

$ ./js-dbg-64-dm-ts-linux-8be0e21fd300-1006301-c7-ae2ba773efa6 --ion-parallel-compile=off --ion-eager 623-interesting.js
Segmentation fault (core dumped)

(does not crash without the patch)

Configuration parameters:

AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 12

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

Attachment: Stack for testcase in comment 11

Comment 13

[Dan Gohman [:sunfish]]

Attachment: fixed-truncate-kinds.patch

Attached is a new patch which fixes the asm.js issue.

When MCompare converts to integer, it may do so with the understanding that its operands can bail out if they produce a value which is not integer, but under asm.js, there are no resume points, so attempting to setup a bailout does not go well. This patch fixes the problem by having MCompare::truncate check whether there is a resume point present before converting to integer.

Comment 14

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8419189 [details] [diff] [review]
fixed-truncate-kinds.patch

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

This patch add MMod and MDiv operandTruncateKind functions, as well as preventing the optimization on MCompare when we have no way to fallback in case of mistake.

When a definition is successfully MMod::truncate, it is added to the list of instructions where we are adjusting the inputs of the definition, in order to correctly truncate the operands.  Truncating the operands fix the lowering issue, as we were specializing to the MMod to an Int32 operation.

We need to ensure that we we specialize the operation in ::truncate, then we correctly specialize the operands of it in ::operandTruncateKind.

::: js/src/jit/RangeAnalysis.cpp
@@ +2376,5 @@
> +    // they have values that aren't int32. Also, in that case, type inference
> +    // or asm.js will have hopefully already converted relevant comparisons
> +    // to integer already.
> +    if (!block()->entryResumePoint())
> +       return false;

nit: I suggest that we follow the same rule as in the MIRGenerator[2].  And maybe even up-lift this function to the CompileInfo class, which is supposed to know this kind of things.

Also, this should be the first check in MCompare::truncate.

[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGenerator.h#90

Comment 15

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8419189 [details] [diff] [review]
fixed-truncate-kinds.patch

Note: I think we need to update AdjustTruncatedInputs such as we do not eagerly truncate inputs if we have a truncate kind of TruncateAfterBailouts.

Comment 16

[Dan Gohman [:sunfish]]

Attachment: fixed-truncate-kinds.patch

To :gkw and :decoder; requesting fuzzing for this new patch, which is based on 4cafec48a1f0 (top of tree as of this writing).

This patch has an r+ above with comments, and I'm posting an updated patch with minor changes to address the comments, so I guess it doesn't need another r?.

(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> Comment on attachment 8419189 [details] [diff] [review]
> fixed-truncate-kinds.patch
> 
> Review of attachment 8419189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch add MMod and MDiv operandTruncateKind functions, as well as
> preventing the optimization on MCompare when we have no way to fallback in
> case of mistake.
> 
> When a definition is successfully MMod::truncate, it is added to the list of
> instructions where we are adjusting the inputs of the definition, in order
> to correctly truncate the operands.  Truncating the operands fix the
> lowering issue, as we were specializing to the MMod to an Int32 operation.
> 
> We need to ensure that we we specialize the operation in ::truncate, then we
> correctly specialize the operands of it in ::operandTruncateKind.

Yes. With this patch, the operators that specialize in ::truncate are MAdd, MSub, MMul, MDiv, and MMod, and all of these operators specialize their operands in ::operandTruncateKind.

> ::: js/src/jit/RangeAnalysis.cpp
> @@ +2376,5 @@
> > +    // they have values that aren't int32. Also, in that case, type inference
> > +    // or asm.js will have hopefully already converted relevant comparisons
> > +    // to integer already.
> > +    if (!block()->entryResumePoint())
> > +       return false;
> 
> nit: I suggest that we follow the same rule as in the MIRGenerator[2].  And
> maybe even up-lift this function to the CompileInfo class, which is supposed
> to know this kind of things.

Done. I also up-lifted the function to CompileInfo.

> Also, this should be the first check in MCompare::truncate.

Done.

(In reply to Nicolas B. Pierron [:nbp] from comment #15)
> Note: I think we need to update AdjustTruncatedInputs such as we do not
> eagerly truncate inputs if we have a truncate kind of TruncateAfterBailouts.

Makes sense. I added code to the patch to make AdjustTrunatedInputs use MToInt32 instead of MTruncateToInt32 for the TruncateAfterBailouts case.

Comment 17

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

Comment on attachment 8419434 [details] [diff] [review]
fixed-truncate-kinds.patch

Tentatively marking as feedback+ after a few hours' of fuzzing - due to request backlog on my side, I think this can land soon without it eventually blowing up jsfunfuzz later.

Comment 18

[Dan Gohman [:sunfish]]

Thanks Gary!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66e279688a1

Comment 19

[Dan Gohman [:sunfish]]

Comment on attachment 8419434 [details] [diff] [review]
fixed-truncate-kinds.patch

Cancelling feedback? as :gkw already responded.

Comment 20

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

This landed:

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

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This landed without sec approval?

Comment 23

[Dan Gohman [:sunfish]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> This landed without sec approval?

As identified in comment 0, this was a recent regression on mozilla-central.