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: JavaScriptCoreAssignee: 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: P2    
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

Description Filip Pizlo 2013-01-14 21:04:41 PST
This will be painful.
Comment 1 Filip Pizlo 2013-01-14 21:05:41 PST
Created attachment 182693 [details]
work in progress
Comment 2 Filip Pizlo 2013-01-15 16:14:58 PST
Created attachment 182866 [details]
work in progress
Comment 3 Filip Pizlo 2013-01-15 16:54:34 PST
Created attachment 182874 [details]
work in progress
Comment 4 Filip Pizlo 2013-01-15 19:29:39 PST
Created attachment 182907 [details]
work in progress
Comment 5 Filip Pizlo 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.
Comment 6 Filip Pizlo 2013-01-21 00:22:36 PST
Created attachment 183726 [details]
work in progress
Comment 7 Filip Pizlo 2013-01-21 15:07:12 PST
Created attachment 183835 [details]
work in progress
Comment 8 Filip Pizlo 2013-01-22 15:53:57 PST
Created attachment 184064 [details]
work in progres
Comment 9 Filip Pizlo 2013-01-22 22:41:44 PST
Created attachment 184144 [details]
work in progress
Comment 10 Filip Pizlo 2013-01-23 12:10:04 PST
Created attachment 184276 [details]
work in progress
Comment 11 Filip Pizlo 2013-01-23 17:01:53 PST
Created attachment 184356 [details]
work in progress

Backing up before I do another rebase.
Comment 12 Filip Pizlo 2013-01-24 12:58:54 PST
Created attachment 184559 [details]
WRONG PATCH
Comment 13 Filip Pizlo 2013-01-24 12:59:33 PST
Created attachment 184560 [details]
work in progress
Comment 14 Filip Pizlo 2013-01-24 13:51:38 PST
Created attachment 184572 [details]
work in progress
Comment 15 Filip Pizlo 2013-01-26 13:43:14 PST
Created attachment 184883 [details]
work in progress
Comment 16 Filip Pizlo 2013-01-26 16:29:02 PST
Created attachment 184891 [details]
work in progress
Comment 17 Filip Pizlo 2013-01-26 18:27:51 PST
Created attachment 184894 [details]
work in progress
Comment 18 Filip Pizlo 2013-01-27 13:32:53 PST
Created attachment 184922 [details]
work in progress
Comment 19 Filip Pizlo 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.
Comment 20 Filip Pizlo 2013-01-28 14:05:34 PST
Created attachment 185058 [details]
it compiles

Crashes on launch, of course
Comment 21 Filip Pizlo 2013-01-28 15:15:17 PST
Created attachment 185080 [details]
it runs things

Haven't run all of the tests, yet.
Comment 22 Filip Pizlo 2013-01-28 16:36:21 PST
Created attachment 185099 [details]
1% SunSpider speed-up so far
Comment 23 Filip Pizlo 2013-01-28 18:24:50 PST
Created attachment 185127 [details]
the patch

It passes tests and stuff.
Comment 24 Oliver Hunt 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
Comment 25 WebKit Review Bot 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.
Comment 26 Filip Pizlo 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.
Comment 27 Oliver Hunt 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?
Comment 28 Filip Pizlo 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.
Comment 29 Build Bot 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
Comment 30 Oliver Hunt 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.
Comment 31 kov's GTK+ EWS bot 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
Comment 32 Filip Pizlo 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).
Comment 33 Oliver Hunt 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
Comment 34 Filip Pizlo 2013-01-28 21:36:39 PST
Created attachment 185150 [details]
patch for landing

Attempting to fix Windows, Gtk, and style.
Comment 35 Filip Pizlo 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.
Comment 36 WebKit Review Bot 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.
Comment 37 Build Bot 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
Comment 38 Filip Pizlo 2013-01-28 22:53:43 PST
Created attachment 185166 [details]
patch for landing
Comment 39 WebKit Review Bot 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.
Comment 40 Filip Pizlo 2013-01-29 00:03:04 PST
Landed in http://trac.webkit.org/changeset/141069