Summary: | [Win] Compile errors when enabling DFG JIT. | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | peavo | ||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, buildbot, cmarcelo, commit-queue, mhahnenberg, rniwa | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | PlatformOnly | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 121001 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
peavo
2013-09-08 00:54:40 PDT
Created attachment 210968 [details]
Patch
Created attachment 211646 [details]
Patch
Modified previous patch slightly by including a crash fix. Added __declspec(naked) attribute to definition of function getHostCallReturnValue(), and modified the assembler code according to its gcc x86 counterpart. Created attachment 213239 [details]
Patch
Created attachment 213240 [details]
Patch
Created attachment 213241 [details]
Patch
Updated patch with changes in trunk. Comment on attachment 213241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213241&action=review Looks good! I'll try it locally. I notice you don't activate DFG -- is that because the implementation is still not right? > Source/JavaScriptCore/jit/JITOperationWrappers.h:310 > +#elif COMPILER(MSVC) && CPU(X86) Unfortunately, none of this can be easily used for 64-bit builds because MSVC does not allow in-line assembly code. We are going to have to move this code to an *.asm file at some point. Created attachment 214450 [details]
Patch
Updated patch with changes in trunk. (In reply to comment #8) > (From update of attachment 213241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213241&action=review > > Looks good! I'll try it locally. I notice you don't activate DFG -- is that because the implementation is still not right? > > > Source/JavaScriptCore/jit/JITOperationWrappers.h:310 > > +#elif COMPILER(MSVC) && CPU(X86) > > Unfortunately, none of this can be easily used for 64-bit builds because MSVC does not allow in-line assembly code. We are going to have to move this code to an *.asm file at some point. Thanks for looking into this :) I believe there are a couple of issues which needs to be fixed before DFG can be activated. There is bug 121001, which should fix a crash with generated code. Also the file wtf/CompilationThread.cpp needs to add code for platforms which doesn't use pthreads. I guess we can quickly whip up something there :) When I tested it, I enabled pthreads locally, so I didn't have to change this code. With those two changes in place, it seemed to work well, I got another JSC related crash after leaving the computer idle for awhile, but this was a simple NULL pointer crash, which we should be able to sort out :) (In reply to comment #10) > Updated patch with changes in trunk. The patch doesn't seem to apply, probably because of some line ending issues. Created attachment 214456 [details]
Patch
Created attachment 214649 [details]
Patch
Updated patch with changes in trunk. I also implemented needed functionality in wtf/CompilationThread.cpp for platforms which doesn't use pthreads. Together with the patch in bug 121001, and DFG enabled, this seems to run fine in debug mode with "normal" browsing. I guess there still might be problems with applying the patch, I think this is because of mixed line ending style in the project files. Comment on attachment 214649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214649&action=review I think this looks fine. I had a couple of questions I wanted to check with you about before approving it. > Source/JavaScriptCore/dfg/DFGAllocator.h:81 > + void startBumpingIn(Region*); I don't understand why we are removing the scoping here. Is this to silence a warning in Visual Studio? > Source/JavaScriptCore/jit/JITOperationWrappers.h:332 > +__declspec(naked) EncodedJSValue JIT_OPERATION function(ExecState*, EncodedJSValue, StringImpl*) \ These macros are going to be hard to replicate for 64-bit with Visual Studio since it prohibits inline assembly. > Source/WTF/wtf/CompilationThread.cpp:55 > + initializeCompilationThreadsMutex.unlock(); We should probably create a WTF/ThreadingOnce.h/.cpp that encapsulates this pattern. We use the pthread_once logic in a number of places, and it would be useful to only do this one way throughout our code base. You don't need to do that in this bug, of course :-) > I think this looks fine. I had a couple of questions I wanted to check with you about before approving it. > > > Source/JavaScriptCore/dfg/DFGAllocator.h:81 > > + void startBumpingIn(Region*); > > I don't understand why we are removing the scoping here. Is this to silence a warning in Visual Studio? > It was actually a compiler error. I don't remember the exact error right now, but can look it up :) > > Source/JavaScriptCore/jit/JITOperationWrappers.h:332 > > +__declspec(naked) EncodedJSValue JIT_OPERATION function(ExecState*, EncodedJSValue, StringImpl*) \ > > These macros are going to be hard to replicate for 64-bit with Visual Studio since it prohibits inline assembly. > > > Source/WTF/wtf/CompilationThread.cpp:55 > > + initializeCompilationThreadsMutex.unlock(); > Ok, I see, so we need to create an .asm file for this for 64-bit? > We should probably create a WTF/ThreadingOnce.h/.cpp that encapsulates this pattern. We use the pthread_once logic in a number of places, and it would be useful to only do this one way throughout our code base. > > You don't need to do that in this bug, of course :-) Good point, I can look into this in a separate bug :) (In reply to comment #17) > > > > > Source/JavaScriptCore/dfg/DFGAllocator.h:81 > > > + void startBumpingIn(Region*); > > > > I don't understand why we are removing the scoping here. Is this to silence a warning in Visual Studio? > > > > It was actually a compiler error. I don't remember the exact error right now, but can look it up :) This is the error I was getting: c:\webkit\source\javascriptcore\dfg\DFGAllocator.h(80): warning C4346: 'JSC::DFG::Allocator::Region' : dependent name is not a type prefix with 'typename' to indicate a type c:\webkit\source\javascriptcore\dfg\DFGAllocator.h(87) : see reference to class template instantiation 'JSC::DFG::Allocator' being compiled c:\webkit\source\javascriptcore\dfg\DFGAllocator.h(80): error C2061: syntax error : identifier 'Region' Created attachment 214843 [details]
Patch
Created attachment 214847 [details]
Patch
Comment on attachment 214847 [details] Patch Attachment 214847 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4998350 Comment on attachment 214847 [details] Patch Attachment 214847 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/8818282 Created attachment 214857 [details]
Patch
Comment on attachment 214857 [details] Patch Attachment 214857 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/8798317 Comment on attachment 214857 [details] Patch Attachment 214857 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/8778328 Created attachment 214863 [details]
Patch
(In reply to comment #16) > > We should probably create a WTF/ThreadingOnce.h/.cpp that encapsulates this pattern. We use the pthread_once logic in a number of places, and it would be useful to only do this one way throughout our code base. > Updated patch with a ThreadingOnce class, since the functionality was needed in two places. Comment on attachment 214863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214863&action=review Thanks for resolving those issues. This looks fine to me. r=me > Source/WTF/wtf/ThreadingOnce.h:68 > + bool m_calledOnce; It's a shame that Windows doesn't provide this functionality. A future improvement might be to base this on whatever Windows uses to handle "do this once" functionality. Perhaps that is just a mutex and a flag, but when we move to 64-bit, we will have access to more modern OS features, which might be helpful here. Comment on attachment 214863 [details] Patch Clearing flags on attachment: 214863 Committed r158282: <http://trac.webkit.org/changeset/158282> All reviewed patches have been landed. Closing bug. (In reply to comment #28) > (From update of attachment 214863 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214863&action=review > > Thanks for resolving those issues. This looks fine to me. > > r=me > Thanks for helping me out with this one :) |