Fix --disable-strict-error-checking and -W... options handling (#1091)

Commit 8b082ed (Activate extra compiler checks):

* started adding -Werror in --disable-strict-error-checking builds
* stopped adding -Wall to CXXFLAGS
* forgot that -Werror is spelled $squid_cv_cc_option_werror
* forgot that "=" is converted to "_" in $squid_cv_cc_arg... name
* forgot that "W" is present in $squid_cv_cc_arg... name
* checked squid_cv_cc_... name but set squid_cv_cc_arg... name
* used warning-generating SQUID_CC_CHECK_ARGUMENT code for testing
  compiler support for new -W options (while also using -Werror)

Correctly detecting support for a compiler warning option is difficult:

* Some compilers (e.g., clang) exit successfully when given an
  unsupported command line option unless we also give -Werror. With
  those compilers, we must use "-Werror -Wx" to test for -Wx support.
  However, that does not mean we should add -Werror to all Squid builds.

* Adding -Werror does not work for detecting supported GCC warnings: GCC
  needs -Werror=no-x to fail on unsupported -Wno-x warnings.

* We do not even know how to detect (via compiler exit code) supported
  warnings for compilers other than clang and GCC. We still add -Werror
  to all compilers other than GCC when testing -X, but, based on quick
  godbolt.org tests, -Werror does not make icc fail on unsupported -Wx.

Also delayed adding warnings until we are done adding such C++ compiler
options as -std=c++11 (and, for Irix, optimization level) because those
options may affect the set of warnings supported by the compiler. We
also (now) need SQUID_CC_GUESS_VARIANT and SQUID_CC_GUESS_OPTIONS. We
might need AC_PROG_CPP. All those things are settled after the official
code attempted to add warnings. The best code location probably needs
more work, but at least all warnings are handled in the same code now.

These changes also fix Bug 5167 (GCC -Wno-unused-private-field
miscategorized as supported).

Also fixed clang (-Wall => -Wmost) warnings exposed by the above fixes.

diff --git a/acinclude/compiler-flags.m4 b/acinclude/compiler-flags.m4
index 48fae2fe8facae49560fa7657d05b5a77b39cea5..048e3ecf8da7bbd60f0e84f3421a3da784776933 100644 (file)
--- a/acinclude/compiler-flags.m4
+++ b/acinclude/compiler-flags.m4
@@ -17,7 +17,7 @@ AC_DEFUN([SQUID_CC_CHECK_ARGUMENT],[
     SQUID_STATE_SAVE([ARGCHECK])
     CFLAGS="$CFLAGS $2"
     CXXFLAGS="$CXXFLAGS $2"
-    AC_TRY_LINK([],[int foo; ],
+    AC_TRY_LINK([],[],
       [$1=yes],[$1=no])
     SQUID_STATE_ROLLBACK([ARGCHECK])
   ])
@@ -113,18 +113,44 @@ AC_DEFUN([SQUID_CC_GUESS_VARIANT], [
   ]) dnl AC_CACHE_CHECK
  ]) dnl AC_DEFUN
 
-AC_DEFUN([SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED_INTERNAL],[
+dnl SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED helper
+dnl $1 is a compiler warning option (e.g., -Wall).
+dnl $2 is a "squid_cv_cc_arg<snake_case_warning_name_equivalent>" string.
+AC_DEFUN([SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED_],[
+  AC_REQUIRE([SQUID_CC_GUESS_VARIANT])
   SQUID_STATE_SAVE([CXXARGTEST])
   CXXFLAGS="$CXXFLAGS $SQUID_CXXFLAGS"
-  SQUID_CC_CHECK_ARGUMENT([$2],[$1])
+  AS_CASE([$squid_cv_compiler],
+    [gcc],[
+      # Testing with -Werror -Wfoobar does not work well because GCC ignores
+      # unsupported _negative_ options, so we test with -Werror=foobar instead
+      # (where "foobar" is a name of a warning that may be given to us in
+      # positive -Wfoobar or negative -Wno-foobar form).
+      SQUID_CC_CHECK_ARGUMENT([$2],m4_bpatsubst([$1],[^-W],[-Werror=]))
+    ],
+    [clang],[
+      # Testing with -Werror=foobar (as we do for GCC above) is useless
+      # because clang does not recognize that pattern as a -Werror
+      # specialization, so we test with -Werror -Wfoobar instead.
+      SQUID_CC_CHECK_ARGUMENT([$2],[-Werror $1])
+    ],
+    [
+      # We lack code to reliably test whether this compiler supports a given
+      # warning. Some compilers (e.g, icc) succeed with bogus warning names.
+      # If $squid_cv_cxx_option_werror is set, we add that option because it
+      # helps in some (but not all) known cases.
+      SQUID_CC_CHECK_ARGUMENT([$2],[$squid_cv_cxx_option_werror $1])
+    ]
+  )
   SQUID_STATE_ROLLBACK([CXXARGTEST])
   AS_IF([test "x${$2}" = "xyes"],[SQUID_CXXFLAGS="$SQUID_CXXFLAGS $1"])
 ])
 
-dnl argument is a compiler flag. It will be attempted, and if suppported
-dnl it will be added to SQUID_CXXFLAGS in the same order as calls to the macro
-AC_DEFUN([SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED],[
-  SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED_INTERNAL($1,m4_bpatsubst(m4_tolower([squid_cv_cc_arg$1]),[[^a-zA-Z0-9_]], [_]))
+dnl The argument is a compiler warning option (e.g. -Wall). If linking a
+dnl warning-free program while using the given warning succeeds, then the
+dnl option is added to SQUID_CXXFLAGS in the same order as calls to the macro.
+AC_DEFUN([SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED],[
+  SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED_($1,m4_bpatsubst(m4_tolower([squid_cv_cc_arg$1]),[[^a-zA-Z0-9_]],[_]))
 ])
 
 # define the flag to use to have the compiler treat warnings as errors

diff --git a/configure.ac b/configure.ac
index e343cfbb916780a20e750a8004a30b587c08e24a..bacb8ced3d286ccbe5c16cebe51019426d08d842 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -93,32 +93,6 @@ if test "x$squid_host_os" = "solaris" -a "x$GCC" != "x" ; then
        AC_USE_SYSTEM_EXTENSIONS
 fi
 
-AC_MSG_CHECKING([compiler name-version])
-squid_cv_cc_name=`$CC --version | head -1 | cut -f1 -d" "`
-AS_CASE([$squid_cv_cc_name],
-  [gcc|clang],[squid_cv_cc_majversion=`$CC -dumpversion | cut -f1 -d.`],
-  [squid_cv_cc_majversion="unknown"]
-)
-squid_cv_cc_simplified="$squid_cv_cc_name-$squid_cv_cc_majversion"
-AC_MSG_RESULT($squid_cv_cc_simplified)
-
-# warning flags
-AS_IF([test "$squid_cv_cc_simplified" = "gcc-4"],[
-    AC_MSG_NOTICE([skipping -Wextra on $squid_cv_cc_simplified])
-],[
-    # -Werror must be first
-    SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Werror])
-    SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wextra])
-    # -Wunused-private-field causes a huge number of false positives
-    #    in unit tests. Disable that
-    SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wno-unused-private-field])
-    # ...=2: This flexibility level allows GCC to "understand" our fallthrough
-    # comments. TODO: Switch to [[fallthrough]] attributes with C++17.
-    SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wimplicit-fallthrough=2])
-    AS_IF([test "x$squid_cv_cc_implicit_fallthrough2" = "xno"], [
-        SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wno-implicit-fallthrough])
-    ])
-])
 # If the user did not specify a C++ version.
 user_cxx=`echo "$PRESET_CXXFLAGS" | grep -o -E "\-std="`
 if test "x$user_cxx" = "x"; then
@@ -339,13 +313,47 @@ if test "x$PRESET_CFLAGS" = "x"; then
   fi
 fi
 
+AC_MSG_CHECKING([compiler name-version])
+squid_cv_cc_name=`$CC --version | head -1 | cut -f1 -d" "`
+AS_CASE([$squid_cv_cc_name],
+  [gcc|clang],[squid_cv_cc_majversion=`$CC -dumpversion | cut -f1 -d.`],
+  [squid_cv_cc_majversion="unknown"]
+)
+squid_cv_cc_simplified="$squid_cv_cc_name-$squid_cv_cc_majversion"
+AC_MSG_RESULT($squid_cv_cc_simplified)
+
+# warning flags
+
+# -Wall
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([$squid_cv_cc_option_wall])
+
+# -Wextra
+AS_IF([test "$squid_cv_cc_simplified" = "gcc-4"],[
+    AC_MSG_NOTICE([skipping -Wextra on $squid_cv_cc_simplified])
+],[
+    SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wextra])
+
+    # Now disable or configure certain warnings enabled by -Wextra
+
+    # -Wunused-private-field causes a huge number of false positives
+    #    in unit tests. Disable that
+    SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wno-unused-private-field])
+
+    # ...=2: This flexibility level allows GCC to "understand" our fallthrough
+    # comments. TODO: Switch to [[fallthrough]] attributes with C++17.
+    SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wimplicit-fallthrough=2])
+    AS_IF([test "x$squid_cv_cc_arg_wimplicit_fallthrough_2" != "xyes"], [
+        SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wno-implicit-fallthrough])
+    ])
+])
+
 # useful warnings that may not be included in -Wall -Wextra
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wpointer-arith])
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wwrite-strings])
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wcomments])
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wshadow])
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Wmissing-declarations])
-SQUID_CC_ADD_CXXFLAG_IF_SUPPORTED([-Woverloaded-virtual])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wpointer-arith])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wwrite-strings])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wcomments])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wshadow])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Wmissing-declarations])
+SQUID_CC_ADD_CXXFLAG_WARNING_IF_SUPPORTED([-Woverloaded-virtual])
 
 dnl CentOS (and RHEL) still define ntohs() using deprecated C++ features
 SQUID_CC_REQUIRE_ARGUMENT([ac_cv_require_wno_deprecated_register],[-Werror -Wno-deprecated-register],[[#include <arpa/inet.h>]],[[int fox=ntohs(1);]])

diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc
index 3fb298bd51b8e28d689676eb5990cf01928dbd72..7e78bba4865957744a74853e20b27a27d8f6dbc0 100644 (file)
--- a/src/HttpHeader.cc
+++ b/src/HttpHeader.cc
@@ -75,7 +75,7 @@ static HttpHeaderMask ReplyHeadersMask;     /* set run-time using ReplyHeaders *
 
 /* header accounting */
 // NP: keep in sync with enum http_hdr_owner_type
-static std::array<HttpHeaderStat, hoEnd> HttpHeaderStats = {
+static std::array<HttpHeaderStat, hoEnd> HttpHeaderStats = {{
     HttpHeaderStat(/*hoNone*/ "all", nullptr),
 #if USE_HTCP
     HttpHeaderStat(/*hoHtcpReply*/ "HTCP reply", &ReplyHeadersMask),
@@ -86,7 +86,7 @@ static std::array<HttpHeaderStat, hoEnd> HttpHeaderStats = {
     , HttpHeaderStat(/*hoErrorDetail*/ "error detail templates", nullptr)
 #endif
     /* hoEnd */
-};
+}};
 
 static int HeaderEntryParsedCount = 0;
 

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 5eed9f1c54088596b8bdea2eb117def045e1758e..aa9d6ec7dba00843cdecb5b9576e206002c98856 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -74,8 +74,6 @@
 #include "ssl/support.h"
 #endif
 
-static const char *const crlf = "\r\n";
-
 #if FOLLOW_X_FORWARDED_FOR
 static void clientFollowXForwardedForCheck(Acl::Answer answer, void *data);
 #endif /* FOLLOW_X_FORWARDED_FOR */

diff --git a/src/http/RegisteredHeaders.h b/src/http/RegisteredHeaders.h
index 83762e51431a8f1d942833dff5b874a441a6d975..88a87c2095a5a4e749f229f0603de96df442328a 100644 (file)
--- a/src/http/RegisteredHeaders.h
+++ b/src/http/RegisteredHeaders.h
@@ -151,7 +151,8 @@ enum HdrKind {
 };
 
 /* POD for HeaderTable */
-struct HeaderTableRecord {
+class HeaderTableRecord {
+public:
     HeaderTableRecord() = default;
     HeaderTableRecord(const char *n);
     HeaderTableRecord(const char *, Http::HdrType, Http::HdrFieldType, int /* HdrKind */);

diff --git a/src/refresh.cc b/src/refresh.cc
index c5a656f06d0d5d0bf9051824803a8bfad8e6a1ca..e021a1ee1f963734794acd645aa5c864f516d5bf 100644 (file)
--- a/src/refresh.cc
+++ b/src/refresh.cc
@@ -657,22 +657,21 @@ refreshCountsStats(StoreEntry * sentry, struct RefreshCounts &rc)
     storeAppendPrintf(sentry, "\n\n%s histogram:\n", rc.proto);
     storeAppendPrintf(sentry, "Count\t%%Total\tCategory\n");
 
-    int sum = 0;
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_REQUEST_MAX_STALE_ALL, "Fresh: request max-stale wildcard");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_REQUEST_MAX_STALE_VALUE, "Fresh: request max-stale value");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_EXPIRES, "Fresh: expires time not reached");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_LMFACTOR_RULE, "Fresh: refresh_pattern last-mod factor percentage");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_MIN_RULE, "Fresh: refresh_pattern min value");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_OVERRIDE_EXPIRES, "Fresh: refresh_pattern override-expires");
-    sum += refreshCountsStatsEntry(sentry, rc, FRESH_OVERRIDE_LASTMOD, "Fresh: refresh_pattern override-lastmod");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_MUST_REVALIDATE, "Stale: response has must-revalidate");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_RELOAD_INTO_IMS, "Stale: changed reload into IMS");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_FORCED_RELOAD, "Stale: request has no-cache directive");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE, "Stale: age exceeds request max-age value");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_EXPIRES, "Stale: expires time reached");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_MAX_RULE, "Stale: refresh_pattern max age rule");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_LMFACTOR_RULE, "Stale: refresh_pattern last-mod factor percentage");
-    sum += refreshCountsStatsEntry(sentry, rc, STALE_DEFAULT, "Stale: by default");
+    refreshCountsStatsEntry(sentry, rc, FRESH_REQUEST_MAX_STALE_ALL, "Fresh: request max-stale wildcard");
+    refreshCountsStatsEntry(sentry, rc, FRESH_REQUEST_MAX_STALE_VALUE, "Fresh: request max-stale value");
+    refreshCountsStatsEntry(sentry, rc, FRESH_EXPIRES, "Fresh: expires time not reached");
+    refreshCountsStatsEntry(sentry, rc, FRESH_LMFACTOR_RULE, "Fresh: refresh_pattern last-mod factor percentage");
+    refreshCountsStatsEntry(sentry, rc, FRESH_MIN_RULE, "Fresh: refresh_pattern min value");
+    refreshCountsStatsEntry(sentry, rc, FRESH_OVERRIDE_EXPIRES, "Fresh: refresh_pattern override-expires");
+    refreshCountsStatsEntry(sentry, rc, FRESH_OVERRIDE_LASTMOD, "Fresh: refresh_pattern override-lastmod");
+    refreshCountsStatsEntry(sentry, rc, STALE_MUST_REVALIDATE, "Stale: response has must-revalidate");
+    refreshCountsStatsEntry(sentry, rc, STALE_RELOAD_INTO_IMS, "Stale: changed reload into IMS");
+    refreshCountsStatsEntry(sentry, rc, STALE_FORCED_RELOAD, "Stale: request has no-cache directive");
+    refreshCountsStatsEntry(sentry, rc, STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE, "Stale: age exceeds request max-age value");
+    refreshCountsStatsEntry(sentry, rc, STALE_EXPIRES, "Stale: expires time reached");
+    refreshCountsStatsEntry(sentry, rc, STALE_MAX_RULE, "Stale: refresh_pattern max age rule");
+    refreshCountsStatsEntry(sentry, rc, STALE_LMFACTOR_RULE, "Stale: refresh_pattern last-mod factor percentage");
+    refreshCountsStatsEntry(sentry, rc, STALE_DEFAULT, "Stale: by default");
     storeAppendPrintf(sentry, "\n");
 }
 

diff --git a/src/security/Session.cc b/src/security/Session.cc
index 092528d43ed7cef2414a9e4d1a9d300d52ae4527..863c6b0e89fe114f50d6eff2e7be05c0003341c6 100644 (file)
--- a/src/security/Session.cc
+++ b/src/security/Session.cc
@@ -383,10 +383,10 @@ Security::SetSessionCacheCallbacks(Security::ContextPointer &ctx)
 }
 #endif /* USE_OPENSSL */
 
+#if USE_OPENSSL
 static void
 initializeSessionCache()
 {
-#if USE_OPENSSL
     // Check if the MemMap keys and data are enough big to hold
     // session ids and session data
     assert(SSL_SESSION_ID_SIZE >= MEMMAP_SLOT_KEY_SIZE);
@@ -404,8 +404,8 @@ initializeSessionCache()
         if (s->secure.staticContext)
             Security::SetSessionCacheCallbacks(s->secure.staticContext);
     }
-#endif
 }
+#endif
 
 /// initializes shared memory segments used by MemStore
 class SharedSessionCacheRr: public Ipc::Mem::RegisteredRunner

diff --git a/src/tests/testRandomUuid.cc b/src/tests/testRandomUuid.cc
index 5efa31775d3d2c8292bec917517d851e49a81271..81c019a240bd13139992fb68f8f7d419d38587e9 100644 (file)
--- a/src/tests/testRandomUuid.cc
+++ b/src/tests/testRandomUuid.cc
@@ -39,14 +39,14 @@ typedef std::map<SBuf, RandomUuid::Serialized> RandomIds;
 // Generated by https://www.uuidgenerator.net/version4
 // binary representation of the generated UUID in network byte order
 static const RandomIds ExternalIds {
-    { SBuf("bd1b1c07-f7fa-428a-b019-7e390133b0e5"), { 0xbd, 0x1b, 0x1c, 0x07, 0xf7, 0xfa, 0x42, 0x8a, 0xb0, 0x19, 0x7e, 0x39, 0x01, 0x33, 0xb0, 0xe5 } },
-    { SBuf("f63ccd5a-9d25-41a5-a36c-a7b0c6b5c678"), { 0xf6, 0x3c, 0xcd, 0x5a, 0x9d, 0x25, 0x41, 0xa5, 0xa3, 0x6c, 0xa7, 0xb0, 0xc6, 0xb5, 0xc6, 0x78 } },
-    { SBuf("9c8363ac-9c62-44e9-941f-86b7edc25dc7"), { 0x9c, 0x83, 0x63, 0xac, 0x9c, 0x62, 0x44, 0xe9, 0x94, 0x1f, 0x86, 0xb7, 0xed, 0xc2, 0x5d, 0xc7 } }
+    { SBuf("bd1b1c07-f7fa-428a-b019-7e390133b0e5"), {{ 0xbd, 0x1b, 0x1c, 0x07, 0xf7, 0xfa, 0x42, 0x8a, 0xb0, 0x19, 0x7e, 0x39, 0x01, 0x33, 0xb0, 0xe5 }} },
+    { SBuf("f63ccd5a-9d25-41a5-a36c-a7b0c6b5c678"), {{ 0xf6, 0x3c, 0xcd, 0x5a, 0x9d, 0x25, 0x41, 0xa5, 0xa3, 0x6c, 0xa7, 0xb0, 0xc6, 0xb5, 0xc6, 0x78 }} },
+    { SBuf("9c8363ac-9c62-44e9-941f-86b7edc25dc7"), {{ 0x9c, 0x83, 0x63, 0xac, 0x9c, 0x62, 0x44, 0xe9, 0x94, 0x1f, 0x86, 0xb7, 0xed, 0xc2, 0x5d, 0xc7 }} }
 };
 
 static const RandomIds InvalidIds {
-    { SBuf("00000000-0000-0000-0000-000000000000"), { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 } },
-    { SBuf("ffffffff-ffff-ffff-ffff-ffffffffffff"), { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff } }
+    { SBuf("00000000-0000-0000-0000-000000000000"), {{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }} },
+    { SBuf("ffffffff-ffff-ffff-ffff-ffffffffffff"), {{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }} }
 };
 
 void