Closed Bug 1072071 Opened 5 years ago Closed 5 years ago

win64 warning spam is out of control

Categories

(Core :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

As Win64 is getting close to becoming a tier1 platform, we need to do something about the out of control warning spam that one gets when compiling for Win64.  Most of these warnings take the form of:

warning C4267: '$CONTEXT' : conversion from 'size_t' to '$32_BIT_TYPE', possible loss of data

and are caused by nsTArray and its various usages throughout the tree.

Finding actual problems amidst the warning spam is extremely difficult, especially because the messages are verbose, the consoles on win32 have limited scrollback, and the number of messages is...large.  I'm also fairly sure that the sheer number of messages slows down compiles; a win32 build on my machine takes ~35 minutes and a win64 build takes ~55 minutes.  Some of that is probably due to 32/64-bitness, but I'd bet the bulk of it is due to warning spam.

Probably a little weird to have a buildwarning bug that depends on other buildwarning bugs, but I feel it's justified in this case.
Version: unspecified → Trunk
We already disable C4244, which warns on other integer narrowing conversions.  C4267 is just a special case of C4244 that is only enabled for 64-bit compilations, so it should be disabled too.
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #8494492 - Flags: review?(mshal)
glandium expressed some concern over IRC that these warnings are useful, because they do represent the real possibility of data loss, or perhaps security holes.  He saw the existence of these warnings on Win64 as a good thing, because we might be motivated to fix all the warnings, since they're not being silently disregarded, as they are on other platforms.

I don't agree that the warnings on Win64 are any more valuable that warnings on Linux64 or OS X: we have them disabled there, after all, and seemingly without any ill effects.

I briefly looked through the warnings generated from a win64 compile; I'll group them into three broad camps:

1. Things related to nsTArray's interfaces being size_t.  This is not a problem, since nsTArray is limited to INT32_MAX in size anyway, so there's no data loss here.
2. Things related to ns*String.  I'd have to look more carefully at ns*String, but I don't think this is a problem either.
3. Hodgepodge of warnings, typically from third-party code, mostly because sizeof(foo) is now 64 bits.  None of these looked particularly interesting.

I realize it only takes one bad example to make a security hole, but there were ~10-20K (!) instances of this one warning generated, and if you trust my sampling, none of them looked bad.  I'd say it's probably not worth it without some work put in.
Comment on attachment 8494492 [details] [diff] [review]
disable warnings about narrowing size_t to a 32-bit type

>diff --git a/js/src/configure.in b/js/src/configure.in
>--- a/js/src/configure.in
>+++ b/js/src/configure.in
>@@ -1696,18 +1696,21 @@ ia64*-hpux*)
>             CFLAGS="$CFLAGS -FS"
>             CXXFLAGS="$CXXFLAGS -FS"
>         fi
>         # khuey says we can safely ignore MSVC warning C4251
>         # MSVC warning C4244 (implicit type conversion may lose data) warns
>         # and requires workarounds for perfectly valid code.  Also, GCC/clang
>         # don't warn about it by default. So for consistency/sanity, we turn
>         # it off on MSVC, too.
>-        CFLAGS="$CFLAGS -wd4244"
>-        CXXFLAGS="$CXXFLAGS -wd4244 -wd4251"
>+  	    # MSVC warning C4267 warns for narrowing type conversions from size_t
>+	    # to 32-bit integer types on 64-bit platforms.  Since this is virtually
>+	    # the same thing as C4244, we disable C4267, too.

nit: You seem to have stuck some tabs in this comment block.
Attachment #8494492 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/71fa9295bec1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.