Changeset 184960 in webkit


Ignore:
Timestamp:
May 28, 2015 1:57:48 PM (9 years ago)
Author:
benjamin@webkit.org
Message:

[iOS8][ARMv7(s)] Optimized Object.create in 'use strict' context sometimes breaks.
https://bugs.webkit.org/show_bug.cgi?id=138038

Reviewed by Michael Saboff.

TL;DR: sometimes the baseline JIT could accidentally nuke the tag before calling

to C++, making put_by_id behave erratically.

The bug was that put_by_id would randomly not work correctly in 32bits. It happened
in the baseline JIT if we were unlucky enough:
-The code get hot enough and the structure is stable so we get a fast path for

put_by_id.

-We repatch the fast-path branch with a stub generated by

emitPutTransitionStubAndGetOldStructure().

-In emitPutTransitionStubAndGetOldStructure(), we only preserve the payload of the base

register, the tag register is ignored.

-emitPutTransitionStubAndGetOldStructure() allocate 2 to 3 registers. Any of those

could be the one used for the base's tag before the fast path and the value is trashed.

-If we hit one of the failure case, we fallback to the slow path, but we destroyed

the tag pointer.

-We now have unrelated bits in the tag, the most likely value type is now "double"

and we fail the put_by_id because we try to set a property on a number.

The most obvious solution would be to change emitPutTransitionStubAndGetOldStructure()
to preserve the tag register in addition to the value register.
I decided against that option because of the added complexity. The DFG does not need
that case, so I would have to add branches everywhere to distinguish the cases
were we need to preserve the tag or not.

Instead, I just load the tag back from memory in the slow path. The function in the slow
path is several order of magnitude slower than a load, it is not worth eliminating it,
especially in baseline JIT.

I also discovered 4 useless loads in the fast path, so even with my extra load, this patch
makes the baseline faster :)

  • jit/JITPropertyAccess32_64.cpp:

(JSC::JIT::emitSlow_op_put_by_id):
(JSC::JIT::emit_op_put_by_id): Deleted.

  • tests/stress/put-by-id-on-new-object-after-prototype-transition-non-strict.js: Added.

(opaqueNewObject):
(putValueOnNewObject):

  • tests/stress/put-by-id-on-new-object-after-prototype-transition-strict.js: Added.

(string_appeared_here.opaqueNewObject):
(putValueOnNewObject):

Location:
trunk/Source/JavaScriptCore
Files:
2 added
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r184959 r184960  
     12015-05-28  Benjamin Poulain  <benjamin@webkit.org>
     2
     3        [iOS8][ARMv7(s)] Optimized Object.create in 'use strict' context sometimes breaks.
     4        https://bugs.webkit.org/show_bug.cgi?id=138038
     5
     6        Reviewed by Michael Saboff.
     7
     8        TL;DR: sometimes the baseline JIT could accidentally nuke the tag before calling
     9               to C++, making put_by_id behave erratically.
     10
     11        The bug was that put_by_id would randomly not work correctly in 32bits. It happened
     12        in the baseline JIT if we were unlucky enough:
     13        -The code get hot enough and the structure is stable so we get a fast path for
     14         put_by_id.
     15        -We repatch the fast-path branch with a stub generated by
     16         emitPutTransitionStubAndGetOldStructure().
     17        -In emitPutTransitionStubAndGetOldStructure(), we only preserve the payload of the base
     18         register, the tag register is ignored.
     19        -emitPutTransitionStubAndGetOldStructure() allocate 2 to 3 registers. Any of those
     20         could be the one used for the base's tag before the fast path and the value is trashed.
     21        -If we hit one of the failure case, we fallback to the slow path, but we destroyed
     22         the tag pointer.
     23        -We now have unrelated bits in the tag, the most likely value type is now "double"
     24         and we fail the put_by_id because we try to set a property on a number.
     25
     26        The most obvious solution would be to change emitPutTransitionStubAndGetOldStructure()
     27        to preserve the tag register in addition to the value register.
     28        I decided against that option because of the added complexity. The DFG does not need
     29        that case, so I would have to add branches everywhere to distinguish the cases
     30        were we need to preserve the tag or not.
     31
     32        Instead, I just load the tag back from memory in the slow path. The function in the slow
     33        path is several order of magnitude slower than a load, it is not worth eliminating it,
     34        especially in baseline JIT.
     35
     36        I also discovered 4 useless loads in the fast path, so even with my extra load, this patch
     37        makes the baseline faster :)
     38
     39        * jit/JITPropertyAccess32_64.cpp:
     40        (JSC::JIT::emitSlow_op_put_by_id):
     41        (JSC::JIT::emit_op_put_by_id): Deleted.
     42        * tests/stress/put-by-id-on-new-object-after-prototype-transition-non-strict.js: Added.
     43        (opaqueNewObject):
     44        (putValueOnNewObject):
     45        * tests/stress/put-by-id-on-new-object-after-prototype-transition-strict.js: Added.
     46        (string_appeared_here.opaqueNewObject):
     47        (putValueOnNewObject):
     48
    1492015-05-28  Benjamin Poulain  <benjamin@webkit.org>
    250
  • trunk/Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp

    r184324 r184960  
    536536   
    537537    emitJumpSlowCaseIfNotJSCell(base, regT1);
    538    
    539     emitLoad(base, regT1, regT0);
    540     emitLoad(value, regT3, regT2);
    541538
    542539    JITPutByIdGenerator gen(
     
    560557   
    561558    Label coldPathBegin(this);
    562    
     559
     560    // JITPutByIdGenerator only preserve the value and the base's payload, we have to reload the tag.
     561    emitLoadTag(base, regT1);
     562
    563563    JITPutByIdGenerator& gen = m_putByIds[m_putByIdIndex++];
    564564   
Note: See TracChangeset for help on using the changeset viewer.