Closed Bug 1026589 Opened 6 years ago Closed 3 years ago

Enable more GCC/Clang compiler warnings (-Wextra) for security/certverifier

Categories

(Core :: Security: PSM, defect, P1)

x86_64
Linux
defect

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
Attached patch more-warnings (obsolete) — Splinter Review
Assignee: nobody → cviecco
Attached patch remove-shadow-variable-1 (obsolete) — Splinter Review
Attached patch no-extra-for-copy-in-string (obsolete) — Splinter Review
Attachment #8441475 - Flags: review?(brian)
Attachment #8441476 - Flags: review?(brian)
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 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 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+
Attachment #8441476 - Flags: review?(brian) → review+
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]
Depends on: 1329243
(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]
Attachment #8441475 - Attachment is obsolete: true
Attachment #8441476 - Attachment is obsolete: true
Attachment #8441477 - Attachment is obsolete: true
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+
(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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/dbc95156d490
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.