Summary: | [Win] Update solutions and projects to support 64-bit build | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Brent Fulgham
2013-10-02 11:33:14 PDT
Created attachment 213180 [details]
Patch
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 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 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. Created attachment 213307 [details]
Patch only touches the project and solution files.
(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. Committed r156862: <http://trac.webkit.org/changeset/156862> |