Changeset 55379 in webkit


Ignore:
Timestamp:
Mar 1, 2010 2:14:22 PM (14 years ago)
Author:
barraclough@apple.com
Message:

Bug 35537 - put_by_id does will incorrectly cache writes where a specific value exists,

where at the point of caching the same value is being written.

Reviewed by Oliver Hunt.

JavaScriptCore:

When performing a put_by_id that is replacing a property already present on the object,
there are three interesting cases regarding the state of the specific value:

(1) No specific value set - nothing to do, leave the structure in it's current state,

can cache.

(2) A specific value was set, the new put is not of a specified value (i.e. function),

or is of a different specific value - in these cases we need to perform a despecifying
transition to clear the specific value in the structure, but having done so this is a
normal property so as such we can again cache normally.

(3) A specific value was set, and we are overwriting with the same value - in these cases

leave the structure unchanged, but since a specific value is set we cannot cache this
put (we would need the JIT to dynamically check the value being written matched).

Unfortunately, the current behaviour does not match this. the checks for a specific value
being present & the value matching are combined in such a way that in case (2), above we
will unnecessarily prevent the transition being cached, but in case (3) we will incorrectly
fail to prevent caching.

The bug exposes itself if multiple puts of the same specific value are performed to a
property, and erroneously the put is allowed to be cached by the JIT. Method checks may be
generated caching calls of this structure. Subsequent puts performed from JIT code may
write different values without triggering a despecify transition, and as such cached method
checks will continue to pass, despite the value having changed.

  • runtime/JSObject.h:

(JSC::JSObject::putDirectInternal):

LayoutTests:

Add test case.

  • fast/js/method-check-expected.txt:
  • fast/js/script-tests/method-check.js:

(addOne):
(addOneHundred):
(totalizer.makeCall):

Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/ChangeLog

    r55367 r55379  
     12010-03-01  Gavin Barraclough  <barraclough@apple.com>
     2
     3        Reviewed by Oliver Hunt.
     4
     5        Bug 35537 - put_by_id does will incorrectly cache writes where a specific value exists,
     6                    where at the point of caching the same value is being written.
     7
     8        When performing a put_by_id that is replacing a property already present on the object,
     9        there are three interesting cases regarding the state of the specific value:
     10
     11        (1) No specific value set - nothing to do, leave the structure in it's current state,
     12            can cache.
     13        (2) A specific value was set, the new put is not of a specified value (i.e. function),
     14            or is of a different specific value - in these cases we need to perform a despecifying
     15            transition to clear the specific value in the structure, but having done so this is a
     16            normal property so as such we can again cache normally.
     17        (3) A specific value was set, and we are overwriting with the same value - in these cases
     18            leave the structure unchanged, but since a specific value is set we cannot cache this
     19            put (we would need the JIT to dynamically check the value being written matched).
     20
     21        Unfortunately, the current behaviour does not match this.  the checks for a specific value
     22        being present & the value matching are combined in such a way that in case (2), above we
     23        will unnecessarily prevent the transition being cached, but in case (3) we will incorrectly
     24        fail to prevent caching.
     25
     26        The bug exposes itself if multiple puts of the same specific value are performed to a
     27        property, and erroneously the put is allowed to be cached by the JIT.  Method checks may be
     28        generated caching calls of this structure.  Subsequent puts performed from JIT code may
     29        write different values without triggering a despecify transition, and as such cached method
     30        checks will continue to pass, despite the value having changed.
     31
     32        * runtime/JSObject.h:
     33        (JSC::JSObject::putDirectInternal):
     34
    1352010-03-01  Tor Arne Vestbø  <tor.arne.vestbo@nokia.com>
    236
    … …  
    686720        rope & string elements, in a fashion similar to the recently-removed
    687721        PtrAndFlags class (see https://bugs.webkit.org/show_bug.cgi?id=33731 ).  Again,
    688         this causes a problem for Leaks – refactor to remove the magic pointer
     722        this causes a problem for Leaks - refactor to remove the magic pointer
    689723        mangling.
    690724
    … …  
    81628196
    81638197        The ARMv7 JIT is currently using ARMv7Assembler::ConditionEQ to branch
    8164         for DoubleEqualOrUnordered, however this is incorrect – ConditionEQ won't
     8198        for DoubleEqualOrUnordered, however this is incorrect - ConditionEQ won't
    81658199        branch on unordered operands.  Similarly, DoubleLessThanOrUnordered &
    81668200        DoubleLessThanOrEqualOrUnordered use ARMv7Assembler::ConditionLO &

  • trunk/JavaScriptCore/runtime/JSObject.h

    r54100 r55379  
    437437        size_t offset = m_structure->get(propertyName, currentAttributes, currentSpecificFunction);
    438438        if (offset != WTF::notFound) {
     439            // If there is currently a specific function, and there now either isn't,
     440            // or the new value is different, then despecify.
    439441            if (currentSpecificFunction && (specificFunction != currentSpecificFunction))
    440442                m_structure->despecifyDictionaryFunction(propertyName);
    … …  
    442444                return;
    443445            putDirectOffset(offset, value);
    444             if (!specificFunction && !currentSpecificFunction)
     446            // At this point, the objects structure only has a specific value set if previously there
     447            // had been one set, and if the new value being specified is the same (otherwise we would
     448            // have despecified, above).  So, if currentSpecificFunction is not set, or if the new
     449            // value is different (or there is no new value), then the slot now has no value - and
     450            // as such it is cachable.
     451            // If there was previously a value, and the new value is the same, then we cannot cache.
     452            if (!currentSpecificFunction || (specificFunction != currentSpecificFunction))
    445453                slot.setExistingProperty(this, offset);
    446454            return;
    … …  
    469477        setStructure(structure.release());
    470478        putDirectOffset(offset, value);
    471         // See comment on setNewProperty call below.
     479        // This is a new property; transitions with specific values are not currently cachable,
     480        // so leave the slot in an uncachable state.
    472481        if (!specificFunction)
    473482            slot.setNewProperty(this, offset);
    … …  
    482491            return;
    483492
    484         if (currentSpecificFunction && (specificFunction != currentSpecificFunction)) {
     493        // There are three possibilities here:
     494        //  (1) There is an existing specific value set, and we're overwriting with *the same value*.
     495        //       * Do nothing – no need to despecify, but that means we can't cache (a cached
     496        //         put could write a different value). Leave the slot in an uncachable state.
     497        //  (2) There is a specific value currently set, but we're writing a different value.
     498        //       * First, we have to despecify.  Having done so, this is now a regular slot
     499        //         with no specific value, so go ahead & cache like normal.
     500        //  (3) Normal case, there is no specific value set.
     501        //       * Go ahead & cache like normal.
     502        if (currentSpecificFunction) {
     503            // case (1) Do the put, then return leaving the slot uncachable.
     504            if (specificFunction == currentSpecificFunction) {
     505                putDirectOffset(offset, value);
     506                return;
     507            }
     508            // case (2) Despecify, fall through to (3).
    485509            setStructure(Structure::despecifyFunctionTransition(m_structure, propertyName));
    486             putDirectOffset(offset, value);
    487             // Function transitions are not currently cachable, so leave the slot in an uncachable state.
    488             return;
    489         }
     510        }
     511
     512        // case (3) set the slot, do the put, return.
     513        slot.setExistingProperty(this, offset);
    490514        putDirectOffset(offset, value);
    491         slot.setExistingProperty(this, offset);
    492515        return;
    493516    }
    … …  
    511534    setStructure(structure.release());
    512535    putDirectOffset(offset, value);
    513     // Function transitions are not currently cachable, so leave the slot in an uncachable state.
     536    // This is a new property; transitions with specific values are not currently cachable,
     537    // so leave the slot in an uncachable state.
    514538    if (!specificFunction)
    515539        slot.setNewProperty(this, offset);

  • trunk/LayoutTests/ChangeLog

    r55376 r55379  
     12010-03-01  Gavin Barraclough  <barraclough@apple.com>
     2
     3        Reviewed by Oliver Hunt.
     4
     5        Bug 35537 - put_by_id does will incorrectly cache writes where a specific value exists,
     6                    where at the point of caching the same value is being written.
     7
     8        Add test case.
     9
     10        * fast/js/method-check-expected.txt:
     11        * fast/js/script-tests/method-check.js:
     12        (addOne):
     13        (addOneHundred):
     14        (totalizer.makeCall):
     15
    1162010-03-01  Kenneth Russell  <kbr@google.com>
    217

  • trunk/LayoutTests/fast/js/method-check-expected.txt

    r46982 r55379  
    1 This test yields PASS, if malloc does not reuse the memory address for the structure of String prototype
     1Test method-check related bugs
    22
    33On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
    44
    55
     6PASS total is 200
    67PASS successfullyParsed is true
    78

  • trunk/LayoutTests/fast/js/script-tests/method-check.js

    r48651 r55379  
    11description(
    2 "This test yields PASS, if malloc does not reuse the memory address for the structure of String prototype"
     2"Test method-check related bugs"
    33);
    44
    55function func2() { }
    66
     7// This test yields PASS, if malloc does not reuse the memory address for the structure of String prototype
    78function func()
    89{
    … …  
    3132func()
    3233
     34// Test that method caching correctly invalidates (doesn't incorrectly continue to call a previously cached function).
     35var total = 0;
     36function addOne()
     37{
     38    ++total;
     39}
     40function addOneHundred()
     41{
     42    total+=100;
     43}
     44var totalizer = {
     45    makeCall: function(callback)
     46    {
     47        this.callback = callback;
     48        this.callback();
     49    }
     50};
     51for (var i=0; i<100; ++i)
     52    totalizer.makeCall(addOne);
     53totalizer.makeCall(addOneHundred);
     54shouldBe('total', '200');
     55
    3356var successfullyParsed = true;
Note: See TracChangeset for help on using the changeset viewer.