Bug 170833 – Fix 32bit Windows build by giving correct parameters to MASM

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Bill Ming]

The issue is pretty much like https://trac.webkit.org/changeset/194434/webkit.
Win32 build (not Wincairo) is failing due to giving ml.exe too many unsupported parameters.

A patch is on the way.

[Bill Ming]

Created attachment 307047 [details]
Patch

[Build Bot]

Comment on attachment 307047 [details]
Patch

Attachment 307047 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3531366

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307061 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Bill Ming]

Seems like the failure on mac-wk2 is not relevant.

[Don Olmstead]

Comment on attachment 307047 [details]
Patch

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

Perhaps it'd be better to find a way to remove the flags that its appending that aren't relevant?

> Source/JavaScriptCore/CMakeLists.txt:1202
> +        COMMAND ml ${LLINT_MASM_FLAGS} ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/LowLevelInterpreterWin.obj ${DERIVED_SOURCES_JAVASCRIPTCORE_DIR}/LowLevelInterpreterWin.asm

ml is 32-bit only. Its ml64 for 64-bit so this command is wrong.

[Bill Ming]

Indeed. I will fix it

[Don Olmstead]

Okay so I looked into things a bit more since I'm also hitting this when using clang-cl for building WinCairo.

It really looks like a larger issue with how WebKit is using CMake that in this case is hitting an issue with ml failing when you give it too many unsupported parameters.

I looked at the ninja.build file and saw the FLAGS being sent to it included stuff like the block of /wd's in Source/cmake/OptionsWin.cmake.

If it sees an add_definitions and its a compile option like /wd's then those end up on the flags. If you use add_compile_options it also ends up on the flags.

However you can make something like add_compile_options conditional. Doing the following makes it so those warnings don't end up on the FLAGS in the call to the assembler.

    set(VS_WARNINGS
        /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189
        /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305
        /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459
        /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646
        /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011
        /wd6031 /wd6211 /wd6246 /wd6255 /wd6387
    )

    add_compile_options(
        "$<$<COMPILE_LANGUAGE:C>:${VS_WARNINGS}>"
        "$<$<COMPILE_LANGUAGE:CXX>:${VS_WARNINGS}>"
    )

Perhaps this is a better way to approach the problem?

[Bill Ming]

(In reply to Don Olmstead from comment #7)
> Okay so I looked into things a bit more since I'm also hitting this when
> using clang-cl for building WinCairo.
> 
> It really looks like a larger issue with how WebKit is using CMake that in
> this case is hitting an issue with ml failing when you give it too many
> unsupported parameters.
> 
> I looked at the ninja.build file and saw the FLAGS being sent to it included
> stuff like the block of /wd's in Source/cmake/OptionsWin.cmake.
> 
> If it sees an add_definitions and its a compile option like /wd's then those
> end up on the flags. If you use add_compile_options it also ends up on the
> flags.
> 
> However you can make something like add_compile_options conditional. Doing
> the following makes it so those warnings don't end up on the FLAGS in the
> call to the assembler.
> 
>     set(VS_WARNINGS
>         /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180
> /wd4189
>         /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291
> /wd4305
>         /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458
> /wd4459
>         /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611
> /wd4646
>         /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996
> /wd6011
>         /wd6031 /wd6211 /wd6246 /wd6255 /wd6387
>     )
> 
>     add_compile_options(
>         "$<$<COMPILE_LANGUAGE:C>:${VS_WARNINGS}>"
>         "$<$<COMPILE_LANGUAGE:CXX>:${VS_WARNINGS}>"
>     )
> 
> Perhaps this is a better way to approach the problem?

Yeah, /wd is something break the build. but maybe we should not pass other cl flags to the ml.

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-

[Don Olmstead]

(In reply to Bill Ming from comment #8)
> (In reply to Don Olmstead from comment #7)
> > Okay so I looked into things a bit more since I'm also hitting this when
> > using clang-cl for building WinCairo.
> > 
> > It really looks like a larger issue with how WebKit is using CMake that in
> > this case is hitting an issue with ml failing when you give it too many
> > unsupported parameters.
> > 
> > I looked at the ninja.build file and saw the FLAGS being sent to it included
> > stuff like the block of /wd's in Source/cmake/OptionsWin.cmake.
> > 
> > If it sees an add_definitions and its a compile option like /wd's then those
> > end up on the flags. If you use add_compile_options it also ends up on the
> > flags.
> > 
> > However you can make something like add_compile_options conditional. Doing
> > the following makes it so those warnings don't end up on the FLAGS in the
> > call to the assembler.
> > 
> >     set(VS_WARNINGS
> >         /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180
> > /wd4189
> >         /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291
> > /wd4305
> >         /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458
> > /wd4459
> >         /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611
> > /wd4646
> >         /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996
> > /wd6011
> >         /wd6031 /wd6211 /wd6246 /wd6255 /wd6387
> >     )
> > 
> >     add_compile_options(
> >         "$<$<COMPILE_LANGUAGE:C>:${VS_WARNINGS}>"
> >         "$<$<COMPILE_LANGUAGE:CXX>:${VS_WARNINGS}>"
> >     )
> > 
> > Perhaps this is a better way to approach the problem?
> 
> Yeah, /wd is something break the build. but maybe we should not pass other
> cl flags to the ml.
> 
> 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-

I agree its just quite a bit more surgery to get right.

[Bill Ming]

Created attachment 309230 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 309230 [details]
Patch

Clearing flags on attachment: 309230

Committed r216332: <http://trac.webkit.org/changeset/216332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

*** Bug 161517 has been marked as a duplicate of this bug. ***