Bug 20911 – REGRESSION(r36480?): Reproducible assertion failure below derefStructureIDs 64-bit JavaScriptCore

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

After building jsc 64-bit:

> function f() { a } a; f()
ASSERTION FAILED: !m_deletionHasBegun
(./wtf/RefCounted.h:47 void WTF::RefCounted<T>::deref() [with T = JSC::StructureID])

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00000000bbadbeef
0x00000001000c89a6 in WTF::RefCounted<JSC::StructureID>::deref (this=0x100000000) at RefCounted.h:47
47	        ASSERT(!m_deletionHasBegun);
(gdb) bt
#0  0x00000001000c89a6 in WTF::RefCounted<JSC::StructureID>::deref (this=0x100000000) at RefCounted.h:47
#1  0x000000010011b246 in JSC::CodeBlock::derefStructureIDs (this=0x1006175b0, vPC=0x100617850) at JavaScriptCore/VM/CodeBlock.cpp:938
#2  0x000000010011b367 in JSC::CodeBlock::~CodeBlock (this=0x1006175b0) at JavaScriptCore/VM/CodeBlock.cpp:898
#3  0x00000001000b42f5 in JSC::ProgramCodeBlock::~ProgramCodeBlock (this=0x1006175b0) at CodeBlock.h:297
#4  0x00000001000b430d in JSC::ProgramCodeBlock::~ProgramCodeBlock (this=0x1006175b0) at CodeBlock.h:297
#5  0x00000001000c3536 in WTF::deleteOwnedPtr<JSC::ProgramCodeBlock> (ptr=0x1006175b0) at OwnPtr.h:51
#6  0x00000001000c3562 in WTF::OwnPtr<JSC::ProgramCodeBlock>::~OwnPtr (this=0x100830c40) at OwnPtr.h:69
#7  0x00000001000c3583 in WTF::OwnPtr<JSC::ProgramCodeBlock>::~OwnPtr (this=0x100830c40) at OwnPtr.h:69
#8  0x00000001000c3a4e in JSC::ProgramNode::~ProgramNode (this=0x100830a00) at nodes.h:2195
#9  0x000000010003d925 in JSC::ParserRefCounted::deref (this=0x100830a00) at nodes.cpp:107
#10 0x00000001000b9979 in WTF::RefPtr<JSC::ProgramNode>::~RefPtr (this=0x7fff5fbff450) at RefPtr.h:50
#11 0x00000001000b9991 in WTF::RefPtr<JSC::ProgramNode>::~RefPtr (this=0x7fff5fbff450) at RefPtr.h:50
#12 0x0000000100073c86 in JSC::Interpreter::evaluate (exec=0x100608d70, scopeChain=@0x100608c20, sourceURL=@0x10000f650, startingLineNumber=1, source=@0x7fff5fbff570, thisValue=0x0) at interpreter.cpp:90
#13 0x0000000100073d16 in JSC::Interpreter::evaluate (exec=0x100608d70, scopeChain=@0x100608c20, sourceURL=@0x10000f650, startingLineNumber=1, code=@0x7fff5fbff5d0, thisV=0x0) at interpreter.cpp:62
#14 0x0000000100001459 in runInteractive (globalObject=0x100560000) at JavaScriptCore/kjs/Shell.cpp:381
#15 0x0000000100002aa2 in jscmain (argc=1, argv=0x7fff5fbff728, globalData=0x100809400) at JavaScriptCore/kjs/Shell.cpp:479
#16 0x0000000100002b1b in main (argc=1, argv=0x7fff5fbff728) at JavaScriptCore/kjs/Shell.cpp:307
(gdb) 

This results in a crash in release build that breaks most JavaScript execution and all JSCore tests.

[Mark Rowe (bdash)]

Code inside CodeGenerator::emitResolve is doing:
        instructions().append(0);

This appends an Instruction with "operand" set to zero, which leaves the high bits of the Instruction unset.  This causes a comparison with 0 to fail when we access the value via the "structureID" member of the union, as the high bits of the value contain non-zero data.

[Mark Rowe (bdash)]

Created attachment 23524 [details]
Make "operand" member pointer-sized

The attached patch makes the "operand" member of the union pointer-sized, which ensures that the high bits of the union will also be set when initializing an Instruction with 0.  This fixes the assertion failures and crashes in the layout tests.

I don't have time to perf test this change tonight.  I wouldn't expect it to have any effect in 32-bit, but it may have some effect in 64-bit.

[Mark Rowe (bdash)]

There is other code in CodeGenerator that does instructions().append(0);, so this change may fix other yet-to-be-diagnosed problems too.

[Stephanie Lewis]

I tested the perf.  The last good build on jsc 64 bit I found was 36479.  Here are the 64 bit results
TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.011x as slow*  986.1ms +/- 0.3%   997.2ms +/- 1.3%     significant

=============================================================================

  3d:                  ??                155.4ms +/- 0.8%   155.8ms +/- 0.8%     not conclusive: might be *1.002x as slow*
    cube:              ??                 53.0ms +/- 0.3%    53.2ms +/- 0.8%     not conclusive: might be *1.005x as slow*
    morph:             -                  59.3ms +/- 1.7%    59.3ms +/- 1.1% 
    raytrace:          ??                 43.2ms +/- 0.4%    43.4ms +/- 0.6%     not conclusive: might be *1.003x as slow*

  access:              *1.029x as slow*   99.9ms +/- 0.4%   102.8ms +/- 4.8%     significant
    binary-trees:      ??                 17.3ms +/- 1.3%    17.6ms +/- 3.6%     not conclusive: might be *1.014x as slow*
    fannkuch:          *1.020x as slow*   17.4ms +/- 1.3%    17.7ms +/- 1.2%     significant
    nbody:             *1.034x as slow*   56.6ms +/- 0.4%    58.5ms +/- 7.3%     significant
    nsieve:            ??                  8.7ms +/- 2.6%     9.0ms +/- 5.3%     not conclusive: might be *1.040x as slow*

  bitops:              *1.020x as slow*   63.2ms +/- 0.6%    64.5ms +/- 1.6%     significant
    3bit-bits-in-byte: ??                 10.7ms +/- 2.1%    10.8ms +/- 2.5%     not conclusive: might be *1.019x as slow*
    bits-in-byte:      *1.031x as slow*   12.9ms +/- 1.1%    13.3ms +/- 4.0%     significant
    bitwise-and:       ??                 21.6ms +/- 1.3%    21.6ms +/- 1.4%     not conclusive: might be *1.005x as slow*
    nsieve-bits:       *1.030x as slow*   18.1ms +/- 0.8%    18.6ms +/- 2.7%     significant

  controlflow:         ??                 15.3ms +/- 1.5%    15.6ms +/- 1.8%     not conclusive: might be *1.013x as slow*
    recursive:         ??                 15.3ms +/- 1.5%    15.6ms +/- 1.8%     not conclusive: might be *1.013x as slow*

  crypto:              *1.021x as slow*   64.0ms +/- 0.4%    65.3ms +/- 2.0%     significant
    aes:               *1.027x as slow*   18.6ms +/- 1.3%    19.1ms +/- 3.4%     significant
    md5:               ??                 23.1ms +/- 0.7%    23.5ms +/- 4.1%     not conclusive: might be *1.015x as slow*
    sha1:              *1.023x as slow*   22.2ms +/- 0.9%    22.7ms +/- 2.4%     significant

  date:                *1.008x as slow*   95.3ms +/- 0.4%    96.1ms +/- 1.0%     significant
    format-tofte:      ??                 43.0ms +/- 0.5%    43.1ms +/- 0.7%     not conclusive: might be *1.002x as slow*
    format-xparb:      *1.012x as slow*   52.3ms +/- 0.4%    53.0ms +/- 1.4%     significant

  math:                ??                127.7ms +/- 1.1%   128.9ms +/- 1.5%     not conclusive: might be *1.010x as slow*
    cordic:            *1.013x as slow*   38.8ms +/- 0.7%    39.3ms +/- 1.6%     significant
    partial-sums:      ??                 63.2ms +/- 1.4%    64.0ms +/- 1.7%     not conclusive: might be *1.013x as slow*
    spectral-norm:     -                  25.8ms +/- 4.1%    25.7ms +/- 1.7% 

  regexp:              ??                 48.1ms +/- 1.0%    48.5ms +/- 1.5%     not conclusive: might be *1.006x as slow*
    dna:               ??                 48.1ms +/- 1.0%    48.5ms +/- 1.5%     not conclusive: might be *1.006x as slow*

  string:              *1.009x as slow*  317.1ms +/- 0.3%   319.9ms +/- 1.0%     significant
    base64:            -                  24.6ms +/- 1.1%    24.6ms +/- 1.4% 
    fasta:             ??                 59.0ms +/- 0.9%    59.1ms +/- 1.4%     not conclusive: might be *1.003x as slow*
    tagcloud:          *1.014x as slow*  111.8ms +/- 0.6%   113.3ms +/- 1.3%     significant
    unpack-code:       ??                 70.3ms +/- 0.8%    70.7ms +/- 0.8%     not conclusive: might be *1.006x as slow*
    validate-input:    ??                 51.5ms +/- 1.3%    52.1ms +/- 2.0%     not conclusive: might be *1.014x as slow*


and the 32 bit results 

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.019x as slow*  984.5ms +/- 0.3%   1003.2ms +/- 1.3%     significant

=============================================================================

  3d:                  *1.014x as slow*  155.8ms +/- 0.4%    157.9ms +/- 1.7%     significant
    cube:              *1.016x as slow*   53.1ms +/- 0.8%     54.0ms +/- 2.0%     significant
    morph:             *1.028x as slow*   58.5ms +/- 0.4%     60.2ms +/- 2.3%     significant
    raytrace:          -                  44.0ms +/- 0.7%     43.7ms +/- 1.0% 

  access:              *1.010x as slow*   99.6ms +/- 0.7%    100.6ms +/- 0.6%     significant
    binary-trees:      -                  17.1ms +/- 2.4%     17.0ms +/- 0.9% 
    fannkuch:          -                  17.8ms +/- 1.4%     17.6ms +/- 1.6% 
    nbody:             *1.019x as slow*   56.0ms +/- 0.6%     57.0ms +/- 0.9%     significant
    nsieve:            ??                  8.8ms +/- 2.2%      8.9ms +/- 2.1%     not conclusive: might be *1.017x as slow*

  bitops:              ??                 63.6ms +/- 0.8%     64.1ms +/- 1.6%     not conclusive: might be *1.007x as slow*
    3bit-bits-in-byte: 1.103x as fast     11.8ms +/- 1.6%     10.7ms +/- 3.2%     significant
    bits-in-byte:      *1.024x as slow*   12.7ms +/- 1.7%     13.0ms +/- 1.6%     significant
    bitwise-and:       *1.028x as slow*   21.4ms +/- 1.1%     22.0ms +/- 2.8%     significant
    nsieve-bits:       *1.037x as slow*   17.7ms +/- 1.9%     18.4ms +/- 1.5%     significant

  controlflow:         ??                 15.6ms +/- 1.8%     15.7ms +/- 1.7%     not conclusive: might be *1.010x as slow*
    recursive:         ??                 15.6ms +/- 1.8%     15.7ms +/- 1.7%     not conclusive: might be *1.010x as slow*

  crypto:              ??                 64.0ms +/- 0.9%     64.7ms +/- 1.2%     not conclusive: might be *1.011x as slow*
    aes:               ??                 18.6ms +/- 1.3%     18.6ms +/- 1.5%     not conclusive: might be *1.003x as slow*
    md5:               ??                 23.1ms +/- 1.8%     23.5ms +/- 1.6%     not conclusive: might be *1.013x as slow*
    sha1:              ??                 22.2ms +/- 1.1%     22.6ms +/- 1.6%     not conclusive: might be *1.016x as slow*

  date:                *1.025x as slow*   95.1ms +/- 0.3%     97.4ms +/- 2.8%     significant
    format-tofte:      ??                 43.0ms +/- 0.4%     43.0ms +/- 1.1%     not conclusive: might be *1.002x as slow*
    format-xparb:      *1.043x as slow*   52.1ms +/- 0.3%     54.4ms +/- 4.8%     significant

  math:                *1.027x as slow*  126.8ms +/- 0.8%    130.3ms +/- 2.4%     significant
    cordic:            ??                 40.3ms +/- 0.5%     40.8ms +/- 6.4%     not conclusive: might be *1.011x as slow*
    partial-sums:      *1.041x as slow*   61.1ms +/- 0.9%     63.6ms +/- 1.3%     significant
    spectral-norm:     ??                 25.4ms +/- 1.7%     25.9ms +/- 1.6%     not conclusive: might be *1.018x as slow*

  regexp:              ??                 48.2ms +/- 0.9%     48.9ms +/- 1.6%     not conclusive: might be *1.013x as slow*
    dna:               ??                 48.2ms +/- 0.9%     48.9ms +/- 1.6%     not conclusive: might be *1.013x as slow*

  string:              *1.025x as slow*  316.1ms +/- 0.7%    323.9ms +/- 1.7%     significant
    base64:            ??                 24.5ms +/- 1.2%     24.6ms +/- 2.1%     not conclusive: might be *1.006x as slow*
    fasta:             *1.021x as slow*   59.0ms +/- 1.2%     60.3ms +/- 1.9%     significant
    tagcloud:          *1.021x as slow*  111.7ms +/- 0.8%    114.0ms +/- 1.5%     significant
    unpack-code:       *1.037x as slow*   69.7ms +/- 0.9%     72.3ms +/- 2.2%     significant
    validate-input:    *1.029x as slow*   51.1ms +/- 1.3%     52.6ms +/- 3.0%     significant

[Stephanie Lewis]

ignore the 32bit results they are actually 64bit.  32bit does not have a regression

[Geoffrey Garen]

Surprising that the size of the instruction stream matters to performance with CTI enabled, since CTI doesn't use the instruction stream.

To avoid the performance controversy, we could just prohibit initialization of an instruction using an integral value, and force the initializing code to specify a type, in this case, Instruction*.

[Mark Rowe (bdash)]

I don't think this is a regression with CTI enabled based on Stephanie's comment in comment #5.  The 64-bit regression will probably also go away once we make CTI work for x86_64.

[Oliver Hunt]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/VM/Instruction.h
Committed r36637