Closed Bug 1124033 Opened 9 years ago Closed 8 years ago

Replace blanket disabling of VS2015 warnings with disabling of specific warnings

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: briansmith, Assigned: gps)

References

(Blocks 5 open bugs)

Details

Attachments

(6 files, 2 obsolete files)

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: VC14
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)
Attached file 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
Attached file 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.
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).
Attached patch warnings.diff (obsolete) — Splinter Review
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.
Attached patch warnings.diffSplinter Review
This time with -U0 instead of -U1. Sorry for the bugspam.
Attachment #8729803 - Attachment is obsolete: true
Depends on: 1256024
Depends on: 1256025
Depends on: 1256027
Depends on: 1256028
Depends on: 1256029
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)
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 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)
Depends on: 1256464
Depends on: 1256473
Depends on: 1256482
Depends on: 1256484
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)
Depends on: 1256490
Depends on: 1256492
Depends on: 1256498
Depends on: 1256501
Depends on: 1256505
Depends on: 1256509
Depends on: 1256514
Assignee: nobody → gps
Status: NEW → ASSIGNED
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.
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...
Depends on: 1256530
Depends on: 1256533
Depends on: 1256535
Depends on: 1256545
Depends on: 1256548
Depends on: 1256552
Depends on: 1256558
Depends on: 1256559
Depends on: 1256750
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 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+
Depends on: 1257036
Depends on: 1257040
(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.
Depends on: 1257303
Depends on: 1257305
Depends on: 1257317
Depends on: 1257320
Depends on: 1257325
Depends on: 1257331
Depends on: 1257346
Depends on: 1257350
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)
Depends on: 1257408
Depends on: 1257410
Depends on: 1257411
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.
(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)
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)
Fantastic!
> WebRTC, ICU, and Chromium sandbox are the biggest offenders there.

That's a familiar pattern :/
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.
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)
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 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 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/
Depends on: 1258579
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 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+
Depends on: 1256502
Blocks: 1259291
Blocks: 1259293
Blocks: 1257317
No longer depends on: 1257317
Blocks: 1257346
No longer depends on: 1257346
Blocks: 1259304
Blocks: 1259308
Blocks: 1259310
Blocks: 1257411
No longer depends on: 1257411
Blocks: 1257323
Blocks: 1259315
Blocks: 1259317
Blocks: 1259319
Blocks: 1259320
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.