From b4dea9d65b0741d22cecbad2be3b9098c240f8c1 Mon Sep 17 00:00:00 2001 From: Toly Kournik <50289004+toly-kournik@users.noreply.github.com> Date: Sat, 6 May 2023 01:57:42 +0300 Subject: [PATCH] CPPCHECK improvements (#3004) - enable uninitMemberVar and corresponding code fixes; - set cmake options when CPPCHECK option is enabled; --- .cppcheck/suppressions.txt | 1 - .github/workflows/cppcheck.yml | 2 +- CMakeLists.txt | 18 ++++--------- bftengine/include/bftengine/EpochManager.hpp | 4 +-- bftengine/src/bftengine/DebugStatistics.hpp | 26 +++++++++---------- .../bftengine/ReplicaSpecificInfoManager.hpp | 6 ++--- .../src/bftengine/RetransmissionsManager.cpp | 4 +-- .../messages/PreProcessResultMsg.hpp | 2 +- .../bftclient/include/bftclient/fake_comm.h | 4 +-- cmake/cppcheck.cmake | 5 +++- kvbc/include/KVBCInterfaces.h | 6 ++--- libs/util/throughput.hpp | 2 +- 12 files changed, 37 insertions(+), 43 deletions(-) diff --git a/.cppcheck/suppressions.txt b/.cppcheck/suppressions.txt index ea5ecf5e7c..6fbc4689e1 100644 --- a/.cppcheck/suppressions.txt +++ b/.cppcheck/suppressions.txt @@ -4,7 +4,6 @@ # TODO [TK] - do we want enable cppckeck for tests? incorrectStringBooleanError -uninitMemberVar noCopyConstructor noOperatorEq diff --git a/.github/workflows/cppcheck.yml b/.github/workflows/cppcheck.yml index 1ad89b01e6..3514350b6a 100644 --- a/.github/workflows/cppcheck.yml +++ b/.github/workflows/cppcheck.yml @@ -22,5 +22,5 @@ jobs: mkdir build cd build echo $(date +%y-%m-%d_%H-%M-%S) > timestamp - cmake -DBUILD_TESTING=OFF -DBUILD_UTT=OFF -DUSE_LOG4CPP=OFF -DCPPCHECK=ON .. + cmake -DCPPCHECK=ON .. make -j$(nproc) diff --git a/CMakeLists.txt b/CMakeLists.txt index dfd286216d..667576d051 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,14 +4,15 @@ project(concord-bft VERSION 0.1.0.0 LANGUAGES CXX) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) -set(SLEEP_FOR_DBG FALSE) + # targets with generic names like "format" may already exist in an imported libs cmake_policy(SET CMP0002 OLD) set(ALLOW_DUPLICATE_CUSTOM_TARGETS TRUE) + set(MIN_BOOST_VERSION 1.80) set(YAML_CPP_VERSION 0.7.0) -# Default to debug builds +# Defaults to debug builds # Release builds can be enabled by running cmake with -DCMAKE_BUILD_TYPE=Release if(NOT CMAKE_BUILD_TYPE) set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Enable debug or release builds" FORCE) @@ -21,10 +22,7 @@ option(USE_LOG4CPP "Enable LOG4CPP" ON) option(RUN_APOLLO_TESTS "Enable Apollo tests run" ON) option(KEEP_APOLLO_LOGS "Retains logs from replicas in separate folder for each test in build/tests/apollo/logs" ON) option(TXN_SIGNING_ENABLED "Enable External concord client transcattion signing" ON) -option(LEAKCHECK "Enable Address and Leak Sanitizers" OFF) -option(HEAPTRACK "Enable Heaptrack - a heap memory profiler for Linux" OFF) -option(THREADCHECK "Enable Thread Sanitizer" OFF) -option(UNDEFINED_BEHAVIOR_CHECK "Enable Undefined Behavior Sanitizer" OFF) + # Rocksdb is required for storage now. Consider removing this flag. option(BUILD_ROCKSDB_STORAGE "Enable building of RocksDB storage library" ON) option(USE_S3_OBJECT_STORE "Enable S3 Object Store" ON) @@ -45,10 +43,6 @@ if((USE_OPENSSL) AND NOT BUILD_THIRDPARTY) set(OPENSSL_ROOT_DIR /usr/local/ssl) # not to confuse with system ssl libs endif() -if(SLEEP_FOR_DBG) - add_definitions(-DSLEEP_DBG) -endif() - # include compiler specific options include(cmake/${CMAKE_CXX_COMPILER_ID}.cmake) include(cmake/cppcheck.cmake) @@ -56,9 +50,7 @@ include(cmake/cppcheck.cmake) list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/cmake") include(cmake/grpc_utils.cmake) -if(BUILD_TESTING) - include(CTest) -endif() +include(CTest) if(USE_S3_OBJECT_STORE) add_compile_definitions(USE_S3_OBJECT_STORE=1) diff --git a/bftengine/include/bftengine/EpochManager.hpp b/bftengine/include/bftengine/EpochManager.hpp index 06f21cb0a8..19f4fa4c6f 100644 --- a/bftengine/include/bftengine/EpochManager.hpp +++ b/bftengine/include/bftengine/EpochManager.hpp @@ -20,7 +20,7 @@ namespace bftEngine { class EpochManager : public bftEngine::ResPagesClient { struct EpochData : public concord::serialize::SerializableFactory { - uint64_t epochNumber_; + uint64_t epochNumber_ = 0; EpochData() = default; EpochData(uint64_t epochNumber) : epochNumber_{epochNumber} {} @@ -79,4 +79,4 @@ class EpochManager : public bftEngine::ResPagesClient { concordMetrics::Component metrics_; concordMetrics::GaugeHandle epoch_number_gauge_; }; -} // namespace bftEngine \ No newline at end of file +} // namespace bftEngine diff --git a/bftengine/src/bftengine/DebugStatistics.hpp b/bftengine/src/bftengine/DebugStatistics.hpp index 95435320ec..ac17a7e401 100644 --- a/bftengine/src/bftengine/DebugStatistics.hpp +++ b/bftengine/src/bftengine/DebugStatistics.hpp @@ -41,21 +41,21 @@ class DebugStatistics { private: struct DebugStatDesc { - bool initialized; + bool initialized = false; Time lastCycleTime; - size_t receivedMessages; - std::atomic sendMessages; - size_t completedReadOnlyRequests; - size_t completedReadWriteRequests; - size_t numberOfReceivedSTMessages; - size_t numberOfReceivedStatusMessages; - size_t numberOfReceivedCommitMessages; - int64_t lastExecutedSequenceNumber; - - size_t prePrepareMessages; - size_t batchRequests; - size_t pendingRequests; + size_t receivedMessages = 0; + std::atomic sendMessages = 0; + size_t completedReadOnlyRequests = 0; + size_t completedReadWriteRequests = 0; + size_t numberOfReceivedSTMessages = 0; + size_t numberOfReceivedStatusMessages = 0; + size_t numberOfReceivedCommitMessages = 0; + int64_t lastExecutedSequenceNumber = 0; + + size_t prePrepareMessages = 0; + size_t batchRequests = 0; + size_t pendingRequests = 0; DebugStatDesc() : initialized(false) {} }; diff --git a/bftengine/src/bftengine/ReplicaSpecificInfoManager.hpp b/bftengine/src/bftengine/ReplicaSpecificInfoManager.hpp index e580d58b49..e832b26ffb 100644 --- a/bftengine/src/bftengine/ReplicaSpecificInfoManager.hpp +++ b/bftengine/src/bftengine/ReplicaSpecificInfoManager.hpp @@ -34,8 +34,8 @@ class RsiItem { private: std::string data_; - uint64_t index_; - uint64_t req_seq_num_; + uint64_t index_ = 0; + uint64_t req_seq_num_ = 0; }; class RsiDataManager { @@ -58,4 +58,4 @@ class RsiDataManager { std::unordered_map clientsIndex_; }; } // namespace impl -} // namespace bftEngine \ No newline at end of file +} // namespace bftEngine diff --git a/bftengine/src/bftengine/RetransmissionsManager.cpp b/bftengine/src/bftengine/RetransmissionsManager.cpp index a7a3cf25ef..633db9b09a 100644 --- a/bftengine/src/bftengine/RetransmissionsManager.cpp +++ b/bftengine/src/bftengine/RetransmissionsManager.cpp @@ -169,8 +169,8 @@ class RetransmissionsLogic { SeqNum seqNumber = 0; bool ackOrAbort = false; - uint16_t numOfTransmissions; // valid only if ackOrAbort==false - Time timeOfTransmission; // valid only if ackOrAbort==false + uint16_t numOfTransmissions = 0; // valid only if ackOrAbort==false + Time timeOfTransmission; // valid only if ackOrAbort==false }; class ReplicaInfo { diff --git a/bftengine/src/preprocessor/messages/PreProcessResultMsg.hpp b/bftengine/src/preprocessor/messages/PreProcessResultMsg.hpp index 462b8fb3f9..c493d25179 100644 --- a/bftengine/src/preprocessor/messages/PreProcessResultMsg.hpp +++ b/bftengine/src/preprocessor/messages/PreProcessResultMsg.hpp @@ -63,7 +63,7 @@ class PreProcessResultMsg : public ClientRequestMsg { struct PreProcessResultSignature { std::vector signature; NodeIdType sender_replica; - bftEngine::OperationResult pre_process_result; + bftEngine::OperationResult pre_process_result = bftEngine::OperationResult::UNKNOWN; PreProcessResultSignature() = default; diff --git a/client/bftclient/include/bftclient/fake_comm.h b/client/bftclient/include/bftclient/fake_comm.h index 41ca6954ef..9821b5a936 100644 --- a/client/bftclient/include/bftclient/fake_comm.h +++ b/client/bftclient/include/bftclient/fake_comm.h @@ -78,7 +78,7 @@ class BehaviorThreadRunner { void stop() { stop_ = true; } private: - IReceiver* receiver_; + IReceiver* receiver_ = nullptr; std::atomic stop_ = false; Behavior behavior_; @@ -131,7 +131,7 @@ class FakeCommunication : public bft::communication::ICommunication { void restartCommunication(NodeNum i) override {} private: - IReceiver* receiver_; + IReceiver* receiver_ = nullptr; BehaviorThreadRunner runner_; std::thread fakeCommThread_; }; diff --git a/cmake/cppcheck.cmake b/cmake/cppcheck.cmake index 492264cf4d..053c037f7e 100644 --- a/cmake/cppcheck.cmake +++ b/cmake/cppcheck.cmake @@ -1,7 +1,10 @@ option(CPPCHECK "Perform cppcheck" OFF) if(CPPCHECK) - message(STATUS "CPPCHECK is ON") + set(BUILD_TESTING OFF) + set(BUILD_UTT OFF) + set(USE_LOG4CPP OFF) + message(STATUS "CPPCHECK is ON") find_program(cppcheck cppcheck HINTS "/usr/local/bin/cppcheck" REQUIRED) message(STATUS "cppcheck ${cppcheck}") # Create work folder for whole program analysis, for faster analysis and to store some useful debug information diff --git a/kvbc/include/KVBCInterfaces.h b/kvbc/include/KVBCInterfaces.h index f67bf934b5..d9d6bfeda3 100644 --- a/kvbc/include/KVBCInterfaces.h +++ b/kvbc/include/KVBCInterfaces.h @@ -33,17 +33,17 @@ class ICommandsHandler; struct ClientConfig { // F value - max number of faulty/malicious replicas. fVal >= 1 - uint16_t fVal; + uint16_t fVal = 0; // C value. cVal >=0 - uint16_t cVal; + uint16_t cVal = 0; // unique identifier of the client. // clientId should also represent this client in ICommunication. // In the current version, replicaId should be a number between N and // N+numOfClientProxies-1 (N is the number replicas in the system. // numOfClientProxies is part of the replicas' configuration) - uint16_t clientId; + uint16_t clientId = 0; }; ///////////////////////////////////////////////////////////////////////////// diff --git a/libs/util/throughput.hpp b/libs/util/throughput.hpp index ac870863f9..0549410838 100644 --- a/libs/util/throughput.hpp +++ b/libs/util/throughput.hpp @@ -191,7 +191,7 @@ class Throughput { Stats overall_stats_; Stats current_window_stats_; Stats previous_window_stats_; - uint64_t previous_window_index_; + uint64_t previous_window_index_ = 0; uint64_t reports_counter_ = 0; std::string name_; }; // class Throughput