Bug 99349 – Bytecode should not have responsibility for determining how to perform non-local resolves

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Bytecode should not have responsibility for determining how to perform non-local resolves

[Oliver Hunt]

Created attachment 168762 [details]
Patch

[Eric Seidel (no email)]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1980:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3000:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3001:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3002:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3008:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3009:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 51 files


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

[Build Bot]

Comment on attachment 168762 [details]
Patch

Attachment 168762 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14296866

New failing tests:
fast/workers/worker-event-listener.html

[Gavin Barraclough]

Comment on attachment 168762 [details]
Patch

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

> Source/JavaScriptCore/bytecode/ResolveOperation.h:153
> +    enum Kind { Uninitialised, Generic, Readonly, GlobalVariablePut, GlobalVariablePutChecked, GlobalPropertyPut, VariablePut };

ResolveOperation <-> ResolveOperationType
PutToBaseOperation <-> PutToBaseOperation::Kind

Seems like these could be more consistent.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1411
>      // We can't optimise at all :-(

I think this comment is now misleading – we don't try to optimize at this stage, so no need for sadface.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1425
>      // We can't optimise at all :-(

ditto

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1472
>      switch (resolveResult.type()) {

I think this switch only handles two cases, and does so in the same way.
Can we replace the switch with:
    ASSERT(resolveResult.type() == ResolveResult::Register || resolveResult.type() == ResolveResult::ReadOnlyRegister);
?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1874
> +bool ByteCodeParser::parseResolveOperations(SpeculatedType prediction, unsigned identifier, unsigned operations, unsigned putToBaseOperation, NodeIndex* base, NodeIndex* value)

In cases where parseResolveOperations returns false, no nodes may be inserted into the graph that would reference the values of base & value.  This makes me think that in cases where we hit one of the forced OSR exits, the base or value could have been incorrectly removed by DCE.  What prevents this from happening?, could we add test cases to cover this?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3039
> +                addToGraph(PutById, OpInfo(identifier), get(base), get(value));

Personally, I like a break at the end of a switch, to avoid accidental fall-through if someone adds an extra case.  Just a thought.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3088
> +                set(valueDst, addToGraph(GarbageValue));

What's the purpose of these garbage values? - please comment.

> Source/JavaScriptCore/dfg/DFGCapabilities.h:123
> +        return CanCompile;

return CannotCompile; ?
Otherwise, this entire function can be much simpler!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:196
> +    while (nextNode->codeOrigin == at(m_compileIndex).codeOrigin) {

Is this code in this patch to handle the fact that a resolve node may have multiple register results? – a comment would help.

[Gavin Barraclough]

Comment on attachment 168762 [details]
Patch

My one major concern was the DCE thing – could omitting inserting any node that references base/value result in these values not being generated & handed back out the the baseline JIT, in the cases this patch forceOSRExits.  Since I'm out of office tomorrow, please discuss with Filip – if he agrees with you that this is safe as is, r+.

[Oliver Hunt]

(In reply to comment #4)
> (From update of attachment 168762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168762&action=review
> 
> > Source/JavaScriptCore/bytecode/ResolveOperation.h:153
> > +    enum Kind { Uninitialised, Generic, Readonly, GlobalVariablePut, GlobalVariablePutChecked, GlobalPropertyPut, VariablePut };
> 
> ResolveOperation <-> ResolveOperationType
> PutToBaseOperation <-> PutToBaseOperation::Kind
> 
> Seems like these could be more consistent.

Yup. Fixed. 
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1411
> >      // We can't optimise at all :-(
> 
> I think this comment is now misleading – we don't try to optimize at this stage, so no need for sadface.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1425
> >      // We can't optimise at all :-(
> 
> ditto

Fixed.

> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1472
> >      switch (resolveResult.type()) {
> 
> I think this switch only handles two cases, and does so in the same way.
> Can we replace the switch with:
>     ASSERT(resolveResult.type() == ResolveResult::Register || resolveResult.type() == ResolveResult::ReadOnlyRegister);
> ?

Done.

> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1874
> > +bool ByteCodeParser::parseResolveOperations(SpeculatedType prediction, unsigned identifier, unsigned operations, unsigned putToBaseOperation, NodeIndex* base, NodeIndex* value)
> 
> In cases where parseResolveOperations returns false, no nodes may be inserted into the graph that would reference the values of base & value.  This makes me think that in cases where we hit one of the forced OSR exits, the base or value could have been incorrectly removed by DCE.  What prevents this from happening?, could we add test cases to cover this?

We already do -- now that many resolves end up producing resolve_with_* almost every test the dfg has triggers these code paths, as do a bunch of other random tests in jsc-tests and general layout tests (I verified by adding breakpoints when these paths were hit, and 'lo many tests crashed (EXC_BREAKPOINT ftw!)

wrt to what happens if we have an OSR we come to:

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3088
> > +                set(valueDst, addToGraph(GarbageValue));
> 
> What's the purpose of these garbage values? - please comment.
>

GarbageValue is a node type, it follows the osr exits (as we need to actually produce the results nodes in the graph if we aren't terminating the compilation), and by being a distinct Node we're able to kill all speculation that the CSE and CFA may try to do (which shouldn't matter as they should only exist immediately following a forced osr exit).  If we do get so far as to believe that we should codegen a GarbageValue node, we CRASH() because it means something has gone really, really wrong somewhere.
 
> > Source/JavaScriptCore/dfg/DFGCapabilities.h:123
> > +        return CanCompile;
> 
> return CannotCompile; ?
> Otherwise, this entire function can be much simpler!

Actually the function is completely unnecessary now.

[Oliver Hunt]

Committed r131516: <http://trac.webkit.org/changeset/131516>

[Yuqiang Xian]

This seems to cause some problems if LLINT is disabled (but JIT and DFG_JIT are enabled).

Is this the expected behavior, i.e, supporting baseline JIT and DFG JIT implies that LLINT needs to be supported from now on?

Thanks.


Program received signal SIGSEGV, Segmentation fault.
0x000000000078eb41 in JSC::DFG::ByteCodeParser::parseResolveOperations (this=0x7fffffff8a20, prediction=32768, identifier=1, 
    operations=1, putToBaseOperation=0, base=0x0, value=0x7fffffff7c8c)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2025
2025            CRASH();
(gdb) bt
#0  0x000000000078eb41 in JSC::DFG::ByteCodeParser::parseResolveOperations (this=0x7fffffff8a20, prediction=32768, 
    identifier=1, operations=1, putToBaseOperation=0, base=0x0, value=0x7fffffff7c8c)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2025
#1  0x00000000007941b8 in JSC::DFG::ByteCodeParser::parseBlock (this=0x7fffffff8a20, limit=107)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2966
#2  0x00000000007974a0 in JSC::DFG::ByteCodeParser::parseCodeBlock (this=0x7fffffff8a20)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3664
#3  0x000000000079772b in JSC::DFG::ByteCodeParser::parse (this=0x7fffffff8a20)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3702
#4  0x0000000000797b49 in JSC::DFG::parse (exec=0x7fffb73d9178, graph=...)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3762
#5  0x00000000006376ee in JSC::DFG::compile (compileMode=JSC::DFG::CompileOther, exec=0x7fffb73d9178, codeBlock=0xe03590, 
    jitCode=..., jitCodeWithArityCheck=0x0, osrEntryBytecodeIndex=0)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGDriver.cpp:102
#6  0x00000000006371e0 in JSC::DFG::tryCompile (exec=0x7fffb73d9178, codeBlock=0xe03590, jitCode=..., bytecodeIndex=0)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/dfg/DFGDriver.cpp:168
#7  0x0000000000513942 in JSC::jitCompileIfAppropriate<JSC::EvalCodeBlock> (exec=0x7fffb73d9178, codeBlock=..., jitCode=..., 
    jitType=JSC::JITCode::DFGJIT, bytecodeIndex=0, effort=JSC::JITCompilationCanFail)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/jit/JITDriver.h:57
#8  0x00000000005140c2 in JSC::prepareForExecution<JSC::EvalCodeBlock> (exec=0x7fffb73d9178, codeBlock=..., jitCode=..., 
    jitType=JSC::JITCode::DFGJIT, bytecodeIndex=0)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/runtime/ExecutionHarness.h:49
#9  0x000000000050e674 in JSC::EvalExecutable::compileInternal (this=0x7fffb73ca180, exec=0x7fffb73d9178, 
    scope=0x7fffb725ff60, jitType=JSC::JITCode::DFGJIT, bytecodeIndex=0)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/runtime/Executable.cpp:229
#10 0x000000000050ddab in JSC::EvalExecutable::compileOptimized (this=0x7fffb73ca180, exec=0x7fffb73d9178, 
    scope=0x7fffb725ff60, bytecodeIndex=0) at /home/yuqiang/WebKit/Source/JavaScriptCore/runtime/Executable.cpp:159
#11 0x0000000000612e57 in JSC::EvalCodeBlock::compileOptimized (this=0xdf6ac0, exec=0x7fffb73d9178, scope=0x7fffb725ff60, 
    bytecodeIndex=0) at /home/yuqiang/WebKit/Source/JavaScriptCore/bytecode/CodeBlock.cpp:2603
#12 0x00000000004ec7e8 in JSC::cti_optimize (args=0x7fffffffb610)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/jit/JITStubs.cpp:2087
#13 0x00000000004e8beb in JSC::JITThunks::tryCacheGetByID (callFrame=0xffffb510, 
    codeBlock=0x50ddab <JSC::EvalExecutable::compileOptimized(JSC::ExecState*, JSC::JSScope*, unsigned int)+299>, 
    returnAddress=..., baseValue=..., propertyName=..., slot=..., stubInfo=0x7fff00000000)
    at /home/yuqiang/WebKit/Source/JavaScriptCore/jit/JITStubs.cpp:1035
#14 0x00007fffffffb640 in ?? ()
#15 0x00007fff00000000 in ?? ()
#16 0x0000000000000000 in ?? ()
(gdb) l
2020            NodeIndex getScopeRegisters = addToGraph(GetScopeRegisters, localBase);
2021            *value = addToGraph(GetScopedVar, OpInfo(resolveValueOperation->m_offset), OpInfo(prediction), getScopeRegisters);
2022            return true;
2023        }
2024        default:
2025            CRASH();
2026            return false;
2027        }
2028
2029    }
(gdb) p resolveValueOperation->m_operation
$1 = JSC::ResolveOperation::CheckForDynamicEntriesBeforeGlobalScope

[Yuqiang Xian]

Forgot to mention the failed cases: 10 of the JavaScriptCore tests. One example is
"ecma/String/15.5.4.11-1.js"

[Csaba Osztrogonác]

And it mad 500+ tests crash on 32 bit platforms - https://bugs.webkit.org/show_bug.cgi?id=99545

[Csaba Osztrogonác]

and it broke the GTK build as the EWS noticed you,
but you committed a non-buildable intentionally:

make: *** No rule to make target `Source/JavaScriptCore/bytecode/ResolveOperations.h', needed by `DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h'.  Stop.
make: *** Waiting for unfinished jobs....

[Csaba Osztrogonác]

GTK buildfix landed in http://trac.webkit.org/changeset/131550

[Csaba Osztrogonác]

Reverted r131516 for reason:

It caused zillion different problem on different platforms

Committed r131552: <http://trac.webkit.org/changeset/131552>

[Csaba Osztrogonác]

Additionally it made perf tests crash on 64 bit platforms (Mac, Qt):

Mac crashes:
-------------
Running Dromaeo/dromaeo-object-array.html (47 of 102)
crash: Dromaeo/dromaeo-object-array.html
FAILED
Finished: 25.391377 s

Running Dromaeo/dromaeo-object-regexp.html (48 of 102)
crash: Dromaeo/dromaeo-object-regexp.html
FAILED
Finished: 4.725511 s

Running Dromaeo/dromaeo-object-string.html (49 of 102)
crash: Dromaeo/dromaeo-object-string.html
FAILED
Finished: 40.940793 s

Qt crashes:
------------
Running Dromaeo/cssquery-dojo.html (38 of 102)
error: Dromaeo/cssquery-dojo.html
1   0x7f0e1ddc4668 /home/webkitbuildbot/slaves/release64bit-perf/buildslave/qt-linux-64-release-perf-tests/build/WebKitBuild/Release/lib/libQtWebKitWidgets.so.5(+0x190e668) [0x7f0e1ddc4668]
2   0x7f0e1a292420 /lib/x86_64-linux-gnu/libc.so.6(+0x36420) [0x7f0e1a292420]
3   0x7f0dcf2fc10e [0x7f0dcf2fc10e]

FAILED
Finished: 86.213021 s

Running Dromaeo/dromaeo-object-array.html (47 of 102)
error: Dromaeo/dromaeo-object-array.html
1   0x7f20fa379668 /home/webkitbuildbot/slaves/release64bit-perf/buildslave/qt-linux-64-release-perf-tests/build/WebKitBuild/Release/lib/libQtWebKitWidgets.so.5(+0x190e668) [0x7f20fa379668]
2   0x7f20f6847420 /lib/x86_64-linux-gnu/libc.so.6(+0x36420) [0x7f20f6847420]
3   0x7f20ab8e4317 [0x7f20ab8e4317]

FAILED
Finished: 21.916984 s

Running Dromaeo/dromaeo-object-regexp.html (48 of 102)
error: Dromaeo/dromaeo-object-regexp.html
1   0x7fd7ff2fe668 /home/webkitbuildbot/slaves/release64bit-perf/buildslave/qt-linux-64-release-perf-tests/build/WebKitBuild/Release/lib/libQtWebKitWidgets.so.5(+0x190e668) [0x7fd7ff2fe668]
2   0x7fd7fb7cc420 /lib/x86_64-linux-gnu/libc.so.6(+0x36420) [0x7fd7fb7cc420]
3   0x7fd7b084c293 [0x7fd7b084c293]

FAILED
Finished: 1.047346 s

Running Dromaeo/dromaeo-object-string.html (49 of 102)
error: Dromaeo/dromaeo-object-string.html
1   0x7faaeefaa668 /home/webkitbuildbot/slaves/release64bit-perf/buildslave/qt-linux-64-release-perf-tests/build/WebKitBuild/Release/lib/libQtWebKitWidgets.so.5(+0x190e668) [0x7faaeefaa668]
2   0x7faaeb478420 /lib/x86_64-linux-gnu/libc.so.6(+0x36420) [0x7faaeb478420]
3   0x7faaa04e220d [0x7faaa04e220d]

FAILED
Finished: 86.067940 s

[Oliver Hunt]

Committed r131645: <http://trac.webkit.org/changeset/131645>

[Mark Lam]

Was rolled out by Oliver in <http://trac.webkit.org/changeset/131676>.

[Oliver Hunt]

Committed r131822: <http://trac.webkit.org/changeset/131822>

[Csaba Osztrogonác]

(In reply to comment #17)
> Committed r131822: <http://trac.webkit.org/changeset/131822>
and fix landed in http://trac.webkit.org/changeset/131830 without any reference ...

... and it made 500+ tests crash again on 32 bit platforms (Qt,GTK have 32 bit bots)

I really don't understand why do you commit a buggy patch again and 
again. Please don't do it in the future. Unfortunately I can't roll 
them out now because of conflicts.

[Csaba Osztrogonác]

I filed a new bug report to fix the regression you caused: https://bugs.webkit.org/show_bug.cgi?id=99349

[Csaba Osztrogonác]

(In reply to comment #19)
> I filed a new bug report to fix the regression you caused: https://bugs.webkit.org/show_bug.cgi?id=99349

copy/paste error :) I meant https://bugs.webkit.org/show_bug.cgi?id=99814.

[Oliver Hunt]

> I really don't understand why do you commit a buggy patch again and 
> again. Please don't do it in the future. Unfortunately I can't roll 
> them out now because of conflicts.

Had it occurred to you that I might have thought I had fixed the various bugs in it?  Apparently not.

[Simon Hausmann]

(In reply to comment #21)
> > I really don't understand why do you commit a buggy patch again and 
> > again. Please don't do it in the future. Unfortunately I can't roll 
> > them out now because of conflicts.
> 
> Had it occurred to you that I might have thought I had fixed the various bugs in it?  Apparently not.

Looks like a slight misunderstanding :(

Anyway, thanks Oliver for fixing this! It is appreciated!

I've re-enabled the LLINT for PLATFORM(QT) in r132067