Closed
Bug 1026589
Opened 10 years ago
Closed 7 years ago
Enable more GCC/Clang compiler warnings (-Wextra) for security/certverifier
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: cviecco, Assigned: Cykesiopka)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file, 3 obsolete files)
Bug 1010634 added -Wall flags to certverifier, which is great for VS. However in GCC -Wall does not enable all the warning and -Wextra is what we want. This should also be done for Clang
Reporter | ||
Comment 1•10 years ago
|
||
Assignee: nobody → cviecco
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Comment 3•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8441475 -
Flags: review?(brian)
Reporter | ||
Updated•10 years ago
|
Attachment #8441476 -
Flags: review?(brian)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8441477 [details] [diff] [review] no-extra-for-copy-in-string Review of attachment 8441477 [details] [diff] [review]: ----------------------------------------------------------------- There is a warning here due to the copy constructor not using a copy constructor for the base class. I just removed the warning, but I would really appreciate if you could point me to someone who can either r+ or fix the issue.
Attachment #8441477 -
Flags: feedback?(brian)
Comment 5•10 years ago
|
||
Comment on attachment 8441477 [details] [diff] [review] no-extra-for-copy-in-string Review of attachment 8441477 [details] [diff] [review]: ----------------------------------------------------------------- I would ask :ehsan how we wants to deal with this. He reviewed my similar patches in XPCOM.
Attachment #8441477 -
Flags: feedback?(brian)
Comment 6•10 years ago
|
||
Comment on attachment 8441475 [details] [diff] [review] more-warnings Review of attachment 8441475 [details] [diff] [review]: ----------------------------------------------------------------- Please do the same for security/pkix. You shouldn't need any warning exclusions in security/pkix. ::: security/certverifier/moz.build @@ +58,5 @@ > '-wd4127', # conditional expression is constant > '-wd4946', # reinterpret_cast used between related types > ] > > +if CONFIG['GNU_CC']: Could you please add a comment saying which compilers are affected here? Is it just clang or just GCC or both?
Attachment #8441475 -
Flags: review?(brian) → review+
Updated•10 years ago
|
Attachment #8441476 -
Flags: review?(brian) → review+
Comment 7•8 years ago
|
||
Camilo, are your security/certverifier patches still relevant? Brian r+'d them, but they never landed. :)
Flags: needinfo?(cviecco)
Assignee: cviecco → nobody
Flags: needinfo?(cviecco)
Priority: -- → P3
Whiteboard: [psm-cleanup]
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #7) > Camilo, are your security/certverifier patches still relevant? Brian r+'d > them, but they never landed. :) The patches are still relevant - I'll post an unbitrotted version soon.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [psm-cleanup] → [psm-assigned]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8441475 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8441476 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8441477 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8840457 [details] Bug 1026589 - Enable more GCC/Clang compiler warnings (-Wextra) for security/certverifier. https://reviewboard.mozilla.org/r/114958/#review116490 Cool - thanks. ::: security/certverifier/moz.build:81 (Diff revision 1) > <span class="indent">>>>></span> '-wd4710', # 'function': function not inlined > <span class="indent">>>>></span> '-wd4711', # function 'function' selected for inline expansion > <span class="indent">>>>></span> '-wd4820', # 'bytes' bytes padding added after construct 'member_name' > <span class="indent">>></span> ] > > - # MSVC 2010's headers trigger these > + # MSVC 2010's headers trigger these Hmmm - do we support MSVC 2010 any longer? We might be able to prune some of these (probably something for a cleanup bug, though). ::: security/certverifier/moz.build:104 (Diff revision 1) > - ] > + ] > > if CONFIG['GNU_CXX']: > - CXXFLAGS += ['-Wno-error=shadow'] > + CXXFLAGS += [ > + '-Wextra', > + '-Wunreachable-code', Is -Wunreachable-code not in -Wextra? I also noted clang has -Weverything - do we want to do something more like https://dxr.mozilla.org/mozilla-central/source/security/pkix/warnings.mozbuild ?
Attachment #8840457 -
Flags: review?(dkeeler) → review+
Comment 11•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #10) > > - # MSVC 2010's headers trigger these > > + # MSVC 2010's headers trigger these > > Hmmm - do we support MSVC 2010 any longer? We might be able to prune some of > these (probably something for a cleanup bug, though). No. As of Firefox 50 (bug 1186064), we require Visual Studio 2015 Update 2 or later. > Is -Wunreachable-code not in -Wextra? No. clang does not include -Wunreachable-code in -Wall or -Wextra. gcc removed support -Wunreachable-code years ago (because it produced inconsistent results depending on optimization level). The closest thing clang has to documentation for its warning flags is https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td and http://fuckingclangwarnings.com/. > I also noted clang has -Weverything - > do we want to do something more like > https://dxr.mozilla.org/mozilla-central/source/security/pkix/warnings. > mozbuild ? clang's -Weverything is VERY noisy. It might be viable for a greenfield project like mozpkix, but it's probably more trouble than it's worth for an older code base. But I don't want to discourage anyone! :) I have compiled Firefox with -Weverything and it reports hundreds of thousands of warnings.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8840457 [details] Bug 1026589 - Enable more GCC/Clang compiler warnings (-Wextra) for security/certverifier. https://reviewboard.mozilla.org/r/114958/#review116490 Thanks for the review. > Hmmm - do we support MSVC 2010 any longer? We might be able to prune some of these (probably something for a cleanup bug, though). I suspect we can as well. I plan on progressively enabling more warnings, so I should get to it eventually. > Is -Wunreachable-code not in -Wextra? I also noted clang has -Weverything - do we want to do something more like https://dxr.mozilla.org/mozilla-central/source/security/pkix/warnings.mozbuild ? As cpeterson noted (Thanks!), -Weverything is very noisy. It also means any headers we happen to include will also be subjected to -Weverything, which seems more effort than it's worth at the moment.
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6feef68fabe04d3ecb5b24bd86f1b4aa2af8baa7
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dbc95156d490 Enable more GCC/Clang compiler warnings (-Wextra) for security/certverifier. r=keeler
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbc95156d490
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•