Bug 120998

Summary: [Win] Compile errors when enabling DFG JIT.
Product: WebKit Reporter: peavo
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description peavo 2013-09-08 00:54:40 PDT
When enabling DFG JIT on Windows, I get some compile errors.
Comment 1 peavo 2013-09-08 01:10:22 PDT
Created attachment 210968 [details]
Patch
Comment 2 peavo 2013-09-14 06:17:28 PDT
Created attachment 211646 [details]
Patch
Comment 3 peavo 2013-09-14 06:21:13 PDT
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.
Comment 4 peavo 2013-10-03 04:10:24 PDT
Created attachment 213239 [details]
Patch
Comment 5 peavo 2013-10-03 04:29:27 PDT
Created attachment 213240 [details]
Patch
Comment 6 peavo 2013-10-03 05:18:51 PDT
Created attachment 213241 [details]
Patch
Comment 7 peavo 2013-10-03 05:36:22 PDT
Updated patch with changes in trunk.
Comment 8 Brent Fulgham 2013-10-16 11:43:06 PDT
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.
Comment 9 peavo 2013-10-17 06:08:48 PDT
Created attachment 214450 [details]
Patch
Comment 10 peavo 2013-10-17 06:10:29 PDT
Updated patch with changes in trunk.
Comment 11 peavo 2013-10-17 06:48:44 PDT
(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 :)
Comment 12 peavo 2013-10-17 06:50:25 PDT
(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.
Comment 13 peavo 2013-10-17 07:44:38 PDT
Created attachment 214456 [details]
Patch
Comment 14 peavo 2013-10-19 04:57:27 PDT
Created attachment 214649 [details]
Patch
Comment 15 peavo 2013-10-19 05:01:13 PDT
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 16 Brent Fulgham 2013-10-21 10:17:29 PDT
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 :-)
Comment 17 peavo 2013-10-21 10:43:59 PDT
> 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 :)
Comment 18 peavo 2013-10-21 12:42:10 PDT
(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'
Comment 19 peavo 2013-10-22 05:45:57 PDT
Created attachment 214843 [details]
Patch
Comment 20 peavo 2013-10-22 06:04:53 PDT
Created attachment 214847 [details]
Patch
Comment 21 Build Bot 2013-10-22 06:31:40 PDT
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 22 Build Bot 2013-10-22 06:48:45 PDT
Comment on attachment 214847 [details]
Patch

Attachment 214847 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/8818282
Comment 23 peavo 2013-10-22 07:43:29 PDT
Created attachment 214857 [details]
Patch
Comment 24 Build Bot 2013-10-22 08:11:56 PDT
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 25 Build Bot 2013-10-22 08:26:43 PDT
Comment on attachment 214857 [details]
Patch

Attachment 214857 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/8778328
Comment 26 peavo 2013-10-22 09:23:35 PDT
Created attachment 214863 [details]
Patch
Comment 27 peavo 2013-10-22 09:59:30 PDT
(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 28 Brent Fulgham 2013-10-30 10:11:16 PDT
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 29 WebKit Commit Bot 2013-10-30 10:48:09 PDT
Comment on attachment 214863 [details]
Patch

Clearing flags on attachment: 214863

Committed r158282: <http://trac.webkit.org/changeset/158282>
Comment 30 WebKit Commit Bot 2013-10-30 10:48:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 peavo 2013-10-30 10:52:57 PDT
(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 :)