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

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 2 bugs, {assertion, regression})

Trunk
mozilla47
x86_64
Linux
assertion, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [jsbugmon:ignore])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1244831
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 2

2 years ago
Created attachment 8714778 [details] [diff] [review]
Ensure enough ballast space in TypeAnalyzer::adjustInputs.

This patch alone should solve the issue seen in comment 0 and in Bug 1244831.
Attachment #8714778 - Flags: review?(hv1989)
(Assignee)

Comment 3

2 years ago
Created attachment 8714779 [details] [diff] [review]
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.
Attachment #8714779 - Flags: review?(hv1989)
(Assignee)

Comment 4

2 years ago
Created attachment 8714781 [details] [diff] [review]
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8714782 [details] [diff] [review]
Ensure enough ballast space in AllDoublePolicy::adjustInputs.

MHypot is a varidic instruction too, and has a AllDoublePolicy.
(Assignee)

Comment 6

2 years ago
Created attachment 8714783 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8714781 - Flags: review?(bbouvier)
(Assignee)

Updated

2 years ago
Attachment #8714782 - Flags: review?(bbouvier)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 10

2 years ago
(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?
(Assignee)

Comment 12

2 years ago
(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+
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8714781 - Attachment is obsolete: true

Comment 15

2 years ago
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

2 years ago
backoutbugherderlanding
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

2 years ago
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1245178
You need to log in before you can comment on or make changes to this bug.