1124033 - Replace blanket disabling of VS2015 warnings with disabling of specific warnings

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In bug 1119072, I added the /Wv flag to /configure.in and js/src/configure.in to silence ALL warnings that are new in VS2015, in order to get warnings-as-errors builds to build. However, it would be better to be more judicious in disabling warnings. In particular, it seems like the interpretation of printf patterns has changed since earlier versions of MSVC, and the warnings about printf patterns may be useful.

Comment 1

[Gregory Szorc [:gps]]

This is blocking the vs2015 tracker bug and therefore blocks the rollout of VS2015.

ehsan: could you please weigh in on the importance of this bug? Does it block rollout or is it something we can do after?

Here is out current code for cflags on vs2015: https://dxr.mozilla.org/mozilla-central/rev/946ed22cad04431c75ab5093989dfedf1bae5a3e/old-configure.in#483. We are currently passing /Wv:18, which will disable all new warnings in Visual Studio 2015. This scares me somewhat. But I don't know how we typically handle new warnings when we upgrade toolchains...

Comment 2

[Gregory Szorc [:gps]]

Attachment: VS2015 build log

Here is the output from `mach build` when building on VS2015 *without* /Wv:18.

It's also apparent that mach's warning detection code is somewhat broken, as it only detects 17 compiler warnings despite dozens being present.

Comment 3

[Gregory Szorc [:gps]]

Attachment: warnings.txt

I fixed the Visual Studio warning detection code and now we see 375 warnings with /Wv:18 removed. The list of warnings is attached.

Comment 4

[Gregory Szorc [:gps]]

Attachment: warnings-suppressed.txt

Here is the list of warnings with the existing configuration that disables warnings via /Wv:18. It shows 158 fewer warnings (217 compared to 375).

Comment 5

[Gregory Szorc [:gps]]

Attachment: warnings.diff

Here is a diff of warnings between /Wv:18 and no /Wv:18. In other words, this is the set of warnings that are currently being suppressed that this bug tracks exposing/fixing.

Comment 6

[Gregory Szorc [:gps]]

Attachment: warnings.diff

This time with -U0 instead of -U1. Sorry for the bugspam.

Comment 7

[(no longer active)]

C5026 and C5027 seem pretty pointless, so I think it's OK to disable those explicitly.  The rest of the warnings need to be looked at, I think, and not disabled en masse.  Especially the printf format string warnings.  The narrowing ones may also be masking real bugs.

(Note that we may decide to disable these warnings in third-party code, depending on the module owner's willingness to deal with them.)

Comment 8

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

Wv:18 was added in bug 1119072 as a quick way to get the tree building
with VS2015. Now that we're closer to rolling out VS2015 support, we
want to expose its new warnings.

This patch replaces the blanket disabling of new warnings in VS2015 with
just disabling C5026 and C5027, which relate to symbols being implicitly
defined as deleted.

Review commit: https://reviewboard.mozilla.org/r/39827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39827/

Comment 9

[(no longer active)]

Comment on attachment 8730385 [details]
MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

https://reviewboard.mozilla.org/r/39827/#review36433

r+ with these only applied to CXXFLAGS.

::: old-configure.in:488
(Diff revision 1)
> -            # -Wv:18 disables all warnings introduced after VS2013
> -            # See http://blogs.msdn.com/b/vcblog/archive/2014/11/12/improvements-to-warnings-in-the-c-compiler.aspx
> -            CFLAGS="$CFLAGS -Wv:18"
> -            CXXFLAGS="$CXXFLAGS -Wv:18"
> +            # C5026: move constructor was implicitly defined as deleted
> +            CFLAGS="$CFLAGS -wd5026"
> +            CXXFLAGS="$CXXFLAGS -wd5026"
> +
> +            # C5027: move assignment operator was implicitly defined as deleted
> +            CFLAGS="$CFLAGS -wd5027"

You don't need either of these in CFLAGS, as they're both inapplicable to C.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8730385 [details]
MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39827/diff/1-2/

Comment 13

[Gregory Szorc [:gps]]

OK. I /think/ I found every warning turned error as a result of removing /Wv:18 and have patches/reviews in flight to disable those warnings. I think I'll go ahead and land the patch in this bug because it will expose the warnings to developers running VS2015 locally. This will break VS2015 with warnings as errors enabled. But my understanding is developers don't run warnings as errors locally, so this should have minimal impact. Please correct me if I'm wrong and I'll hold off landing until all the blocking bugs have landed.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to Gregory Szorc [:gps] from comment #13)
> But my understanding is developers don't run warnings as errors locally

Count me in as a counter-example. I use --enable-warnings-as-errors on Linux, Mac and Windows :)

Comment 15

[Masatoshi Kimura [:emk]]

I explicitly enables warnings-as-errors locally because I would not like to waste try server resources. But I can live without it until the official switch to VS2015.

Comment 16

[Gregory Szorc [:gps]]

I don't want to disrupt anybody without just cause. So I'll hold off landing this until the warnings/errors are all fixed.

Also, I found more warnings in automation. Just when I thought I had flushed everything out...

Comment 17

[(no longer active)]

Hmm, so I looked at a few of the dependencies filed against this, and it looks like you're submitting patches everywhere to disable the warnings where we have them.  I don't think that's necessarily the right way to approach this.  In comment 7, I really meant it when I said these warnings need to be looked at, not just disabled...  But I also don't want to spend the time to have this discussion in every one of those bugs.  :/  Disabling these warnings per directory is not necessarily better than the status quo.

Comment 18

[(no longer active)]

Comment on attachment 8730385 [details]
MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

https://reviewboard.mozilla.org/r/39827/#review36691

This patch is fine, but I'm not happy about the state of this...  See comment 17 please.

Comment 19

[Gregory Szorc [:gps]]

(In reply to :Ehsan Akhgari from comment #17)
> Hmm, so I looked at a few of the dependencies filed against this, and it
> looks like you're submitting patches everywhere to disable the warnings
> where we have them.  I don't think that's necessarily the right way to
> approach this.  In comment 7, I really meant it when I said these warnings
> need to be looked at, not just disabled...  But I also don't want to spend
> the time to have this discussion in every one of those bugs.  :/  Disabling
> these warnings per directory is not necessarily better than the status quo.

I agree with you. That's why in the commit messages I've been encouraging reviewers to reject the patch and fix the warning. So far, I'd say about half the reviewers have pushed back and written patches to fix the warnings. In general, the places where this isn't the case is where the warning is in third party code, which we don't have much control over.

Comment 20

[Gregory Szorc [:gps]]

ehsan: this bug is dragging on and I worry it is jeopardizing our planned transition to VS2015 this release cycle. Nearly everything else is taken care of except this bug. Or at least I think other things are in a state where we can talk about trialing VS2015 on Nightly for a few days, even if there are a few regressions.

In the spirit of perfect is the enemy of done, would you support replacing the /Wv:18 with say /wd5026 /wd5027 (already r+ in this bug) along with /wd4302 /wd4312 and possibly /wd4838? The goal is to unblock the switch to VS2015 and then follow up with fixing the new warnings later - before the end of the release cycle. The way I see it, I'd rather have the new toolchain bake a few extra days or weeks with extra warnings disabled than delay landing the new toolchain. But this isn't my area of expertise and I'd insist on your approval.

Comment 21

[Gregory Szorc [:gps]]

2 days after I thought I found all warnings (see lucky comment #13), I finally found all warnings. Turns out there were a lot more warnings on 64-bit builds. It didn't help that I was taking shortcuts and only testing 32-bit opt builds at times to find warnings. Lesson learned.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67fec88c4f61 contains a push on top of a recent mozilla-central head (5cfc10c14aae) that has green builds with VS2015 everywhere. If you want to build that locally:

  $ hg pull -r 56546c312b9b https://hg.mozilla.org/try

The first changeset in that DAG head is 987d72a100b8. So if you want to rebase e.g. on top of latest central, do something like (assuming firefoxtree enabled):

  $ hg pull central
  $ hg pull -r 56546c312b9b https://hg.mozilla.org/try
  $ hg rebase -s 987d72a100b8 -d central
    (or hg rebase -b 56546c312b9b -d central)

Some of the commits in that DAG head have already landed on inbound. So the rebase could prune changesets that result in empty diffs depending on when you do this. Hopefully there should be no conflicts.

If you just want to build with VS2015 on Try, you can graft/cherry-pick https://hg.mozilla.org/try/rev/07bffae7ec36. If you do that, make sure you have the follow-up from bug 1137561 (https://hg.mozilla.org/integration/mozilla-inbound/rev/22b32ab5ce8f) or you'll hit a compilation failure.

Comment 22

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #20)
> ehsan: this bug is dragging on and I worry it is jeopardizing our planned
> transition to VS2015 this release cycle. Nearly everything else is taken
> care of except this bug. Or at least I think other things are in a state
> where we can talk about trialing VS2015 on Nightly for a few days, even if
> there are a few regressions.
> 
> In the spirit of perfect is the enemy of done, would you support replacing
> the /Wv:18 with say /wd5026 /wd5027 (already r+ in this bug) along with
> /wd4302 /wd4312 and possibly /wd4838? The goal is to unblock the switch to
> VS2015 and then follow up with fixing the new warnings later - before the
> end of the release cycle. The way I see it, I'd rather have the new
> toolchain bake a few extra days or weeks with extra warnings disabled than
> delay landing the new toolchain. But this isn't my area of expertise and I'd
> insist on your approval.

Sure.

Can we disable these warnings per-directory for now?  Or do we hit some of them in headers that are frequently included all over the place?

Comment 23

[Gregory Szorc [:gps]]

When I wrote comment #20, I was in a state of despair because I had spent the entire day tracking down lingering warnings and felt like the end wasn't near. But I did manage to finally track down all the warnings a few hours later. So globally disabling is probably off the table since the per-directory overrides are all known.

The patch series I've been pushing to Try do per-directory disabling or per-file disabling (if possible). There are a few warnings in commonly included headers. WebRTC, ICU, and Chromium sandbox are the biggest offenders there.

Comment 24

[(no longer active)]

Fantastic!

Comment 25

[Nicholas Nethercote [inactive]]

> WebRTC, ICU, and Chromium sandbox are the biggest offenders there.

That's a familiar pattern :/

Comment 26

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: MozReview Request: 1124033 - Use std::deque<int32_t> instead of nsDeque - r?cpearce

The queue of pending block indexes was implemented using nsDeque where
item pointers were perverted into pure 32-bit numbers, causing a size
mismatch on 64-bit platforms, which was picked by VS2015.

which would be unreasonably-difficult with a pure queue.

Review commit: https://reviewboard.mozilla.org/r/41227/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41227/

Comment 27

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8732578 [details]
MozReview Request: 1124033 - Use std::deque<int32_t> instead of nsDeque - r?cpearce

Wrong bug number

Comment 28

[Nicholas Nethercote [inactive]]

(In reply to Gerald Squelart [:gerald] from comment #26)
> MozReview Request: 1124033 - Use std::deque<int32_t> instead of nsDeque -

I'll advise caution here: froydnj and I have both had surprisingly bad experiences with std::deque performance on Mac recently. froydnj ended up converting a std::deque to nsDeque to fix his problem, and I found a way to avoid using the std::deque in most cases to fix mine.

Comment 29

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1124033 - Disable C4311 and C4312 in directories exhibiting warnings; r?ehsan

There are a long tail of C4311 and C4312 warnings in VS2015. Rather than
wait until all of them are fixed to land VS2015, we're taking the easy
way out and disabling these warnings in every directory currently
exhibiting a warning. This is evil. But it is a lesser evil than
globally disabling C4311 and C4312. At least with this approach new
C4311 and C4312 warnings in directories that aren't suppressing them
shouldn't be introduced.

Review commit: https://reviewboard.mozilla.org/r/41481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41481/

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8730385 [details]
MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39827/diff/2-3/

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8732972 [details]
MozReview Request: Bug 1124033 - Disable C4311 and C4312 in directories exhibiting warnings; r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41481/diff/1-2/

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8730385 [details]
MozReview Request: Bug 1124033 - Replace blanket disabling of VS2015 warnings with C5026 and C5027; r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39827/diff/3-4/

Comment 34

[Gregory Szorc [:gps]]

We're getting really close to closing this bug out. Only a handful of blocking bugs not related to C4311 and C4312 (which I'd like to mass disable because getting them fixed is taking a while).

I'm not working on Tuesday. Any help closing out blockers would be much appreciated. Maybe, just maybe, we can land VS2015 on Wednesday (or even Tuesday evening if I'm around).

Latest green Try push with all the patches needed to build with VS2015 in automation is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d0d878ce72f.

Comment 35

[(no longer active)]

Comment on attachment 8732972 [details]
MozReview Request: Bug 1124033 - Disable C4311 and C4312 in directories exhibiting warnings; r?ehsan

https://reviewboard.mozilla.org/r/41481/#review38239

I guess this is fine.  I'm assuming that there are bugs on file for all of these.  If not, please file them.  Thanks!

::: security/sandbox/staticruntime/moz.build:36
(Diff revision 2)
>          CXXFLAGS += [
>              '-wd4275', # non dll-interface class exception used as base for dll-interface class
>              '-wd4717', # recursive on all control paths, function will cause runtime stack overflow
>              '-wd4996', # 'GetVersionExW': was declared deprecated
>              '-wd4302', # 'reinterpret_cast': truncation from 'LPCSTR' to 'WORD'
> +            '-wd4311',

Nit: please add a comment here similar to the surrounding ones.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24210e21e08a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a06363bdde17

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e56c2c50197

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24210e21e08a
https://hg.mozilla.org/mozilla-central/rev/a06363bdde17
https://hg.mozilla.org/mozilla-central/rev/3e56c2c50197