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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure enough ballast space in TypeAnalyzer::adjustInputs.

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

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure enough ballast space in TypeAnalyzer::adjustPhiInputs.

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.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure enough ballast space in SimdShufflePolicy::adjustInputs.

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.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure enough ballast space in AllDoublePolicy::adjustInputs.

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

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Ensure enough ballast space in CallPolicy::adjustInputs.

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

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

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 8

[Benjamin Bouvier [:bbouvier] (inactive)]

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 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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...

Comment 10

[Nicolas B. Pierron [:nbp]]

(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.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 12

[Nicolas B. Pierron [:nbp]]

(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 13

[Hannes Verschore [:h4writer]]

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...

Comment 14

[Nicolas B. Pierron [:nbp]]

(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.

Comment 15

[Pulsebot]

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

Comment 16

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

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

Comment 17

[Pulsebot]

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

Comment 18

[Carsten Book [:Tomcat]]

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