1090497 - (winclang-werror) Re-enable warnings as errors on clang-cl

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

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.

Comment 1

[(Away)]

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?

Comment 2

[Mike Hommey [:glandium]]

It's not since bug 1403346 but I'm not sure the change was intentional. I didn't catch this during review :(

Comment 3

[Chris Manchester (limited bugmail, email directly)]

I'm pretty sure this still happens, here: https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/python/mozbuild/mozbuild/frontend/context.py#496

Comment 4

[(Away)]

Aha, that's exactly what I was looking for. Thanks Chris!

Comment 5

[(Away)]

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

Comment 6

[(Away)]

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.

Comment 7

[(Away)]

cpeterson, IIRC you've taken interest in similar bugs in the past. Any interest in helping with this one?

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to David Major [:dmajor] from comment #7)
> cpeterson, IIRC you've taken interest in similar bugs in the past. Any
> interest in helping with this one?

Sorry, I don't have time right now.

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. You set -W0 in old-configure.in, but I think you also need to set -W0 in js/src/old-configure.in.

According to the clang manual [1], -W0 will disable all warnings while -W1, -W2, -W3, -W4, and -Wall will enable -Weverything. I don't know why clang-cl -Wall would be different than clang -Wall (which is like gcc -Wall, not clang -Weverything). Maybe this is a bug in the manual.

[1] https://clang.llvm.org/docs/UsersManual.html

Comment 9

[(Away)]

(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.

Comment 10

[(Away)]

I was mistaken about -Wall coming from `add_gcc_warning('-Wall')`. It's actually coming from the security directories: https://searchfox.org/mozilla-central/rev/b80994a43e5d92c2f79160ece176127eed85dcc9/security/pkix/warnings.mozbuild#17 and https://searchfox.org/mozilla-central/rev/b80994a43e5d92c2f79160ece176127eed85dcc9/security/certverifier/moz.build#65

Comment 11

[Masatoshi Kimura [:emk]]

(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.

Comment 12

[Chris Peterson [:cpeterson]]

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

https://clang.llvm.org/docs/UsersManual.html

Comment 13

[(Away)]

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.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Re-enable warnings as errors on clang-cl

We should prevent new warnings from sneaking into directories that are already warning-free.

Comment 15

[(Away)]

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 16

[Nathan Froyd [:froydnj]]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5950c9d63c3b
Re-enable warnings as errors on clang-cl. r=froydnj

Comment 18

[Cosmin Sabou [:CosminS]]

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: 
https://treeherder.mozilla.org/logviewer.html#?job_id=191638820&repo=mozilla-inbound&lineNumber=4740
https://treeherder.mozilla.org/logviewer.html#?job_id=191638816&repo=mozilla-inbound&lineNumber=4626


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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/997fd8419a4b
Re-enable warnings as errors on clang-cl. r=froydnj

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/997fd8419a4b