Bug 106868

Summary:

DFG should not use a graph that is a vector, Nodes shouldn't move after allocation, and we should always refer to nodes by Node*

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ggaren, gtk-ews, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, oliver, rakuco, sam, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

107147, 107381, 107616, 107860, 107996

Bug Blocks:

Attachments:

Description	Flags
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progres	none
work in progress	none
work in progress	none
work in progress	none
WRONG PATCH	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
work in progress	none
it's starting to compile	none
it compiles	none
it runs things	none
1% SunSpider speed-up so far	none
the patch	oliver: review+, buildbot: commit-queue-
patch for landing	buildbot: commit-queue-
patch for landing	none

Filip Pizlo

Reported 2013-01-14 21:04:41 PST

This will be painful.

Attachments
work in progress (49.79 KB, patch) 2013-01-14 21:05 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (78.79 KB, patch) 2013-01-15 16:14 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (92.94 KB, patch) 2013-01-15 16:54 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (129.11 KB, patch) 2013-01-15 19:29 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (148.08 KB, patch) 2013-01-18 00:31 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (143.79 KB, patch) 2013-01-21 00:22 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (264.95 KB, patch) 2013-01-21 15:07 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progres (331.25 KB, patch) 2013-01-22 15:53 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (385.56 KB, patch) 2013-01-22 22:41 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (433.83 KB, patch) 2013-01-23 12:10 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (459.21 KB, patch) 2013-01-23 17:01 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
WRONG PATCH (8.46 KB, patch) 2013-01-24 12:58 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (534.52 KB, patch) 2013-01-24 12:59 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (547.14 KB, patch) 2013-01-24 13:51 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (602.05 KB, patch) 2013-01-26 13:43 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (660.78 KB, patch) 2013-01-26 16:29 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (deleted) 2013-01-26 18:27 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (deleted) 2013-01-27 13:32 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it's starting to compile (964.00 KB, patch) 2013-01-27 19:33 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it compiles (deleted) 2013-01-28 14:05 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it runs things (deleted) 2013-01-28 15:15 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
1% SunSpider speed-up so far (deleted) 2013-01-28 16:36 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (deleted) 2013-01-28 18:24 PST, Filip Pizlo	oliver: review+ buildbot: commit-queue-	Details Formatted Diff Diff
patch for landing (deleted) 2013-01-28 21:36 PST, Filip Pizlo	buildbot: commit-queue-	Details Formatted Diff Diff
patch for landing (1.20 MB, patch) 2013-01-28 22:53 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
Show Obsolete (24) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2013-01-14 21:05:41 PST

Created attachment 182693 [details] work in progress

Filip Pizlo

Comment 2 2013-01-15 16:14:58 PST

Created attachment 182866 [details] work in progress

Filip Pizlo

Comment 3 2013-01-15 16:54:34 PST

Created attachment 182874 [details] work in progress

Filip Pizlo

Comment 4 2013-01-15 19:29:39 PST

Created attachment 182907 [details] work in progress

Filip Pizlo

Comment 5 2013-01-18 00:31:57 PST

Created attachment 183394 [details] work in progress Adding a notion of a NodeIterator, which will make walking over nodes, and inserting nodes, easier and less error-prone.

Filip Pizlo

Comment 6 2013-01-21 00:22:36 PST

Created attachment 183726 [details] work in progress

Filip Pizlo

Comment 7 2013-01-21 15:07:12 PST

Created attachment 183835 [details] work in progress

Filip Pizlo

Comment 8 2013-01-22 15:53:57 PST

Created attachment 184064 [details] work in progres

Filip Pizlo

Comment 9 2013-01-22 22:41:44 PST

Created attachment 184144 [details] work in progress

Filip Pizlo

Comment 10 2013-01-23 12:10:04 PST

Created attachment 184276 [details] work in progress

Filip Pizlo

Comment 11 2013-01-23 17:01:53 PST

Created attachment 184356 [details] work in progress Backing up before I do another rebase.

Filip Pizlo

Comment 12 2013-01-24 12:58:54 PST

Created attachment 184559 [details] WRONG PATCH

Filip Pizlo

Comment 13 2013-01-24 12:59:33 PST

Created attachment 184560 [details] work in progress

Filip Pizlo

Comment 14 2013-01-24 13:51:38 PST

Created attachment 184572 [details] work in progress

Filip Pizlo

Comment 15 2013-01-26 13:43:14 PST

Created attachment 184883 [details] work in progress

Filip Pizlo

Comment 16 2013-01-26 16:29:02 PST

Created attachment 184891 [details] work in progress

Filip Pizlo

Comment 17 2013-01-26 18:27:51 PST

Created attachment 184894 [details] work in progress

Filip Pizlo

Comment 18 2013-01-27 13:32:53 PST

Created attachment 184922 [details] work in progress

Filip Pizlo

Comment 19 2013-01-27 19:33:31 PST

Created attachment 184930 [details] it's starting to compile Haven't done the 32-bit specific parts yet. It doesn't compile yet, but most of the bigger issues appear to be resolved.

Filip Pizlo

Comment 20 2013-01-28 14:05:34 PST

Created attachment 185058 [details] it compiles Crashes on launch, of course

Filip Pizlo

Comment 21 2013-01-28 15:15:17 PST

Created attachment 185080 [details] it runs things Haven't run all of the tests, yet.

Filip Pizlo

Comment 22 2013-01-28 16:36:21 PST

Created attachment 185099 [details] 1% SunSpider speed-up so far

Filip Pizlo

Comment 23 2013-01-28 18:24:50 PST

Created attachment 185127 [details] the patch It passes tests and stuff.

Oliver Hunt

Comment 24 2013-01-28 18:28:34 PST

(In reply to comment #23) > Created an attachment (id=185127) [details] > the patch > > It passes tests and stuff. how's basic browsing? (or is that "and stuff"?) Might be worth trying doing some tests where every additional node causes a realloc to try and confirm that you've got all the possible release after free paths

WebKit Review Bot

Comment 25 2013-01-28 18:29:15 PST

Attachment 185127 [details] did not pass style-queue: Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp:431: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp:708: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGBasicBlock.h:105: The parameter name "valueParams" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGAbstractState.cpp:596: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGAbstractState.cpp:841: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.cpp:341: _node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:343: _childIdx is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:344: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.cpp:443: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:383: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:476: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:478: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:647: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:188: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2853: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGGraph.h:610: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:159: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:166: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:167: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGValidate.cpp:168: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3263: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3264: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3265: Weird number of spaces Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGAdjacencyList.h', u'Source/JavaScriptCore/dfg/DFGAllocator.h', u'Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.h', u'Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGBasicBlockInlines.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCFAPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInsertionSet.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGLongLivedState.cpp', u'Source/JavaScriptCore/dfg/DFGLongLivedState.h', u'Source/JavaScriptCore/dfg/DFGMinifiedID.h', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.cpp', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeAllocator.h', u'Source/JavaScriptCore/dfg/DFGOSRExit.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExit.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', u'Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.h', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/Options.h']" exit_code: 1 at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:581: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:582: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:643: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:644: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:675: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:839: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:942: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:955: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 36 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 26 2013-01-28 18:31:11 PST

(In reply to comment #24) > (In reply to comment #23) > > Created an attachment (id=185127) [details] [details] > > the patch > > > > It passes tests and stuff. > > how's basic browsing? (or is that "and stuff"?) > > Might be worth trying doing some tests where every additional node causes a realloc to try and confirm that you've got all the possible release after free paths What do you mean by realloc? There is no realloc. There is no vector. Nodes don't move.

Oliver Hunt

Comment 27 2013-01-28 18:34:55 PST

(In reply to comment #26) > (In reply to comment #24) > > (In reply to comment #23) > > > Created an attachment (id=185127) [details] [details] [details] > > > the patch > > > > > > It passes tests and stuff. > > > > how's basic browsing? (or is that "and stuff"?) > > > > Might be worth trying doing some tests where every additional node causes a realloc to try and confirm that you've got all the possible release after free paths > > What do you mean by realloc? There is no realloc. There is no vector. Nodes don't move. okie dokie; basic browsing?

Filip Pizlo

Comment 28 2013-01-28 18:37:06 PST

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #24) > > > (In reply to comment #23) > > > > Created an attachment (id=185127) [details] [details] [details] [details] > > > > the patch > > > > > > > > It passes tests and stuff. > > > > > > how's basic browsing? (or is that "and stuff"?) > > > > > > Might be worth trying doing some tests where every additional node causes a realloc to try and confirm that you've got all the possible release after free paths > > > > What do you mean by realloc? There is no realloc. There is no vector. Nodes don't move. > > okie dokie; basic browsing? I just successfully sent myself an e-mail in gmail. In a debug build.

Build Bot

Comment 29 2013-01-28 18:50:14 PST

Comment on attachment 185127 [details] the patch Attachment 185127 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16160841

Oliver Hunt

Comment 30 2013-01-28 19:10:41 PST

Comment on attachment 185127 [details] the patch Could you remove DFG::Allocator::free ? Also wonder if we should make DFG::Allocator require T have a member that shows we know it's safe w.r.t DFG::Allocator, but that's not necessary for this patch.

kov's GTK+ EWS bot

Comment 31 2013-01-28 19:14:42 PST

Comment on attachment 185127 [details] the patch Attachment 185127 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16185037

Filip Pizlo

Comment 32 2013-01-28 19:37:25 PST

(In reply to comment #30) > (From update of attachment 185127 [details]) > Could you remove DFG::Allocator::free ? Also wonder if we should make DFG::Allocator require T have a member that shows we know it's safe w.r.t DFG::Allocator, but that's not necessary for this patch. I was keeping free() because I want to use it in my next patch. I mean, if we're super crazy about not introducing dead code then I should also remove all of the free list logic. I can do it, but I'd rather be able to use Allocator without further changes for the next stuff I want to do (refactor phi building).

Oliver Hunt

Comment 33 2013-01-28 20:56:03 PST

(In reply to comment #32) > (In reply to comment #30) > > (From update of attachment 185127 [details] [details]) > > Could you remove DFG::Allocator::free ? Also wonder if we should make DFG::Allocator require T have a member that shows we know it's safe w.r.t DFG::Allocator, but that's not necessary for this patch. > > I was keeping free() because I want to use it in my next patch. > > I mean, if we're super crazy about not introducing dead code then I should also remove all of the free list logic. > > I can do it, but I'd rather be able to use Allocator without further changes for the next stuff I want to do (refactor phi building). okie dokie, my primary concern is that by switching from node index to node* you expose paths that may not previously have been concerned by it to pointer invalidation

Filip Pizlo

Comment 34 2013-01-28 21:36:39 PST

Created attachment 185150 [details] patch for landing Attempting to fix Windows, Gtk, and style.

Filip Pizlo

Comment 35 2013-01-28 21:38:52 PST

(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #30) > > > (From update of attachment 185127 [details] [details] [details]) > > > Could you remove DFG::Allocator::free ? Also wonder if we should make DFG::Allocator require T have a member that shows we know it's safe w.r.t DFG::Allocator, but that's not necessary for this patch. > > > > I was keeping free() because I want to use it in my next patch. > > > > I mean, if we're super crazy about not introducing dead code then I should also remove all of the free list logic. > > > > I can do it, but I'd rather be able to use Allocator without further changes for the next stuff I want to do (refactor phi building). > > okie dokie, my primary concern is that by switching from node index to node* you expose paths that may not previously have been concerned by it to pointer invalidation Of course we would do so. But my plan is to only call free() on nodes that have zero ref count, and that are already Phantoms or Nops, and are being removed from their basic blocks. That should be safe. And of course there may be paths that somehow stash Node* (previously, NodeIndex) to nodes that have zero ref count and are Phantom or Nop. But hey, this is the fun of software. You can always get it wrong.

WebKit Review Bot

Comment 36 2013-01-28 21:41:53 PST

Attachment 185150 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGAdjacencyList.h', u'Source/JavaScriptCore/dfg/DFGAllocator.h', u'Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.h', u'Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGBasicBlockInlines.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCFAPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInsertionSet.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGLongLivedState.cpp', u'Source/JavaScriptCore/dfg/DFGLongLivedState.h', u'Source/JavaScriptCore/dfg/DFGMinifiedID.h', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.cpp', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeAllocator.h', u'Source/JavaScriptCore/dfg/DFGOSRExit.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExit.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', u'Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.h', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/Options.h']" exit_code: 1 Source/JavaScriptCore/dfg/DFGBasicBlock.h:105: The parameter name "valueParams" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGGraph.cpp:341: _node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:343: _childIdx is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 37 2013-01-28 22:07:09 PST

Comment on attachment 185150 [details] patch for landing Attachment 185150 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16182330

Filip Pizlo

Comment 38 2013-01-28 22:53:43 PST

Created attachment 185166 [details] patch for landing

WebKit Review Bot

Comment 39 2013-01-28 22:56:08 PST

Attachment 185166 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/DataFormat.h', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.h', u'Source/JavaScriptCore/dfg/DFGAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGAdjacencyList.h', u'Source/JavaScriptCore/dfg/DFGAllocator.h', u'Source/JavaScriptCore/dfg/DFGArgumentsSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp', u'Source/JavaScriptCore/dfg/DFGArrayMode.h', u'Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGBasicBlock.h', u'Source/JavaScriptCore/dfg/DFGBasicBlockInlines.h', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCFAPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCSEPhase.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.cpp', u'Source/JavaScriptCore/dfg/DFGCommon.h', u'Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.cpp', u'Source/JavaScriptCore/dfg/DFGDisassembler.h', u'Source/JavaScriptCore/dfg/DFGDriver.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.cpp', u'Source/JavaScriptCore/dfg/DFGEdge.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGenerationInfo.h', u'Source/JavaScriptCore/dfg/DFGGraph.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGInsertionSet.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGLongLivedState.cpp', u'Source/JavaScriptCore/dfg/DFGLongLivedState.h', u'Source/JavaScriptCore/dfg/DFGMinifiedID.h', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.cpp', u'Source/JavaScriptCore/dfg/DFGMinifiedNode.h', u'Source/JavaScriptCore/dfg/DFGNode.cpp', u'Source/JavaScriptCore/dfg/DFGNode.h', u'Source/JavaScriptCore/dfg/DFGNodeAllocator.h', u'Source/JavaScriptCore/dfg/DFGOSRExit.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExit.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.cpp', u'Source/JavaScriptCore/dfg/DFGPhase.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGScoreBoard.h', u'Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h', u'Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h', u'Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.cpp', u'Source/JavaScriptCore/dfg/DFGValidate.h', u'Source/JavaScriptCore/dfg/DFGValueSource.cpp', u'Source/JavaScriptCore/dfg/DFGValueSource.h', u'Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.cpp', u'Source/JavaScriptCore/runtime/FunctionExecutableDump.h', u'Source/JavaScriptCore/runtime/JSGlobalData.cpp', u'Source/JavaScriptCore/runtime/JSGlobalData.h', u'Source/JavaScriptCore/runtime/Options.h']" exit_code: 1 Source/JavaScriptCore/dfg/DFGBasicBlock.h:105: The parameter name "valueParams" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/dfg/DFGGraph.cpp:341: _node is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:343: _childIdx is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/JavaScriptCore/dfg/DFGGraph.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 69 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 40 2013-01-29 00:03:04 PST

Landed in http://trac.webkit.org/changeset/141069

Note You need to log in before you can comment on or make changes to this bug.