Bug 122462 – sunspider-1.0/math-spectral-norm.js.dfg-eager occasionally fails with Trap 5 (i.e int $3)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

I see it when running with Tools/Scripts/run-javascriptcore-tests --debug --ftl-jit

[Mark Lam]

I'm seeing SIGTRAPs reloading https://github.com/WebKit/webkit while debugging https://bugs.webkit.org/show_bug.cgi?id=122445.  Could be the same issue.  FYI.

[Mark Lam]

(In reply to comment #1)
> I'm seeing SIGTRAPs reloading https://github.com/WebKit/webkit while debugging https://bugs.webkit.org/show_bug.cgi?id=122445.  Could be the same issue.  FYI.

Sorry.  The interesting point here is that I'm not running with the FTL JIT.

[Filip Pizlo]

(In reply to comment #2)
> (In reply to comment #1)
> > I'm seeing SIGTRAPs reloading https://github.com/WebKit/webkit while debugging https://bugs.webkit.org/show_bug.cgi?id=122445.  Could be the same issue.  FYI.
> 
> Sorry.  The interesting point here is that I'm not running with the FTL JIT.

No.  The test fails in the DFG.  So it could be the same issue.

I only point out that I'm building with --ftl-jit since this is a heisenbug that triggers very rarely, so building with the FTL JIT *might* have something to do with it.  Or it might not.  Who knows - it's a heisenbug.  But it's not an FTL bug because that test definitely doesn't tier-up to the FTL.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > I'm seeing SIGTRAPs reloading https://github.com/WebKit/webkit while debugging https://bugs.webkit.org/show_bug.cgi?id=122445.  Could be the same issue.  FYI.
> > 
> > Sorry.  The interesting point here is that I'm not running with the FTL JIT.
> 
> No.  The test fails in the DFG.  So it could be the same issue.
> 
> I only point out that I'm building with --ftl-jit since this is a heisenbug that triggers very rarely, so building with the FTL JIT *might* have something to do with it.  Or it might not.  Who knows - it's a heisenbug.  But it's not an FTL bug because that test definitely doesn't tier-up to the FTL.

I should also note that a trap is the way that the DFG does JIT assertions.  So saying that your bug is the same as this bug because it traps in JIT code is like saying that some ASSERT() failure that you saw is a duplicate of another ASSERT() failure just because both bugs involved ASSERT().

[Mark Lam]

(In reply to comment #4)
> I should also note that a trap is the way that the DFG does JIT assertions.  So saying that your bug is the same as this bug because it traps in JIT code is like saying that some ASSERT() failure that you saw is a duplicate of another ASSERT() failure just because both bugs involved ASSERT().

I see.  So, the two may not be related at all.  Thanks for the clarification.

[Filip Pizlo]

*** Bug 122665 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

This looks like a missing type check.  Even according to the abstract state, the first operand of the DFG GetByVal Int32 is not known to have Int32Shape.

[Filip Pizlo]

The type check is being eliminated by constant folding.

[Filip Pizlo]

Here's what's happening: DFG OSR entry checks if it can enter at a given loop by looking at what the DFG's abstract interpreter (AI) proved about all values on the stack.  If the values on the stack don't contravene the proof, then we can enter.

But to increase the likelihood that the AI won't construct a proof that makes it impossible for the current values on the stack to "fit", the AI will inject the values on the stack that were seen at the beginning of compilation at some basic block that is the DFG's guess of where we'll OSR enter.  This is through the DFG::Plan::mustHandleValues.

Unfrotunately, mustHandleValues are JSValues.  The DFG AI does the injection by converting the JSValues into AbstractValues, and it may do this more than once.  But the JSValues may be heap pointers, so each time you do it, you might get different structures, resulting in contradictory AbstractValues.  Hence multiple runs of the DFG AI may contract each other, leading to one run of AI removing type checks for some type T and another run of AI deciding that some different type S is legal; OSR entry then feels that it's OK to enter with type S, even though the graph requires type T.

[Filip Pizlo]

Fixing that appears to reduce the likelihood of crashes, but there is still something else going on that leads to type check being removed.

[Filip Pizlo]

Oh boo, in some cases we're proving that the only way to enter the loop is via OSR.  And then we're constant folding every use of the variable whose type morphs.  The constant folder is now folding based on the AbstractValue from before when the type morphed.

[Filip Pizlo]

Well, the constant folder has a really fun bug: lets say we've proven that a node has a constant object value and that object must have a particular structure.  And then we replace that node with a constant.

That's wrong.

The JSConstant that we introduce into the graph could then have any structure, and previously we would have removed checks for the particular structure.

The solution is to not constant fold objects if we've proven a particular structure.

[Filip Pizlo]

Created attachment 214033 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 214033 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214033&action=review

r=me

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:342
> +            // we've proven for this node wouldn't widen the proof. If so, then we cannot

Aren't we checking if it *would* widen the proof, and then skipping over if it does? As I understand it, proofs are supposed to get narrower and narrower as we prove more and more things, right?

[Filip Pizlo]

(In reply to comment #14)
> (From update of attachment 214033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214033&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:342
> > +            // we've proven for this node wouldn't widen the proof. If so, then we cannot
> 
> Aren't we checking if it *would* widen the proof, and then skipping over if it does? As I understand it, proofs are supposed to get narrower and narrower as we prove more and more things, right?

Yeah I meant to say something exactly like what you said.  I'll change the text to:

            // Check if merging the abstract value of the constant into the abstract value
            // we've proven for this node wouldn't widen the proof. If it widens the proof
            // (i.e. says that the set contains more things in it than it previously did)
            // then we refuse to fold.

[Mark Hahnenberg]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 214033 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214033&action=review
> > 
> > r=me
> > 
> > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:342
> > > +            // we've proven for this node wouldn't widen the proof. If so, then we cannot
> > 
> > Aren't we checking if it *would* widen the proof, and then skipping over if it does? As I understand it, proofs are supposed to get narrower and narrower as we prove more and more things, right?
> 
> Yeah I meant to say something exactly like what you said.  I'll change the text to:
> 
>             // Check if merging the abstract value of the constant into the abstract value
>             // we've proven for this node wouldn't widen the proof. If it widens the proof
>             // (i.e. says that the set contains more things in it than it previously did)
>             // then we refuse to fold.

Sounds good to me.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/157327

[Nadav Rotem]

Thanks. This test was also failing for me from time to time.