Bug 151839 – [ES6] Add support for Symbol.hasInstance

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

[ES6] Add support for Symbol.hasInstance

[Keith Miller]

Created attachment 266587 [details]
Patch

[Keith Miller]

Created attachment 266588 [details]
Benchmark results

64-bit benchmark results.

[Keith Miller]

Created attachment 266823 [details]
Patch

[Keith Miller]

Created attachment 266830 [details]
Patch

[WebKit Commit Bot]

Attachment 266830 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 266833 [details]
Patch

[WebKit Commit Bot]

Attachment 266833 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        [WIP] Still need to do 32-bit, add tests, and cleanup but should be roughly done.

Need to delete this.

[Saam Barati]

Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        needs non-default behavior for resolving the expression. i.e. The constructor has a Symbol.hasInstance that differs from the one on

From reading this sentence I would think that jsTrue=>non-default behavior and that jsFalse=>default behavior but it's the opposite.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2191
> +    instructions().append(hasInstanceSymbol->index());

Nit: I think the name hasInstanceSymbol is a bit misleading here (and in other places) because it's the result of doing a GetById with the hasInstanceSymbol and not the symbol itself.
Maybe something like "symbolHasInstanceResult" or "hasInstanceSymbolResult" or just "hasInstanceFunction"?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:603
> +        RegisterID* emitIsFunction(RegisterID* dst, RegisterID* src);

This isn't called

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4222
> +        if (!hasInstanceSymbolNode->isCellConstant()) {
> +
> +            notDefault = m_jit.branchPtr(MacroAssembler::NotEqual, hasInstanceSymbol.gpr(), TrustedImmPtr(defaultHasInstanceFunction));
> +        }

Style: This can be an "if" with no curly braces.

> Source/JavaScriptCore/jit/JSInterfaceJIT.h:165
> +    ALWAYS_INLINE JSInterfaceJIT::JumpList JSInterfaceJIT::emitJumpIfNull(JSValueRegs reg, GPRReg scratch1, GPRReg scratch2) {

This really jumps if it's undefined or null, right? If so, we should name it emitJumpIfNullOrUndefined

> Source/JavaScriptCore/llint/LLIntData.cpp:158
> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));

Why is this here?

[Keith Miller]

Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2191
>> +    instructions().append(hasInstanceSymbol->index());
> 
> Nit: I think the name hasInstanceSymbol is a bit misleading here (and in other places) because it's the result of doing a GetById with the hasInstanceSymbol and not the symbol itself.
> Maybe something like "symbolHasInstanceResult" or "hasInstanceSymbolResult" or just "hasInstanceFunction"?

How do you feel about hasInstanceValue (since it may or may not be a function)?

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:603
>> +        RegisterID* emitIsFunction(RegisterID* dst, RegisterID* src);
> 
> This isn't called

Deleted.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4222
>> +        }
> 
> Style: This can be an "if" with no curly braces.

Fixed.

>> Source/JavaScriptCore/llint/LLIntData.cpp:158
>> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));
> 
> Why is this here?

This verifies that the offset of the jsvalue in a WriteBarrier is at the zero offset.

[Keith Miller]

> >> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));
> > 
> > Why is this here?
> 
> This verifies that the offset of the jsvalue in a WriteBarrier is at the
> zero offset.

I should mention that we use this fact in the LLInt as part of the ptr test on Function.prototype[Symbol.hasInstance].

[Geoffrey Garen]

Need updated es6.yaml results.

Please rename "check has instance" to "overrides has instance".

[Keith Miller]

Created attachment 266927 [details]
Patch

[Keith Miller]

Created attachment 267058 [details]
Patch

[WebKit Commit Bot]

Attachment 267058 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

r=me with comments

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
> +    // Note that we intentionally clobber the hasInstanceValue register here on x86 because we
> +    // are short on registers. This is valid because we know InstanceOfCustom is the last use
> +    // of the GetById which produces it.

This seems a bit sketchy to me and also probably unnecessary.
We probably don't care much about X86.
Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2997
> +    MacroAssembler::Jump slowCase = m_jit.jump();

I don't think you need a variable for this (or above).

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5780
> +        // Unlike, the DFG we don't worry about cleaning this code up for the case where we have proven the hasInstanceValue is a constant as LLVM should fix it for us.

Style: "Unlike, the DFG" => "Unlike in the DFG,"

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));

I think you should have unlikely(continuation) and likely(defaultHasInstance)

> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
> +    int proto = currentInstruction[3].u.operand;

style: You call this "proto" here and "constructor" below. I say just go w/ constructor.

> Source/JavaScriptCore/runtime/JSTypeInfo.h:41
> +static const unsigned OverridesHasInstanceFlag = 1 << 2; // FixMe: This is only trivially used by the runtime and should be removed: https://bugs.webkit.org/show_bug.cgi?id=152005

style: FixMe => FIXME

> LayoutTests/js/Object-getOwnPropertyNames-expected.txt:61
>  PASS getSortedOwnPropertyNames(Error.prototype) is ['constructor', 'message', 'name', 'toString']

What happened to all the other tests?

[Saam Barati]

Also, have you run perf tests?
Are they neutral? Can you post them?

[Keith Miller]

(In reply to comment #17)
> Also, have you run perf tests?
> Are they neutral? Can you post them?

They are attached above and are neutral (The !s are on tests that don't use instanceof).

[Keith Miller]

Created attachment 267064 [details]
Benchmark results

More recent benchmark results.

[Keith Miller]

Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
>> +    // of the GetById which produces it.
> 
> This seems a bit sketchy to me and also probably unnecessary.
> We probably don't care much about X86.
> Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.

I suppose I could add a function that returns an int32_t or something that contains the boolean result of the slow path but I'm not sure that feels any less sketchy.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2997
>> +    MacroAssembler::Jump slowCase = m_jit.jump();
> 
> I don't think you need a variable for this (or above).

While you are correct in this case, I dealt with bugs related assigning a gpr after a jump due to calls occurring in the wrong order. I'd feel a bit safer when the order is extra explicit in this context.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5780
>> +        // Unlike, the DFG we don't worry about cleaning this code up for the case where we have proven the hasInstanceValue is a constant as LLVM should fix it for us.
> 
> Style: "Unlike, the DFG" => "Unlike in the DFG,"

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
>> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));
> 
> I think you should have unlikely(continuation) and likely(defaultHasInstance)

I had that at one point but this node is almost never generated unless JS code uses a custom hasInstance in some form. So it makes sense to not punish users when they have a custom hasInstance.

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
>> +    int proto = currentInstruction[3].u.operand;
> 
> style: You call this "proto" here and "constructor" below. I say just go w/ constructor.

This is just the diff being laid out poorly... All of this code is just deleted emitSlow_op_check_has_instance being mixed with existing emitSlow_op_instanceof that I did not change... "proto" is actually the prototype of the constructor not the constructor itself.

>> Source/JavaScriptCore/runtime/JSTypeInfo.h:41
>> +static const unsigned OverridesHasInstanceFlag = 1 << 2; // FixMe: This is only trivially used by the runtime and should be removed: https://bugs.webkit.org/show_bug.cgi?id=152005
> 
> style: FixMe => FIXME

Fixed.

>> LayoutTests/js/Object-getOwnPropertyNames-expected.txt:61
>>  PASS getSortedOwnPropertyNames(Error.prototype) is ['constructor', 'message', 'name', 'toString']
> 
> What happened to all the other tests?

Ughh... I forgot to add them... I'll re-upload another version with them again.

[Keith Miller]

Created attachment 267069 [details]
Patch

[Saam Barati]

Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
>>> +    // of the GetById which produces it.
>> 
>> This seems a bit sketchy to me and also probably unnecessary.
>> We probably don't care much about X86.
>> Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.
> 
> I suppose I could add a function that returns an int32_t or something that contains the boolean result of the slow path but I'm not sure that feels any less sketchy.

Maybe just have one path for everything then?
Up to you.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
>>> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));
>> 
>> I think you should have unlikely(continuation) and likely(defaultHasInstance)
> 
> I had that at one point but this node is almost never generated unless JS code uses a custom hasInstance in some form. So it makes sense to not punish users when they have a custom hasInstance.

makes sense

>>> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
>>> +    int proto = currentInstruction[3].u.operand;
>> 
>> style: You call this "proto" here and "constructor" below. I say just go w/ constructor.
> 
> This is just the diff being laid out poorly... All of this code is just deleted emitSlow_op_check_has_instance being mixed with existing emitSlow_op_instanceof that I did not change... "proto" is actually the prototype of the constructor not the constructor itself.

oops I missed that. ok

[WebKit Commit Bot]

Attachment 267069 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 267142 [details]
Patch

[WebKit Commit Bot]

Attachment 267142 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

After some investigation, I think you are right that we should use a non-encodedjsvalue to hold the result of the custom slow path. It looks like we use int32_t for that so I changed JIT_OPERATION to return an int32_t. If you want to give it another quick look over, I would appreciate it.

[Saam Barati]

Comment on attachment 267142 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 267142 [details]
Patch

Clearing flags on attachment: 267142

Committed r193974: <http://trac.webkit.org/changeset/193974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #28)
> Comment on attachment 267142 [details]
> Patch
> 
> Clearing flags on attachment: 267142
> 
> Committed r193974: <http://trac.webkit.org/changeset/193974>

It made many JSC tests crash on Linux bots:
- GTK X86_64 - https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/12637
- GTK X86 - https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/57694
- EFL X86_64 - https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25878

[Csaba Osztrogonác]

(In reply to comment #30)
> (In reply to comment #28)
> > Comment on attachment 267142 [details]
> > Patch
> > 
> > Clearing flags on attachment: 267142
> > 
> > Committed r193974: <http://trac.webkit.org/changeset/193974>
> 
> It made many JSC tests crash on Linux bots:
> - GTK X86_64 -
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12637
> - GTK X86 -
> https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/57694
> - EFL X86_64 -
> https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> builds/25878

- EFL Linux AArch64: https://build.webkit.org/builders/EFL%20Linux%20AArch64%20Release/builds/4700
- GTK ARMv7(?): https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/9652

(EFL ARMv7 bots work fine.)

[Csaba Osztrogonác]

I filed a new bug report to fix this regression: bug152245

[Carlos Alberto Lopez Perez]

The debug bots are giving this assertion since r193974:


ASSERTION FAILED: i < m_numBits
../../Source/WTF/wtf/FastBitVector.h(151) : void WTF::FastBitVector::set(size_t)
WTFCrashWithSecurityImplication(...)


Example:

[...]
stress/instanceof-not-cell.js.no-llint: ASSERTION FAILED: i < m_numBits
stress/instanceof-not-cell.js.no-llint: ../../Source/WTF/wtf/FastBitVector.h(151) : void WTF::FastBitVector::set(size_t)
stress/instanceof-not-cell.js.no-llint: 1   0x7f58e30974ae /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrashWithSecurityImplication+0x1e) [0x7f58e30974ae]
stress/instanceof-not-cell.js.no-llint: 2   0x7f58e2719b66 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF13FastBitVector3setEm+0x42) [0x7f58e2719b66]
stress/instanceof-not-cell.js.no-llint: 3   0x7f58e2716964 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7964) [0x7f58e2716964]
stress/instanceof-not-cell.js.no-llint: 4   0x7f58e2717a96 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e8a96) [0x7f58e2717a96]
stress/instanceof-not-cell.js.no-llint: 5   0x7f58e2718543 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e9543) [0x7f58e2718543]
stress/instanceof-not-cell.js.no-llint: 6   0x7f58e2717af5 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e8af5) [0x7f58e2717af5]
stress/instanceof-not-cell.js.no-llint: 7   0x7f58e27169dc /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e79dc) [0x7f58e27169dc]
stress/instanceof-not-cell.js.no-llint: 8   0x7f58e2716ae6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7ae6) [0x7f58e2716ae6]
stress/instanceof-not-cell.js.no-llint: 9   0x7f58e2716bad /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7bad) [0x7f58e2716bad]
stress/instanceof-not-cell.js.no-llint: 10  0x7f58e2716de5 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysis19runLivenessFixpointEv+0x231) [0x7f58e2716de5]
stress/instanceof-not-cell.js.no-llint: 11  0x7f58e27179b9 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysis7computeEv+0x6b) [0x7f58e27179b9]
stress/instanceof-not-cell.js.no-llint: 12  0x7f58e271665a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysisC1EPNS_9CodeBlockE+0x68) [0x7f58e271665a]
stress/instanceof-not-cell.js.no-llint: 13  0x7f58e2951e6a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZSt11make_uniqueIN3JSC24BytecodeLivenessAnalysisEIPNS0_9CodeBlockEEENSt10_Unique_ifIT_E14_Single_objectEDpOT0_+0x3a) [0x7f58e2951e6a]
stress/instanceof-not-cell.js.no-llint: 14  0x7f58e295024d /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC9CodeBlock16livenessAnalysisEv+0x91) [0x7f58e295024d]
stress/instanceof-not-cell.js.no-llint: 15  0x7f58e294d53d /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph11livenessForEPNS_9CodeBlockE+0xd9) [0x7f58e294d53d]
stress/instanceof-not-cell.js.no-llint: 16  0x7f58e294d643 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph11livenessForEPNS_15InlineCallFrameE+0x35) [0x7f58e294d643]
stress/instanceof-not-cell.js.no-llint: 17  0x7f58e294d9a1 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph16isLiveInBytecodeENS_15VirtualRegisterENS_10CodeOriginE+0x179) [0x7f58e294d9a1]
stress/instanceof-not-cell.js.no-llint: 18  0x7f58e2a2af02 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fbf02) [0x7f58e2a2af02]
stress/instanceof-not-cell.js.no-llint: 19  0x7f58e2a2acf6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fbcf6) [0x7f58e2a2acf6]
stress/instanceof-not-cell.js.no-llint: 20  0x7f58e2a2a8be /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fb8be) [0x7f58e2a2a8be]
stress/instanceof-not-cell.js.no-llint: 21  0x7f58e2a2b753 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fc753) [0x7f58e2a2b753]
stress/instanceof-not-cell.js.no-llint: 22  0x7f58e2a2b173 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fc173) [0x7f58e2a2b173]
stress/instanceof-not-cell.js.no-llint: 23  0x7f58e2a2ad60 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG23performPhantomInsertionERNS0_5GraphE+0x2b) [0x7f58e2a2ad60]
stress/instanceof-not-cell.js.no-llint: 24  0x7f58e2a2f940 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG4Plan19compileInThreadImplERNS0_14LongLivedStateE+0x5c6) [0x7f58e2a2f940]
stress/instanceof-not-cell.js.no-llint: 25  0x7f58e2a2efdf /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG4Plan15compileInThreadERNS0_14LongLivedStateEPNS0_10ThreadDataE+0x165) [0x7f58e2a2efdf]
stress/instanceof-not-cell.js.no-llint: 26  0x7f58e2b1a9af /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG8Worklist9runThreadEPNS0_10ThreadDataE+0x2d7) [0x7f58e2b1a9af]
stress/instanceof-not-cell.js.no-llint: 27  0x7f58e2b1ad5c /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG8Worklist14threadFunctionEPv+0x2a) [0x7f58e2b1ad5c]
stress/instanceof-not-cell.js.no-llint: 28  0x7f58e30b2598 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a83598) [0x7f58e30b2598]
stress/instanceof-not-cell.js.no-llint: 29  0x7f58e30b2748 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a83748) [0x7f58e30b2748]
stress/instanceof-not-cell.js.no-llint: 30  0x7f58e2b3d046 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZNKSt8functionIFvvEEclEv+0x32) [0x7f58e2b3d046]
stress/instanceof-not-cell.js.no-llint: 31  0x7f58e30b247a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a8347a) [0x7f58e30b247a]
stress/instanceof-not-cell.js.no-llint: Segmentation fault
stress/instanceof-not-cell.js.no-llint: ERROR: Unexpected exit code: 139
[...]

More at: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/6385/steps/jscore-test/logs/stdio

[Chris Dumez]

Reopening as it was rolled out in <http://trac.webkit.org/changeset/194036> for causing crashes.

[Keith Miller]

Created attachment 267579 [details]
Patch

[WebKit Commit Bot]

Attachment 267579 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:490:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 267579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267579&action=review

r=me again

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:76
> +        ASSERT(opcodeLengths[opcodeID] >= 1);

All these asserts should be > and not >=.

[Keith Miller]

Comment on attachment 267579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267579&action=review

>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:76
>> +        ASSERT(opcodeLengths[opcodeID] >= 1);
> 
> All these asserts should be > and not >=.

Fixed.

[Keith Miller]

Committed r194248: <http://trac.webkit.org/changeset/194248>

[Joseph Pecoraro]

*** Bug 151120 has been marked as a duplicate of this bug. ***