Bug 122225

Summary: [Win] Update solutions and projects to support 64-bit build
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: JavaScriptCoreAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cmarcelo, commit-queue, ggaren, msaboff, oliver, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch only touches the project and solution files. andersca: review+

Description Brent Fulgham 2013-10-02 11:33:14 PDT
Extend Alex Christensen's work on WinCairo to support a full 64-bit build of JavaScriptCore on Windows.
Comment 1 Radar WebKit Bug Importer 2013-10-02 11:34:10 PDT
<rdar://problem/15133543>
Comment 2 Brent Fulgham 2013-10-02 11:44:00 PDT
Created attachment 213180 [details]
Patch
Comment 3 WebKit Commit Bot 2013-10-02 11:44:59 PDT
Attachment 213180 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.submit.sln', u'Source/JavaScriptCore/assembler/MacroAssembler.h', u'Source/JavaScriptCore/assembler/MacroAssemblerX86Common.cpp', u'Source/JavaScriptCore/assembler/X86Assembler.h', u'Source/JavaScriptCore/heap/MachineStackMarker.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/yarr/YarrJIT.cpp', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.vcxproj/WTF.submit.sln', u'Source/WTF/wtf/Platform.h']" exit_code: 1
Source/JavaScriptCore/assembler/MacroAssembler.h:53:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Darin Adler 2013-10-02 11:59:07 PDT
Comment on attachment 213180 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +        * assembler/MacroAssembler.h: Swap ordering of #elif CPU(X86) and #elif CPU(X86_64)
> +        MSVC defines both for 64-bit builds.

Do we really want CPU(X86) to be true when CPU(X86_64) is? The comment talks about MSVC, but it doesn’t seem that this is a MSVC decision. It’s a “design of the WebKit platform macros” question.

> Source/WTF/wtf/Platform.h:567
> +#if PLATFORM(WIN)
> +#if CPU(X86_64)
> +#if !defined(_M_AMD64)
> +#define _M_AMD64 1
> +#endif
> +#endif
> +#endif

You should write this as a single #if  with && instead of three nested ifs. You should have a comment about why this is needed. I’m not sure the change log makes it clear why it’s needed, and I’m especially unclear why Platform.h is the right header for something like this.
Comment 5 Brent Fulgham 2013-10-02 12:57:43 PDT
Comment on attachment 213180 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:14
>> +        MSVC defines both for 64-bit builds.
> 
> Do we really want CPU(X86) to be true when CPU(X86_64) is? The comment talks about MSVC, but it doesn’t seem that this is a MSVC decision. It’s a “design of the WebKit platform macros” question.

This actually might be an issue with the CoreFoundation headers. It looks like they define __i386__ for Windows builds in general, which tricks us into enabling CPU(X86).  Let me track that down, as fixing that might make the rest of this patch (except for solution changes) unnecessary.
Comment 6 Brent Fulgham 2013-10-03 15:44:20 PDT
Created attachment 213307 [details]
Patch only touches the project and solution files.
Comment 7 Brent Fulgham 2013-10-03 15:48:00 PDT
(In reply to comment #4)
> (From update of attachment 213180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213180&action=review
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        * assembler/MacroAssembler.h: Swap ordering of #elif CPU(X86) and #elif CPU(X86_64)
> > +        MSVC defines both for 64-bit builds.
> 
> Do we really want CPU(X86) to be true when CPU(X86_64) is? The comment talks about MSVC, but it doesn’t seem that this is a MSVC decision. It’s a “design of the WebKit platform macros” question.
> 
[...] 
> 
> You should write this as a single #if  with && instead of three nested ifs. You should have a comment about why this is needed. I’m not sure the change log makes it clear why it’s needed, and I’m especially unclear why Platform.h is the right header for something like this.

It turns out that all of this stuff was unneeded. I was confused by Visual Studio reporting that the 64-bit definitions were undefined. It looks like there is a bug in their syntax highlighting/symbol lookup logic that causes the _M_X86 and _M_X86_64 to both be considered active. When this happens, it marks the #else cases improperly, making it appear that the symbols are missing.

With these project changes I can get a working 64-bit JavaScriptCore build.
Comment 8 Brent Fulgham 2013-10-03 15:54:04 PDT
Committed r156862: <http://trac.webkit.org/changeset/156862>