Bug 109933 – JSC asserting on division in debug mode ARM traditional

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Gabor Rapcsanyi]

JSC asserting on graph validation after the fixup phase because of a division operation.
When fixupNode() function creates the newDivision node it increases the m_refCount of it with the copied node refcounts.

DFGFixupPhase:338
        Node* newDivision = m_insertionSet.insertNode(
                    m_indexInBlock, DontRefChildren, RefNode, SpecDouble, *node);
        node->setOp(DoubleAsInt32);
        node->children.initialize(Edge(newDivision, DoubleUse), Edge(), Edge());


The problem is that the addNode() function copies the references of the node and then puts on two more.

DFGGraph.h:207
        Node* node = new (m_allocator) Node(valueArgs); \
        node->predict(type); \
        if (node->flags() & NodeMustGenerate) \
            node->ref(); \
        if (refNodeMode == RefNode) \
            node->ref(); \


After this fixup phase the validate() function is asserts when it checks the nodes refcounts.

DFGValidate.cpp:151
        V_EQUAL((node), myRefCounts.get(node), node->adjustedRefCount());


Is this the expected behavior of addNode() function?

A possible workaround could be to decrease the refcounts of the newDivision with the original node refcounts after the node creation like this:

        newDivision->setRefCount(newDivision->refCount() - node->refCount());

[Gabor Rapcsanyi]

Created attachment 188555 [details]
test.js

ASSERTION FAILED: myRefCounts.get(node) == node->adjustedRefCount()
/home/rgabor/WebKit/Source/JavaScriptCore/dfg/DFGValidate.cpp(151) : void JSC::DFG::Validate::validate()

[Gabor Rapcsanyi]

Created attachment 188560 [details]
proposed fix

[Gabor Rapcsanyi]

This bug is valid on ARMv7 as well.

[Filip Pizlo]

Comment on attachment 188560 [details]
proposed fix

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

I don't think that setting the ref count like this is a good idea. Can you explain the justification, beyond just "it makes the assertions go away"?

Also this code look like it is now obsolete. We don't do ref counting on nodes in ToT.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:317
> +                newDivision->setRefCount(newDivision->refCount() - node->refCount());

This is almost certainly wrong.

[Gabor Rapcsanyi]

(In reply to comment #4)
> (From update of attachment 188560 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188560&action=review
> 
> I don't think that setting the ref count like this is a good idea. Can you explain the justification, beyond just "it makes the assertions go away"?
> 
> Also this code look like it is now obsolete. We don't do ref counting on nodes in ToT.
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:317
> > +                newDivision->setRefCount(newDivision->refCount() - node->refCount());
> 
> This is almost certainly wrong.

Yes, you are right this is obsolete now. I tried this test on the newest revision and the problem is gone.