1008636 - Crash [@ js::jit::LRecoverInfo::appendResumePoint]

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Attachment: stack

(function(stdlib, n, heap) {
    "use asm"
    var Int16ArrayView = new stdlib.Int16Array(heap)
    function f() {
        var x = 4.
        Int16ArrayView[~~((1 ? .0 : .9) % x) >> 1] = 0
        u(x)
    }
})()

crashes js debug and opt shell on m-c changeset b66e279688a1 with --ion-parallel-compile=off --ion-eager at js::jit::LRecoverInfo::appendResumePoint

My configure flags are:

AR=ar sh /home/odroid/trees/mozilla-inbound/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>

===

(gdb) x/i $pc
=> 0x11e6da <js::jit::LRecoverInfo::appendResumePoint(js::jit::MResumePoint*)+6>:       ldr     r5, [r1, #28]
(gdb) x/i $r5
   0xf079e0:    pop     {r3, r5, r7}
(gdb) x/i $r1
   0x0: Cannot access memory at address 0x0
(gdb) x/i $r3
   0xffffffff:  Cannot access memory at address 0xfffffffe
(gdb) x/i $r7
   0x2010:      Cannot access memory at address 0x2010
(gdb)

Setting s-s because this may involve accessing 0xfffffffe.

Comment 1

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

Attachment: stack

(function(stdlib, n, heap) {
    "use asm";
    var abs = stdlib.Math.abs;
    var Float64ArrayView = new stdlib.Float64Array(heap)
    function f(x) {
        x = x | 0;
        x = (~~((+abs((+(0xff439dbd >> 0xffffffff)) - (0xff73ea8c ? 67108863.0 : -17179869185.0))) % 4097.0))
        Float64ArrayView[x >> 3] = 262145.0
    }
})()

crashes js debug shell on Mac with --ion-parallel-compile=off --ion-eager at js::jit::LRecoverInfo::appendResumePoint


:sunfish, is bug 998580 or bug 1006301 a likely regressor? (judging by messy bisection results due to multiple landings of that patch)

Comment 2

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

Assuming worse-case sec-critical again.

Comment 3

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

This is likely a regressor coming after the addition of Bug 990106, where DCE eagerly make an instruction recoverable where there is no resume point to capture it.

In such case, I guess we should just remove all instructions flagged as recovered.

Comment 4

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

Correction, this would be a range analysis error, I suggest we add an assertion such as we abort as soon as we enter buildSnapshot from a MIR instruction compiled with asmjs.

The above case would work seamlessly as we do not Lower these instructions.

Comment 5

[Christian Holler (:decoder)]

I don't fully understand the disassembly analysis in comment 0 and why this should be sec-critical. This is loading something into r5, based on r1 right? and r1 is 0x0. If thats the case, then I think we shouldn't set sec-critical unless we're really reading/writing some bad address or have other evidence that something like this might have happened.

Comment 6

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

(In reply to Christian Holler (:decoder) from comment #5)
> I don't fully understand the disassembly analysis in comment 0 and why this
> should be sec-critical. This is loading something into r5, based on r1
> right? and r1 is 0x0. If thats the case, then I think we shouldn't set
> sec-critical unless we're really reading/writing some bad address or have
> other evidence that something like this might have happened.

From what I understand r1 is null, because we are in an AsmJS compilation.  So we should just never call buildSnapshot.  I think the problem is coming from the fact that in AsmJS we do not have fallible truncate.

So it seems that we should always truncate inputs in AsmJS, independently of the fallible case in AdjustTruncatedInputs, or we should never produce a TruncateAfterBailout when we are using AsmJS.

Comment 7

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

Attachment: Always truncate when compiling AsmJS code.

This modifies AdjustTruncatedInputs such as we ignore the different levels
of truncations which might exists for Ion.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 8420709 [details] [diff] [review]
Always truncate when compiling AsmJS code.

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

::: js/src/jit/RangeAnalysis.cpp
@@ +2495,5 @@
>              truncated->replaceOperand(i, input->getOperand(0));
> +        } else if (kind == MDefinition::TruncateAfterBailouts && !block->info().compilingAsmJS()) {
> +            // When we are compiling with AsmJS, we have no opportunity to bail
> +            // to anything. This means that in all cases we will flow into a
> +            // truncated operation.

This doesn't look safe. A truncated MMod doesn't fully truncate its inputs. For example, (1.1 % 1.2)|0 is not equal to ((1.1|0) % (1.2|0))|0. If we get here with an MMod in asm.js mode, I think it's already too late.

I think it would be better to disable truncation of MMod in asm.js mode in the first place, since the only thing it will be able to do is apply TruncateWithBailouts to its operands.

In fact, it may make sense to just skip RangeAnalysis::truncate() for asm.js entirely. asm.js gives us types for every expression ahead of time, so in theory there shouldn't be any interesting truncation happening anyway.

Comment 9

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

(In reply to Dan Gohman [:sunfish] from comment #8)
> In fact, it may make sense to just skip RangeAnalysis::truncate() for asm.js
> entirely. asm.js gives us types for every expression ahead of time, so in
> theory there shouldn't be any interesting truncation happening anyway.

Luke tried to disable Range Analysis before, but he didn't, I don't know the reasons.

I think you might be right, in which case the range analysis would only be useful for removing bounds check.

Comment 10

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

Attachment: Disable automatic truncation for Asm.JS

This patch disables the automatic truncation done by Range Analysis.
I still need to check AWFY performance to see if this has any effect.

Comment 11

[Dan Gohman [:sunfish]]

Comment on attachment 8420722 [details] [diff] [review]
Disable automatic truncation for Asm.JS

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

LGTM. If there are any performance regressions, we may need to file bugs against Emscripten.

Comment 12

[Christian Holler (:decoder)]

Null-deref, not s-s.

Comment 13

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

Re-locking s-s for the moment even though this is a null deref, I have a bunch of stacks that came from testcases that showed varying symptoms, so let's set this csec-dos and hidden for now.

Comment 14

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

Attachment: yet another stack

Judging by the different testcases in comment 0, comment 1 and as well as this stack from fuzzing instances which reduce to the same issue, let's keep this hidden for now.

Comment 15

[Alon Zakai (:azakai)]

I ran the emscripten benchmark suite on this patch, and saw no regressions.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a8e28507e6

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32a8e28507e6

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32