Closed Bug 1244828 Opened 4 years ago Closed 4 years ago

Assertion failure: result ([OOM] Is it really infallible?), at js/src/ds/LifoAlloc.h:281 involving TypeAnalyzer::adjustInputs

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, Whiteboard: [jsbugmon:ignore])

Attachments

(4 files, 1 obsolete file)

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
Duplicate of this bug: 1244831
Summary: Assertion failure: result ([OOM] Is it really infallible?), at js/src/ds/LifoAlloc.h:281 involving js::jit::MBox::New → Assertion failure: result ([OOM] Is it really infallible?), at js/src/ds/LifoAlloc.h:281 involving TypeAnalyzer::adjustInputs
This patch alone should solve the issue seen in comment 0 and in Bug 1244831.
Attachment #8714778 - Flags: review?(hv1989)
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.
Attachment #8714779 - Flags: review?(hv1989)
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.
Attachment #8714781 - Flags: review?(bbouvier)
Attachment #8714782 - Flags: review?(bbouvier)
Attachment #8714783 - Flags: review?(bbouvier)
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?
Attachment #8714781 - Flags: review?(bbouvier) → review+
Attachment #8714782 - Flags: review?(bbouvier) → review+
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...
Attachment #8714783 - Flags: review?(bbouvier) → review+
(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 :)
Attachment #8714779 - Flags: review?(hv1989) → review+
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...
Attachment #8714778 - Flags: review?(hv1989) → review+
(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.
Attachment #8714781 - Attachment is obsolete: true
Duplicate of this bug: 1245178
You need to log in before you can comment on or make changes to this bug.