Closed Bug 1005309 Opened 5 years ago Closed 5 years ago

Enable more compiler warnings for mozilla::pkix

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files, 1 obsolete file)

Let's try to enable -Wall and (for VS2013) -W4. This should catch most subtle integer type conversion mistakes and also things like "if (x = y)" instead of "if (x == y)".
The static_cast from unsigned to signed type is required. Note that eventually I want to move HashBuf to the TrustDomain as part of removing the hard-coded dependencies on NSS; this is also somewhat of a baby step towards that.
Attachment #8416710 - Flags: review?(mmc)
The changes to pkixocsp.cpp are needed to avoid signed/unsigned conversion warnings on VS2013.

I didn't do the same for the test code because GTest and other things make it more complicated to do so.

https://tbpl.mozilla.org/?tree=Try&rev=504ddba31484
Attachment #8416712 - Flags: review?(mmc)
Attachment #8416710 - Flags: review?(mmc) → review+
Comment on attachment 8416712 [details] [diff] [review]
Enable -Wall and (on Windows) -W4 for mozilla::pkix

Review of attachment 8416712 [details] [diff] [review]:
-----------------------------------------------------------------

dummy.c(2) : fatal error C1083: Cannot open include file: 'pthread.h': No such file or directory
c:\PROGRA~2\MICROS~2.0\vc\include\stdint.h(105) : error C2220: warning treated as error - no 'object' file generated

::: security/pkix/moz.build
@@ +25,5 @@
> +CXXFLAGS += ['-Wall']
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows':
> +    # C4514: 'function': unreferenced inline function has been removed
> +    # C4820: 'bytes' bytes padding added after construct 'member_name'
> +    CXXFLAGS += ['-W4', '-wd4514', '-wd4820']

Windows is red.
Attachment #8416712 - Flags: review?(mmc) → review-
Comment on attachment 8416710 [details] [diff] [review]
Part 1: Wrap calls to PK11_HashBuf with extra bounds checks and required typecast

Review of attachment 8416710 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/a939ec127378
Attachment #8416710 - Flags: checkin+
Try run is green now:
https://tbpl.mozilla.org/?tree=Try&rev=b0ad262f6941

I also read the documentation and found that /Wall enables everything /W4 enables plus more, so I removed the /W4 part of the patch. See http://msdn.microsoft.com/en-us/library/thxezb7y.aspx
Attachment #8416712 - Attachment is obsolete: true
Attachment #8416977 - Flags: review?(mmc)
Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

Review of attachment 8416977 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/moz.build
@@ +29,5 @@
> +    '-wd4514', # 'function': unreferenced inline function has been removed
> +    '-wd4668', # 'symbol' is not defined as a preprocessor macro...
> +    '-wd4710', # 'function': function not inlined
> +    '-wd4711', # function 'function' selected for inline expansion
> +    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'    

watch out for pink. Thanks for including this list!
Attachment #8416977 - Flags: review?(mmc) → review+
Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c584aaabfb5
Attachment #8416977 - Flags: checkin+
Comment on attachment 8416977 [details] [diff] [review]
Part 2: Enable -Wall on all platforms with some specific warnings disabled on MSVC++

Review of attachment 8416977 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/moz.build
@@ +29,5 @@
> +    '-wd4514', # 'function': unreferenced inline function has been removed
> +    '-wd4668', # 'symbol' is not defined as a preprocessor macro...
> +    '-wd4710', # 'function': function not inlined
> +    '-wd4711', # function 'function' selected for inline expansion
> +    '-wd4820', # 'bytes' bytes padding added after construct 'member_name'    

Fixed before pushing. Thanks for the review.
moz.build should test for compiler, not the target platform, for compiler-specific flags. I landed a trivial fixup (fixes mingw build):

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc9635af8e9
Jacek, next time something like this happens, it's probably best to file a new bug (and get review or at least need-info the appropriate people) so that everyone who works on this code is aware of issues like this. As it is, the author of this patch will probably have no idea anything was wrong because this is likely to get lost in bugmail.
Flags: needinfo?(jacek)
I assumed that this falls into 'trivial one-line fixup' not worth bothering reviewer. Apparently I was wrong and I'm sorry about that.

For the record: While the only tier 1 compiler for Windows is MSVC, it's also possible to build with mingw, which uses GCC. Those have very different compiler arguments. |CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows'| tests that we're doing a build for Windows, it says nothing about the compiler, so it will try to use MSVC-specific options on GCC as well. The right thing is to test for compiler instead.
Flags: needinfo?(jacek)
You're right - I just meant in terms of us doing the right thing the first time in the future. Thanks for working on this and the information about windows/mingw :)
You need to log in before you can comment on or make changes to this bug.