1121299 - Crash [@ ??] or [@ js::jit::IonCannon] or Assertion failure: to->toStackSlot()->slot() % SimdMemoryAlignment == 0, at jit/LIR.cpp with SIMD and recursion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 3d846527576f (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --enable-debug, run with --fuzzing-safe --thread-count=2):

function test() {
  var a = SIMD.float32x4(1.1, 2.2, 3.3, 4.6);
  SIMD.int32x4.fromFloat32x4(a);
  test();
}
test();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0xf766f28d in ?? ()
#0  0xf766f28d in ?? ()
[...]
#127 0x096f7148 in ?? ()
eax	0xf6515eb0	-162439504
ebx	0xffffff88	-120
ecx	0xf6515ed0	-162439472
edx	0x0	0
esi	0x0	0
edi	0x96f7148	158298440
ebp	0xfffd59c4	4294793668
esp	0xfffd592c	4294793516
eip	0xf766f28d	4150719117
=> 0xf766f28d:	vmovaps %xmm0,0x60(%esp)
   0xf766f293:	add    $0x50,%esp



Looks like a stack space overflow (infinite recursion), therefore probably not s-s. But this crashes pretty easily and causes lots of hard to match issues.

Comment 1

[Christian Holler (:decoder)]

Needinfo from sunfish because it's in SIMD.

Comment 2

[Dan Gohman [:sunfish]]

%esp is misaligned for the vmovaps access here, so this is an alignment trap rather than a stack overflow. Confirmed that it's not s-s. nbp, do you know the plan for stack alignment in Ion?

Comment 3

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

// Randomly chosen test: js/src/tests/ecma_7/SIMD/float32x4reify.js
var float32x4 = SIMD.float32x4;
function test() {
    var Array = float32x4.array(3);
    var array = new Array([
        float32x4(1, 2, 3, 4),
        float32x4(5, 6, 7, 8),
        float32x4(9, 10, 11, 12)
    ]);
    if (typeof reportCompare === "function")
        reportCompare(true, true);
}
test();
// Randomly chosen test: js/src/tests/ecma_2/Exceptions/lexical-003.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/basic/bug787847.js
evaluate("test(); test();", {
    compileAndGo: true
})

asserts js debug shell on m-c rev 63006936ab99 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: to->toStackSlot()->slot() % SimdMemoryAlignment == 0, at jit/LIR.cpp

// Randomly chosen test: js/src/tests/ecma_7/SIMD/float32x4reify.js
var float32x4 = SIMD.float32x4;
function test() {
    Array([
        float32x4(9, 10, 11, 12)
    ]);
}
test();
// Randomly chosen test: js/src/tests/ecma_2/Exceptions/lexical-003.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/basic/bug787847.js
evaluate("test(); test();", {
    compileAndGo: true
})

crashes js debug shell on m-c rev 63006936ab99 with --fuzzing-safe --no-threads --ion-eager at js::jit::IonCannon

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/63006936ab99/js/src/tests/ecma_7/SIMD/float32x4reify.js
http://hg.mozilla.org/mozilla-central/file/63006936ab99/js/src/tests/ecma_2/Exceptions/lexical-003.js
http://hg.mozilla.org/mozilla-central/file/63006936ab99/js/src/jit-test/tests/basic/bug787847.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ac3b15d06665
user:        Nicolas B. Pierron
date:        Mon Jan 12 16:45:55 2015 +0100
summary:     Bug 1112154 - Add MSimdBox and inline calls to SIMD constructors. r=bbouvier,jandem

Nicolas, is bug 1112154 a likely regressor?

Comment 4

[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/ac3b15d06665
user:        Nicolas B. Pierron
date:        Mon Jan 12 16:45:55 2015 +0100
summary:     Bug 1112154 - Add MSimdBox and inline calls to SIMD constructors. r=bbouvier,jandem

This iteration took 645.201 seconds to run.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Dan Gohman [:sunfish] from comment #2)
> %esp is misaligned for the vmovaps access here, so this is an alignment trap
> rather than a stack overflow. Confirmed that it's not s-s. nbp, do you know
> the plan for stack alignment in Ion?

The plan was to add it later as there is long tail of bugs before getting this work.

Also, this case were not supposed to happen.  Normally, every MSimdValueX4 is followed by an MSimdBox.  Thus the SIMD register is not supposed to be needed for any snapshot/safepoint.

The fact that we spill the SIMD register as part of a safepoint highlights an optimization issue in our register allocator.

I will investigate more and try to find a work-around or a fix for our register allocator.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> The fact that we spill the SIMD register as part of a safepoint highlights
> an optimization issue in our register allocator.
> 
> I will investigate more and try to find a work-around or a fix for our
> register allocator.

Ok, the problem is that we inline the test fucntion (comment 0) in it-self when we compile it, which cause GVN to fold identical MSimdConstant.  I guess we can fix this easily by folding MSimdBox as well.

I will make a patch.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Let GVN fold MSimdBox when it can fold MSimdConstants.

Unfortunately, doing so fix these issues but it adds a differential
behaviour between the interpreter and Ion.  This differential behviour is
caused by the fact that by making the MSimdBox congruent, we are removing
the object identity which is still honored by the interpreter.

In the long term we want to get rid of this object identity, and have a
value type identity instead. (Bug 1112167)

Gary, decoder, would this solution/patch be better for the moment?

Comment 8

[Christian Holler (:decoder)]

I'm not doing any differential testing at the moment, but fixing this bug is much more important than differential testing. In the long term, it would of course be nice to find a solution to fix both, but right now, it's a top crasher that can barely be matched properly with signatures, so we should fix that first and immediately. Thanks!

Comment 9

[Dan Gohman [:sunfish]]

Comment on attachment 8548823 [details] [diff] [review]
Let GVN fold MSimdBox when it can fold MSimdConstants.

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

::: js/src/jit/MIR.h
@@ +2963,5 @@
>          return initialHeap_;
>      }
>  
> +    bool congruentTo(const MDefinition *ins) const MOZ_OVERRIDE {
> +        return congruentIfOperandsEqual(ins);

This doesn't take into account the initialHeap_ or templateObject_ members. Please either (a) make congruency depend on them being the same, (b) assert that they are the same, or (c) add a comment confirming that it's ok that they might differ.

Comment 10

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

(In reply to Christian Holler (:decoder) from comment #8)
> In the long term, it would of course be nice to find a solution to fix
> both, but right now, it's a top crasher that can barely be matched
> properly with signatures, so we should fix that first and immediately.
> Thanks!

Agreed. While I am running differential testing, fixing this crash/assert is more important and we can have follow-up bugs about differential testing after that.

Comment 11

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

Comment on attachment 8548823 [details] [diff] [review]
Let GVN fold MSimdBox when it can fold MSimdConstants.

(Setting feedback+ on behalf of fuzzers)

Comment 12

[Christian Holler (:decoder)]

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

Comment 13

[Nicolas B. Pierron [:nbp]]

(Try) https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b7527da917
(Inbound) https://hg.mozilla.org/integration/mozilla-inbound/rev/b00198e105b6

Comment 14

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

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