Bug 122225 – [Win] Update solutions and projects to support 64-bit build

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

[Brent Fulgham]

Extend Alex Christensen's work on WinCairo to support a full 64-bit build of JavaScriptCore on Windows.

[Radar WebKit Bug Importer]

<rdar://problem/15133543>

[Brent Fulgham]

Created attachment 213180 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 213307 [details]
Patch only touches the project and solution files.

[Brent Fulgham]

(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.

[Brent Fulgham]

Committed r156862: <http://trac.webkit.org/changeset/156862>