Bug 173308 – [Win64] Fails to build Asm.lib (PaintHooks.asm) since Bug 173132

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Fujii Hironori]

WinCairo build fails.
trunk@217994

https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/3095

> [1/907] Building ASM_MASM object Source\WebKit\CMakeFiles\Asm.dir\win\Plugins\PaintHooks.asm.obj
> MASM : warning A4018:invalid command-line option : /wd4018
> MASM : warning A4018:invalid command-line option : /wd4068
> MASM : warning A4018:invalid command-line option : /wd4099
> MASM : warning A4018:invalid command-line option : /wd4100
> MASM : warning A4018:invalid command-line option : /wd4127
> MASM : warning A4018:invalid command-line option : /wd4138
> MASM : warning A4018:invalid command-line option : /wd4146
> MASM : warning A4018:invalid command-line option : /wd4180
> MASM : warning A4018:invalid command-line option : /wd4189
> MASM : warning A4018:invalid command-line option : /wd4201
> MASM : warning A4018:invalid command-line option : /wd4206
> MASM : warning A4018:invalid command-line option : /wd4244
> MASM : warning A4018:invalid command-line option : /wd4251
> MASM : warning A4018:invalid command-line option : /wd4267
> MASM : warning A4018:invalid command-line option : /wd4275
> MASM : warning A4018:invalid command-line option : /wd4288
> MASM : warning A4018:invalid command-line option : /wd4291
> MASM : warning A4018:invalid command-line option : /wd4305
> MASM : warning A4018:invalid command-line option : /wd4309
> MASM : warning A4018:invalid command-line option : /wd4344
> MASM : warning A4018:invalid command-line option : /wd4355
> MASM : warning A4018:invalid command-line option : /wd4389
> MASM : warning A4018:invalid command-line option : /wd4396
> MASM : warning A4018:invalid command-line option : /wd4456
> MASM : warning A4018:invalid command-line option : /wd4457
> MASM : warning A4018:invalid command-line option : /wd4458
> MASM : warning A4018:invalid command-line option : /wd4459
> MASM : warning A4018:invalid command-line option : /wd4481
> MASM : warning A4018:invalid command-line option : /wd4503
> MASM : warning A4018:invalid command-line option : /wd4505
> MASM : warning A4018:invalid command-line option : /wd4510
> MASM : warning A4018:invalid command-line option : /wd4512
> MASM : warning A4018:invalid command-line option : /wd4530
> MASM : warning A4018:invalid command-line option : /wd4610
> MASM : warning A4018:invalid command-line option : /wd4611
> MASM : warning A4018:invalid command-line option : /wd4646
> MASM : warning A4018:invalid command-line option : /wd4702
> MASM : warning A4018:invalid command-line option : /wd4706
> MASM : warning A4018:invalid command-line option : /wd4722
> MASM : warning A4018:invalid command-line option : /wd4800
> MASM : warning A4018:invalid command-line option : /wd4819
> MASM : warning A4018:invalid command-line option : /wd4951
> MASM : warning A4018:invalid command-line option : /wd4952
> MASM : warning A4018:invalid command-line option : /wd4996
> MASM : warning A4018:invalid command-line option : /wd6011
> MASM : warning A4018:invalid command-line option : /wd6031
> MASM : warning A4018:invalid command-line option : /wd6211
> MASM : warning A4018:invalid command-line option : /wd6246
> MASM : warning A4018:invalid command-line option : /wd6255
> MASM : warning A4018:invalid command-line option : /wd6387
> MASM : warning A4018:invalid command-line option : /GS
> MASM : warning A4018:invalid command-line option : /EHa-
> MASM : warning A4018:invalid command-line option : /EHc-
> MASM : warning A4018:invalid command-line option : /EHs-
> MASM : warning A4018:invalid command-line option : /fp:except-
> MASM : warning A4018:invalid command-line option : /analyze-
> MASM : warning A4018:invalid command-line option : /bigobj
> MASM : warning A4018:invalid command-line option : /openmp-
> MASM : warning A4018:invalid command-line option : /GF-
> MASM : warning A4018:invalid command-line option : /Oy-
>  Assembling: ..\..\Source\WebKit\win\Plugins\PaintHooks.asm
> Microsoft (R) Macro Assembler (x64) Version 14.10.25019.0
> Copyright (C) Microsoft Corporation.  All rights reserved.
> [3/907] Linking ASM_MASM static library lib64\Asm.lib
> FAILED: lib64/Asm.lib 
> cmd.exe /C "cd . && "" cr lib64\Asm.lib /machine:x64 Source\WebKit\CMakeFiles\Asm.dir\win\Plugins\PaintHooks.asm.obj  && "" lib64\Asm.lib  && cd ."
> '""' is not recognized as an internal or external command,
> operable program or batch file.

[Fujii Hironori]

Created attachment 312762 [details]
WIP patch

[Fujii Hironori]

Created attachment 312764 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 312764 [details]
Patch

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

> Source/WebKit/PlatformWin.cmake:257
> +    if (MSVC)

In JSC CMakeLists.txt, we have enable_langualge(ASM_MASM). Is it unnecessary?

> Source/WebKit/PlatformWin.cmake:259
> +        set(LLINT_MASM_FLAGS /c /Fo)

It's not related to LLINT. Let's rename the other one, like, MASM_FLAGS.

> Source/WebKit/PlatformWin.cmake:263
> +            COMMAND ${MASM_EXECUTABLE} ${LLINT_MASM_FLAGS}

Ditto.

> Source/WebKit/PlatformWin.cmake:269
> +        list(APPEND WebKit_SOURCES
> +            ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/PaintHooks.obj
> +        )

Is it correct? I think SOURCES are for source files basically.

[Alex Christensen]

Comment on attachment 312764 [details]
Patch

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

> Source/WebKit/PlatformWin.cmake:261
> +            OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/PaintHooks.obj

https://trac.webkit.org/changeset/216332/webkit uses a derived sources directory.  This should do the same.

[Fujii Hironori]

Comment on attachment 312764 [details]
Patch

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

Thank you for the review, Yusuke and Alex.

>> Source/WebKit/PlatformWin.cmake:257
>> +    if (MSVC)
> 
> In JSC CMakeLists.txt, we have enable_langualge(ASM_MASM). Is it unnecessary?

I shouldn't remove it. It is required for non MSVC case.

>> Source/WebKit/PlatformWin.cmake:269
>> +        )
> 
> Is it correct? I think SOURCES are for source files basically.

There are the same questions around the world. It seems that this is the common way in CMake.

https://stackoverflow.com/questions/38609303/how-to-add-prebuilt-object-files-to-executable-in-cmake/38610428
https://cmake.org/pipermail/cmake/2010-September/039458.html
https://cmake.org/pipermail/cmake/2009-September/032278.html

[Fujii Hironori]

Created attachment 312852 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312852 [details]
Patch

Clearing flags on attachment: 312852

Committed r218258: <http://trac.webkit.org/changeset/218258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Konstantin Tokarev]

Comment on attachment 312852 [details]
Patch

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

> Source/WebKit/PlatformWin.cmake:261
> +            OUTPUT ${DERIVED_SOURCES_WEBKIT_DIR}/PaintHooks.obj

This is a wrong place for object file. You can use e.g. ${CMAKE_CURRENT_BINARY_DIR}

[Konstantin Tokarev]

Comment on attachment 312764 [details]
Patch

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

>>> Source/WebKit/PlatformWin.cmake:257
>>> +    if (MSVC)
>> 
>> In JSC CMakeLists.txt, we have enable_langualge(ASM_MASM). Is it unnecessary?
> 
> I shouldn't remove it. It is required for non MSVC case.

Out of curiosity, what does "non MSVC case" mean here? Functions from PaintHooks.asm are needed only under condition !COMPILER(GCC) && !defined(_M_IX86)

[Fujii Hironori]

Thank you for reviewing my patch, Konstantin.

(In reply to Konstantin Tokarev from comment #9)
> This is a wrong place for object file. You can use e.g.
> ${CMAKE_CURRENT_BINARY_DIR}

See Alex's comment (Comment 4). I think he prefer consistency.

And, it's important for PS4 port to use ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR} because it uses CMake VS generator without build-webkit script which creates separate build directories for Debug and Release build configurations.


(In reply to Konstantin Tokarev from comment #10)
> Out of curiosity, what does "non MSVC case" mean here? Functions from
> PaintHooks.asm are needed only under condition !COMPILER(GCC) &&
> !defined(_M_IX86)

I mean the following else case.

>     if (MSVC)
>       [...]
>     else ()
>         enable_language(ASM_MASM)
>         list(APPEND WebKit_SOURCES
>             win/plugins/PaintHooks.asm
>         )
>     endif ()

I have preserved this code for ports building for Windows with other than MSVC.
Is this useless?

[Alex Christensen]

I followed up in http://trac.webkit.org/r218430
If we find we want to get rid of the non-msvc path then we can.  People work on MinGW and clang occasionally.