Bug 120998 – [Win] Compile errors when enabling DFG JIT.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

When enabling DFG JIT on Windows, I get some compile errors.

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Brent Fulgham]

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.

[WebKit Commit Bot]

Comment on attachment 214863 [details]
Patch

Clearing flags on attachment: 214863

Committed r158282: <http://trac.webkit.org/changeset/158282>

[WebKit Commit Bot]

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 :)