Bug 122225 - [Win] Update solutions and projects to support 64-bit build
Summary: [Win] Update solutions and projects to support 64-bit build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-10-02 11:33 PDT by Brent Fulgham
Modified: 2013-10-03 15:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.54 KB, patch)
2013-10-02 11:44 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch only touches the project and solution files. (13.52 KB, patch)
2013-10-03 15:44 PDT, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>