From 0ba77a3c30047a93219d766ce5c436e39bc94dbf Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 22 Jul 2022 17:36:44 +0000 Subject: [PATCH] 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. --- acinclude/compiler-flags.m4 | 40 ++++++++++++++++---- configure.ac | 72 ++++++++++++++++++++---------------- src/HttpHeader.cc | 4 +- src/client_side_request.cc | 2 - src/http/RegisteredHeaders.h | 3 +- src/refresh.cc | 31 ++++++++-------- src/security/Session.cc | 4 +- src/tests/testRandomUuid.cc | 10 ++--- 8 files changed, 99 insertions(+), 67 deletions(-) diff --git a/acinclude/compiler-flags.m4 b/acinclude/compiler-flags.m4 index 48fae2fe8f..048e3ecf8d 100644 --- 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" 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 e343cfbb91..bacb8ced3d 100644 --- 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 ]],[[int fox=ntohs(1);]]) diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index 3fb298bd51..7e78bba486 100644 --- 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 HttpHeaderStats = { +static std::array HttpHeaderStats = {{ HttpHeaderStat(/*hoNone*/ "all", nullptr), #if USE_HTCP HttpHeaderStat(/*hoHtcpReply*/ "HTCP reply", &ReplyHeadersMask), @@ -86,7 +86,7 @@ static std::array 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 5eed9f1c54..aa9d6ec7db 100644 --- 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 83762e5143..88a87c2095 100644 --- 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 c5a656f06d..e021a1ee1f 100644 --- 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 092528d43e..863c6b0e89 100644 --- 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 5efa31775d..81c019a240 100644 --- a/src/tests/testRandomUuid.cc +++ b/src/tests/testRandomUuid.cc @@ -39,14 +39,14 @@ typedef std::map 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 -- 2.47.2