Changeset 142491 in webkit


Ignore:
Timestamp:
Feb 11, 2013 11:21:10 AM (11 years ago)
Author:
fpizlo@apple.com
Message:

DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect to constant folding
​https://bugs.webkit.org/show_bug.cgi?id=109387

Reviewed by Oliver Hunt.

Lock in the decision to use a non-speculative constant comparison as early as possible
and don't let the CFA change it by folding constants. This might be a performance
penalty on some really weird code (FWIW, I haven't seen this on benchmarks), but on
the other hand it completely side-steps the unsoundness that the bug speaks of.

  • dfg/DFGAbstractState.cpp:

(JSC::DFG::AbstractState::execute):

  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::isConstantForCompareStrictEq):
(ByteCodeParser):
(JSC::DFG::ByteCodeParser::parseBlock):

  • dfg/DFGCSEPhase.cpp:

(JSC::DFG::CSEPhase::performNodeCSE):

  • dfg/DFGNodeType.h:

(DFG):

  • dfg/DFGPredictionPropagationPhase.cpp:

(JSC::DFG::PredictionPropagationPhase::propagate):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileStrictEq):

  • dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::compile):

Location:
trunk/Source/JavaScriptCore
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r142489 r142491  
     12013-02-10  Filip Pizlo  <fpizlo@apple.com>
     2
     3        DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect to constant folding
     4        https://bugs.webkit.org/show_bug.cgi?id=109387
     5
     6        Reviewed by Oliver Hunt.
     7       
     8        Lock in the decision to use a non-speculative constant comparison as early as possible
     9        and don't let the CFA change it by folding constants. This might be a performance
     10        penalty on some really weird code (FWIW, I haven't seen this on benchmarks), but on
     11        the other hand it completely side-steps the unsoundness that the bug speaks of.
     12
     13        * dfg/DFGAbstractState.cpp:
     14        (JSC::DFG::AbstractState::execute):
     15        * dfg/DFGByteCodeParser.cpp:
     16        (JSC::DFG::ByteCodeParser::isConstantForCompareStrictEq):
     17        (ByteCodeParser):
     18        (JSC::DFG::ByteCodeParser::parseBlock):
     19        * dfg/DFGCSEPhase.cpp:
     20        (JSC::DFG::CSEPhase::performNodeCSE):
     21        * dfg/DFGNodeType.h:
     22        (DFG):
     23        * dfg/DFGPredictionPropagationPhase.cpp:
     24        (JSC::DFG::PredictionPropagationPhase::propagate):
     25        * dfg/DFGSpeculativeJIT.cpp:
     26        (JSC::DFG::SpeculativeJIT::compileStrictEq):
     27        * dfg/DFGSpeculativeJIT64.cpp:
     28        (JSC::DFG::SpeculativeJIT::compile):
     29
    1302013-02-11  Csaba Osztrogonác  <ossy@webkit.org>
    231

  • trunk/Source/JavaScriptCore/dfg/DFGAbstractState.cpp

    r142481 r142491  
    779779    case CompareGreater:
    780780    case CompareGreaterEq:
    781     case CompareEq: {
     781    case CompareEq:
     782    case CompareEqConstant: {
    782783        bool constantWasSet = false;
    783784
    … …  
    810811        }
    811812       
    812         if (!constantWasSet && node->op() == CompareEq) {
     813        if (!constantWasSet && (node->op() == CompareEqConstant || node->op() == CompareEq)) {
    813814            SpeculatedType leftType = forNode(node->child1()).m_type;
    814815            SpeculatedType rightType = forNode(node->child2()).m_type;
    … …  
    825826       
    826827        forNode(node).set(SpecBoolean);
     828       
     829        if (node->op() == CompareEqConstant) {
     830            // We can exit if we haven't fired the MasqueradesAsUndefind watchpoint yet.
     831            node->setCanExit(m_codeBlock->globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
     832            break;
     833        }
    827834       
    828835        Node* left = node->child1().node();
    … …  
    837844            checker = isNumberSpeculation;
    838845        } else if (node->op() == CompareEq) {
    839             if ((m_graph.isConstant(left) && m_graph.valueOfJSConstant(left).isNull())
    840                 || (m_graph.isConstant(right) && m_graph.valueOfJSConstant(right).isNull())) {
    841                 // We can exit if we haven't fired the MasqueradesAsUndefind watchpoint yet.
    842                 node->setCanExit(m_codeBlock->globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
    843                 break;
    844             }
    845            
    846846            if (left->shouldSpeculateString() || right->shouldSpeculateString()) {
    847847                node->setCanExit(false);
    … …  
    883883    }
    884884           
    885     case CompareStrictEq: {
     885    case CompareStrictEq:
     886    case CompareStrictEqConstant: {
    886887        Node* leftNode = node->child1().node();
    887888        Node* rightNode = node->child2().node();
    … …  
    895896        }
    896897        forNode(node).set(SpecBoolean);
    897         if (m_graph.isJSConstant(leftNode)) {
    898             JSValue value = m_graph.valueOfJSConstant(leftNode);
    899             if (!value.isNumber() && !value.isString()) {
    900                 node->setCanExit(false);
    901                 break;
    902             }
    903         }
    904         if (m_graph.isJSConstant(rightNode)) {
    905             JSValue value = m_graph.valueOfJSConstant(rightNode);
    906             if (!value.isNumber() && !value.isString()) {
    907                 node->setCanExit(false);
    908                 break;
    909             }
     898        if (node->op() == CompareStrictEqConstant) {
     899            node->setCanExit(false);
     900            break;
    910901        }
    911902        if (Node::shouldSpeculateInteger(leftNode, rightNode)) {

  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r142377 r142491  
    685685    }
    686686   
     687    bool isConstantForCompareStrictEq(Node* node)
     688    {
     689        if (!node->isConstant())
     690            return false;
     691        JSValue value = valueOfJSConstant(node);
     692        return value.isBoolean() || value.isUndefinedOrNull();
     693    }
     694   
    687695    // These methods create a node and add it to the graph. If nodes of this type are
    688696    // 'mustGenerate' then the node  will implicitly be ref'ed to ensure generation.
    … …  
    24192427        case op_eq_null: {
    24202428            Node* value = get(currentInstruction[2].u.operand);
    2421             set(currentInstruction[1].u.operand, addToGraph(CompareEq, value, constantNull()));
     2429            set(currentInstruction[1].u.operand, addToGraph(CompareEqConstant, value, constantNull()));
    24222430            NEXT_OPCODE(op_eq_null);
    24232431        }
    … …  
    24352443                }
    24362444            }
    2437             set(currentInstruction[1].u.operand, addToGraph(CompareStrictEq, op1, op2));
     2445            if (isConstantForCompareStrictEq(op1))
     2446                set(currentInstruction[1].u.operand, addToGraph(CompareStrictEqConstant, op2, op1));
     2447            else if (isConstantForCompareStrictEq(op2))
     2448                set(currentInstruction[1].u.operand, addToGraph(CompareStrictEqConstant, op1, op2));
     2449            else
     2450                set(currentInstruction[1].u.operand, addToGraph(CompareStrictEq, op1, op2));
    24382451            NEXT_OPCODE(op_stricteq);
    24392452        }
    … …  
    24572470        case op_neq_null: {
    24582471            Node* value = get(currentInstruction[2].u.operand);
    2459             set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareEq, value, constantNull())));
     2472            set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareEqConstant, value, constantNull())));
    24602473            NEXT_OPCODE(op_neq_null);
    24612474        }
    … …  
    24732486                }
    24742487            }
    2475             set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareStrictEq, op1, op2)));
     2488            Node* invertedResult;
     2489            if (isConstantForCompareStrictEq(op1))
     2490                invertedResult = addToGraph(CompareStrictEqConstant, op2, op1);
     2491            else if (isConstantForCompareStrictEq(op2))
     2492                invertedResult = addToGraph(CompareStrictEqConstant, op1, op2);
     2493            else
     2494                invertedResult = addToGraph(CompareStrictEq, op1, op2);
     2495            set(currentInstruction[1].u.operand, addToGraph(LogicalNot, invertedResult));
    24762496            NEXT_OPCODE(op_nstricteq);
    24772497        }
    … …  
    27272747            unsigned relativeOffset = currentInstruction[2].u.operand;
    27282748            Node* value = get(currentInstruction[1].u.operand);
    2729             Node* condition = addToGraph(CompareEq, value, constantNull());
     2749            Node* condition = addToGraph(CompareEqConstant, value, constantNull());
    27302750            addToGraph(Branch, OpInfo(m_currentIndex + relativeOffset), OpInfo(m_currentIndex + OPCODE_LENGTH(op_jeq_null)), condition);
    27312751            LAST_OPCODE(op_jeq_null);
    … …  
    27352755            unsigned relativeOffset = currentInstruction[2].u.operand;
    27362756            Node* value = get(currentInstruction[1].u.operand);
    2737             Node* condition = addToGraph(CompareEq, value, constantNull());
     2757            Node* condition = addToGraph(CompareEqConstant, value, constantNull());
    27382758            addToGraph(Branch, OpInfo(m_currentIndex + OPCODE_LENGTH(op_jneq_null)), OpInfo(m_currentIndex + relativeOffset), condition);
    27392759            LAST_OPCODE(op_jneq_null);

  • trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

    r142481 r142491  
    11121112        case GetScope:
    11131113        case TypeOf:
     1114        case CompareEqConstant:
    11141115            setReplacement(pureCSE(node));
    11151116            break;

  • trunk/Source/JavaScriptCore/dfg/DFGNodeType.h

    r142377 r142491  
    187187    macro(CompareGreaterEq, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
    188188    macro(CompareEq, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
     189    macro(CompareEqConstant, NodeResultBoolean | NodeMustGenerate) \
    189190    macro(CompareStrictEq, NodeResultBoolean) \
     191    macro(CompareStrictEqConstant, NodeResultBoolean) \
    190192    \
    191193    /* Calls. */\

  • trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp

    r142377 r142491  
    509509        case CompareGreaterEq:
    510510        case CompareEq:
     511        case CompareEqConstant:
    511512        case CompareStrictEq:
     513        case CompareStrictEqConstant:
    512514        case InstanceOf:
    513515        case IsUndefined:

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

    r141962 r142491  
    34613461bool SpeculativeJIT::compileStrictEq(Node* node)
    34623462{
    3463     // 1) If either operand is a constant and that constant is not a double, integer,
    3464     //    or string, then do a JSValue comparison.
    3465    
    3466     if (isJSConstant(node->child1().node())) {
    3467         JSValue value = valueOfJSConstant(node->child1().node());
    3468         if (!value.isNumber() && !value.isString())
    3469             return compileStrictEqForConstant(node, node->child2(), value);
    3470     }
    3471    
    3472     if (isJSConstant(node->child2().node())) {
    3473         JSValue value = valueOfJSConstant(node->child2().node());
    3474         if (!value.isNumber() && !value.isString())
    3475             return compileStrictEqForConstant(node, node->child1(), value);
    3476     }
    3477    
    3478     // 2) If the operands are predicted integer, do an integer comparison.
    3479    
    34803463    if (Node::shouldSpeculateInteger(node->child1().node(), node->child2().node())) {
    34813464        unsigned branchIndexInBlock = detectPeepHoleBranch();
    … …  
    34923475        return false;
    34933476    }
    3494    
    3495     // 3) If the operands are predicted double, do a double comparison.
    34963477   
    34973478    if (Node::shouldSpeculateNumber(node->child1().node(), node->child2().node())) {
    … …  
    35263507        return false;
    35273508    }
    3528    
    3529     // 5) Fall back to non-speculative strict equality.
    35303509   
    35313510    return nonSpeculativeStrictEq(node);

  • trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

    r142377 r142491  
    25142514            return;
    25152515        break;
     2516       
     2517    case CompareEqConstant:
     2518        ASSERT(isNullConstant(node->child2().node()));
     2519        if (nonSpeculativeCompareNull(node, node->child1()))
     2520            return;
     2521        break;
    25162522
    25172523    case CompareEq:
    2518         if (isNullConstant(node->child1().node())) {
    2519             if (nonSpeculativeCompareNull(node, node->child2()))
    2520                 return;
    2521             break;
    2522         }
    2523         if (isNullConstant(node->child2().node())) {
    2524             if (nonSpeculativeCompareNull(node, node->child1()))
    2525                 return;
    2526             break;
    2527         }
    25282524        if (compare(node, JITCompiler::Equal, JITCompiler::DoubleEqual, operationCompareEq))
     2525            return;
     2526        break;
     2527
     2528    case CompareStrictEqConstant:
     2529        if (compileStrictEqForConstant(node, node->child1(), valueOfJSConstant(node->child2().node())))
    25292530            return;
    25302531        break;
Note: See TracChangeset for help on using the changeset viewer.