1277796 - Crash [@ js::jit::MSimdSplat::foldsTo] or Crash [@ js::jit::MConstant::toFloat32] or Assertion failure: in->type() == MIRType::Int32 (Boolean SIMD vector requires Int32 lanes.), at jit/TypePolicy.cpp:597 with SIMD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 22047a4eea78 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --disable-debug, run with --fuzzing-safe --ion-eager):

SIMD.Bool16x8()
while (true);


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf666db40 (LWP 27785)]
js::jit::MSimdSplat::foldsTo (this=0xf7a8ebd8, alloc=...) at js/src/jit/MIR.cpp:1184
#0  js::jit::MSimdSplat::foldsTo (this=0xf7a8ebd8, alloc=...) at js/src/jit/MIR.cpp:1184
#1  0x083025fe in simplified (def=0xf7a8ebd8, this=0xf666d180) at js/src/jit/ValueNumbering.cpp:618
#2  js::jit::ValueNumberer::visitDefinition (this=0xf666d180, def=0xf7a8ebd8) at js/src/jit/ValueNumbering.cpp:777
#3  0x08302f95 in js::jit::ValueNumberer::visitBlock (this=this@entry=0xf666d180, block=block@entry=0xf7a8e688, dominatorRoot=dominatorRoot@entry=0xf7a8e688) at js/src/jit/ValueNumbering.cpp:987
#4  0x08303370 in js::jit::ValueNumberer::visitDominatorTree (this=this@entry=0xf666d180, dominatorRoot=dominatorRoot@entry=0xf7a8e688) at js/src/jit/ValueNumbering.cpp:1032
#5  0x08303430 in js::jit::ValueNumberer::visitGraph (this=0xf666d180) at js/src/jit/ValueNumbering.cpp:1070
#6  0x083034c5 in js::jit::ValueNumberer::run (this=this@entry=0xf666d180, updateAliasAnalysis=updateAliasAnalysis@entry=js::jit::ValueNumberer::UpdateAliasAnalysis) at js/src/jit/ValueNumbering.cpp:1241
#7  0x081f7f65 in js::jit::OptimizeMIR (mir=mir@entry=0xf7a8e138) at js/src/jit/Ion.cpp:1664
#8  0x081f8e90 in js::jit::CompileBackEnd (mir=mir@entry=0xf7a8e138) at js/src/jit/Ion.cpp:1988
#9  0x084a8bb3 in js::HelperThread::handleIonWorkload (this=this@entry=0xf7a18948) at js/src/vm/HelperThreads.cpp:1444
#10 0x084a9a54 in js::HelperThread::threadLoop (this=0xf7a18948) at js/src/vm/HelperThreads.cpp:1747
#11 0x084caaaf in nspr::Thread::ThreadRoutine (arg=0xf7a021e0) at js/src/vm/PosixNSPR.cpp:45
#12 0xf7fb8f59 in start_thread (arg=0xf666db40) at pthread_create.c:312
#13 0xf7d85c4e in clone () from /lib32/libc.so.6
eax	0x62	98
ebx	0x95242f8	156386040
ecx	0xf7a8ebd8	-139924520
edx	0xf7a8e010	-139927536
esi	0xf7a8eb90	-139924592
edi	0xf7a8ebd8	-139924520
ebp	0xf666cfb8	4133932984
esp	0xf666cee0	4133932768
eip	0x8254870 <js::jit::MSimdSplat::foldsTo(js::jit::TempAllocator&)+720>
=> 0x8254870 <js::jit::MSimdSplat::foldsTo(js::jit::TempAllocator&)+720>:	movl   $0x4a0,0x0
   0x825487a <js::jit::MSimdSplat::foldsTo(js::jit::TempAllocator&)+730>:	call   0x8096f40 <abort()>


This is fairly easy to trigger, marking as fuzzblocker.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160531084741" and the hash "e5f0088f8ca2ebd070812488f4296e81ca111ad2".
The "bad" changeset has the timestamp "20160531090146" and the hash "c19c99878a6076e928690f45b37403b110cd5482".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e5f0088f8ca2ebd070812488f4296e81ca111ad2&tochange=c19c99878a6076e928690f45b37403b110cd5482

Comment 2

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

Jakob, is bug 1136226 a likely regressor?

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Yes, thanks. I'll take a look.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Bug 1277796 - Fix boolean SIMD constructors.

Add better test coverage of boolean SIMD constructors with insufficient
arguments.

Add better test coverage of 8x16 and 16x8 constructor calls with missing args.

Fix a crash caused by a missing bool-to-int conversion before a boolean splat
when inlining a boolean constructor.

Fix a crash in the foldsTo method for boolean splats. The 8x16 and 16x8 booleans
were not implemented.

Review commit: https://reviewboard.mozilla.org/r/57750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57750/

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8760002 [details]
Bug 1277796 - Fix boolean SIMD constructors.

https://reviewboard.mozilla.org/r/57750/#review54714

Thanks for fixing this. I think the two lines in MCallOptimize.cpp should be removed; if they can't, we probably need a different change in this function.

::: js/src/jit/MCallOptimize.cpp:3464
(Diff revision 1)
>          current->add(values);
>      } else {
>          // For general constructor calls, start from splat(defVal), insert one
>          // lane at a time.
> +        if (laneType == MIRType::Boolean)
> +            defVal = constant(Int32Value(0));

Why is this needed? It seems defVal should already be assigned a constant Boolean value "false" when the laneType is MIRType::Boolean.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

https://reviewboard.mozilla.org/r/57750/#review54714

> Why is this needed? It seems defVal should already be assigned a constant Boolean value "false" when the laneType is MIRType::Boolean.

The boolean splat and insertLane operators require an int32 input with the value 0 or -1. 

Do you think this intention would be clearer with `defVal = convertToBooleanSimdLane(defVal)`? Both require some constant folding, which is ok, I think.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #6)
> https://reviewboard.mozilla.org/r/57750/#review54714
> 
> > Why is this needed? It seems defVal should already be assigned a constant Boolean value "false" when the laneType is MIRType::Boolean.
> 
> The boolean splat and insertLane operators require an int32 input with the
> value 0 or -1. 
> 
> Do you think this intention would be clearer with `defVal =
> convertToBooleanSimdLane(defVal)`? Both require some constant folding, which
> is ok, I think.

Okay, I've re-read the code and it is a bit messy, as defVal is used both in the SimdValueX4 branch *and* in the else branch. So doing what you suggest works fine; with a bit more tidying up, we could group the |for| loop in the case |lanes == 4| and convertToBooleanSimdLane iff |i < callinfo.argc()|; then we could create the proper defVal for boolean (-1 or 0) directly up there. Both work for me (i.e. r=me for either way).

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8760002 [details]
Bug 1277796 - Fix boolean SIMD constructors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57750/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6ca5927195
Fix boolean SIMD constructors. r=bbouvier

Comment 10

[Fuzzing Team]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f6ca5927195

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like this also definitely affects 49, but the patch here missed the merge cutoff. Can you request uplift to aurora? Thanks.

Comment 13

[Jakob Stoklund Olesen [:jolesen]]

SIMD is still only enabled in nightly builds, so I don't think this is necessary to merge.