Bug 1090497 (winclang-werror)

Re-enable warnings as errors on clang-cl

RESOLVED FIXED in Firefox 63


5 years ago
10 months ago


(Reporter: Ehsan, Assigned: emk)


(Depends on 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)



(1 attachment)

I turned it off in bug 1089613.

One case that I was hitting was clang-cl warning about the -Fd argument being unused.  Hopefully at some point that option will be hooked up to something on clang-cl, which would make that warning go away.  Not sure what else would block turning this back on.
Blocks: winclang
Depends on: 1193546
Product: Core → Firefox Build System
With 3 years of build refactoring having passed, I can no longer find where warnings-as-errors is disabled for clang-cl. Is this still an issue?
Flags: needinfo?(mh+mozilla)
It's not since bug 1403346 but I'm not sure the change was intentional. I didn't catch this during review :(
Flags: needinfo?(mh+mozilla) → needinfo?(cmanchester)
Aha, that's exactly what I was looking for. Thanks Chris!
I did a try push where I turned AllowCompilerWarnings() into -W0, so we wouldn't see them, and thus the remaining warnings were ones that would trip warnings-as-errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87227ff1c87b1e1e78693a43af6a777be7726768

  10370 -Wc++98-compat
    945 -Wc++98-compat-pedantic
    876 -Wpragma-pack
    661 -Wzero-as-null-pointer-constant
    420 -Wdisabled-macro-expansion
    290 -Wglobal-constructors
    271 -Wreserved-id-macro
    262 -Wmissing-braces
    259 -Wparentheses
    236 -Wdeprecated-dynamic-exception-spec
    191 -Wgnu-include-next
    125 -Wold-style-cast
    124 -Wincompatible-pointer-types
     82 -Wunused-variable
     80 -Wgnu-zero-variadic-macro-arguments
     79 -Wmissing-prototypes
     59 -Wused-but-marked-unused
     58 -Wlanguage-extension-token
     57 -Wnon-virtual-dtor
     41 -Wc++98-compat-local-type-template-args
     32 -Wunused-const-variable
     31 -Wundef
     31 -Wmissing-noreturn
     30 -Wunused-function
     30 -Wc++98-c++11-compat
     29 -Wformat
     25 -Wmissing-variable-declarations
     24 -Wmacro-redefined
     24 -Wextra-semi
     24 -Wdeprecated-declarations
     21 -Wexit-time-destructors
     21 -Wdocumentation-deprecated-sync
     20 -Wwritable-strings
     20 -Woverloaded-virtual
     14 -Wdocumentation
     11 -Wsign-conversion
     11 -Wheader-hygiene
     11 -Wcovered-switch-default
     10 -Wduplicate-decl-specifier
      9 -Wunused-result
      8 -Wundefined-func-template
      8 -Wdocumentation-unknown-command
      8 -Wcast-qual
      6 -Wunused-template
      6 -Wtypename-missing
      6 -Wsometimes-uninitialized
      6 -Wmicrosoft-goto
      5 -Wmicrosoft-unqualified-friend
      5 -Wlogical-op-parentheses
      5 -Winvalid-noreturn
      5 -Wimplicit-function-declaration
      5 -Wfloat-equal
      5 -Wduplicate-enum
      4 -Wunused-private-field
      4 -Wmicrosoft-template
      4 -Winconsistent-missing-destructor-override
      4 -Winconsistent-dllimport
      4 -Wextern-initializer
      3 -Wunused-lambda-capture
      2 -Wunused-macros
      2 -Wswitch
      2 -Wpragma-pack-suspicious-include
      2 -Wnonportable-system-include-path
      2 -Winfinite-recursion
      2 -Wdouble-promotion
      1 -Wunused-value
      1 -Wunsequenced
      1 -Wswitch-enum
      1 -Wshift-negative-value
      1 -Wparentheses-equality
      1 -Wnested-anon-types
      1 -Wmicrosoft-exception-spec
      1 -Wdangling-else
      1 -Wconversion
      1 -Wcast-calling-convention
Currently we're getting -Wall from https://searchfox.org/mozilla-central/source/build/moz.configure/warnings.configure#20

I wonder if, in the short term, we should disable -Wall when doing -Werror? Getting a basic level of protection in place would be better than leaving -Werror disabled for ages.

>   10370 -Wc++98-compat
>     945 -Wc++98-compat-pedantic

We should probably just disable these two, regardless.
cpeterson, IIRC you've taken interest in similar bugs in the past. Any interest in helping with this one?
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson [:cpeterson] from comment #8)
> Sorry, I don't have time right now.

OK, it was worth a try. Thanks for looking!

> From the list of warnings in comment 5, it appears that -Weverything is set,
> which enables *ALL* warnings. (-Wall is a misnomer because it does not
> enable all warnings.) -Weverything is interesting for testing, but not
> practical for production. 

We really do set -Wall, not -Weverything: https://taskcluster-artifacts.net/Nu6e7PcHQoScUkOWRjzRiA/0/public/logs/live_backing.log

> You set -W0 in old-configure.in, but I think you
> also need to set -W0 in js/src/old-configure.in.

I transformed AllowCompilerWarnings into -W0 in a shared location. It shouldn't matter though because there are no instances of AllowCompilerWarnings under js/src.

I think what you meant is that I removed -WX only from old-configure.in, and not from js/src/old-configure.in. This does point out something quite interesting: my builds succeeded, so we are -WX clean in js/src already! \o/ Note that we didn't pick up -Wall there, so maybe `add_gcc_warning('-Wall')` and friends are not active for js code? In any case this kind of ties into my earlier comment: if we can be -WX clean without -Wall, let's enable that as a first step, and deal with full-clean-ness later.
Alias: winclang-werror
Depends on: 1476474
(In reply to Chris Peterson [:cpeterson] from comment #8)
> According to the clang manual [1], -W0 will disable all warnings while -W1,
> -W2, -W3, -W4, and -Wall will enable -Weverything.

This is not correct. Although clang-cl will translate -W1, -W2, and -W3 into clang -Wall, -W4 into clang -Wall -Wextra, and -Wall into -Weverything, it does not recursively translate the resulting -Wall.
Thanks. You're correct. Here is what the clang manual says:

  /W0                     Disable all warnings
  /W1                     Enable -Wall
  /W2                     Enable -Wall
  /W3                     Enable -Wall
  /W4                     Enable -Wall and -Wextra
  /Wall                   Enable -Weverything

Depends on: 1477744
Great progress here...

    894 -Wpragma-pack # bug 1472254
    518 -Wparentheses # bug 1478000
    268 -Wmissing-braces # bug 1478008
    191 -Wgnu-include-next # bug 1478014 
    143 -Wmicrosoft-template # bug 1478020
    118 -Wmicrosoft-cast
     80 -Wunused-variable
     63 -Wwritable-strings
     58 -Wunused-function
     58 -Wlanguage-extension-token
     54 -Wint-to-void-pointer-cast
     54 -Wdeprecated-declarations
     50 -Wgnu-zero-variadic-macro-arguments
     45 -Wunused-result
     37 -Wreturn-std-move
     28 -Wundef
     28 -Wpessimizing-move
     28 -Wmissing-noreturn
     26 -Woverloaded-virtual
     22 -Wformat
     20 -Wduplicate-decl-specifier
     19 -Wtypename-missing
     19 -Wlogical-op-parentheses
     16 -Wincompatible-pointer-types
     14 -Wsometimes-uninitialized
     12 -Wunused-const-variable
<warnings with fewer than 10 occurrences not listed>

Note that this is from a build where `AllowCompilerWarnings()` is translated into `-W0` so that we only count warnings that would trigger -Werror.
We should prevent new warnings from sneaking into directories that are already warning-free.
Attachment #8996710 - Flags: review?(core-build-config-reviews)
Assignee: nobody → VYV03354
Comment on attachment 8996710 [details] [diff] [review]
Re-enable warnings as errors on clang-cl

I'd like to request that this code be Searchfox-friendly, in other words that we could distinguish these clang-cl workarounds from other search results at https://searchfox.org/mozilla-central/search?q=AllowCompilerWarnings

The easiest way to achieve this would be to put the comment on the same line as AllowCompilerWarnings(). Maybe we could do something more fancy like AllowCompilerWarnings('clang-cl') or something but perhaps that's unnecessary.

The change to security/pkix/warnings.mozbuild should probably be a separate patch or bug.
Comment on attachment 8996710 [details] [diff] [review]
Re-enable warnings as errors on clang-cl

Review of attachment 8996710 [details] [diff] [review]:

Assuming all of these are suitably warning-free...I am a little worried about headers inadvertently causing warnings, but we can see what this looks like in practice.
Attachment #8996710 - Flags: review?(core-build-config-reviews) → review+
Pushed by VYV03354@nifty.ne.jp:
Re-enable warnings as errors on clang-cl. r=froydnj
Backed out for build bustages on several files

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=5950c9d63c3b4fd63a25464a7b50944aaec7079f&tochange=f7f4ba3486b5e44b6ccdc3b150f5aa0aab76ffb7&selectedJob=191638820

Failure log: 

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f4ba3486b5e44b6ccdc3b150f5aa0aab76ffb7

16:57:44     INFO -  clang.exe: error: unknown argument ignored in clang-cl: '-std=c11' [-Werror,-Wunknown-argument]
16:57:44     INFO -  z:/build/build/src/config/rules.mk:768: recipe for target 'TestNANTestingExprC.obj' failed
16:57:44     INFO -  mozmake.EXE[4]: *** [TestNANTestingExprC.obj] Error 1
16:57:44     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/build/clang-plugin/tests'
16:57:44     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....
16:57:44     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/build/clang-plugin/tests'
16:57:44     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/cla
Flags: needinfo?(VYV03354)
Depends on: 1480657
Depends on: 1306642
No longer depends on: 1480657
Flags: needinfo?(VYV03354)
Pushed by VYV03354@nifty.ne.jp:
Re-enable warnings as errors on clang-cl. r=froydnj
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1481511
No longer blocks: 1481719
Depends on: 1491615
Depends on: 1493413
Depends on: 1493414
Depends on: 1493415
You need to log in before you can comment on or make changes to this bug.