If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla23

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
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
(In reply to comment #2)
> Maybe disabling these warnings completely would be OK... I hate having to
> resort to that, though.

Why?
(Assignee)

Comment 4

5 years ago
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.
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.
(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).
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.
(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?
(Assignee)

Comment 9

5 years ago
GCC doesn't warn for implicit type conversion a
(Assignee)

Comment 10

5 years ago
[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
Ok, so -wd4244 seems like a no-brainer then.
(Assignee)

Updated

5 years ago
Summary: Make the spammiest MSVC warnings always be treated as warnings (the ones exempting it from FAIL_ON_WARNINGS all over the place) → Disable/neuter the spammiest MSVC warnings (the ones exempting MSVC from FAIL_ON_WARNINGS all over the place)
(Assignee)

Comment 12

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

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

Comment 13

5 years ago
Created attachment 733425 [details] [diff] [review]
fix: disable MSVC build warning C4244
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #733425 - Flags: review?(ted)
(Assignee)

Comment 14

5 years ago
(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.)
Attachment #733425 - Flags: review?(ted) → review+
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 858224
(Assignee)

Comment 16

5 years ago
[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)
Summary: Disable/neuter the spammiest MSVC warnings (the ones exempting MSVC from FAIL_ON_WARNINGS all over the place) → Disable MSVC warning C4244, since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place

Updated

5 years ago
Summary: Disable MSVC warning C4244, since it's making us exempt MSVC from FAIL_ON_WARNINGS all over the place → 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
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f832cf6485f
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/6f832cf6485f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Updated

5 years ago
Blocks: 851306
Blocks: 592425
Blocks: 389320
You need to log in before you can comment on or make changes to this bug.