Replace blanket disabling of VS2015 warnings with disabling of specific warnings

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: briansmith, Assigned: gps)

Tracking

(Blocks: 11 bugs)

Trunk
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

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.
No longer blocks: 1119082
(Assignee)

Comment 1

3 years ago
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...
Flags: needinfo?(ehsan)
(Assignee)

Comment 2

3 years ago
Created attachment 8729793 [details]
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.
Component: CSS Parsing and Computation → Build Config
Summary: Replace blankett disabling of VS2015 warnings with disabling of specific warnings → Replace blanket disabling of VS2015 warnings with disabling of specific warnings
(Assignee)

Comment 3

3 years ago
Created attachment 8729798 [details]
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8729802 [details]
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).
(Assignee)

Comment 5

3 years ago
Created attachment 8729803 [details] [diff] [review]
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8729804 [details] [diff] [review]
warnings.diff

This time with -U0 instead of -U1. Sorry for the bugspam.
Attachment #8729803 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1256024
(Assignee)

Updated

3 years ago
Depends on: 1256025
(Assignee)

Updated

3 years ago
Depends on: 1256027
(Assignee)

Updated

3 years ago
Depends on: 1256028
(Assignee)

Updated

3 years ago
Depends on: 1256029

Comment 7

3 years ago
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.)
Flags: needinfo?(ehsan)
(Assignee)

Comment 8

3 years ago
Created attachment 8730385 [details]
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/
Attachment #8730385 - Flags: review?(ehsan)

Comment 9

3 years ago
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.
Attachment #8730385 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1256464
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1256029
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1256025
(Assignee)

Updated

3 years ago
Depends on: 1256473
(Assignee)

Updated

3 years ago
Depends on: 1256482
(Assignee)

Updated

3 years ago
Depends on: 1256484
(Assignee)

Comment 12

3 years ago
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/
Attachment #8730385 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1256490
(Assignee)

Updated

3 years ago
Depends on: 1256492
(Assignee)

Updated

3 years ago
Depends on: 1256498
(Assignee)

Updated

3 years ago
Depends on: 1256501
(Assignee)

Updated

3 years ago
Depends on: 1256505
(Assignee)

Updated

3 years ago
Depends on: 1256509
(Assignee)

Updated

3 years ago
Depends on: 1256514
(Assignee)

Updated

3 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
(Assignee)

Comment 13

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

Comment 16

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

Updated

3 years ago
Depends on: 1256530
(Assignee)

Updated

3 years ago
Depends on: 1256533
(Assignee)

Updated

3 years ago
Depends on: 1256535
(Assignee)

Updated

3 years ago
Depends on: 1256545
(Assignee)

Updated

3 years ago
Depends on: 1256548
(Assignee)

Updated

3 years ago
Depends on: 1256552
(Assignee)

Updated

3 years ago
Depends on: 1256558
(Assignee)

Updated

3 years ago
Depends on: 1256559
(Assignee)

Updated

3 years ago
Depends on: 1256750

Comment 17

3 years ago
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

3 years ago
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.
Attachment #8730385 - Flags: review?(ehsan) → review+
(Assignee)

Updated

3 years ago
Depends on: 1257036
(Assignee)

Updated

3 years ago
Depends on: 1257040
(Assignee)

Comment 19

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

Updated

3 years ago
Depends on: 1257303
(Assignee)

Updated

3 years ago
Depends on: 1257305
(Assignee)

Updated

3 years ago
Depends on: 1257317
(Assignee)

Updated

3 years ago
Depends on: 1257320
(Assignee)

Updated

3 years ago
Depends on: 1257325
(Assignee)

Updated

3 years ago
Depends on: 1257331
(Assignee)

Updated

3 years ago
Depends on: 1257346
(Assignee)

Updated

3 years ago
Depends on: 1257350
(Assignee)

Comment 20

3 years ago
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.
Flags: needinfo?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1257408
(Assignee)

Updated

3 years ago
Depends on: 1257410
(Assignee)

Updated

3 years ago
Depends on: 1257411
(Assignee)

Comment 21

3 years ago
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

3 years ago
(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?
Flags: needinfo?(ehsan) → needinfo?(gps)
(Assignee)

Comment 23

3 years ago
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.
Flags: needinfo?(gps)

Comment 24

3 years ago
Fantastic!
> WebRTC, ICU, and Chromium sandbox are the biggest offenders there.

That's a familiar pattern :/
Created attachment 8732578 [details]
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/
Attachment #8732578 - Flags: review?(cpearce)
Comment on attachment 8732578 [details]
MozReview Request: 1124033 - Use std::deque<int32_t> instead of nsDeque - r?cpearce

Wrong bug number
Attachment #8732578 - Attachment is obsolete: true
Attachment #8732578 - Flags: review?(cpearce)
(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.
(Assignee)

Comment 29

3 years ago
Created attachment 8732972 [details]
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/
Attachment #8732972 - Flags: review?(ehsan)
(Assignee)

Comment 30

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

Comment 31

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

Comment 32

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

Updated

3 years ago
Depends on: 1258579
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1121290
(Assignee)

Comment 34

3 years ago
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

3 years ago
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.
Attachment #8732972 - Flags: review?(ehsan) → review+
(Assignee)

Updated

3 years ago
Depends on: 1256502
(Assignee)

Updated

3 years ago
Blocks: 1259291
(Assignee)

Updated

3 years ago
Blocks: 1259293
(Assignee)

Updated

3 years ago
Blocks: 1257317
No longer depends on: 1257317
(Assignee)

Updated

3 years ago
Blocks: 1257346
No longer depends on: 1257346
(Assignee)

Updated

3 years ago
Blocks: 1259304
(Assignee)

Updated

3 years ago
Blocks: 1259308
(Assignee)

Updated

3 years ago
Blocks: 1259310
(Assignee)

Updated

3 years ago
Blocks: 1257411
No longer depends on: 1257411
(Assignee)

Updated

3 years ago
Blocks: 1257323
(Assignee)

Updated

3 years ago
Blocks: 1259315
(Assignee)

Updated

3 years ago
Blocks: 1259317
(Assignee)

Updated

3 years ago
Blocks: 1259319
(Assignee)

Updated

3 years ago
Blocks: 1259320

Comment 38

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24210e21e08a
https://hg.mozilla.org/mozilla-central/rev/a06363bdde17
https://hg.mozilla.org/mozilla-central/rev/3e56c2c50197
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.