Bug 121749 – Move DFG inline caching logic into jit/

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 212281 [details]
the patch

[WebKit Commit Bot]

Attachment 212281 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CallLinkInfo.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/StructureStubInfo.h', u'Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGRegisterSet.h', u'Source/JavaScriptCore/dfg/DFGRepatch.cpp', u'Source/JavaScriptCore/dfg/DFGRepatch.h', u'Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.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/DFGThunks.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/RegisterSet.h', u'Source/JavaScriptCore/jit/Repatch.cpp', u'Source/JavaScriptCore/jit/Repatch.h', u'Source/JavaScriptCore/jit/ScratchRegisterAllocator.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.h']" exit_code: 1
Source/JavaScriptCore/jit/Repatch.cpp:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:561:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:722:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:759:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:796:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:880:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:117:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:130:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:145:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:156:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:177:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:188:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/Repatch.h:26:  #ifndef header guard has wrong style, please use: Repatch_h  [build/header_guard] [5]
Total errors found: 17 in 33 files


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

[Sam Weinig]

Comment on attachment 212281 [details]
the patch

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

> Source/JavaScriptCore/jit/JITCall.cpp:135
>  void JIT::compileCallEval(Instruction* instruction)
>  {
> +    
>      JITStubCall stubCall(this, cti_op_call_eval); // Initializes ScopeChain; ReturnPC; CodeBlock.

You didn't mean to add this line.

> Source/JavaScriptCore/jit/Repatch.h:27
> +#define DFGRepatch_h

You should fix the header guard.

[Sam Weinig]

rs=me.

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 212281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212281&action=review
> 
> > Source/JavaScriptCore/jit/JITCall.cpp:135
> >  void JIT::compileCallEval(Instruction* instruction)
> >  {
> > +    
> >      JITStubCall stubCall(this, cti_op_call_eval); // Initializes ScopeChain; ReturnPC; CodeBlock.
> 
> You didn't mean to add this line.

Word.

> 
> > Source/JavaScriptCore/jit/Repatch.h:27
> > +#define DFGRepatch_h
> 
> You should fix the header guard.

Yeah.

[Filip Pizlo]

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

[Jon Lee]

This broke the Windows port:
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505

[Filip Pizlo]

(In reply to comment #7)
> This broke the Windows port:
> http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505

Fixing...

[Filip Pizlo]

(In reply to comment #8)
> (In reply to comment #7)
> > This broke the Windows port:
> > http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/52505
> 
> Fixing...

Actually, I will revert.  There's no way to fix Windows.

[Filip Pizlo]

Created attachment 212289 [details]
the patch

[Filip Pizlo]

...

[WebKit Commit Bot]

Attachment 212289 [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.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/bytecode/CallLinkInfo.cpp', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/StructureStubInfo.h', u'Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGOSRExitCompiler.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGRegisterSet.h', u'Source/JavaScriptCore/dfg/DFGRepatch.cpp', u'Source/JavaScriptCore/dfg/DFGRepatch.h', u'Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.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/DFGThunks.cpp', u'Source/JavaScriptCore/dfg/DFGThunks.h', u'Source/JavaScriptCore/ftl/FTLIntrinsicRepository.h', u'Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp', u'Source/JavaScriptCore/ftl/FTLOSRExitCompiler.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITOperationWrappers.h', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITOperations.h', u'Source/JavaScriptCore/jit/RegisterSet.h', u'Source/JavaScriptCore/jit/Repatch.cpp', u'Source/JavaScriptCore/jit/Repatch.h', u'Source/JavaScriptCore/jit/ScratchRegisterAllocator.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.h']" exit_code: 1
Source/JavaScriptCore/jit/Repatch.cpp:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:561:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:722:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:759:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:796:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/jit/Repatch.cpp:880:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeBlock.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:124:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:137:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:152:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:163:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:184:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/JITOperationWrappers.h:195:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 34 files


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

[Mark Lam]

Created attachment 212656 [details]
revised patch: rebased to latest revision, and attempted fixes for Windows build. 

Uploading this patch so that we can try this on a Windows build.

[Mark Lam]

(In reply to comment #13)
> Created an attachment (id=212656) [details]
> revised patch: rebased to latest revision, and attempted fixes for Windows build. 
> 
> Uploading this patch so that we can try this on a Windows build.

Patch 3 is still failing to build on Windows.  Investigating.

[Mark Lam]

Created attachment 212660 [details]
patch 4: more Windows build fixes.

[Mark Lam]

Created attachment 212665 [details]
patch 5: one more Windows build fix.

[Mark Lam]

Comment on attachment 212665 [details]
patch 5: one more Windows build fix.

Patch 5 builds jsc properly although I encountered other build issues in WebCore that are unrelated to this patch.  That said, I think this patch should be reviewed before I land it.  The changes that I made in addition to the original patch from http://trac.webkit.org/changeset/156235 are:

1. Rebase up to the latest svn revision.
2. Added FUNCTION_WRAPPER_WITH_RETURN_ADDRESS macros for COMPILER(MSVC) && CPU(X86).
3. Moved implementations of getHostCallReturnValue and getHostCallReturnValueWithExecState to JITOperations.cpp from DFGOperations.cpp.  These pieces of code needs to be built even if the DFG is not enabled for build.  Hence, they don't belong in DFGOperations.cpp.
4. I added an implementation of getHostCallReturnValue for COMPILER(MSVC) && CPU(X86).

[Geoffrey Garen]

Comment on attachment 212665 [details]
patch 5: one more Windows build fix.

r=me, assuming it builds and passes tests.

Can you do a pass with the EWS bot before landing? It looks like this version of the patch didn't apply cleanly on the EWS bot.

[Mark Lam]

Created attachment 212726 [details]
patch 6: svn up'ed to see if that fixes the merge issue on the EWS bots

[Mark Lam]

Created attachment 212729 [details]
patch 7: one more after rollout of r156474 which broke the 32-bt build.

[Mark Lam]

Created attachment 212735 [details]
patch 8: one more time since the EWS bots seems to be working again now.

[Mark Lam]

It looks like the EWS patching system is unable to deal with the file deletions and additions that this patch has.  I've already tries many times, and it doesn't seem to make any difference no matter how many times I svn up to the latest.

The patch builds and runs jsc.exe properly on Windows.  I was not able to run testapi.exe as it crashes due to some pre-existing bug.

I'm not able to build all of WebKit because of a pre-existing build failure in WebCore/platform/graphics/cg/GraphicsContextCG.cpp even without this patch.

I'll proceed with landing this patch and deal with any fallout if needed.

[Geoffrey Garen]

> I was not able to run testapi.exe as it crashes due to some pre-existing bug.

Do we have a bug filed for this?

[Mark Lam]

(In reply to comment #23)
> > I was not able to run testapi.exe as it crashes due to some pre-existing bug.
> 
> Do we have a bug filed for this?

I will file a bug.

Just ran run-javascriptcore-tests on Windows with testapi omitted, and it passed.  However, it only ran the Mozilla suite.  It didn't run the LayoutTests and stress test suites as it would have done on Mac.  This is without me making any changes to omit those other tests.

[Mark Lam]

Thanks for the review.  Landed in r156490: <http://trac.webkit.org/r156490>.

FYI, bug for the testapi crash: https://bugs.webkit.org/show_bug.cgi?id=121972.
Bug for the WebCore build errors: https://bugs.webkit.org/show_bug.cgi?id=121974.