Nicolas requests that each stack should have it's own bug, blocking meta bug 1244824. Assigning to him by default. #0 js::LifoAlloc::allocInfallibleOrAssert (this=<optimized out>, n=<optimized out>) at js/src/ds/LifoAlloc.h:281 #1 js::jit::TempAllocator::allocateInfallible (this=<optimized out>, bytes=<optimized out>) at js/src/jit/JitAllocPolicy.h:40 #2 0x0000000000694483 in js::jit::TempObject::operator new (nbytes=136, alloc=...) at js/src/jit/JitAllocPolicy.h:174 #3 js::jit::MBox::New (ins=<optimized out>, alloc=...) at js/src/jit/MIR.h:4433 #4 js::jit::AlwaysBoxAt (alloc=..., at=<optimized out>, operand=<optimized out>) at js/src/jit/TypePolicy.cpp:43 #5 BoxAt (at=0x2f10300, operand=<optimized out>, alloc=...) at js/src/jit/TypePolicy.cpp:53 #6 js::jit::BoxPolicy<1u>::staticAdjustInputs (alloc=..., ins=0x2f10300) at js/src/jit/TypePolicy.cpp:629 #7 0x00000000006a5c54 in js::jit::MixPolicy<js::jit::ObjectPolicy<0u>, js::jit::BoxPolicy<1u> >::staticAdjustInputs (alloc=..., ins=0x2f10300) at js/src/jit/TypePolicy.h:419 #8 js::jit::MixPolicy<js::jit::ObjectPolicy<0u>, js::jit::BoxPolicy<1u> >::adjustInputs (this=<optimized out>, alloc=..., ins=0x2f10300) at js/src/jit/TypePolicy.h:422 #9 0x000000000057cb5a in (anonymous namespace)::TypeAnalyzer::adjustInputs (def=0x2f10300, this=<optimized out>) at js/src/jit/IonAnalysis.cpp:1499 #10 (anonymous namespace)::TypeAnalyzer::insertConversions (this=<optimized out>) at js/src/jit/IonAnalysis.cpp:1562 #11 (anonymous namespace)::TypeAnalyzer::analyze (this=<optimized out>) at js/src/jit/IonAnalysis.cpp:1806 #12 js::jit::ApplyTypeInformation (mir=<optimized out>, graph=...) at js/src/jit/IonAnalysis.cpp:1818 #13 0x0000000000572790 in js::jit::OptimizeMIR (mir=0x2ebe120) at js/src/jit/Ion.cpp:1631 #14 0x0000000000573f89 in js::jit::CompileBackEnd (mir=0x2ebe120) at js/src/jit/Ion.cpp:2008 #15 0x0000000000828285 in js::HelperThread::handleIonWorkload (this=0x2b38800) at js/src/vm/HelperThreads.cpp:1276 #16 0x0000000000827cce in js::HelperThread::threadLoop (this=0x2b38800) at js/src/vm/HelperThreads.cpp:1603 #17 0x00000000008cc569 in nspr::Thread::ThreadRoutine (arg=0x2b3cef0) at js/src/vm/PosixNSPR.cpp:45 #18 0x00007ff6026dc6aa in start_thread (arg=0x7ff60164b700) at pthread_create.c:333 #19 0x00007ff601752eed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

This patch alone should solve the issue seen in comment 0 and in Bug 1244831.

Phi nodes can have a variadic number of operands because of large switch case. We have to ensure that we fail if we are unable to allocate more ballast space to satisfy unfallible allocations.

MSimdGeneralShuffle is a variadic instruction, thus when its type policy iterates over its list of operands, we have to ensure that there is enough ballast space to unbox/coerce the operands.

MHypot is a varidic instruction too, and has a AllDoublePolicy.

MCall is a variadic instruction and it has a CallPolicy to prevent float32 to flow in any of its operands.

Random thought: could a call to alloc.ensureBallast() in TypeAnalyzer::insertConversions or adjustInputs be a more a general fix? Calling it at random places seems like a band aid likely to not prevent the issue from showing up again in the future. Or is it that we need to call it inside loops because adjustInputs can recursively call other adjustInputs? In this case, maybe adjustInputs could call in shared code first (which would ensureBallast) and then only call into the MInstruction-specialized part of adjustInputs?

Comment on attachment 8714781 [details] [diff] [review] Ensure enough ballast space in SimdShufflePolicy::adjustInputs. Review of attachment 8714781 [details] [diff] [review]: ----------------------------------------------------------------- Assuming we can't do what I've proposed in my previous comment, r+-ing these patches. ::: js/src/jit/TypePolicy.cpp @@ +858,5 @@ > MSimdGeneralShuffle* s = ins->toSimdGeneralShuffle(); > > for (unsigned i = 0; i < s->numVectors(); i++) { > + if (!alloc.ensureBallast()) > + return false; Do we need to call in the loop? Or can we just call at the top of this function once?

Comment on attachment 8714783 [details] [diff] [review] Ensure enough ballast space in CallPolicy::adjustInputs. Review of attachment 8714783 [details] [diff] [review]: ----------------------------------------------------------------- Really, please tell me we can do something better than that :-) ::: js/src/jit/TypePolicy.cpp @@ +923,1 @@ > EnsureOperandNotFloat32(alloc, call, MCall::IndexOfStackArg(i)); At this point, it seems that making all these functions (here EnsureOperandNotFloat32) fallible would be equivalent, if not better than manually ensuring ballast *then* calling these implicitly-fallible functions...

(In reply to Benjamin Bouvier [:bbouvier] from comment #7) > Random thought: could a call to alloc.ensureBallast() in > TypeAnalyzer::insertConversions or adjustInputs be a more a general fix? > Calling it at random places seems like a band aid likely to not prevent the > issue from showing up again in the future. The ballast is made to ensure that we have enough memory for making infallible allocations after, but if the number of allocations is unbounded or bounded but larger than the ballast space, then we might have ways to consume the whole ballast space before allocating more space. Thus, if an inner loop is unbounded, then it should check that we have enough ballast space in this inner loop. In particular, MHypot / MCall / MPhi can have a larger number of operands. I am not completely sure if a bound exists for MSimdShuffle but it seems to be unbounded as well. By the way, what you are suggesting are made in the patches that Hannes has to review, as most of the type policies are bounded (and the one which are not should check too), but the number of instructions is not. > Or is it that we need to call it inside loops because adjustInputs can > recursively call other adjustInputs? adjustInputs might be recursive, but it remains bounded, as the reason why we do a recursion is generally to box&unbox a value which does not have the right MIR Type. So, as long as the recursion is bounded and the number of operand is bounded as well, and lower than the ballast space we have no obligation to ensure that there is enough ballast space.

OK. SimdShuffle's number of inputs is bounded: we have up to numVectors + numLanes operands, which is in the worst case (int8x16) 2 vectors and 16 lanes, that is 18 operands. Maybe your SimdShuffle patch needs an update, in this case?

(In reply to Benjamin Bouvier [:bbouvier] from comment #11) > OK. SimdShuffle's number of inputs is bounded: we have up to numVectors + > numLanes operands, which is in the worst case (int8x16) 2 vectors and 16 > lanes, that is 18 operands. Maybe your SimdShuffle patch needs an update, in > this case? Ok, I will discard the SimdShufflePolicy patch then :)

Comment on attachment 8714778 [details] [diff] [review] Ensure enough ballast space in TypeAnalyzer::adjustInputs. Review of attachment 8714778 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonAnalysis.cpp @@ +1559,5 @@ > // AdjustInputs can add/remove/mutate instructions before and after the > // current instruction. Only increment the iterator after it is finished. > for (MInstructionIterator iter(block->begin()); iter != block->end(); iter++) { > + if (!alloc().ensureBallast()) > + return false; Can we move this inside "adjustInputs"? Since we are doing this check also inside adjustPhiInputs...

(In reply to Hannes Verschore [:h4writer] from comment #13) > Comment on attachment 8714778 [details] [diff] [review] > Ensure enough ballast space in TypeAnalyzer::adjustInputs. > > Review of attachment 8714778 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/IonAnalysis.cpp > @@ +1559,5 @@ > > // AdjustInputs can add/remove/mutate instructions before and after the > > // current instruction. Only increment the iterator after it is finished. > > for (MInstructionIterator iter(block->begin()); iter != block->end(); iter++) { > > + if (!alloc().ensureBallast()) > > + return false; > > Can we move this inside "adjustInputs"? Since we are doing this check also > inside adjustPhiInputs... If you meant inside TypeAnalysis::adjustInputs, the answer is no because we are iterating over an unbounded set of instructions (see comment 10). If you meant inside all the *Policy::adjustInputs, then I think this might be quite redundant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/343732a03719 https://hg.mozilla.org/integration/mozilla-inbound/rev/05ea15825d81 https://hg.mozilla.org/integration/mozilla-inbound/rev/90b24727ebcd https://hg.mozilla.org/integration/mozilla-inbound/rev/9668af33990e

I had to back this out for breaking asm tests in mochitest-1: https://treeherder.mozilla.org/logviewer.html#?job_id=21287226&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ae0cecf92f88 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c067cb416a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/26ad4312962f https://hg.mozilla.org/integration/mozilla-inbound/rev/98d1f904166d

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbd942a9cac https://hg.mozilla.org/integration/mozilla-inbound/rev/db1d8b455d54 https://hg.mozilla.org/integration/mozilla-inbound/rev/ecae2990b068 https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd1e1b817f2

https://hg.mozilla.org/mozilla-central/rev/fdbd942a9cac https://hg.mozilla.org/mozilla-central/rev/db1d8b455d54 https://hg.mozilla.org/mozilla-central/rev/ecae2990b068 https://hg.mozilla.org/mozilla-central/rev/4dd1e1b817f2