Closed Bug 1090497 (winclang-werror) Opened 10 years ago Closed 6 years ago

Re-enable warnings as errors on clang-cl


(Firefox Build System :: General, defect)

Not set


(firefox63 fixed)

Tracking Status
firefox63 --- fixed


(Reporter: ehsan.akhgari, Assigned: emk)


(Depends on 2 open bugs)



(1 file)

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:

  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

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:

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

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, and not from js/src/ 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

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
Re-enable warnings as errors on clang-cl. r=froydnj
Backed out for build bustages on several files

Push with failures:

Failure log:

Backout link:

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/ 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
Re-enable warnings as errors on clang-cl. r=froydnj
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1481511
Blocks: 1481719
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.