586113 - libffi passes invalid flags to cl.exe on VS10

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[[e-mail]]

cl -MD -nologo -W3 -DHAVE_CONFIG_H -I. -I/d/build/mozilla-central/js/src/ctypes/
libffi -I. -I/d/build/mozilla-central/js/src/ctypes/libffi/include -Iinclude -I/
d/build/mozilla-central/js/src/ctypes/libffi/src -Zi -DEBUG -O2 -OPT:REF -OPT:IC
F -INCREMENTAL:NO -c /d/build/mozilla-central/js/src/ctypes/libffi/src/debug.c -
Fosrc/debug.obj -Fdsrc/debug -Fpsrc/debug -Fasrc/debug
cl : Command line warning D9002 : ignoring unknown option '-OP'
cl : Command line warning D9002 : ignoring unknown option '-OT'
cl : Command line warning D9002 : ignoring unknown option '-O:'
cl : Command line warning D9002 : ignoring unknown option '-OR'
cl : Command line warning D9002 : ignoring unknown option '-OE'
cl : Command line warning D9002 : ignoring unknown option '-OF'
cl : Command line warning D9002 : ignoring unknown option '-OP'
cl : Command line warning D9002 : ignoring unknown option '-OT'
cl : Command line warning D9002 : ignoring unknown option '-O:'
cl : Command line warning D9002 : ignoring unknown option '-OI'
cl : Command line warning D9002 : ignoring unknown option '-OC'
cl : Command line warning D9002 : ignoring unknown option '-OF'

In VS9, -OPT:REF -OPT:ICF was accepted by cl; in VS10 apparently it isn't. We need to pass these flags to the linker instead.

I think this will require an msvcld.sh script to supplement msvcc.sh.

Not urgent, since this just affects optimization flags and isn't fatal.

Comment 1

[Mike Pesce (:By-Tor)]

AFAICT, all that's needed is /link before the linker options.  I've looked at cl.exe parameters going back to VS2005 and it shouldn't break anything with earlier compilers.

Comment 2

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 4

[Ryan VanderMeulen [:RyanVM]]

IIUC, we've been effectively ignoring these flags for awhile now with no ill consequences. When I look at the MSVC documentation for Zi, it says "/Zi does not affect optimizations." I guess we were trying to make it possible to generate debug symbols in opt builds without inadvertent de-optimization? Even then, we're needlessly adding a -DEBUG flag after -Zi even though it's implied.

I've got a try run going with a -link added in front of those options in msvcc.sh to see if it works, but I'm not sure it will since MS says "The /link option and its linker options must appear after any file names and CL options."

However, I can't help but wonder if we even need them at all.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Ah, I see it now:
"/DEBUG changes the defaults for the /OPT option from REF to NOREF and from ICF to NOICF (so, you will need to explicitly specify /OPT:REF or /OPT:ICF)."

That said, we've still be effectively *not* running with these set since we switched to VC10 as our default compiler. Still makes me wonder if there's much to gain by properly setting them again.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Attachment: pass the flags via -link after all others

Actually, let's just fix this. I've confirmed on Try that this works as expected (at least that the flags show up at the end of the list - not sure how to confirm that the linker is actually *using* them). Plan is to submit it for upstream inclusion if it gets r+ and pull it in with bug 810631.

This also includes fixes for some other upstream behavior changes:
1) Convert -O3 to -O2. This makes us match existing behavior (the current in-tree libffi builds with -O2 on MSVC).
2) Ignore "-warn all". We already ignore -Wall, so this seems safe to just disregard.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8384217 [details] [diff] [review]
pass the flags via -link after all others

This breaks debug builds due to a clash between O2 and RTC1.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

The issue is the the current in-tree libffi only sends -O2 for opt builds, but the current tip version sends -O3 no matter what, which we then dutifully convert to -O2. I can think of ways to fix this robustly in msvcc.sh, but I wonder if that behavior change upstream was really intended or not.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Attachment: updated patch

Much better :)
https://tbpl.mozilla.org/?tree=Try&rev=a801b15fb540

So, to summarize the changes made in this patch:
* Record the original arguments passed to msvcc.sh for later use.
* If passed an -O* argument, do nothing if -DFFI_DEBUG is present in the original arguments. Otherwise, change -O3 to -O2 or preserve the original argument that was sent (silently dropping non-O3 arguments was another bug in the original patch). Also record that this is optimized for later use (the first part of the fix for this specific bug).
* Remove the extraneous -DEBUG argument when passed with -Zi. Per the MSVC documentation, it's implicit anyway.
* For optimized builds, append the linker overrides after all other arguments via the -link flag (the rest of the fix for this specific bug).

Example output for opt builds:
libtool: compile:  c:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/msvcc.sh -DHAVE_CONFIG_H -I. -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi -I. -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/include -Iinclude -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/src -O3 -warn all -c -showIncludes c:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/src/types.c -o src/types.obj
cl -MD -nologo -W3 -DHAVE_CONFIG_H -I. -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi -I. -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/include -Iinclude -Ic:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/src -O2 -c -showIncludes c:/builds/moz2_slave/try-w32-0000000000000000000000/build/js/src/ctypes/libffi/src/types.c -Fosrc/types.obj -Fdsrc/types -Fpsrc/types -Fasrc/types -link -OPT:REF -OPT:ICF -INCREMENTAL:NO

Example output for debug builds:
libtool: compile:  c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/msvcc.sh -DHAVE_CONFIG_H -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/include -Iinclude -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src -DFFI_DEBUG -O3 -warn all -c -showIncludes c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src/types.c -o src/types.obj
cl -MDd -nologo -W3 -DHAVE_CONFIG_H -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi -I. -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/include -Iinclude -Ic:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src -RTC1 -c -showIncludes c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/ctypes/libffi/src/types.c -Fosrc/types.obj -Fdsrc/types -Fpsrc/types -Fasrc/types

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8384342 [details] [diff] [review]
updated patch

Review of attachment 8384342 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/libffi/msvcc.sh
@@ +41,5 @@
>      ;;
>      -O*)
> +      # Optimization flags are not compatible with -RTC1, so
> +      # only set them if FFI_DEBUG is not set.
> +      if [[ $args_orig != *-DFFI_DEBUG* ]]; then

bashisms are usually frowned upon. Use
case $args_orig in
*-DFFI_DEBUG*)
    ...
    ;;
esac

That being said, I think the right fix here is to make D9002 fail instead of warn (if possible), in which case the configure checks for -O3 and -warn all would fail.

@@ +155,5 @@
> +# by MSVC. Add back those optimizations if this is an optimized build.
> +# NOTE: These arguments must come after all others.
> +if [ -n "$opt" ]; then
> +    args="$args -link -OPT:REF -OPT:ICF -INCREMENTAL:NO"
> +fi

You'll still need this hunk, though.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: updated patch

Per the comment above, IRC discussions, and discussion with libffi's maintainer:

* msvcc.sh now returns 1 if an invalid option is sent to cl.exe, making it more useful for feature check tests (instead of just silently eating them and returning 0). Thanks to glandium for that little ditty :). Note that arguments are still printed the same as before (rather than in the ugly form the eval now takes) for readability's sake.
* The ax_cc_maxopt.m4 macro now detects the MSVC compiler and tries to use -O2 by default instead of falling back on the generic "-O3 if nothing else works" logic. The hope is to get this upstreamed to the autoconf folks where this macro originated and then get it landed in libffi and on down from there. I'm working on that now.
* Switches the -DFFI_DEBUG check from the previous format to the more canonical case format. Since it's msvcc.sh that's setting the incompatible -RTC1 flag, I think it's reasonable that msvcc.sh carry the weight of detecting that situation and de-optimizing as needed. Basically, the only reason this worked before was due to luck.
* Now that msvcc.sh returns 1 for invalid options, the -warn check now fails and falls back to the -Wall case msvcc.sh was already handling. This adds -pedantic to the skip list since it is included in the -Wall check.

This works nicely on Try, so I'm hoping this looks good for submitting upstream :).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8388960 [details] [diff] [review]
updated patch

I went ahead and got this upstreamed with a workaround for the -O3 issue included for now. Once autoconf-archive takes the patch for MSVC support, it can be reverted in libffi's tree.

https://github.com/atgreen/libffi/commit/28fb197079cf1d11da4eef7c8c243ab05590c528

Comment 13

[Ryan VanderMeulen [:RyanVM]]

libffi 3.1 has landed and Windows builds are now warning-free in libffi code.