Bytecode should not have responsibility for determining how to perform non-local resolves
Created attachment 168762 [details] Patch
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.
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
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.
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+.
(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.
Committed r131516: <http://trac.webkit.org/changeset/131516>
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
Forgot to mention the failed cases: 10 of the JavaScriptCore tests. One example is "ecma/String/15.5.4.11-1.js"
And it mad 500+ tests crash on 32 bit platforms - https://bugs.webkit.org/show_bug.cgi?id=99545
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....
GTK buildfix landed in http://trac.webkit.org/changeset/131550
Reverted r131516 for reason: It caused zillion different problem on different platforms Committed r131552: <http://trac.webkit.org/changeset/131552>
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
Committed r131645: <http://trac.webkit.org/changeset/131645>
Was rolled out by Oliver in <http://trac.webkit.org/changeset/131676>.
Committed r131822: <http://trac.webkit.org/changeset/131822>
(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.
I filed a new bug report to fix the regression you caused: https://bugs.webkit.org/show_bug.cgi?id=99349
(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.
> 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.
(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