From 55ff684f347adca71f370c550de695fbe6016507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Tue, 18 Jan 2022 22:02:25 +0100 Subject: [PATCH] added unusedFunction self check to CI / cleanups (#3526) --- .github/workflows/selfcheck.yml | 75 +++++++++++++++++ .selfcheck_unused_suppressions | 13 +++ cli/threadexecutor.cpp | 83 +++++++------------ cli/threadexecutor.h | 10 ++- gui/checkthread.cpp | 1 + gui/codeeditstylecontrols.cpp | 2 + .../testtranslationhandler.cpp | 11 ++- gui/translationhandler.cpp | 9 -- gui/translationhandler.h | 7 -- lib/checkunusedfunctions.cpp | 5 +- lib/forwardanalyzer.cpp | 1 + lib/library.cpp | 3 + lib/mathlib.cpp | 4 + lib/programmemory.cpp | 2 + lib/token.cpp | 2 + lib/tokenize.cpp | 20 +++-- test/testlibrary.cpp | 4 +- test/testuninitvar.cpp | 2 +- test/testvalueflow.cpp | 2 +- 19 files changed, 170 insertions(+), 86 deletions(-) create mode 100644 .github/workflows/selfcheck.yml create mode 100644 .selfcheck_unused_suppressions diff --git a/.github/workflows/selfcheck.yml b/.github/workflows/selfcheck.yml new file mode 100644 index 000000000..2ed4a2602 --- /dev/null +++ b/.github/workflows/selfcheck.yml @@ -0,0 +1,75 @@ +# Syntax reference https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions +# Environment reference https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners +name: selfcheck + +on: [push, pull_request] + +jobs: + build: + + runs-on: ubuntu-20.04 + + steps: + - uses: actions/checkout@v2 + + - name: Install missing software + run: | + sudo apt-get update + sudo apt-get install z3 libz3-dev + + - name: Install Qt + uses: jurplel/install-qt-action@v2 + with: + version: '5.15.2' + modules: 'qtcharts' + + # TODO: cache this - perform same build as for the other self check + - name: Self check (build) + run: | + make clean + make -j$(nproc) -s CXXFLAGS="-O2 -w" MATCHCOMPILER=yes + + - name: CMake + run: | + mkdir cmake.output + pushd cmake.output + cmake -G "Unix Makefiles" -DUSE_Z3=On -DHAVE_RULES=On -DBUILD_TESTS=On -DBUILD_GUI=ON -DWITH_QCHART=ON -DCMAKE_GLOBAL_AUTOGEN_TARGET=On .. + + - name: Generate dependencies + run: | + # make sure the precompiled headers exist + make -C cmake.output lib/CMakeFiles/lib_objs.dir/cmake_pch.hxx.cxx + make -C cmake.output test/CMakeFiles/testrunner.dir/cmake_pch.hxx.cxx + # make sure auto-generated GUI files exist + make -C cmake.output autogen + make -C cmake.output gui-build-deps + + # TODO: find a way to report unmatched suppressions without need to add information checks + - name: Self check (unusedFunction) + if: false # TODO: fails with preprocessorErrorDirective - see #10667 + run: | + ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__GNUC__ -DQT_VERSION=0x050000 -DQ_MOC_OUTPUT_REVISION=67 --inconclusive --enable=unusedFunction --exception-handling -rp=. --project=cmake.output/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr + env: + DISABLE_VALUEFLOW: 1 + + # the following steps are duplicated from above since setting up the buld node in a parallel step takes longer than the actual steps + - name: CMake (no test) + run: | + mkdir cmake.output.notest + pushd cmake.output.notest + cmake -G "Unix Makefiles" -DUSE_Z3=On -DHAVE_RULES=On -DBUILD_TESTS=0 -DBUILD_GUI=ON -DWITH_QCHART=ON -DCMAKE_GLOBAL_AUTOGEN_TARGET=On .. + + - name: Generate dependencies (no test) + run: | + # make sure the precompiled headers exist + make -C cmake.output.notest lib/CMakeFiles/lib_objs.dir/cmake_pch.hxx.cxx + # make sure auto-generated GUI files exist + make -C cmake.output.notest autogen + make -C cmake.output.notest gui-build-deps + + # TODO: find a way to report unmatched suppressions without need to add information checks + - name: Self check (unusedFunction / no test) + run: | + ./cppcheck -q --template=selfcheck --error-exitcode=1 --library=cppcheck-lib --library=qt -D__GNUC__ -DQT_VERSION=0x050000 -DQ_MOC_OUTPUT_REVISION=67 --inconclusive --enable=unusedFunction --exception-handling -rp=. --project=cmake.output.notest/compile_commands.json --suppressions-list=.selfcheck_unused_suppressions --inline-suppr + env: + DISABLE_VALUEFLOW: 1 \ No newline at end of file diff --git a/.selfcheck_unused_suppressions b/.selfcheck_unused_suppressions new file mode 100644 index 000000000..f66bcc905 --- /dev/null +++ b/.selfcheck_unused_suppressions @@ -0,0 +1,13 @@ +# we are not using all methods of their interfaces +unusedFunction:externals/tinyxml2/tinyxml2.cpp +unusedFunction:externals/simplecpp/simplecpp.cpp + +# TODO: fix these +# false positive - # 10660 +unusedFunction:gui/mainwindow.cpp +unusedFunction:gui/resultstree.cpp +unusedFunction:gui/codeeditor.cpp +# usage is disabled +unusedFunction:lib/symboldatabase.cpp +# false positive - #10661 +unusedFunction:oss-fuzz/main.cpp \ No newline at end of file diff --git a/cli/threadexecutor.cpp b/cli/threadexecutor.cpp index 99e7c3c1e..699905bc9 100644 --- a/cli/threadexecutor.cpp +++ b/cli/threadexecutor.cpp @@ -55,7 +55,6 @@ using std::memset; ThreadExecutor::ThreadExecutor(const std::map &files, Settings &settings, ErrorLogger &errorLogger) : mFiles(files), mSettings(settings), mErrorLogger(errorLogger), mFileCount(0) - // Not initialized mFileSync, mErrorSync, mReportSync { #if defined(THREADING_MODEL_FORK) mWpipe = 0; @@ -68,10 +67,23 @@ ThreadExecutor::ThreadExecutor(const std::map &files, } ThreadExecutor::~ThreadExecutor() +{} + +// cppcheck-suppress unusedFunction - only used in unit tests +void ThreadExecutor::addFileContent(const std::string &path, const std::string &content) { - //dtor + mFileContents[path] = content; } +void ThreadExecutor::reportErr(const ErrorMessage &msg) +{ + report(msg, MessageType::REPORT_ERROR); +} + +void ThreadExecutor::reportInfo(const ErrorMessage &msg) +{ + report(msg, MessageType::REPORT_INFO); +} /////////////////////////////////////////////////////////////////////////////// ////// This code is for platforms that support fork() only //////////////////// @@ -79,11 +91,6 @@ ThreadExecutor::~ThreadExecutor() #if defined(THREADING_MODEL_FORK) -void ThreadExecutor::addFileContent(const std::string &path, const std::string &content) -{ - mFileContents[path] = content; -} - int ThreadExecutor::handleRead(int rpipe, unsigned int &result) { char type = 0; @@ -351,21 +358,26 @@ void ThreadExecutor::reportOut(const std::string &outmsg, Color c) writeToPipe(REPORT_OUT, ::toString(c) + outmsg + ::toString(Color::Reset)); } -void ThreadExecutor::reportErr(const ErrorMessage &msg) -{ - writeToPipe(REPORT_ERROR, msg.serialize()); -} - -void ThreadExecutor::reportInfo(const ErrorMessage &msg) -{ - writeToPipe(REPORT_INFO, msg.serialize()); -} - void ThreadExecutor::bughuntingReport(const std::string &str) { writeToPipe(REPORT_VERIFICATION, str); } +void ThreadExecutor::report(const ErrorMessage &msg, MessageType msgType) +{ + PipeSignal pipeSignal; + switch (msgType) { + case MessageType::REPORT_ERROR: + pipeSignal = REPORT_ERROR; + break; + case MessageType::REPORT_INFO: + pipeSignal = REPORT_INFO; + break; + } + + writeToPipe(pipeSignal, msg.serialize()); +} + void ThreadExecutor::reportInternalChildErr(const std::string &childname, const std::string &msg) { std::list locations; @@ -383,11 +395,6 @@ void ThreadExecutor::reportInternalChildErr(const std::string &childname, const #elif defined(THREADING_MODEL_WIN) -void ThreadExecutor::addFileContent(const std::string &path, const std::string &content) -{ - mFileContents[path] = content; -} - unsigned int ThreadExecutor::check() { std::vector> threadFutures; @@ -481,15 +488,6 @@ void ThreadExecutor::reportOut(const std::string &outmsg, Color c) mErrorLogger.reportOut(outmsg, c); } -void ThreadExecutor::reportErr(const ErrorMessage &msg) -{ - report(msg, MessageType::REPORT_ERROR); -} - -void ThreadExecutor::reportInfo(const ErrorMessage &msg) -{ - report(msg, MessageType::REPORT_INFO); -} void ThreadExecutor::bughuntingReport(const std::string & /*str*/) { @@ -513,7 +511,6 @@ void ThreadExecutor::report(const ErrorMessage &msg, MessageType msgType) } } - if (reportError) { std::lock_guard lg(mReportSync); @@ -527,26 +524,4 @@ void ThreadExecutor::report(const ErrorMessage &msg, MessageType msgType) } } } - -#else - -void ThreadExecutor::addFileContent(const std::string & /*path*/, const std::string & /*content*/) -{} - -unsigned int ThreadExecutor::check() -{ - return 0; -} - -void ThreadExecutor::reportOut(const std::string & /*outmsg*/, Color) -{} -void ThreadExecutor::reportErr(const ErrorMessage & /*msg*/) -{} - -void ThreadExecutor::reportInfo(const ErrorMessage & /*msg*/) -{} - -void ThreadExecutor::bughuntingReport(const std::string & /*str*/) -{} - #endif diff --git a/cli/threadexecutor.h b/cli/threadexecutor.h index 2d5d88df0..87d1906df 100644 --- a/cli/threadexecutor.h +++ b/cli/threadexecutor.h @@ -34,6 +34,8 @@ #define THREADING_MODEL_WIN #include "importproject.h" #include +#else +#error "No threading moodel defined" #endif class Settings; @@ -68,11 +70,15 @@ public: void addFileContent(const std::string &path, const std::string &content); private: + enum class MessageType {REPORT_ERROR, REPORT_INFO}; + const std::map &mFiles; Settings &mSettings; ErrorLogger &mErrorLogger; unsigned int mFileCount; + void report(const ErrorMessage &msg, MessageType msgType); + #if defined(THREADING_MODEL_FORK) /** @brief Key is file name, and value is the content of the file */ @@ -119,8 +125,6 @@ public: #elif defined(THREADING_MODEL_WIN) private: - enum class MessageType {REPORT_ERROR, REPORT_INFO}; - std::map mFileContents; std::map::const_iterator mItNextFile; std::list::const_iterator mItNextFileSettings; @@ -135,8 +139,6 @@ private: std::mutex mReportSync; - void report(const ErrorMessage &msg, MessageType msgType); - static unsigned __stdcall threadProc(ThreadExecutor *threadExecutor); public: diff --git a/gui/checkthread.cpp b/gui/checkthread.cpp index 6e14ab419..d966f46b3 100644 --- a/gui/checkthread.cpp +++ b/gui/checkthread.cpp @@ -83,6 +83,7 @@ void CheckThread::analyseWholeProgram(const QStringList &files) start(); } +// cppcheck-suppress unusedFunction - TODO: false positive void CheckThread::run() { mState = Running; diff --git a/gui/codeeditstylecontrols.cpp b/gui/codeeditstylecontrols.cpp index 7f492211c..79c6b288b 100644 --- a/gui/codeeditstylecontrols.cpp +++ b/gui/codeeditstylecontrols.cpp @@ -57,6 +57,7 @@ void SelectColorButton::setColor(const QColor& color) updateColor(); } +// cppcheck-suppress unusedFunction const QColor& SelectColorButton::getColor() { return mColor; @@ -114,6 +115,7 @@ void SelectFontWeightCombo::setWeight(const QFont::Weight& weight) updateWeight(); } +// cppcheck-suppress unusedFunction const QFont::Weight& SelectFontWeightCombo::getWeight() { return mWeight; diff --git a/gui/test/translationhandler/testtranslationhandler.cpp b/gui/test/translationhandler/testtranslationhandler.cpp index 533fe5b7e..d59e50b36 100644 --- a/gui/test/translationhandler/testtranslationhandler.cpp +++ b/gui/test/translationhandler/testtranslationhandler.cpp @@ -20,10 +20,19 @@ #include "testtranslationhandler.h" #include "translationhandler.h" +static const QStringList getTranslationNames(const TranslationHandler& handler) +{ + QStringList names; + foreach (TranslationInfo translation, handler.getTranslations()) { + names.append(translation.mName); + } + return names; +} + void TestTranslationHandler::construct() { TranslationHandler handler; - QCOMPARE(handler.getNames().size(), 13); // 12 translations + english + QCOMPARE(getTranslationNames(handler).size(), 13); // 12 translations + english QCOMPARE(handler.getCurrentLanguage(), QString("en")); } diff --git a/gui/translationhandler.cpp b/gui/translationhandler.cpp index 4deb0a023..eac429265 100644 --- a/gui/translationhandler.cpp +++ b/gui/translationhandler.cpp @@ -61,15 +61,6 @@ TranslationHandler::TranslationHandler(QObject *parent) : TranslationHandler::~TranslationHandler() {} -const QStringList TranslationHandler::getNames() const -{ - QStringList names; - foreach (TranslationInfo translation, mTranslations) { - names.append(translation.mName); - } - return names; -} - bool TranslationHandler::setLanguage(const QString &code) { bool failure = false; diff --git a/gui/translationhandler.h b/gui/translationhandler.h index ac04df0ab..ab96c2132 100644 --- a/gui/translationhandler.h +++ b/gui/translationhandler.h @@ -65,13 +65,6 @@ public: explicit TranslationHandler(QObject *parent = nullptr); virtual ~TranslationHandler(); - /** - * @brief Get a list of available translation names. - * @return List of available translation names. - * - */ - const QStringList getNames() const; - /** * @brief Get a list of available translations. * @return List of available translations. diff --git a/lib/checkunusedfunctions.cpp b/lib/checkunusedfunctions.cpp index 5b6700783..87631e759 100644 --- a/lib/checkunusedfunctions.cpp +++ b/lib/checkunusedfunctions.cpp @@ -436,7 +436,10 @@ void CheckUnusedFunctions::analyseWholeProgram(ErrorLogger * const errorLogger, for (std::map::const_iterator decl = decls.begin(); decl != decls.end(); ++decl) { const std::string &functionName = decl->first; - if (functionName == "main" || functionName == "WinMain" || functionName == "_tmain" || + // TODO: move to configuration files + // TODO: WinMain, wmain and _tmain only apply to Windows code + // TODO: also skip other known entry functions i.e. annotated with "constructor" and "destructor" attributes + if (functionName == "main" || functionName == "WinMain" || functionName == "wmain" || functionName == "_tmain" || functionName == "if") continue; diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 67c48347d..bf94b39fd 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -89,6 +89,7 @@ struct ForwardTraversal { return evalCond(tok, ctx).first; } + // cppcheck-suppress unusedFunction bool isConditionFalse(const Token* tok, const Token* ctx = nullptr) const { return evalCond(tok, ctx).second; } diff --git a/lib/library.cpp b/lib/library.cpp index 806941853..371ca70d4 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -184,6 +184,7 @@ Library::Container::Action Library::Container::actionFrom(const std::string& act return Container::Action::NO_ACTION; } +// cppcheck-suppress unusedFunction - only used in unit tests bool Library::loadxmldata(const char xmldata[], std::size_t len) { tinyxml2::XMLDocument doc; @@ -1577,6 +1578,8 @@ const Token* Library::getContainerFromYield(const Token* tok, Library::Container } return nullptr; } + +// cppcheck-suppress unusedFunction const Token* Library::getContainerFromAction(const Token* tok, Library::Container::Action action) const { if (!tok) diff --git a/lib/mathlib.cpp b/lib/mathlib.cpp index cd9f8b556..ee9747842 100644 --- a/lib/mathlib.cpp +++ b/lib/mathlib.cpp @@ -698,6 +698,7 @@ static bool isValidIntegerSuffixIt(std::string::const_iterator it, std::string:: (state == Status::SUFFIX_UI64)); } +// cppcheck-suppress unusedFunction bool MathLib::isValidIntegerSuffix(const std::string& str, bool supportMicrosoftExtensions) { return isValidIntegerSuffixIt(str.begin(), str.end(), supportMicrosoftExtensions); @@ -1180,16 +1181,19 @@ bool MathLib::isNotEqual(const std::string &first, const std::string &second) return !isEqual(first, second); } +// cppcheck-suppress unusedFunction bool MathLib::isGreater(const std::string &first, const std::string &second) { return toDoubleNumber(first) > toDoubleNumber(second); } +// cppcheck-suppress unusedFunction bool MathLib::isGreaterEqual(const std::string &first, const std::string &second) { return toDoubleNumber(first) >= toDoubleNumber(second); } +// cppcheck-suppress unusedFunction bool MathLib::isLess(const std::string &first, const std::string &second) { return toDoubleNumber(first) < toDoubleNumber(second); diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 6f450205e..cee110dc5 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -28,6 +28,7 @@ const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossib return nullptr; } +// cppcheck-suppress unusedFunction bool ProgramMemory::getIntValue(nonneg int exprid, MathLib::bigint* result) const { const ValueFlow::Value* value = getValue(exprid); @@ -56,6 +57,7 @@ bool ProgramMemory::getTokValue(nonneg int exprid, const Token** result) const return false; } +// cppcheck-suppress unusedFunction bool ProgramMemory::getContainerSizeValue(nonneg int exprid, MathLib::bigint* result) const { const ValueFlow::Value* value = getValue(exprid); diff --git a/lib/token.cpp b/lib/token.cpp index 677af05c9..f36cad061 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1185,6 +1185,7 @@ void Token::printOut(const char *title, const std::vector &fileName std::cout << stringifyList(stringifyOptions::forPrintOut(), &fileNames, nullptr) << std::endl; } +// cppcheck-suppress unusedFunction - used for debugging void Token::printLines(int lines) const { const Token *end = this; @@ -2446,6 +2447,7 @@ const ValueFlow::Value* Token::getMovedValue() const return it == mImpl->mValues->end() ? nullptr : &*it; } +// cppcheck-suppress unusedFunction const ValueFlow::Value* Token::getContainerSizeValue(const MathLib::bigint val) const { if (!mImpl->mValues) diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 4996285d5..b7bc852aa 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -2807,11 +2807,17 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration) if (!mSettings->buildDir.empty()) Summaries::create(this, configuration); - if (mTimerResults) { - Timer t("Tokenizer::simplifyTokens1::ValueFlow", mSettings->showtime, mTimerResults); - ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings); - } else { - ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings); + // TODO: do not run valueflow if no checks are being performed at all - e.g. unusedFunctions only + const char* disableValueflowEnv = std::getenv("DISABLE_VALUEFLOW"); + const bool doValueFlow = !disableValueflowEnv || (std::strcmp(disableValueflowEnv, "1") != 0); + + if (doValueFlow) { + if (mTimerResults) { + Timer t("Tokenizer::simplifyTokens1::ValueFlow", mSettings->showtime, mTimerResults); + ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings); + } else { + ValueFlow::setValues(&list, mSymbolDatabase, mErrorLogger, mSettings); + } } // Warn about unhandled character literals @@ -2827,7 +2833,9 @@ bool Tokenizer::simplifyTokens1(const std::string &configuration) } } - mSymbolDatabase->setArrayDimensionsUsingValueFlow(); + if (doValueFlow) { + mSymbolDatabase->setArrayDimensionsUsingValueFlow(); + } printDebugOutput(1); diff --git a/test/testlibrary.cpp b/test/testlibrary.cpp index da3c44ef1..7006b7647 100644 --- a/test/testlibrary.cpp +++ b/test/testlibrary.cpp @@ -665,8 +665,8 @@ private: ""; Library library; - library.loadxmldata(xmldata1, sizeof(xmldata1)); - library.loadxmldata(xmldata2, sizeof(xmldata2)); + ASSERT_EQUALS(true, library.loadxmldata(xmldata1, sizeof(xmldata1))); + ASSERT_EQUALS(true, library.loadxmldata(xmldata2, sizeof(xmldata2))); ASSERT_EQUALS(library.deallocId("free"), library.allocId("malloc")); ASSERT_EQUALS(library.deallocId("free"), library.allocId("foo")); diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index a8b46305e..735efe7a7 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -814,7 +814,7 @@ private: "" " " ""; - settings.library.loadxmldata(xmldata, sizeof(xmldata)); + ASSERT_EQUALS(true, settings.library.loadxmldata(xmldata, sizeof(xmldata))); checkUninitVar("void f() {\n" " Fred _tm;\n" " _tm.dostuff();\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index b2d3c3d5d..9707c2fcb 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -50,7 +50,7 @@ private: " \n" " true \n" // abort is a noreturn function ""; - settings.library.loadxmldata(cfg, sizeof(cfg)); + ASSERT_EQUALS(true, settings.library.loadxmldata(cfg, sizeof(cfg))); LOAD_LIB_2(settings.library, "std.cfg"); TEST_CASE(valueFlowNumber);