Closed Bug 1407343 Opened 8 years ago Closed 7 years ago

Silence, Ignore, or Otherwise Resolve certain classes of MinGW warnings through compiler and build flags

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

(Whiteboard: [tor])

Attachments

(1 file, 4 obsolete files)

Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set
59 bytes, text/x-review-board-request
glandium
: review+
Details
We want to ignore certain (additional, see Bug 1393222, Bug 1393216) classes of warnings in MinGW. Other times, certain flags shouldn't be set for MinGW. Finally, we're going to whitelist some particularly problematic directories for now while we work on re-enabling warnings-as-errors for at least most of the tree.
This will be a WIP bug, so I'm going to be adding to the patches for a bit before requesting review on them (or at least the ones I expect to be adding to), but if anyone objects to something they see in-progress, feel free to raise issues early!
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review193290 What a violation of "don't repeat yourself"...but I'm not sure there's a better way to handle this.
Attachment #8917078 - Flags: review?(nfroyd) → review+
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review193330 ::: build/moz.configure/warnings.configure:110 (Diff revision 1) > check_and_add_gcc_warning('-Wno-format', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > > +# Add compile-time warnings for unprotected functions and format functions > +# that represent possible security problems. Enable this only when -Wformat > +# is enabled, otherwise it is an error > +check_and_add_gcc_warning('-Wformat-security', > + when=depends(target)(lambda t: t.kernel != 'WINNT')) > +check_and_add_gcc_warning('-Wformat-overflow=2', > + when=depends(target)(lambda t: t.kernel != 'WINNT')) > + > # When compiling for Windows with gcc, we encounter lots of "#pragma warning"'s > # which is an MSVC-only pragma that GCC does not recognize. > check_and_add_gcc_warning('-Wno-unknown-pragmas', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > > # When compiling for Windows with gcc, gcc throws false positives and true > # positives where the callsite is ifdef-ed out > check_and_add_gcc_warning('-Wno-unused-function', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > > check_and_add_gcc_warning('-Wno-switch', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-write-strings', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-unused-value', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-unused-variable', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-unused-but-set-variable', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-ignored-qualifiers', > when=depends(target)(lambda t: t.kernel == 'WINNT')) > check_and_add_gcc_warning('-Wno-parentheses', with only_when(depends(target)(lambda t: t.kernel == 'WINNT')): check_and_add_gcc_warning('-Wno-format') check_and_add_gcc_warning('-Wformat-security') check_and_add_gcc_warning('-Wformat-overflow=2') (...)
Attachment #8919838 - Attachment is obsolete: true
Attachment #8917076 - Flags: review?(n.nethercote)
Attachment #8917077 - Flags: review?(n.nethercote)
Attachment #8919601 - Flags: review?(n.nethercote)
Okay, along with Bug 1412611, the changes in this bug currently let us turn on warning as errors on MinGW. Excepting the directories included in the patch here, and ignoring the warning classes I ignore in the other patch here. I have Bug 1393196 filed for the warnings in gfx and Bug 1393212 for ipc. I'll file bugs to eventually fix up and remove the following classes, but I don't expect to get to it anytime soon. (The ones not mentioned here would remain) > check_and_add_gcc_warning('-Wno-unused-value') > check_and_add_gcc_warning('-Wno-unused-variable') > check_and_add_gcc_warning('-Wno-unused-but-set-variable') > check_and_add_gcc_warning('-Wno-comment') > check_and_add_gcc_warning('-Wno-write-strings') > check_and_add_gcc_warning('-Wno-ignored-qualifiers') > check_and_add_gcc_warning('-Wno-parentheses') > check_and_add_gcc_warning('-Wno-enum-compare')
Comment on attachment 8917076 [details] Bug 1407343 Silence multiple classes of annoying warnings https://reviewboard.mozilla.org/r/188090/#review199294 I'm not a build system peer so I'm not a suitable reviewer for this, so I'll forward to glandium. But I would prefer it if the relevant warnings were fixed rather than disabling them. ::: build/moz.configure/warnings.configure:128 (Diff revision 5) > -# positives where the callsite is ifdef-ed out > + # positives where the callsite is ifdef-ed out > -check_and_add_gcc_warning('-Wno-unused-function', > - when=depends(target)(lambda t: t.kernel == 'WINNT')) > + check_and_add_gcc_warning('-Wno-unused-function') > + > + # When compiling for Windows with gcc, gcc cannot produce this warning > + # correctly, it mistakes DWORD_PTR and ULONG_PTR as types you cannot > + # give NULL to. (You can in fact do that.) It's good that you've explained here why the warning should be disabled. ::: build/moz.configure/warnings.configure:131 (Diff revision 5) > + # When compiling for Windows with gcc, gcc cannot produce this warning > + # correctly, it mistakes DWORD_PTR and ULONG_PTR as types you cannot > + # give NULL to. (You can in fact do that.) > + check_and_add_gcc_warning('-Wno-conversion-null') > + > + # For now, silence low priority gcc warnings globally Why are these low value? ::: build/moz.configure/warnings.configure:137 (Diff revision 5) > + check_and_add_gcc_warning('-Wno-unused-value') > + check_and_add_gcc_warning('-Wno-unused-variable') > + check_and_add_gcc_warning('-Wno-unused-but-set-variable') > + > + # Warn whenever a comment-start sequence ‘/*’ appears in a ‘/*’ comment, or > + # whenever a backslash-newline appears in a ‘//’ comment. For all of these ones the comment describes what the warning does, instead of the reason why it's disabled. I think the latter is more useful. ::: build/moz.configure/warnings.configure:143 (Diff revision 5) > + check_and_add_gcc_warning('-Wno-comment') > + # Warn whenever a switch statement has an index of enumerated type and lacks > + # a case for one or more of the named codes of that enumeration. > + check_and_add_gcc_warning('-Wno-switch') > + # When compiling C, give string constants the type const char[length] so that > + # copying the address of one into a non-const char * pointer produces a warning. "char*" instead of "char *"
Attachment #8917076 - Flags: review?(n.nethercote)
Attachment #8917076 - Flags: review?(mh+mozilla)
Comment on attachment 8917077 [details] Bug 1407343 Allow compiler warnings for MinGW in certain whitelisted directories: gfx, ipc/chromium https://reviewboard.mozilla.org/r/188092/#review199296 Again, a build peer should review this. Having said that, I personally would put "XXX:" in front of all the "We allow..." comments to indicate more clearly that it's something that you're intending to fix.
Attachment #8917077 - Flags: review?(n.nethercote)
Comment on attachment 8919601 [details] Bug 1407343 Re-enable warnings as errors for the mingw build https://reviewboard.mozilla.org/r/190476/#review199298
Attachment #8919601 - Flags: review?(n.nethercote)
Attachment #8917077 - Flags: review?(mh+mozilla)
Attachment #8919601 - Flags: review?(mh+mozilla)
> I would prefer it if the relevant warnings were > fixed rather than disabling them. If the disablings are intended to be temporary, then the comments should make that clearer. Thanks.
Comment on attachment 8917076 [details] Bug 1407343 Silence multiple classes of annoying warnings https://reviewboard.mozilla.org/r/188090/#review199878 njn's comments are all spot on.
Attachment #8917076 - Flags: review?(mh+mozilla)
Comment on attachment 8917077 [details] Bug 1407343 Allow compiler warnings for MinGW in certain whitelisted directories: gfx, ipc/chromium https://reviewboard.mozilla.org/r/188092/#review199880 Considering this change and the previous, and the fact that those builds are tier 2, meaning they can bust and things won't be backed out for it, I question the value of trying to use --enable-warnings-as-errors at all.
Attachment #8917077 - Flags: review?(mh+mozilla)
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review199882 ::: build/moz.configure/warnings.configure:109 (Diff revision 7) > check_and_add_gcc_warning('-Wformat') > > # We use mix of both POSIX and Win32 printf format across the tree, so format > # warnings are useless on mingw. > check_and_add_gcc_warning('-Wno-format', > when=depends(target)(lambda t: t.kernel == 'WINNT')) That seems backwards... You end up adding *both* -Wformat and -Wno-format. You should probably fix that such that -Wformat is only added if target.kernel != 'WINNT'. ::: build/moz.configure/warnings.configure:120 (Diff revision 7) > > +# Add compile-time warnings for unprotected functions and format functions > +# that represent possible security problems. Enable this only when -Wformat > +# is enabled, otherwise it is an error > +check_and_add_gcc_warning('-Wformat-security', > + when=depends(target)(lambda t: t.kernel != 'WINNT')) It would be even better if this actually depended on -Wformat being added. Which would be possible if check_and_add_gcc_warning would be made to return all its result functions. Something like: diff --git a/build/moz.configure/compile-checks.configure b/build/moz.configure/compile-checks.configure index c50914c33bd36..cfc190a3bc940 100644 --- a/build/moz.configure/compile-checks.configure +++ b/build/moz.configure/compile-checks.configure @@ -109,6 +109,8 @@ def check_and_add_gcc_warning(warning, compiler=None, when=None, check=True): if when is None: when = always + results = [] + for c in compilers: assert c in (c_compiler, cxx_compiler) lang, warnings_flags = { @@ -145,6 +147,10 @@ def check_and_add_gcc_warning(warning, compiler=None, when=None, check=True): if result: warnings_flags.append(warning) + results.append(result) + + return results + # Add the given warning to the list of warning flags for the build. # - `warning` is the warning flag (e.g. -Wfoo) # - `compiler` (optional) is the compiler to add the flag for (c_compiler or Then you can do something like: c_format_warning, cxx_format_warning = check_and_add_gcc_warning('-Wformat', when=depends(target)(lambda t: t.kernel != 'WINNT')) check_and_add_gcc_warning('-Wformat-security', when=c_format_warning & cxx_format_warning) etc. ::: dom/security/moz.build:53 (Diff revision 7) > CFLAGS += ['-Wformat-security'] > CXXFLAGS += ['-Wformat-security'] That's redundant with the flags being added at configure time... Just remove this. FWIW, this was added last year, while the warnings have been added globally a few months ago. These lines should have been removed then, but haven't.
Comment on attachment 8919601 [details] Bug 1407343 Re-enable warnings as errors for the mingw build https://reviewboard.mozilla.org/r/190476/#review199886 As mentioned earlier, I really don't think it's worth doing, unless if you're going to make them tier 1.
Attachment #8919601 - Flags: review?(mh+mozilla)
I'm convinced, let's not do warnings-as-errors. I was driving towards that because I thought not doing it was discouraged, but I'm happy to avoid the extra breakage that will come with it. I'll remove the -Wno- entries that could be fixed later but keep the ones that throw false positives.
Attachment #8917076 - Attachment is obsolete: true
Attachment #8917077 - Attachment is obsolete: true
Attachment #8919601 - Attachment is obsolete: true
Attachment #8917078 - Flags: review?(n.nethercote)
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review203676 I'm still not a build system peer :) glandium is a better reviewer for this.
Attachment #8917078 - Flags: review?(n.nethercote)
Attachment #8917078 - Flags: review?(mh+mozilla)
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review204374 ::: build/moz.configure/compile-checks.configure:152 (Diff revision 8) > if result: > warnings_flags.append(warning) > > + results.append(result) > + > + return results maybe return tuple(results) ::: build/moz.configure/warnings.configure:135 (Diff revision 8) > + # When compiling for Windows with gcc, gcc cannot produce this warning > + # correctly: it mistakes DWORD_PTR and ULONG_PTR as types you cannot > + # give NULL to. (You can in fact do that.) > + check_and_add_gcc_warning('-Wno-conversion-null') > + > + # Throughout the codebase we regurally have switch statements off of enums typo: regularly. ::: build/moz.configure/warnings.configure:135 (Diff revision 8) > + # Throughout the codebase we regurally have switch statements off of enums > + # without covering every value in the enum. We don't care about these warnings. > + check_and_add_gcc_warning('-Wno-switch') > + > + # Another code pattern we have is using start and end constants in enums of > + # different types. We do this for safety, but then when comparing it throws > + # an error, which we would like to ignore. > + check_and_add_gcc_warning('-Wno-enum-compare') why is it specifically something we need on mingw gcc, and not all builds?
Attachment #8917078 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #43) > ::: build/moz.configure/warnings.configure:135 > (Diff revision 8) > > + # Throughout the codebase we regurally have switch statements off of enums > > + # without covering every value in the enum. We don't care about these warnings. > > + check_and_add_gcc_warning('-Wno-switch') > > + > > + # Another code pattern we have is using start and end constants in enums of > > + # different types. We do this for safety, but then when comparing it throws > > + # an error, which we would like to ignore. > > + check_and_add_gcc_warning('-Wno-enum-compare') > > why is it specifically something we need on mingw gcc, and not all builds? I can't figure this one out actually. I don't see anything that would cause this warning to be silenced on the normal linux build, the gcc versions are the same (6.4.0), there's nothing in the MinGW source code.
Attachment #8917078 - Flags: review?(n.nethercote)
Comment on attachment 8917078 [details] Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set https://reviewboard.mozilla.org/r/188094/#review204630 Please add a note that somehow those are only causing problems on mingw-gcc.
Attachment #8917078 - Flags: review?(mh+mozilla) → review+
Attachment #8917078 - Flags: review?(n.nethercote)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 22a3ac60eeff -d 962c46110aac: rebasing 434210:22a3ac60eeff "Bug 1407343 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set r=froydnj,glandium" (tip) merging build/moz.configure/compile-checks.configure merging build/moz.configure/warnings.configure merging dom/security/moz.build warning: conflicts while merging build/moz.configure/compile-checks.configure! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/security/moz.build! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(tom)
Attachment #8917078 - Flags: review?(n.nethercote)
Attachment #8917078 - Flags: review?(n.nethercote)
Flags: needinfo?(tom)
Attachment #8917078 - Flags: review?(n.nethercote)
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e972f5af4fc4 Silence multiple classes of warnings for the MinGW build, including not enabling format warnings unless -Wformat is set r=froydnj,glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e972f5af4fc4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: