From ec211348177d43e5ad7f2c1d02644f765f2f4d1c Mon Sep 17 00:00:00 2001 From: Thomas Jarosch Date: Wed, 7 Jan 2015 19:26:16 +0100 Subject: [PATCH] Fix false negatives for local suppressions Introduce a new bool setting jointSuppressionReport that will be set by the analyseWholeProgram() code path. When the flag is enabled, unmatched suppressions are collected after running the final whole program analysis to prevent false positives for the unusedFunction check. The check functions in the unit test for single / multi file suppressions were unified. --- cli/cppcheckexecutor.cpp | 14 ++++++-- lib/cppcheck.cpp | 13 +++++-- lib/cppcheck.h | 4 +++ lib/settings.cpp | 4 ++- lib/settings.h | 5 +++ lib/suppressions.cpp | 9 +++-- lib/suppressions.h | 4 +-- test/testsuppressions.cpp | 73 ++++++++++++++++++++++----------------- 8 files changed, 82 insertions(+), 44 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 7ad27fc93..68e127df0 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -760,6 +760,7 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck, int /*argc*/, const cha unsigned int returnValue = 0; if (settings._jobs == 1) { // Single process + settings.jointSuppressionReport = true; std::size_t totalfilesize = 0; for (std::map::const_iterator i = _files.begin(); i != _files.end(); ++i) { @@ -799,8 +800,17 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck, int /*argc*/, const cha returnValue = executor.check(); } - if (settings.isEnabled("information") || settings.checkConfiguration) - reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(settings._jobs == 1 && settings.isEnabled("unusedFunction"))); + if (settings.isEnabled("information") || settings.checkConfiguration) { + const bool enableUnusedFunctionCheck = cppcheck.unusedFunctionCheckIsEnabled(); + + if (settings.jointSuppressionReport) { + for (std::map::const_iterator i = _files.begin(); i != _files.end(); ++i) { + reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(i->first, enableUnusedFunctionCheck)); + } + } + + reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(enableUnusedFunctionCheck)); + } if (!settings.checkConfiguration) { cppcheck.tooManyConfigsError("",0U); diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index ba0caf8b4..f40459740 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -246,8 +246,11 @@ unsigned int CppCheck::processFile(const std::string& filename, std::istream& fi internalError(filename, e.errorMessage); } - if (_settings.isEnabled("information") || _settings.checkConfiguration) - reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(filename, _settings._jobs == 1 && _settings.isEnabled("unusedFunction"))); + // In jointSuppressionReport mode, unmatched suppressions are + // collected after all files are processed + if (!_settings.jointSuppressionReport && (_settings.isEnabled("information") || _settings.checkConfiguration)) { + reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(filename, unusedFunctionCheckIsEnabled())); + } _errorList.clear(); return exitcode; @@ -677,8 +680,12 @@ void CppCheck::getErrorMessages() void CppCheck::analyseWholeProgram() { - // Analyse the tokens.. + // Analyse the tokens for (std::list::const_iterator it = Check::instances().begin(); it != Check::instances().end(); ++it) (*it)->analyseWholeProgram(fileInfo, *this); } +bool CppCheck::unusedFunctionCheckIsEnabled() const +{ + return (_settings._jobs == 1 && _settings.isEnabled("unusedFunction")); +} diff --git a/lib/cppcheck.h b/lib/cppcheck.h index 27ba73ba4..32bc2350f 100644 --- a/lib/cppcheck.h +++ b/lib/cppcheck.h @@ -132,6 +132,10 @@ public: /** analyse whole program, run this after all TUs has been scanned. */ void analyseWholeProgram(); + /** Check if the user wants to check for unused functions + * and if it's possible at all */ + bool unusedFunctionCheckIsEnabled() const; + private: /** @brief There has been an internal error => Report information message */ diff --git a/lib/settings.cpp b/lib/settings.cpp index 77cae1641..3978e49ce 100644 --- a/lib/settings.cpp +++ b/lib/settings.cpp @@ -30,7 +30,9 @@ Settings::Settings() debugFalsePositive(false), dump(false), exceptionHandling(false), - inconclusive(false), experimental(false), + inconclusive(false), + jointSuppressionReport(false), + experimental(false), _errorsOnly(false), _inlineSuppressions(false), _verbose(false), diff --git a/lib/settings.h b/lib/settings.h index 23b14314d..9d47e58e7 100644 --- a/lib/settings.h +++ b/lib/settings.h @@ -72,6 +72,11 @@ public: /** @brief Inconclusive checks */ bool inconclusive; + /** @brief Collect unmatched suppressions in one run. + * This delays the reporting until all files are checked. + * It is needed by checks that analyse the whole code base. */ + bool jointSuppressionReport; + /** * When this flag is false (default) then experimental * heuristics and checks are disabled. diff --git a/lib/suppressions.cpp b/lib/suppressions.cpp index d5babd279..0b533bdce 100644 --- a/lib/suppressions.cpp +++ b/lib/suppressions.cpp @@ -265,13 +265,12 @@ bool Suppressions::isSuppressedLocal(const std::string &errorId, const std::stri return _suppressions[errorId].isSuppressedLocal(file, line); } -std::list Suppressions::getUnmatchedLocalSuppressions(const std::string &file, bool unusedFunctionChecking) const +std::list Suppressions::getUnmatchedLocalSuppressions(const std::string &file, const bool unusedFunctionChecking) const { - (void)unusedFunctionChecking; std::list r; for (std::map::const_iterator i = _suppressions.begin(); i != _suppressions.end(); ++i) { - if (i->first == "unusedFunction") - continue; // unusedFunction is not a "local" suppression + if (!unusedFunctionChecking && i->first == "unusedFunction") + continue; std::map >::const_iterator f = i->second._files.find(file); if (f != i->second._files.end()) { @@ -285,7 +284,7 @@ std::list Suppressions::getUnmatchedLocalSuppres return r; } -std::list Suppressions::getUnmatchedGlobalSuppressions(bool unusedFunctionChecking) const +std::list Suppressions::getUnmatchedGlobalSuppressions(const bool unusedFunctionChecking) const { std::list r; for (std::map::const_iterator i = _suppressions.begin(); i != _suppressions.end(); ++i) { diff --git a/lib/suppressions.h b/lib/suppressions.h index c76b91912..42959712d 100644 --- a/lib/suppressions.h +++ b/lib/suppressions.h @@ -133,13 +133,13 @@ public: * @brief Returns list of unmatched local (per-file) suppressions. * @return list of unmatched suppressions */ - std::list getUnmatchedLocalSuppressions(const std::string &file, bool unusedFunctionChecking) const; + std::list getUnmatchedLocalSuppressions(const std::string &file, const bool unusedFunctionChecking) const; /** * @brief Returns list of unmatched global (glob pattern) suppressions. * @return list of unmatched suppressions */ - std::list getUnmatchedGlobalSuppressions(bool unusedFunctionChecking) const; + std::list getUnmatchedGlobalSuppressions(const bool unusedFunctionChecking) const; }; /// @} diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp index 256c2b087..5843bfc7a 100644 --- a/test/testsuppressions.cpp +++ b/test/testsuppressions.cpp @@ -22,6 +22,8 @@ #include "cppcheckexecutor.h" #include "threadexecutor.h" +#include +#include #include extern std::ostringstream errout; @@ -115,8 +117,29 @@ private: ASSERT_EQUALS(true, suppressions.isSuppressed("errorid", "a.c", 123)); } + void reportSuppressions(const Settings &settings, const std::map &files) { + // make it verbose that this check is disabled + const bool unusedFunctionCheck = false; + + if (settings.jointSuppressionReport) { + for (std::map::const_iterator i = files.begin(); i != files.end(); ++i) { + reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(i->first, unusedFunctionCheck)); + } + } + + reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(unusedFunctionCheck)); + } + // Check the suppression void checkSuppression(const char code[], const std::string &suppression = emptyString) { + std::map files; + files["test.cpp"] = code; + + checkSuppression(files, suppression); + } + + // Check the suppression for multiple files + void checkSuppression(std::map &files, const std::string &suppression = emptyString) { // Clear the error log errout.str(""); @@ -124,14 +147,18 @@ private: Settings& settings = cppCheck.settings(); settings._inlineSuppressions = true; settings.addEnabled("information"); + settings.jointSuppressionReport = true; if (!suppression.empty()) { std::string r = settings.nomsg.addSuppressionLine(suppression); ASSERT_EQUALS("", r); } - cppCheck.check("test.cpp", code); + for (std::map::const_iterator file = files.begin(); file != files.end(); ++file) { + cppCheck.check(file->first, file->second); + } + cppCheck.analyseWholeProgram(); - reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(true)); + reportSuppressions(settings, files); } void checkSuppressionThreads(const char code[], const std::string &suppression = emptyString) { @@ -154,25 +181,11 @@ private: executor.check(); - reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(false)); - } + std::map files_for_report; + for (std::map::const_iterator file = files.begin(); file != files.end(); ++file) + files_for_report[file->first] = ""; - // Check the suppression for multiple files - void checkSuppression(const char *names[], const char *codes[], const std::string &suppression = emptyString) { - // Clear the error log - errout.str(""); - - CppCheck cppCheck(*this, true); - Settings& settings = cppCheck.settings(); - settings._inlineSuppressions = true; - settings.addEnabled("information"); - if (!suppression.empty()) - settings.nomsg.addSuppressionLine(suppression); - - for (int i = 0; names[i] != NULL; ++i) - cppCheck.check(names[i], codes[i]); - - reportUnmatchedSuppressions(settings.nomsg.getUnmatchedGlobalSuppressions(true)); + reportSuppressions(settings, files_for_report); } void runChecks(void (TestSuppressions::*check)(const char[], const std::string &)) { @@ -303,18 +316,16 @@ private: } void suppressionsMultiFile() { - const char *names[] = {"abc.cpp", "xyz.cpp", NULL}; - const char *codes[] = { - "void f() {\n" - "}\n", - "void f() {\n" - " int a;\n" - " a++;\n" - "}\n", - }; + std::map files; + files["abc.cpp"] = "void f() {\n" + "}\n"; + files["xyz.cpp"] = "void f() {\n" + " int a;\n" + " a++;\n" + "}\n"; // suppress uninitvar for this file and line - checkSuppression(names, codes, "uninitvar:xyz.cpp:3"); + checkSuppression(files, "uninitvar:xyz.cpp:3"); ASSERT_EQUALS("", errout.str()); } @@ -327,7 +338,7 @@ private: void inlinesuppress_unusedFunction() const { // #4210, #4946 - wrong report of "unmatchedSuppression" for "unusedFunction" Suppressions suppressions; suppressions.addSuppression("unusedFunction", "test.c", 3U); - ASSERT_EQUALS(false, !suppressions.getUnmatchedLocalSuppressions("test.c", true).empty()); + ASSERT_EQUALS(true, !suppressions.getUnmatchedLocalSuppressions("test.c", true).empty()); ASSERT_EQUALS(false, !suppressions.getUnmatchedGlobalSuppressions(true).empty()); ASSERT_EQUALS(false, !suppressions.getUnmatchedLocalSuppressions("test.c", false).empty()); ASSERT_EQUALS(false, !suppressions.getUnmatchedGlobalSuppressions(false).empty());