1186006 - Crash at js::jit::LRecoverInfo::appendResumePoint js::jit::LRecoverInfo::init js::jit::LRecoverInfo::New js::jit::LIRGeneratorShared::getRecoverInfo js::jit::LIRGeneratorShared::buildSnapshot

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: windgb information from this url

Found by bughunter and reproduced on a windows 7 trunk debug build

steps to reproduce:
-> Load http://w110.bcn.cat/portal/site/MuseuPicasso/menuitem.61a8b00f5dbc36aca4b2e31220348a0c/?vgnextoid=3d09f87a9950e410VgnVCM1000001947900aRCRD&vgnextchannel=0000000417706709VgnV6CONT00000000000RCRD&vgnextfmt=formatDetall&lang=ca_ES

-->> js::jit::LRecoverInfo::appendResumePoint js::jit::LRecoverInfo::init js::jit::LRecoverInfo::New js::jit::LIRGeneratorShared::getRecoverInfo js::jit::LIRGeneratorShared::buildSnapshot

Comment 1

[Bob Clary [:bc] (inactive)]

fyi, I was testing something where I needed a reproducible crash and found that I could reproduce this in Nightly/42 opt/debug on Linux x86_64. Looking at crash-stats, it's a pretty low volume crasher.

Comment 2

[Nicolas B. Pierron [:nbp]]

Based on the stack, it seems that we have a NULL resume point, which is not supposed to happen unless we have new split-edge blocks, or join blocks without resume-points (as suggested by Bug 1106171).

This is probably related to a new phase added to the compilation pipeline.

Comment 3

[Nicolas B. Pierron [:nbp]]

I tried adding an assertion, but unfortunately I get too many false positive created by SplitCriticalEdgesForBlock.  I think I have an idea which would let us remove tons of false positive created by this function, by inheriting the resume point from the successor instead of the predecessor.

I hope either the patch will fix this issue, or the assertion will highlight from where this issue is coming from.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Add a copy of the successor resume point to the split-edge blocks.

This patch removes the propagation of the resume point from the
predecessors, which was done during the lowering.

Instead, this patch copy the resume point of the successor when the
split-edge block is allocated.  Note, this is different than the default
creation of basic block which are inheriting their resume point from their
predecessor.  Copying from the successor is on the other hand trivial, as
the successor has a resume point, and if it has Phi, then we should just
pick the operands from the current edge.

The difference, is that taking the predecessor one will resume before the
branch (as we did before, with some corner cases), and taking the successor
will resume after the branch (with no corner cases).

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 8656575 [details] [diff] [review]
Add a copy of the successor resume point to the split-edge blocks.

Review of attachment 8656575 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIRGraph.cpp
@@ +341,5 @@
> +
> +        // A split edge is used to simplify the graph to avoid having a
> +        // predecessor with multiple successors as well as a successor with
> +        // multiple predecessors.  As instructions can be moved in this
> +        // split-edge block, we need to give this instruction a resume point. We

s/instruction/block/

@@ +345,5 @@
> +        // split-edge block, we need to give this instruction a resume point. We
> +        // have 2 options, either we take the last resume point of the
> +        // predecessor (which implies iterating backward over instructions), or
> +        // we copy the entry resume points of the successor (which implies
> +        // filtering phi to keep inputs from the current edge).

The last part of this comment is confusing.  Why not just describe the approach which is actually being used?

Comment 6

[Carsten Book [:Tomcat]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 8656575 [details] [diff] [review]
> Add a copy of the successor resume point to the split-edge blocks.
> 
> Review of attachment 8656575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MIRGraph.cpp
> @@ +341,5 @@
> > +
> > +        // A split edge is used to simplify the graph to avoid having a
> > +        // predecessor with multiple successors as well as a successor with
> > +        // multiple predecessors.  As instructions can be moved in this
> > +        // split-edge block, we need to give this instruction a resume point. We
> 
> s/instruction/block/
> 
> @@ +345,5 @@
> > +        // split-edge block, we need to give this instruction a resume point. We
> > +        // have 2 options, either we take the last resume point of the
> > +        // predecessor (which implies iterating backward over instructions), or
> > +        // we copy the entry resume points of the successor (which implies
> > +        // filtering phi to keep inputs from the current edge).
> 
> The last part of this comment is confusing.  Why not just describe the
> approach which is actually being used?

is this now ready for checkin ?

Comment 7

[Nicolas B. Pierron [:nbp]]

This patch found issues in our tests where the assertions does not hold, we should investigate more and make additional patches to fix these tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a558d10f5d5


PROCESS | 2542 | Assertion failure: block->entryResumePoint(), at /builds/slave/try-l64-d-00000000000000000000/build/src/js/src/jit/Lowering.cpp:4355
PROCESS-CRASH | /webvtt/rendering/cues-with-video/processing-model/bidi/u002E_u2028_u05D0.html | application crashed [@ js::jit::LIRGenerator::updateResumeState(js::jit::MBasicBlock*)]
TEST-UNEXPECTED-CRASH | /webvtt/rendering/cues-with-video/processing-model/bidi/u002E_u2028_u05D0.html | expected FAIL

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: (fuzzing) Add a copy of the successor resume point to the split-edge blocks.

This patch still is an intermittent failure on debug builds of Try (Wr) [1]
and I am unable to reproduce it locally, nor on the JS Shell.

If you some spare room for fuzzing, I would be interested in any signature
which looks like an assertion failure like:

  Assertion failure: block->entryResumePoint(), at .../js/src/jit/Lowering.cpp:4372
  SEGV at js::jit::LIRGenerator::updateResumeState(js::jit::MBasicBlock*)

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=995c7cf14ce3

Comment 9

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

Comment on attachment 8677506 [details] [diff] [review]
(fuzzing) Add a copy of the successor resume point to the split-edge blocks.

I take it that jsfunfuzz fuzzing is requested too.

Comment 10

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

Comment on attachment 8677506 [details] [diff] [review]
(fuzzing) Add a copy of the successor resume point to the split-edge blocks.

Didn't find anything out of the ordinary these few days.

Comment 11

[Christian Holler (:decoder)]

Comment on attachment 8677506 [details] [diff] [review]
(fuzzing) Add a copy of the successor resume point to the split-edge blocks.

I fuzzed this for a while but according to nbp, LangFuzz wasn't able to find the intermittent failure he was looking for. Clearing feedback request.

Comment 12

[Carsten Book [:Tomcat]]

nbp: is this ready for checkin ?

Comment 13

[Nicolas B. Pierron [:nbp]]

Ok, I finally found test cases to reproduce this issue (see Bug 1238592 and Bug 1221872).

I will fix this assertion tomorrow.

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that all blocks have an entry resume point, excepts for unreachable one added by the Value Numbering.

To make it short, the previous patch changed the way we create the split
edge blocks, to ensure that we have a resume point by making a copy of the
successor resume point.

This patch, instead of adding a resume point to the fixup block, flag fixup
blocks as unreachable, such that we can assert that unreachable blocks
should have no instructions once we reach the lowering phase.

This helps us remove the complex resume point propagation that we used to
have in the lowering phase, and add stricter assertions than what we used to
have in a few cases.

Comment 15

[Dan Gohman [:sunfish]]

Comment on attachment 8724058 [details] [diff] [review]
Assert that all blocks have an entry resume point, excepts for unreachable one added by the Value Numbering.

Review of attachment 8724058 [details] [diff] [review]:
-----------------------------------------------------------------

The more we can tie the special-casing to OSR, the more we can hopefully keep it from spreading too far. Please (a) mention OSR in comments for code dealing with these OSR-specific special cases, and (b) test for graph.osrBlock() in assertions whenever possible.

::: js/src/jit/Lowering.cpp
@@ +4591,5 @@
>  
>  void
>  LIRGenerator::updateResumeState(MBasicBlock* block)
>  {
> +    MOZ_ASSERT_IF(!mir()->compilingAsmJS() && !block->unreachable(), block->entryResumePoint());

This assert can be made stricter by also testing whether block->graph().osrBlock() when the block is unreachable().

@@ +4606,5 @@
>  
>      definePhis();
>  
> +    // See fixup blocks added by Value Numbering.
> +    MOZ_ASSERT_IF(block->unreachable(), *block->begin() == block->lastIns());

Can we also add a MOZ_ASSERT_IF(block->unreachable(), block->graph().osrBlock())? Also, please mention OSR in the comment.

::: js/src/jit/RangeAnalysis.cpp
@@ -2239,5 @@
>      JitSpew(JitSpew_Range, "Doing range propagation");
>  
>      for (ReversePostorderIterator iter(graph_.rpoBegin()); iter != graph_.rpoEnd(); iter++) {
>          MBasicBlock* block = *iter;
> -        MOZ_ASSERT(!block->unreachable());

Can we keep this assert, but change it to !block->unreachable() || graph_.osrBlock() ?

@@ +3383,5 @@
>          if (!block->unreachable())
>              continue;
>  
> +        // Filter out unreachable fake entries.
> +        if (block->numPredecessors() == 0)

Can this MOZ_ASSERT(graph.osrBlock())? Either way, the comment should mention "OSR" as being associated with this special case.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d95fd1fbee52
https://hg.mozilla.org/integration/mozilla-inbound/rev/130026ae6a1e

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/d95fd1fbee52
https://hg.mozilla.org/mozilla-central/rev/130026ae6a1e