857863 - Disable MSVC warning C4244 ('conversion' conversion from 'type1' to 'type2', possible loss of data), since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

As shown by the third patch on bug 824247, we have a lot of directories that are FAIL_ON_WARNINGS-except-for-MSVC.

As khuey pointed out in a dev.platform post today, this causes pain when a patch builds fine locally but introduces a warning in one of these directories & ends up busting our non-MSVC builds.  If we could get rid of these MSVC exemptions, then our devs who use MSVC would be better-able to catch & address those sorts of issues locally, without burning TBPL.

As I suggested at the end of my response...
  https://groups.google.com/d/msg/mozilla.dev.platform/j-eyeYt9MPg/C7bR_IJ6yDkJ
...I suspect the best way forward is to audit the warnings that MSVC reports for those directories, and treat the spammiest/false-positive-est warnings as warnings even in FAIL_ON_WARNINGS contexts. (assuming MSVC lets us do that)  Then, hopefully, we'll be able to remove the MSVC exemptions, and still report the warnings but not error out on those specific ones.

Comment 1

[Daniel Holbert [:dholbert]]

For reference, bug 716541 is one instance where we did something like this for a GCC warning that had a lot of false positives.

For reference, the "tweak MSVC warnings" chunk of configure.in is here:
https://mxr.mozilla.org/mozilla-central/source/configure.in?rev=81d4bd15053d&mark=2138-2151#2138

Comment 2

[Daniel Holbert [:dholbert]]

I'm actually not sure MSVC supports exactly what I'm talking about.  It looks like you can disable specific warnings completely, but I don't know if you can "neuter" them as I'd like to do.

Maybe disabling these warnings completely would be OK... I hate having to resort to that, though.

Documentation on this is here:
 http://msdn.microsoft.com/en-us/library/thxezb7y.aspx

Comment 3

[(no longer active)]

(In reply to comment #2)
> Maybe disabling these warnings completely would be OK... I hate having to
> resort to that, though.

Why?

Comment 4

[Daniel Holbert [:dholbert]]

Because it feels like a cop-out, for warnings that might point to real bugs (or at least code that's asking for cleanup, like bug 825949). It's nice to at least give the compiler a chance to tell you about it, without necessarily holding ourselves to fixing the many existing false-positive-instances.

Having said that, I don't know the full list of warnings in question; they might be useless enough to just silence.

Comment 5

[(not currently active) Ted Mielczarek]

So I don't think there's a way to disable warnings-as-errors for specific warnings in VC++. I think we have two possible options here:
a) Use -wdNNNN to disable specific spammy warnings.
b) Use -weNNNN to make specific really bad warnings into errors.

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Having said that, I don't know the full list of warnings in question; they
> might be useless enough to just silence.

See bug 824247 comment 12 & 13 (I've mix-up the bug number, sorry).

Comment 7

[(not currently active) Ted Mielczarek]

So the top 3 warnings there are:
C4244 - 'conversion' conversion from 'type1' to 'type2', possible loss of data
C4018 - 'expression' : signed/unsigned mismatch
C4355 - 'this' : used in base member initializer list

We have 1702(!) instances of C4244, that's probably not tractable. All the other warnings in the list have < 50 occurances, and are probably approachable.

Comment 8

[(no longer active)]

(In reply to comment #7)
> So the top 3 warnings there are:
> C4244 - 'conversion' conversion from 'type1' to 'type2', possible loss of data

This is actually a very useful warning at least for some of the types.  I fixed a bug where we were clobbering the high word of a 64-bit number because it was implicitly cast to a 32-bit value a few weeks ago...

> C4018 - 'expression' : signed/unsigned mismatch

This we have enabled on gcc and clang.  Not sure how useful it is in practice.

> C4355 - 'this' : used in base member initializer list

This is actually an indicator of a bug in most cases.

> We have 1702(!) instances of C4244, that's probably not tractable. All the
> other warnings in the list have < 50 occurances, and are probably approachable.

The high number of C4244 makes me really sad.  Perhaps MSVC is just more aggressive than gcc is about implicit type conversion warnings?

Comment 9

[Daniel Holbert [:dholbert]]

GCC doesn't warn for implicit type conversion a

Comment 10

[Daniel Holbert [:dholbert]]

[sorry, cat hit my tab key]
GCC doesn't warn for implicit type conversion at all, by default (with -Wall), IIRC. See:
 http://gcc.gnu.org/wiki/NewWconversion#Frequently_Asked_Questions

Comment 11

[(not currently active) Ted Mielczarek]

Ok, so -wd4244 seems like a no-brainer then.

Comment 12

[Daniel Holbert [:dholbert]]

Turns out we already have -wd4244 in js/src/configure.in. :)

Patch coming up to disable it for the toplevel configure.in, too.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix: disable MSVC build warning C4244

Comment 14

[Daniel Holbert [:dholbert]]

(The patch also shifts the "khuey says we can safely ignore MSVC warning C4251" comment a few lines, to preserve the numeric ordering (by warning ID) of the explanatory comments.)

Comment 15

[Daniel Holbert [:dholbert]]

I verified locally that the patch successfully disables the warning. With the patch applied, we still get 26 instances of C4244, but they're all in nss / nsprpub, since those are built with their own compile options.

Comment 16

[Daniel Holbert [:dholbert]]

[Clarifying bug summary. Per comment 7, 8, & 10, it seems pretty clear that C4244 is by far the main culprit here. If we end up needing to fiddle with other warnings, we can do so in another bug.]

I filed bug 858224 on removing the MSVC exemptions from FAIL_ON_WARNINGS, once this is fixed. (and addressing any hopefully-minor fallout from that)

Comment 17

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f832cf6485f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f832cf6485f