Merge pull request #225 from VectorCamp/feature/cleanup-compiler-warnings

According to https://buildbot-ci.vectorcamp.gr/#/changes/93 most builds succceded and with no compiler warnings. The build failures were only on x86 and Arm for SIMDe builds: x86 because of a bug in SIMDe emulation of own x86 intrinsics in non-native mode and Arm due to clang, unsure if this is actually a bug in SIMDe or clang itself. All the remaining compiler warnings that were suppressed was because they were not possible to fix for the scope of this project. This PR will close #170, code quality improvements however will continue with the integration of #222 or similar static code analyzer to CI and continuous refactoring.
2025-06-28 16:41:01 +03:00 · 2024-01-20 22:41:00 +02:00 · 2024-01-20 22:41:00 +02:00 · 98eb459ac2
commit 98eb459ac2
parent b5ae828e61 01658be05d
17 changed files with 61 additions and 118 deletions
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -156,9 +156,11 @@ include (${CMAKE_MODULE_PATH}/sanitize.cmake)

 if (NOT FAT_RUNTIME)
    if (GNUCC_TUNE)
+        message(STATUS "GNUCC_TUNE is set")
        set(ARCH_C_FLAGS "-${ARCH_FLAG}=${GNUCC_ARCH} -${TUNE_FLAG}=${GNUCC_TUNE}")
        set(ARCH_CXX_FLAGS "-${ARCH_FLAG}=${GNUCC_ARCH} -${TUNE_FLAG}=${GNUCC_TUNE}")
    else()
+        message(STATUS "GNUCC_TUNE is not set")
        set(ARCH_C_FLAGS "-${ARCH_FLAG}=${GNUCC_ARCH} -mtune=${TUNE_FLAG} ${ARCH_C_FLAGS}")
        set(ARCH_CXX_FLAGS "-${ARCH_FLAG}=${GNUCC_ARCH} -mtune=${TUNE_FLAG} ${ARCH_CXX_FLAGS}")
    endif()
--- a/cmake/archdetect.cmake
+++ b/cmake/archdetect.cmake
@ -81,11 +81,11 @@ else()
            set(GNUCC_ARCH power8)
            set(TUNE_FLAG power8)
        else()
-            set(GNUCC_ARCH native)
+            set(GNUCC_ARCH x86-64-v2)
            set(TUNE_FLAG generic)
        endif()
    elseif (ARCH_IA32 OR ARCH_X86_64)
-        set(GNUCC_ARCH native)
+        set(GNUCC_ARCH ${X86_ARCH})
        set(TUNE_FLAG generic)
    elseif(ARCH_AARCH64)
        if (BUILD_SVE2_BITPERM)
--- a/cmake/cflags-generic.cmake
+++ b/cmake/cflags-generic.cmake
@ -1,22 +1,13 @@
 # set compiler flags - more are tested and added later
-set(EXTRA_C_FLAGS "${OPT_C_FLAG} -std=c17 -Wall -Wextra -Wshadow -Wcast-qual -fno-strict-aliasing")
-set(EXTRA_CXX_FLAGS "${OPT_CXX_FLAG} -std=c++17 -Wall -Wextra -Wshadow -Wswitch -Wreturn-type -Wcast-qual -Wno-deprecated -Wnon-virtual-dtor -fno-strict-aliasing")
+set(EXTRA_C_FLAGS "${OPT_C_FLAG} -std=c17 -Wall -Wextra ")
+set(EXTRA_CXX_FLAGS "${OPT_CXX_FLAG} -std=c++17 -Wall -Wextra ")
 if (NOT CMAKE_COMPILER_IS_CLANG)
    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -fno-new-ttp-matching")
 endif()

-if (NOT RELEASE_BUILD)
-    # -Werror is most useful during development, don't potentially break
-    # release builds
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Werror")
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Werror")
-    if (CMAKE_COMPILER_IS_CLANG)
-    	if (CMAKE_C_COMPILER_VERSION VERSION_GREATER "13.0")
-           set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-unused-but-set-variable")
-           set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-unused-but-set-variable")
-        endif()
-    endif()
-endif()
+# Always use -Werror *also during release builds
+set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wall -Werror")
+set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wall -Werror")

 if (DISABLE_ASSERTS)
    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -DNDEBUG")
@ -25,22 +16,11 @@ endif()

 if(CMAKE_COMPILER_IS_GNUCC)
    # spurious warnings?
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-array-bounds -Wno-maybe-uninitialized")
+    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-array-bounds ") #-Wno-maybe-uninitialized")
 endif()

 if(CMAKE_COMPILER_IS_GNUCXX)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-maybe-uninitialized")
-    if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0)
-        set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -fabi-version=0")
-    endif ()
-    # don't complain about abi
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-abi")
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-abi")
-endif()
-
-if (NOT(ARCH_IA32 AND RELEASE_BUILD))
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -fno-omit-frame-pointer")
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -fno-omit-frame-pointer")
+    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-maybe-uninitialized -Wno-uninitialized")
 endif()

 CHECK_INCLUDE_FILES(unistd.h HAVE_UNISTD_H)
@ -71,94 +51,41 @@ if (NOT CMAKE_COMPILER_IS_CLANG)
   CHECK_C_SOURCE_COMPILES("int main(void) { __builtin_constant_p(0); }" HAVE__BUILTIN_CONSTANT_P)
 endif()

-set(C_FLAGS_TO_CHECK
-# Variable length arrays are way bad, most especially at run time
-"-Wvla"
-# Pointer arith on void pointers is doing it wrong.
- "-Wpointer-arith"
-# Build our C code with -Wstrict-prototypes -Wmissing-prototypes
- "-Wstrict-prototypes"
- "-Wmissing-prototypes"
-)
-foreach (FLAG ${C_FLAGS_TO_CHECK})
-    # munge the name so it doesn't break things
-    string(REPLACE "-" "_" FNAME C_FLAG${FLAG})
-    CHECK_C_COMPILER_FLAG("${FLAG}" ${FNAME})
-    if (${FNAME})
-        set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} ${FLAG}")
-    endif()
-endforeach()
-
-# self-assign should be thrown away, but clang whinges
-CHECK_C_COMPILER_FLAG("-Wself-assign" CC_SELF_ASSIGN)
-if (CC_SELF_ASSIGN)
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-self-assign")
-endif()
-CHECK_CXX_COMPILER_FLAG("-Wself-assign" CXX_SELF_ASSIGN)
-if (CXX_SELF_ASSIGN)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-self-assign")
-endif()
-
-# clang gets up in our face for going paren crazy with macros
-CHECK_C_COMPILER_FLAG("-Wparentheses-equality" CC_PAREN_EQUALITY)
-if (CC_PAREN_EQUALITY)
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-parentheses-equality")
-endif()
-
-# clang complains about unused const vars in our Ragel-generated code.
-CHECK_CXX_COMPILER_FLAG("-Wunused-const-variable" CXX_UNUSED_CONST_VAR)
-if (CXX_UNUSED_CONST_VAR)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-unused-const-variable")
-endif()
-
 # clang-14 complains about unused-but-set variable.
 CHECK_CXX_COMPILER_FLAG("-Wunused-but-set-variable" CXX_UNUSED_BUT_SET_VAR)
 if (CXX_UNUSED_BUT_SET_VAR)
    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-unused-but-set-variable")
 endif()

-# clang-14 complains about using bitwise operator instead of logical ones.
-CHECK_CXX_COMPILER_FLAG("-Wbitwise-instead-of-logical" CXX_BITWISE_INSTEAD_OF_LOGICAL)
-if (CXX_BITWISE_INSTEAD_OF_LOGICAL)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-bitwise-instead-of-logical")
-endif()
-
-# clang-14 complains about using bitwise operator instead of logical ones.
-CHECK_CXX_COMPILER_FLAG("-Wbitwise-instead-of-logical" CXX_BITWISE_INSTEAD_OF_LOGICAL)
-if (CXX_BITWISE_INSTEAD_OF_LOGICAL)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-bitwise-instead-of-logical")
-endif()
-
 CHECK_CXX_COMPILER_FLAG("-Wignored-attributes" CXX_IGNORED_ATTR)
-if (CXX_IGNORED_ATTR)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-ignored-attributes")
+if(CMAKE_COMPILER_IS_GNUCC)
+    if (CXX_IGNORED_ATTR)
+        set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-ignored-attributes")
+    endif()
 endif()

-# gcc 9 complains about redundant move for returned variable
-CHECK_CXX_COMPILER_FLAG("-Wredundant-move" CXX_REDUNDANT_MOVE)
-if (CXX_REDUNDANT_MOVE)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-redundant-move")
+CHECK_CXX_COMPILER_FLAG("-Wignored-attributes" CXX_NON_NULL)
+if(CMAKE_COMPILER_IS_GNUCC)
+    if (CXX_NON_NULL)
+        set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-nonnull")
+    endif()
 endif()

 # note this for later, g++ doesn't have this flag but clang does
 CHECK_CXX_COMPILER_FLAG("-Wweak-vtables" CXX_WEAK_VTABLES)
-if (CXX_WEAK_VTABLES)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wweak-vtables")
-endif()

 CHECK_CXX_COMPILER_FLAG("-Wmissing-declarations" CXX_MISSING_DECLARATIONS)
-if (CXX_MISSING_DECLARATIONS)
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wmissing-declarations")
-endif()

 CHECK_CXX_COMPILER_FLAG("-Wunused-local-typedefs" CXX_UNUSED_LOCAL_TYPEDEFS)

 CHECK_CXX_COMPILER_FLAG("-Wunused-variable" CXX_WUNUSED_VARIABLE)

-# gcc 10 complains about this
-CHECK_C_COMPILER_FLAG("-Wstringop-overflow" CC_STRINGOP_OVERFLOW)
-CHECK_CXX_COMPILER_FLAG("-Wstringop-overflow" CXX_STRINGOP_OVERFLOW)
-if(CC_STRINGOP_OVERFLOW OR CXX_STRINGOP_OVERFLOW)
-    set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-stringop-overflow")
-    set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-stringop-overflow")
+# gcc complains about this
+if(CMAKE_COMPILER_IS_GNUCC)
+    CHECK_C_COMPILER_FLAG("-Wstringop-overflow" CC_STRINGOP_OVERFLOW)
+    CHECK_CXX_COMPILER_FLAG("-Wstringop-overflow" CXX_STRINGOP_OVERFLOW)
+    if(CC_STRINGOP_OVERFLOW OR CXX_STRINGOP_OVERFLOW)
+        set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-stringop-overflow -Wno-stringop-overread")
+        set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-stringop-overflow -Wno-stringop-overread")
+    endif()
 endif()
--- a/cmake/cflags-x86.cmake
+++ b/cmake/cflags-x86.cmake
@ -1,29 +1,34 @@
 option(BUILD_AVX512 "Enabling support for AVX512" OFF)
 option(BUILD_AVX512VBMI "Enabling support for AVX512VBMI" OFF)

-set(SKYLAKE_FLAG "-march=skylake-avx512")
-set(ICELAKE_FLAG "-march=icelake-server")
+set(SKYLAKE_ARCH "skylake-avx512")
+set(ICELAKE_ARCH "icelake-server")
+set(SKYLAKE_FLAG "-march=${SKYLAKE_ARCH}")
+set(ICELAKE_FLAG "-march=${ICELAKE_ARCH}")

 if (NOT FAT_RUNTIME)
    if (BUILD_AVX512VBMI)
        message (STATUS "AVX512VBMI implies AVX512, enabling BUILD_AVX512")
        set(BUILD_AVX512 ON)
+        set(BUILD_AVX2 ON)
        set(ARCH_C_FLAGS "${ICELAKE_FLAG}")
        set(ARCH_CXX_FLAGS "${ICELAKE_FLAG}")
-    endif ()
-    if (BUILD_AVX512)
+        set(X86_ARCH "${ICELAKE_ARCH}")
+    elseif (BUILD_AVX512)
        message (STATUS "AVX512 implies AVX2, enabling BUILD_AVX2")
        set(BUILD_AVX2 ON)
        set(ARCH_C_FLAGS "${SKYLAKE_FLAG}")
        set(ARCH_CXX_FLAGS "${SKYLAKE_FLAG}")
-    endif ()
-    if (BUILD_AVX2)
+        set(X86_ARCH "${SKYLAKE_ARCH}")
+    elseif (BUILD_AVX2)
        message (STATUS "Enabling BUILD_AVX2")
        set(ARCH_C_FLAGS "-mavx2")
        set(ARCH_CXX_FLAGS "-mavx2")
+        set(X86_ARCH "core-avx2")
    else()
        set(ARCH_C_FLAGS "-msse4.2")
        set(ARCH_CXX_FLAGS "-msse4.2")
+        set(X86_ARCH "x86-64-v2")
    endif()
 else()
    set(BUILD_AVX512VBMI ON)
@ -31,6 +36,7 @@ else()
    set(BUILD_AVX2 ON)
    set(ARCH_C_FLAGS "-msse4.2")
    set(ARCH_CXX_FLAGS "-msse4.2")
+    set(X86_ARCH "x86-64-v2")
 endif()

 set(CMAKE_REQUIRED_FLAGS "${ARCH_C_FLAGS}")
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@ -6,6 +6,7 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
    if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS CLANGCXX_MINVER)
        message(FATAL_ERROR "A minimum of clang++ ${CLANGCXX_MINVER} is required for C++17 support")
    endif()
+    string (REGEX REPLACE "^([0-9]+)\\.([0-9]+)\\.([0-9]+)$" "\\1" CLANG_MAJOR_VERSION "${CMAKE_CXX_COMPILER_VERSION}")
 endif()

 # compiler version checks TODO: test more compilers
--- a/cmake/simde.cmake
+++ b/cmake/simde.cmake
@ -10,6 +10,14 @@ if (SIMDE_SSE42_H_FOUND)
  if (CMAKE_COMPILER_IS_CLANG)
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DSIMDE_NO_CHECK_IMMEDIATE_CONSTANT")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSIMDE_NO_CHECK_IMMEDIATE_CONSTANT")
+    if (ARCH_PPC64EL)
+        set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-deprecated-altivec-src-compat")
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-altivec-src-compat")
+	if (CLANG_MAJOR_VERSION EQUAL 15)
+            set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-deprecate-lax-vec-conv-all")
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecate-lax-vec-conv-all")
+        endif ()
+    endif()
  endif()

  if (SIMDE_NATIVE)
--- a/src/nfa/goughcompile.cpp
+++ b/src/nfa/goughcompile.cpp
@ -1299,7 +1299,7 @@ unique_ptr<raw_report_info> gough_build_strat::gatherReports(
    *arbReport = MO_INVALID_IDX;
    assert(!ri->rl.empty()); /* all components should be able to generate
                                reports */
-    return std::move(ri);
+    return ri;
 }

 u32 raw_gough_report_info_impl::getReportListSize() const {
--- a/src/nfa/limex_dump.cpp
+++ b/src/nfa/limex_dump.cpp
@ -136,7 +136,7 @@ void dumpLimexReachMasks(u32 model_size, const u8 *reach, u32 reachCount,
    for (u32 i = 0; i < reachCount; i++) {
        char tmp_common[100];
        const u8 *row = reach + (i * (model_size/8));
-        sprintf(tmp_common, "reach mask %u ", i);
+        snprintf(tmp_common, sizeof(tmp_common), "reach mask %u ", i);
        dumpMask(f, tmp_common, row, model_size);
    }
 }
--- a/src/nfa/mcclellancompile.cpp
+++ b/src/nfa/mcclellancompile.cpp
@ -462,7 +462,7 @@ unique_ptr<raw_report_info> mcclellan_build_strat::gatherReports(
        *isSingleReport = 0;
    }

-    return std::move(ri);
+    return ri;
 }

 u32 raw_report_info_impl::getReportListSize() const {
@ -620,7 +620,7 @@ bytecode_ptr<NFA> mcclellanCompile16(dfa_info &info, const CompileContext &cc,
    u8 alphaShift = info.getAlphaShift();
    assert(alphaShift <= 8);

-    u16 count_real_states;
+    u16 count_real_states{0};
    u16 wide_limit;
    if (!allocateFSN16(info, &count_real_states, &wide_limit)) {
        DEBUG_PRINTF("failed to allocate state numbers, %zu states total\n",
--- a/src/nfa/nfa_build_util.cpp
+++ b/src/nfa/nfa_build_util.cpp
@ -347,7 +347,6 @@ template<> struct NFATraits<LBR_NFA_VERM16> {
    UNUSED static const char *name;
    static const NFACategory category = NFA_OTHER;
    static const u32 stateAlign = 8;
-    static const bool fast = true;
    static const nfa_dispatch_fn has_accel;
    static const nfa_dispatch_fn has_repeats;
    static const nfa_dispatch_fn has_repeats_other_than_firsts;
@ -363,7 +362,6 @@ template<> struct NFATraits<LBR_NFA_NVERM16> {
    UNUSED static const char *name;
    static const NFACategory category = NFA_OTHER;
    static const u32 stateAlign = 8;
-    static const bool fast = true;
    static const nfa_dispatch_fn has_accel;
    static const nfa_dispatch_fn has_repeats;
    static const nfa_dispatch_fn has_repeats_other_than_firsts;
--- a/src/nfa/shengcompile.cpp
+++ b/src/nfa/shengcompile.cpp
@ -270,7 +270,7 @@ unique_ptr<raw_report_info> sheng_build_strat::gatherReports(
        *isSingleReport = 0;
    }

-    return std::move(ri);
+    return ri;
 }

 u32 sheng_build_strat::max_allowed_offset_accel() const {
--- a/src/parser/Parser.rl
+++ b/src/parser/Parser.rl
@ -2024,7 +2024,7 @@ unique_ptr<Component> parse(const char *ptr, ParseMode &globalMode) {
        // Ensure that all references are valid.
        checkReferences(*rootSeq, groupIndex, groupNames);

-        return std::move(rootSeq);
+        return rootSeq;
    } catch (LocatedParseError &error) {
        if (ts >= ptr && ts <= pe) {
            error.locate(ts - ptr);
--- a/src/rose/rose_build_matchers.cpp
+++ b/src/rose/rose_build_matchers.cpp
@ -62,8 +62,6 @@ using boost::adaptors::map_values;

 namespace ue2 {

-static const size_t MAX_ACCEL_STRING_LEN = 16;
-
 #if defined(DEBUG) || defined(DUMP_SUPPORT)
 static UNUSED
 string dumpMask(const vector<u8> &v) {
--- a/src/util/arch/x86/x86.h
+++ b/src/util/arch/x86/x86.h
@ -64,7 +64,7 @@
 #define HAVE_SIMD_512_BITS
 #endif

-#if defined(__AVX512VBMI__) && defined(BUILD_AVX512)
+#if defined(__AVX512VBMI__) && defined(BUILD_AVX512VBMI)
 #define HAVE_AVX512VBMI
 #endif

--- a/tools/hsbench/engine_hyperscan.cpp
+++ b/tools/hsbench/engine_hyperscan.cpp
@ -174,7 +174,7 @@ unique_ptr<EngineStream> EngineHyperscan::streamOpen(EngineContext &ectx,
        return nullptr;
    }
    stream->sn = streamId;
-    return std::move(stream);
+    return stream;
 }

 void EngineHyperscan::streamClose(unique_ptr<EngineStream> stream,
--- a/unit/CMakeLists.txt
+++ b/unit/CMakeLists.txt
@ -29,6 +29,10 @@ if(CMAKE_COMPILER_IS_GNUCC)
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-array-bounds")
 endif()

+if(CMAKE_COMPILER_IS_CLANG AND ARCH_PPC64EL)
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-pass-failed")
+endif()
+
 add_definitions(-DGTEST_HAS_PTHREAD=0 -DSRCDIR=${PROJECT_SOURCE_DIR})

 set(unit_hyperscan_SOURCES
--- a/unit/hyperscan/arg_checks.cpp
+++ b/unit/hyperscan/arg_checks.cpp
@ -2430,7 +2430,6 @@ TEST(HyperscanArgChecks, ExpandNoTo) {
    err = hs_compress_stream(stream1, buf, sizeof(buf), &used);
    ASSERT_EQ(HS_SUCCESS, err);

-    hs_stream_t *stream2;
    err = hs_expand_stream(db, nullptr, buf, used);
    ASSERT_EQ(HS_INVALID, err);