Closed Bug 1090497 (winclang-werror) Opened 6 years ago Closed 2 years ago
Re-enable warnings as errors on clang-cl
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.
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?
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)
I'm pretty sure this still happens, here: https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/python/mozbuild/mozbuild/frontend/context.py#496
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?
(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.
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
(In reply to Chris Peterson [:cpeterson] from comment #8) > According to the clang manual , -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 https://clang.llvm.org/docs/UsersManual.html
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
Status: NEW → ASSIGNED
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: https://hg.mozilla.org/integration/mozilla-inbound/rev/5950c9d63c3b 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: 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: *** [TestNANTestingExprC.obj] Error 1 16:57:44 INFO - mozmake.EXE: Leaving directory 'z:/build/build/src/obj-firefox/build/clang-plugin/tests' 16:57:44 INFO - mozmake.EXE: *** Waiting for unfinished jobs.... 16:57:44 INFO - mozmake.EXE: 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
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/997fd8419a4b Re-enable warnings as errors on clang-cl. r=froydnj
You need to log in before you can comment on or make changes to this bug.