880377 - Crash [@ IntersectDominators]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 204de5b7e0a6 (run with --ion-eager):


var actual = '';
var obj0 = {}
obj2 = {};
obj2['b'+1] = 1;
if (actual[0] != 0[0])
  throw new i();
for (var k in obj2) {
  for (var apply in obj0)
    ++count;
}

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Looks like a null-deref:

Program received signal SIGSEGV, Segmentation fault.
0x0845762e in IntersectDominators (block2=<optimized out>, block1=0x92fdd50) at js/src/ion/IonAnalysis.cpp:668
668                 if (idom == finger2)
#0  0x0845762e in IntersectDominators (block2=<optimized out>, block1=0x92fdd50) at js/src/ion/IonAnalysis.cpp:668
#1  ComputeImmediateDominators (graph=...) at js/src/ion/IonAnalysis.cpp:708
#2  js::ion::BuildDominatorTree (graph=...) at js/src/ion/IonAnalysis.cpp:736
#3  0x08559a6e in js::ion::UnreachableCodeElimination::removeUnmarkedBlocksAndCleanup (this=0xffffc554) at js/src/ion/UnreachableCodeElimination.cpp:70
#4  0x0844a154 in js::ion::OptimizeMIR (mir=0x9288800) at js/src/ion/Ion.cpp:1031
#5  0x0844faf2 in CompileBackEnd (mir=0x9288800, maybeMasm=<optimized out>) at js/src/ion/Ion.cpp:1246
[...]
eax     0x0     0
=> 0x845762e <js::ion::BuildDominatorTree(js::ion::MIRGraph&)+1054>:    mov    0x64(%eax),%esi

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/ee14945b452c
parent:      128511:d989eab66df4
user:        Brian Hackett
date:        Thu Apr 11 18:39:32 2013 -0600
summary:     Bug 804676 - Remove dependence of Ion compilation on ScriptAnalysis::analyzeTypes.

This iteration took 11.485 seconds to run.

Comment 4

[Christian Holler (:decoder)]

Needinfo from Brian based on comment 3.

Comment 5

[Brian Hackett [Laid off!]]

This looks like a problem with unreachable code elimination.  The different type information caused UCE to decide to eliminate the preheader for some loop, despite there being an OSR block inside it.  This causes later graph analysis to break.

Comment 6

[Niko Matsakis [:nmatsakis]]

All the blocks which UCE removes are indeed unreachable, but in this case it has the effect of severing the graph into two and removing loop headers. This is not clearly wrong to me, but it does seem to be incompatible with what later passes expect. I have a partial fix which tries to avoid optimizing away tests if they will create this scenario, but I haven't quite found the right way to summarize the set of blocks that should not be deleted.

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Do not attempt UCE if there are blocks only reachable from OSR

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 774066 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR

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

::: js/src/ion/UnreachableCodeElimination.cpp
@@ +105,5 @@
> +    return list.append(block);
> +}
> +
> +MBasicBlock *
> +UnreachableCodeElimination::branchSuccessor(MBasicBlock *block) {

This function should have a comment, or a better name.  Also, nit: { on the next line.

@@ +125,3 @@
>  UnreachableCodeElimination::prunePointlessBranchesAndMarkReachableBlocks()
>  {
>      Vector<MBasicBlock *, 16, SystemAllocPolicy> worklist;

This variable's type should be 'Worklist'

@@ +125,4 @@
>  UnreachableCodeElimination::prunePointlessBranchesAndMarkReachableBlocks()
>  {
>      Vector<MBasicBlock *, 16, SystemAllocPolicy> worklist;
> +    Vector<MBasicBlock *, 16, SystemAllocPolicy> neuter;

Maybe 'blocksWithKnownSuccessors' instead.

@@ +157,5 @@
> +    // Now, if there is an OSR block, check that all of its successors
> +    // were reachable (bug 880377). If not, we are in danger of
> +    // creating a CFG with two disjoint parts, so simply mark all
> +    // blocks as reachable. This generally occurs when the TI info is
> +    // incomplete.

The last sentence could use elaboration.  'This generally occurs when TI info for stack types is incomplete, due to operations that have only executed in the interpreter'

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Do not attempt UCE if there are blocks only reachable from OSR

Carrying over r+ from bhackett

Comment 11

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6cd87d15a492

Comment 12

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42a9972a1667

Comment 13

[Jan de Mooij [:jandem]]

We should backport this to aurora and beta, it's causing browser crashes.

Comment 14

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b576d83f89f

Comment 15

[Niko Matsakis [:nmatsakis]]

Comment on attachment 774658 [details] [diff] [review]
Do not attempt UCE if there are blocks only reachable from OSR

[Approval Request Comment]
Regression caused by (bug #): 820676
User impact if declined: Crashes
Testing completed (on m-c, etc.): Added regression test
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch:

Comment 16

[Alex Keybl [:akeybl]]

Not a top crash, so the highest we'll take this is on Aurora. No need to track, but we'll approve for Aurora once it's had a little bake time.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42a9972a1667
https://hg.mozilla.org/mozilla-central/rev/7b576d83f89f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7255bae7b635
https://hg.mozilla.org/releases/mozilla-aurora/rev/218c4145c066

Comment 20

[Paul Silaghi, QA [:pauly]]

Reproduced in nightly 2013-06-06.
Verified fixed jshell-win32 FF 24b6.

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed jshell-win32 FF 25b2.