Bug 1128413, Part 3: Enable more compiler warnings, r=mmc

--HG--
extra : rebase_source : 2d17605e6b9296b74493526e052b771be18d4260

lib/mozpkix/include/pkix/Input.h

@@ -80,6 +80,9 @@ class Input final
   {
   }
 
+  // This is intentionally not explicit in order to allow value semantics.
+  Input(const Input&) = default;
+
   // Initialize the input. data must be non-null and len must be less than
   // 65536. Init may not be called more than once.
   Result Init(const uint8_t* data, size_t len)
@@ -271,7 +274,7 @@ class Reader final
 
   Result SkipToEnd(/*out*/ Input& skipped)
   {
-    return Skip(static_cast<size_t>(end - input), skipped);
+    return Skip(static_cast<Input::size_type>(end - input), skipped);
   }
 
   Result EnsureLength(Input::size_type len)
@@ -286,6 +289,8 @@ class Reader final
 
   class Mark final
   {
+  public:
+    Mark(const Mark&) = default; // Intentionally not explicit.
   private:
     friend class Reader;
     Mark(const Reader& input, const uint8_t* mark) : input(input), mark(mark) { }

lib/mozpkix/lib/pkixder.h

@@ -55,7 +55,7 @@ enum Constructed
   CONSTRUCTED = 1 << 5
 };
 
-enum Tag
+enum Tag : uint8_t
 {
   BOOLEAN = UNIVERSAL | 0x01,
   INTEGER = UNIVERSAL | 0x02,

lib/mozpkix/lib/pkixnames.cpp

@@ -1642,13 +1642,14 @@ FinishIPv6Address(/*in/out*/ uint8_t (&address)[16], int numComponents,
   }
 
   // Shift components that occur after the contraction over.
-  int componentsToMove = numComponents - contractionIndex;
-  memmove(address + (2u * (8 - componentsToMove)),
-          address + (2u * contractionIndex),
+  size_t componentsToMove = static_cast<size_t>(numComponents -
+                                                contractionIndex);
+  memmove(address + (2u * static_cast<size_t>(8 - componentsToMove)),
+          address + (2u * static_cast<size_t>(contractionIndex)),
           componentsToMove * 2u);
   // Fill in the contracted area with zeros.
-  memset(address + (2u * contractionIndex), 0u,
-         (8u - numComponents) * 2u);
+  memset(address + (2u * static_cast<size_t>(contractionIndex)), 0u,
+         (8u - static_cast<size_t>(numComponents)) * 2u);
 
   return true;
 }
@@ -1785,7 +1786,6 @@ ParseIPv6Address(Input hostname, /*out*/ uint8_t (&out)[16])
       if (contractionIndex != -1) {
         return false; // multiple contractions are not allowed.
       }
-      uint8_t b;
       if (input.Read(b) != Success || b != ':') {
         assert(false);
         return false;

lib/mozpkix/lib/pkixtime.cpp

@@ -24,8 +24,15 @@
 
 #include "pkix/Time.h"
 #include "pkixutil.h"
+
 #ifdef WIN32
+#ifdef _MSC_VER
+#pragma warning(push, 3)
+#endif
 #include "windows.h"
+#ifdef _MSC_VER
+#pragma warning(pop)
+#endif
 #else
 #include "sys/time.h"
 #endif
@@ -53,7 +60,8 @@ Now()
   //   - http://pubs.opengroup.org/onlinepubs/009695399/functions/gettimeofday.html
   timeval tv;
   (void) gettimeofday(&tv, nullptr);
-  seconds = (DaysBeforeYear(1970) * Time::ONE_DAY_IN_SECONDS) + tv.tv_sec;
+  seconds = (DaysBeforeYear(1970) * Time::ONE_DAY_IN_SECONDS) +
+            static_cast<uint64_t>(tv.tv_sec);
 #endif
 
   return TimeFromElapsedSecondsAD(seconds);

lib/mozpkix/lib/pkixutil.h

@@ -53,7 +53,7 @@ class BackCert final
   Result Init();
 
   const Input GetDER() const { return der; }
-  const der::Version GetVersion() const { return version; }
+  der::Version GetVersion() const { return version; }
   const SignedDataWithSignature& GetSignedData() const { return signedData; }
   const Input GetIssuer() const { return issuer; }
   // XXX: "validity" is a horrible name for the structure that holds
@@ -232,24 +232,25 @@ WrappedVerifySignedData(TrustDomain& trustDomain,
 //         MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM
 //       }
 //     }
-#if defined(__clang__) && (__clang_major__ == 3 && __clang_minor__ < 5)
-  // Earlier versions of Clang will warn if not all cases are covered
-  // (-Wswitch-enum) AND they always, inappropriately, assume the default case
-  // is unreachable. This was fixed in
-  // http://llvm.org/klaus/clang/commit/28cd22d7c2d2458575ce9cc19dfe63c6321010ce/
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM // empty
-#elif defined(__GNUC__) || defined(__clang__)
-  // GCC and recent versions of clang will warn if not all cases are covered
-  // (-Wswitch-enum). They do not assume that the default case is unreachable.
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
-         default: assert(false); __builtin_unreachable();
+#if defined(__clang__)
+// Clang will warn if not all cases are covered (-Wswitch-enum) AND it will
+// warn if a switch statement that covers every enum label has a default case
+// (-W-covered-switch-default). Versions prior to 3.5 warned about unreachable
+// code in such default cases (-Wunreachable-code) even when
+// -W-covered-switch-default was disabled, but that changed in Clang 3.5.
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM // empty
+#elif defined(__GNUC__)
+// GCC will warn if not all cases are covered (-Wswitch-enum). It does not
+// assume that the default case is unreachable.
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
+        default: assert(false); __builtin_unreachable();
 #elif defined(_MSC_VER)
-  // MSVC will warn if not all cases are covered (C4061, level 4). It does not
-  // assume that the default case is unreachable.
-# define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
-         default: assert(false); __assume(0);
+// MSVC will warn if not all cases are covered (C4061, level 4). It does not
+// assume that the default case is unreachable.
+#define MOZILLA_PKIX_UNREACHABLE_DEFAULT_ENUM \
+        default: assert(false); __assume(0);
 #else
-# error Unsupported compiler for MOZILLA_PKIX_UNREACHABLE_DEFAULT.
+#error Unsupported compiler for MOZILLA_PKIX_UNREACHABLE_DEFAULT.
 #endif
 
 } } // namespace mozilla::pkix

lib/mozpkix/moz.build

@@ -25,18 +25,7 @@ TEST_DIRS += [
     'test/lib',
 ]
 
-CXXFLAGS += ['-Wall']
-# -Wall with Visual C++ enables too many problematic warnings
-if CONFIG['_MSC_VER']:
-  CXXFLAGS += [
-    '-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'
-  ]
-
-FAIL_ON_WARNINGS = True
+include('warnings.mozbuild')
 
 Library('mozillapkix')
 

lib/mozpkix/test/gtest/moz.build

@@ -30,11 +30,24 @@ LOCAL_INCLUDES += [
 
 FINAL_LIBRARY = 'xul-gtest'
 
-include('/ipc/chromium/chromium-config.mozbuild')
+include('../../warnings.mozbuild')
 
-if CONFIG['_MSC_VER']:
+# These warnings are disabled in order to minimize the amount of boilerplate
+# required to implement tests, and/or because they originate in the GTest
+# framework in a way we cannot otherwise work around.
+if CONFIG['CLANG_CXX']:
   CXXFLAGS += [
+    '-Wno-exit-time-destructors',
+    '-Wno-global-constructors',
+    '-Wno-old-style-cast',
+    '-Wno-used-but-marked-unused',
+  ]
+elif CONFIG['_MSC_VER']:
+  CXXFLAGS += [
+    '-wd4350', # behavior change: 'std::_Wrap_alloc<std::allocator<_Ty>>::...
     '-wd4275', # non dll-interface class used as base for dll-interface class
+    '-wd4548', # Expression before comma has no effect
+    '-wd4625', # copy constructor could not be generated.
+    '-wd4626', # assugment operator could not be generated.
+    '-wd4640', # construction of local static object is not thread safe.
   ]
-
-FAIL_ON_WARNINGS = True

lib/mozpkix/test/gtest/pkixbuild_tests.cpp

@@ -22,8 +22,20 @@
  * limitations under the License.
  */
 
+#if defined(_MSC_VER) && _MSC_VER < 1900
+// When building with -D_HAS_EXCEPTIONS=0, MSVC's <xtree> header triggers
+// warning C4702: unreachable code.
+// https://connect.microsoft.com/VisualStudio/feedback/details/809962
+#pragma warning(push)
+#pragma warning(disable: 4702)
+#endif
+
 #include <map>
-#include "cert.h"
+
+#if defined(_MSC_VER) && _MSC_VER < 1900
+#pragma warning(pop)
+#endif
+
 #include "pkix/pkix.h"
 #include "pkixgtest.h"
 #include "pkixtestutil.h"
@@ -110,7 +122,7 @@ class TestTrustDomain final : public TrustDomain
     return Success;
   }
 
-  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)
+  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time)
                     override
   {
     ByteString subjectDER(InputToByteString(encodedIssuerName));
@@ -147,8 +159,7 @@ class TestTrustDomain final : public TrustDomain
     return TestVerifySignedData(signedData, subjectPublicKeyInfo);
   }
 
-  Result DigestBuf(Input item, /*out*/ uint8_t *digestBuf, size_t digestBufLen)
-                   override
+  Result DigestBuf(Input, /*out*/ uint8_t*, size_t) override
   {
     ADD_FAILURE();
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
@@ -261,9 +272,8 @@ class ExpiredCertTrustDomain final : public TrustDomain
   }
 
   // The CertPolicyId argument is unused because we don't care about EV.
-  Result GetCertTrust(EndEntityOrCA endEntityOrCA, const CertPolicyId&,
-                      Input candidateCert, /*out*/ TrustLevel& trustLevel)
-                      override
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
+                      /*out*/ TrustLevel& trustLevel) override
   {
     Input rootCert;
     Result rv = rootCert.Init(rootDER.data(), rootDER.length());
@@ -278,8 +288,7 @@ class ExpiredCertTrustDomain final : public TrustDomain
     return Success;
   }
 
-  Result FindIssuer(Input encodedIssuerName, IssuerChecker& checker, Time time)
-                    override
+  Result FindIssuer(Input, IssuerChecker& checker, Time) override
   {
     // keepGoing is an out parameter from IssuerChecker.Check. It would tell us
     // whether or not to continue attempting other potential issuers. We only
@@ -343,7 +352,7 @@ TEST_F(pkixbuild, NoRevocationCheckingForExpiredCert)
   ByteString certDER(CreateEncodedCertificate(
                        v3, sha256WithRSAEncryption,
                        serialNumber, issuerDER,
-                       oneDayBeforeNow - Time::ONE_DAY_IN_SECONDS,
+                       oneDayBeforeNow - ONE_DAY_IN_SECONDS_AS_TIME_T,
                        oneDayBeforeNow,
                        subjectDER, *reusedKey, nullptr, *reusedKey,
                        sha256WithRSAEncryption));
@@ -389,8 +398,7 @@ class DSSTrustDomain final : public TrustDomain
     return Success;
   }
 
-  Result VerifySignedData(const SignedDataWithSignature& signedData,
-                          Input subjectPublicKeyInfo) override
+  Result VerifySignedData(const SignedDataWithSignature&, Input) override
   {
     ADD_FAILURE();
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
@@ -463,7 +471,7 @@ class IssuerNameCheckTrustDomain final : public TrustDomain
     return Success;
   }
 
-  Result FindIssuer(Input subjectCert, IssuerChecker& checker, Time) override
+  Result FindIssuer(Input, IssuerChecker& checker, Time) override
   {
     Input issuerInput;
     EXPECT_EQ(Success, issuerInput.Init(issuer.data(), issuer.length()));

lib/mozpkix/test/gtest/pkixcert_extension_tests.cpp

@@ -63,7 +63,7 @@ CreateCertWithOneExtension(const char* subjectStr, const ByteString& extension)
 class TrustEverythingTrustDomain final : public TrustDomain
 {
 private:
-  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
+  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input,
                       /*out*/ TrustLevel& trustLevel) override
   {
     trustLevel = TrustLevel::TrustAnchor;

lib/mozpkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp

@@ -22,7 +22,7 @@
  * limitations under the License.
  */
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixtestutil.h"
 

lib/mozpkix/test/gtest/pkixcheck_CheckValidity_tests.cpp

@@ -22,7 +22,7 @@
  * limitations under the License.
  */
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixtestutil.h"
 

lib/mozpkix/test/gtest/pkixder_input_tests.cpp

@@ -24,7 +24,7 @@
 
 #include <functional>
 #include <vector>
-#include <gtest/gtest.h>
+#include "pkixgtest.h"
 
 #include "pkixder.h"
 
@@ -467,7 +467,7 @@ TEST_F(pkixder_input_tests, MarkAndGetInput)
 }
 
 // Cannot run this test on debug builds because of the NotReached
-#ifndef DEBUG
+#ifdef NDEBUG
 TEST_F(pkixder_input_tests, MarkAndGetInputDifferentInput)
 {
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
@@ -847,7 +847,7 @@ TEST_F(pkixder_input_tests, NestedOf)
                      [&readValues](Reader& r) {
                        return NestedOfHelper(r, readValues);
                      }));
-  ASSERT_EQ((size_t) 3, readValues.size());
+  ASSERT_EQ(3u, readValues.size());
   ASSERT_EQ(0x01, readValues[0]);
   ASSERT_EQ(0x02, readValues[1]);
   ASSERT_EQ(0x03, readValues[2]);
@@ -865,7 +865,7 @@ TEST_F(pkixder_input_tests, NestedOfWithTruncatedData)
                      [&readValues](Reader& r) {
                        return NestedOfHelper(r, readValues);
                      }));
-  ASSERT_EQ((size_t) 0, readValues.size());
+  ASSERT_EQ(0u, readValues.size());
 }
 
 TEST_F(pkixder_input_tests, MatchRestAtEnd)

lib/mozpkix/test/gtest/pkixder_pki_types_tests.cpp

@@ -25,7 +25,7 @@
 #include <functional>
 #include <vector>
 
-#include "gtest/gtest.h"
+#include "pkixgtest.h"
 #include "pkix/pkixtypes.h"
 #include "pkixder.h"