Changeset 178926 in webkit for trunk/Source/JavaScriptCore


Ignore:
Timestamp:
Jan 22, 2015, 11:05:42 AM (10 years ago)
Author:
[email protected]
Message:

BytecodeGenerator::initializeCapturedVariable() sets a misleading value for the 5th operand of op_put_to_scope.
<​https://webkit.org/b/140743>

Reviewed by Oliver Hunt.

Source/JavaScriptCore:

BytecodeGenerator::initializeCapturedVariable() was setting the 5th operand to
op_put_to_scope to an inappropriate value (i.e. 0). As a result, the execution
of put_to_scope could store a wrong inferred value into the VariableWatchpointSet
for which ever captured variable is at local index 0. In practice, this turns
out to be the local for the Arguments object. In this reproduction case in the
bug, the wrong inferred value written there is the boolean true.

Subsequently, DFG compilation occurs and CreateArguments is emitted to first do
a check of the local for the Arguments object. But because that local has a
wrong inferred value, the check always discovers a non-null value and we never
actually create the Arguments object. Immediately after this, an OSR exit
occurs leaving the Arguments object local uninitialized. Later on at arguments
tear off, we run into a boolean true where we had expected to find an Arguments
object, which in turn, leads to the crash.

The fix is to:

  1. In the case where the resolveModeType is LocalClosureVar, change the 5th operand of op_put_to_scope to be a boolean. True means that the local var is watchable. False means it is not watchable. We no longer pass the local index (instead of true) and UINT_MAX (instead of false).

This allows us to express more clearer in the code what that value means,
as well as remove the redundant way of getting the local's identifier.
The identifier is always the one passed in the 2nd operand.

  1. Previously, though intuitively, we know that the watchable variable identifier should be the same as the one that is passed in operand 2, this relationship was not clear in the code. By code analysis, I confirmed that the callers of BytecodeGenerator::emitPutToScope() always use the same identifier for operand 2 and for filling out the ResolveScopeInfo from which we get the watchable variable identifier later. I've changed the code to make this clear now by always using the identifier passed in operand 2.
  1. In the case where the resolveModeType is LocalClosureVar, initializeCapturedVariable() and emitPutToScope() will now query hasWatchableVariable() to determine if the local is watchable or not. Accordingly, we pass the boolean result of hasWatchableVariable() as operand 5 of op_put_to_scope.

Also added some assertions.

  • bytecode/CodeBlock.cpp:

(JSC::CodeBlock::CodeBlock):

  • bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::initializeCapturedVariable):
(JSC::BytecodeGenerator::hasConstant):
(JSC::BytecodeGenerator::emitPutToScope):

  • bytecompiler/BytecodeGenerator.h:

(JSC::BytecodeGenerator::hasWatchableVariable):
(JSC::BytecodeGenerator::watchableVariableIdentifier):
(JSC::BytecodeGenerator::watchableVariable): Deleted.

LayoutTests:

  • js/dfg-osr-exit-between-create-and-tearoff-arguments-expected.txt: Added.
  • js/dfg-osr-exit-between-create-and-tearoff-arguments.html: Added.
  • js/script-tests/dfg-osr-exit-between-create-and-tearoff-arguments.js: Added.

(foo):

Location:
trunk/Source/JavaScriptCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r178918 r178926  
     12015-01-22  Mark Lam  <[email protected]>
     2
     3        BytecodeGenerator::initializeCapturedVariable() sets a misleading value for the 5th operand of op_put_to_scope.
     4        <https://webkit.org/b/140743>
     5
     6        Reviewed by Oliver Hunt.
     7
     8        BytecodeGenerator::initializeCapturedVariable() was setting the 5th operand to
     9        op_put_to_scope to an inappropriate value (i.e. 0).  As a result, the execution
     10        of put_to_scope could store a wrong inferred value into the VariableWatchpointSet
     11        for which ever captured variable is at local index 0.  In practice, this turns
     12        out to be the local for the Arguments object.  In this reproduction case in the
     13        bug, the wrong inferred value written there is the boolean true.
     14
     15        Subsequently, DFG compilation occurs and CreateArguments is emitted to first do
     16        a check of the local for the Arguments object.  But because that local has a
     17        wrong inferred value, the check always discovers a non-null value and we never
     18        actually create the Arguments object.  Immediately after this, an OSR exit
     19        occurs leaving the Arguments object local uninitialized.  Later on at arguments
     20        tear off, we run into a boolean true where we had expected to find an Arguments
     21        object, which in turn, leads to the crash.
     22
     23        The fix is to:
     24        1. In the case where the resolveModeType is LocalClosureVar, change the
     25           5th operand of op_put_to_scope to be a boolean.  True means that the
     26           local var is watchable.  False means it is not watchable.  We no longer
     27           pass the local index (instead of true) and UINT_MAX (instead of false).
     28
     29           This allows us to express more clearer in the code what that value means,
     30           as well as remove the redundant way of getting the local's identifier.
     31           The identifier is always the one passed in the 2nd operand.
     32
     33        2. Previously, though intuitively, we know that the watchable variable
     34           identifier should be the same as the one that is passed in operand 2, this
     35           relationship was not clear in the code.  By code analysis, I confirmed that
     36           the callers of BytecodeGenerator::emitPutToScope() always use the same
     37           identifier for operand 2 and for filling out the ResolveScopeInfo from
     38           which we get the watchable variable identifier later.  I've changed the
     39           code to make this clear now by always using the identifier passed in
     40           operand 2.
     41
     42        3. In the case where the resolveModeType is LocalClosureVar,
     43           initializeCapturedVariable() and emitPutToScope() will now query
     44           hasWatchableVariable() to determine if the local is watchable or not.
     45           Accordingly, we pass the boolean result of hasWatchableVariable() as
     46           operand 5 of op_put_to_scope.
     47
     48        Also added some assertions.
     49
     50        * bytecode/CodeBlock.cpp:
     51        (JSC::CodeBlock::CodeBlock):
     52        * bytecompiler/BytecodeGenerator.cpp:
     53        (JSC::BytecodeGenerator::initializeCapturedVariable):
     54        (JSC::BytecodeGenerator::hasConstant):
     55        (JSC::BytecodeGenerator::emitPutToScope):
     56        * bytecompiler/BytecodeGenerator.h:
     57        (JSC::BytecodeGenerator::hasWatchableVariable):
     58        (JSC::BytecodeGenerator::watchableVariableIdentifier):
     59        (JSC::BytecodeGenerator::watchableVariable): Deleted.
     60
    1612015-01-22  Ryosuke Niwa  <[email protected]>
    262

  • trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

    r178692 r178926  
    19991999            ResolveModeAndType modeAndType = ResolveModeAndType(pc[4].u.operand);
    20002000            if (modeAndType.type() == LocalClosureVar) {
    2001                 if (pc[5].u.index == UINT_MAX) {
    2002                     instructions[i + 5].u.watchpointSet = 0;
     2001                bool isWatchableVariable = pc[5].u.operand;
     2002                if (!isWatchableVariable) {
     2003                    instructions[i + 5].u.watchpointSet = nullptr;
    20032004                    break;
    20042005                }
    2005                 StringImpl* uid = identifier(pc[5].u.index).impl();
     2006                StringImpl* uid = ident.impl();
    20062007                RELEASE_ASSERT(didCloneSymbolTable);
    20072008                if (ident != m_vm->propertyNames->arguments) {

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

    r178894 r178926  
    518518    instructions().append(value->index());
    519519    instructions().append(ResolveModeAndType(ThrowIfNotFound, LocalClosureVar).operand());
    520     instructions().append(0);
     520    int operand = registerFor(dst->index()).index();
     521    bool isWatchableVariable = hasWatchableVariable(operand);
     522    ASSERT(!isWatchableVariable || watchableVariableIdentifier(operand) == propertyName);
     523    instructions().append(isWatchableVariable);
    521524    instructions().append(dst->index());
    522525    return dst;
    … …  
    9991002}
    10001003
     1004bool BytecodeGenerator::hasConstant(const Identifier& ident) const
     1005{
     1006    StringImpl* rep = ident.impl();
     1007    return m_identifierMap.contains(rep);
     1008}
     1009   
    10011010unsigned BytecodeGenerator::addConstant(const Identifier& ident)
    10021011{
    … …  
    13971406    if (info.isLocal()) {
    13981407        instructions().append(ResolveModeAndType(resolveMode, LocalClosureVar).operand());
    1399         instructions().append(watchableVariable(registerFor(info.localIndex()).index()));
     1408        int operand = registerFor(info.localIndex()).index();
     1409        bool isWatchableVariable = hasWatchableVariable(operand);
     1410        ASSERT(!isWatchableVariable || watchableVariableIdentifier(operand) == identifier);
     1411        instructions().append(isWatchableVariable);
    14001412    } else {
     1413        ASSERT(resolveType() != LocalClosureVar);
    14011414        instructions().append(ResolveModeAndType(resolveMode, resolveType()).operand());
    1402         instructions().append(0);
     1415        instructions().append(false);
    14031416    }
    14041417    instructions().append(info.localIndex());

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

    r178882 r178926  
    653653        }
    654654
     655        bool hasConstant(const Identifier&) const;
    655656        unsigned addConstant(const Identifier&);
    656657        RegisterID* addConstantValue(JSValue);
    … …  
    727728        RegisterID* createLazyRegisterIfNecessary(RegisterID*);
    728729       
    729         unsigned watchableVariable(int operand)
     730        bool hasWatchableVariable(int operand) const
    730731        {
    731732            VirtualRegister reg(operand);
    732733            if (!reg.isLocal())
    733                 return UINT_MAX;
     734                return false;
    734735            if (static_cast<size_t>(reg.toLocal()) >= m_watchableVariables.size())
    735                 return UINT_MAX;
    736             Identifier& ident = m_watchableVariables[reg.toLocal()];
     736                return false;
     737            const Identifier& ident = m_watchableVariables[reg.toLocal()];
    737738            if (ident.isNull())
    738                 return UINT_MAX;
    739             return addConstant(ident);
    740         }
    741        
    742         bool hasWatchableVariable(int operand)
    743         {
    744             return watchableVariable(operand) != UINT_MAX;
     739                return false;
     740            ASSERT(hasConstant(ident)); // Should have already been added.
     741            return true;
     742        }
     743       
     744        const Identifier& watchableVariableIdentifier(int operand) const
     745        {
     746            ASSERT(hasWatchableVariable(operand));
     747            VirtualRegister reg(operand);
     748            return m_watchableVariables[reg.toLocal()];
    745749        }
    746750
Note: See TracChangeset for help on using the changeset viewer.