Closed Bug 1121299 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- affected
firefox38 --- affected

People

(Reporter: decoder, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker] [jsbugmon:update,ignore])

Crash Data

Attachments

(1 file)

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.
Needinfo from sunfish because it's in SIMD.
Flags: needinfo?(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?
Flags: needinfo?(sunfish)
// 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?
Blocks: 1112154
Crash Signature: [@ js::jit::IonCannon]
Flags: needinfo?(nicolas.b.pierron)
Keywords: assertion
OS: Linux → All
Hardware: x86 → All
Summary: Crash [@ ??] with SIMD and recursion → Crash [@ ??] or [@ js::jit::IonCannon] or Assertion failure: to->toStackSlot()->slot() % SimdMemoryAlignment == 0, at jit/LIR.cpp with SIMD and recursion
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [jsbugmon:update][fuzzblocker]
Whiteboard: [jsbugmon:update][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
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.
(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.
Flags: needinfo?(nicolas.b.pierron)
(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.
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?
Attachment #8548823 - Flags: review?(sunfish)
Attachment #8548823 - Flags: feedback?(gary)
Attachment #8548823 - Flags: feedback?(choller)
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 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.
Attachment #8548823 - Flags: review?(sunfish) → review+
(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 on attachment 8548823 [details] [diff] [review]
Let GVN fold MSimdBox when it can fold MSimdConstants.

(Setting feedback+ on behalf of fuzzers)
Attachment #8548823 - Flags: feedback?(gary)
Attachment #8548823 - Flags: feedback?(choller)
Attachment #8548823 - Flags: feedback+
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision c1f6345f2803).
https://hg.mozilla.org/mozilla-central/rev/b00198e105b6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.