aligned and optimized unique error handling (#5280)

The handling in `CppCheck::reportErr()` and `Executor::hasToLog()` was
slightly different. I hope this can somehow be shared after the executor
reworking.

We were also using a very inappropriate container for the error list
which caused a lot of overhead.

`-D__GNUC__ --debug-warnings --template=daca2 --check-library -j2
../test/testsymboldatabase.cpp`

Clang 15
main process  `284,218,587` -> `175,691,241`
worker process `9,123,697,183` -> `8,951,903,360`

Makefile

@@ -740,7 +740,7 @@ test/testcondition.o: test/testcondition.cpp externals/simplecpp/simplecpp.h lib
 test/testconstructors.o: test/testconstructors.cpp lib/addoninfo.h lib/check.h lib/checkclass.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testconstructors.cpp
 
-test/testcppcheck.o: test/testcppcheck.cpp lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h test/fixture.h
+test/testcppcheck.o: test/testcppcheck.cpp lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testcppcheck.cpp
 
 test/testerrorlogger.o: test/testerrorlogger.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h lib/xml.h test/fixture.h

cli/cppcheckexecutor.cpp

@@ -140,7 +140,8 @@ private:
     /**
      * Used to filter out duplicate error messages.
      */
-    std::set<std::string> mShownErrors;
+    // TODO: store hashes instead of the full messages
+    std::unordered_set<std::string> mShownErrors;
 
     /**
      * Report progress time
@@ -390,6 +391,8 @@ void CppCheckExecutor::StdLogger::reportErr(const ErrorMessage &msg)
         return;
     }
 
+    // TODO: we generate a different message here then we log below
+    // TODO: there should be no need for verbose and default messages here
     // Alert only about unique errors
     if (!mShownErrors.insert(msg.toString(mSettings.verbose)).second)
         return;

cli/executor.cpp

@@ -37,15 +37,21 @@ Executor::Executor(const std::list<std::pair<std::string, std::size_t>> &files,
     assert(!(!files.empty() && !fileSettings.empty()));
 }
 
+// TODO: this logic is duplicated in CppCheck::reportErr()
 bool Executor::hasToLog(const ErrorMessage &msg)
 {
+    if (!mSettings.library.reportErrors(msg.file0))
+        return false;
+
     if (!mSuppressions.isSuppressed(msg, {}))
     {
+        // TODO: there should be no need for verbose and default messages here
         std::string errmsg = msg.toString(mSettings.verbose);
+        if (errmsg.empty())
+            return false;
 
         std::lock_guard<std::mutex> lg(mErrorListSync);
-        if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) == mErrorList.cend()) {
+        if (mErrorList.emplace(std::move(errmsg)).second) {
-            mErrorList.emplace_back(std::move(errmsg));
             return true;
         }
     }

cli/executor.h

@@ -23,6 +23,7 @@
 #include <list>
 #include <mutex>
 #include <string>
+#include <unordered_set>
 #include <utility>
 
 class Settings;
@@ -74,7 +75,8 @@ protected:
 
 private:
     std::mutex mErrorListSync;
-    std::list<std::string> mErrorList;
+    // TODO: store hashes instead of the full messages
+    std::unordered_set<std::string> mErrorList;
 };
 
 /// @}

lib/cppcheck.cpp

@@ -1565,6 +1565,7 @@ void CppCheck::purgedConfigurationMessage(const std::string &file, const std::st
 
 //---------------------------------------------------------------------------
 
+// TODO: part of this logic is duplicated in Executor::hasToLog()
 void CppCheck::reportErr(const ErrorMessage &msg)
 {
     if (msg.severity == Severity::none && (msg.id == "logChecker" || endsWith(msg.id, "-logChecker"))) {
@@ -1575,17 +1576,6 @@ void CppCheck::reportErr(const ErrorMessage &msg)
     if (!mSettings.library.reportErrors(msg.file0))
         return;
 
-    const std::string errmsg = msg.toString(mSettings.verbose);
-    if (errmsg.empty())
-        return;
-
-    // Alert only about unique errors
-    if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) != mErrorList.cend())
-        return;
-
-    if (!mSettings.buildDir.empty())
-        mAnalyzerInformation.reportErr(msg);
-
     std::set<std::string> macroNames;
     if (!msg.callStack.empty()) {
         const std::string &file = msg.callStack.back().getfile(false);
@@ -1602,12 +1592,24 @@ void CppCheck::reportErr(const ErrorMessage &msg)
         return;
     }
 
+    // TODO: there should be no need for the verbose and default messages here
+    std::string errmsg = msg.toString(mSettings.verbose);
+    if (errmsg.empty())
+        return;
+
+    // Alert only about unique errors.
+    // This makes sure the errors of a single check() call are unique.
+    // TODO: get rid of this? This is forwarded to another ErrorLogger which is also doing this
+    if (!mErrorList.emplace(std::move(errmsg)).second)
+        return;
+
+    if (!mSettings.buildDir.empty())
+        mAnalyzerInformation.reportErr(msg);
+
     if (!mSettings.nofail.isSuppressed(errorMessage) && !mSettings.nomsg.isSuppressed(errorMessage)) {
         mExitCode = 1;
     }
 
-    mErrorList.push_back(errmsg);
-
     mErrorLogger.reportErr(msg);
     // check if plistOutput should be populated and the current output file is open and the error is not suppressed
     if (!mSettings.plistOutput.empty() && mPlistFile.is_open() && !mSettings.nomsg.isSuppressed(errorMessage)) {

lib/cppcheck.h

@@ -35,6 +35,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -215,7 +216,8 @@ private:
      */
     void reportOut(const std::string &outmsg, Color c = Color::Reset) override;
 
-    std::list<std::string> mErrorList;
+    // TODO: store hashes instead of the full messages
+    std::unordered_set<std::string> mErrorList;
     Settings mSettings;
 
     void reportProgress(const std::string &filename, const char stage[], const std::size_t value) override;

lib/errorlogger.cpp

@@ -618,11 +618,14 @@ static void replaceColors(std::string& source) {
     replace(source, substitutionMap);
 }
 
+// TODO: remove default parameters
 std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const
 {
     // Save this ErrorMessage in plain text.
 
+    // TODO: should never happen - remove this
     // No template is given
+    // (not 100%) equivalent templateFormat: {callstack} ({severity}{inconclusive:, inconclusive}) {message}
     if (templateFormat.empty()) {
         std::string text;
         if (!callStack.empty()) {

test/helpers.h

@@ -70,6 +70,10 @@ public:
         return mFullPath;
     }
 
+    const std::string& name() const {
+        return mName;
+    }
+
     ScopedFile(const ScopedFile&) = delete;
     ScopedFile(ScopedFile&&) = delete;
     ScopedFile& operator=(const ScopedFile&) = delete;

test/testcppcheck.cpp

@@ -19,7 +19,9 @@
 #include "color.h"
 #include "cppcheck.h"
 #include "errorlogger.h"
+#include "filesettings.h"
 #include "fixture.h"
+#include "helpers.h"
 
 #include <algorithm>
 #include <list>
@@ -34,30 +36,37 @@ private:
 
     class ErrorLogger2 : public ErrorLogger {
     public:
-        std::list<std::string> id;
+        std::list<std::string> ids;
+        std::list<ErrorMessage> errmsgs;
 
+    private:
         void reportOut(const std::string & /*outmsg*/, Color /*c*/ = Color::Reset) override {}
 
         void reportErr(const ErrorMessage &msg) override {
-            id.push_back(msg.id);
+            ids.push_back(msg.id);
+            errmsgs.push_back(msg);
         }
     };
 
     void run() override {
         TEST_CASE(getErrorMessages);
+        TEST_CASE(checkWithFile);
+        TEST_CASE(checkWithFS);
+        TEST_CASE(suppress_error_library);
+        TEST_CASE(unique_errors);
     }
 
     void getErrorMessages() const {
         ErrorLogger2 errorLogger;
         CppCheck::getErrorMessages(errorLogger);
-        ASSERT(!errorLogger.id.empty());
+        ASSERT(!errorLogger.ids.empty());
 
         // Check if there are duplicate error ids in errorLogger.id
         std::string duplicate;
-        for (std::list<std::string>::const_iterator it = errorLogger.id.cbegin();
+        for (std::list<std::string>::const_iterator it = errorLogger.ids.cbegin();
-             it != errorLogger.id.cend();
+             it != errorLogger.ids.cend();
              ++it) {
-            if (std::find(errorLogger.id.cbegin(), it, *it) != it) {
+            if (std::find(errorLogger.ids.cbegin(), it, *it) != it) {
                 duplicate = "Duplicate ID: " + *it;
                 break;
             }
@@ -67,7 +76,7 @@ private:
         // Check for error ids from this class.
         bool foundPurgedConfiguration = false;
         bool foundTooManyConfigs = false;
-        for (const std::string & it : errorLogger.id) {
+        for (const std::string & it : errorLogger.ids) {
             if (it == "purgedConfiguration")
                 foundPurgedConfiguration = true;
             else if (it == "toomanyconfigs")
@@ -76,6 +85,102 @@ private:
         ASSERT(foundPurgedConfiguration);
         ASSERT(foundTooManyConfigs);
     }
+
+    void checkWithFile() const
+    {
+        ScopedFile file("test.cpp",
+                        "int main()\n"
+                        "{\n"
+                        "  int i = *((int*)0);\n"
+                        "  return 0;\n"
+                        "}");
+
+        ErrorLogger2 errorLogger;
+        CppCheck cppcheck(errorLogger, false, {});
+        ASSERT_EQUALS(1, cppcheck.check(file.path()));
+        // TODO: how to properly disable these warnings?
+        errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
+            return id == "logChecker";
+        }), errorLogger.ids.end());
+        ASSERT_EQUALS(1, errorLogger.ids.size());
+        ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+    }
+
+    void checkWithFS() const
+    {
+        ScopedFile file("test.cpp",
+                        "int main()\n"
+                        "{\n"
+                        "  int i = *((int*)0);\n"
+                        "  return 0;\n"
+                        "}");
+
+        ErrorLogger2 errorLogger;
+        CppCheck cppcheck(errorLogger, false, {});
+        FileSettings fs;
+        fs.filename = file.path();
+        ASSERT_EQUALS(1, cppcheck.check(fs));
+        // TODO: how to properly disable these warnings?
+        errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
+            return id == "logChecker";
+        }), errorLogger.ids.end());
+        ASSERT_EQUALS(1, errorLogger.ids.size());
+        ASSERT_EQUALS("nullPointer", *errorLogger.ids.cbegin());
+    }
+
+    void suppress_error_library() const
+    {
+        ScopedFile file("test.cpp",
+                        "int main()\n"
+                        "{\n"
+                        "  int i = *((int*)0);\n"
+                        "  return 0;\n"
+                        "}");
+
+        ErrorLogger2 errorLogger;
+        CppCheck cppcheck(errorLogger, false, {});
+        const char xmldata[] = R"(<def format="2"><markup ext=".cpp" reporterrors="false"/></def>)";
+        const Settings s = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
+        cppcheck.settings() = s;
+        ASSERT_EQUALS(0, cppcheck.check(file.path()));
+        // TODO: how to properly disable these warnings?
+        errorLogger.ids.erase(std::remove_if(errorLogger.ids.begin(), errorLogger.ids.end(), [](const std::string& id) {
+            return id == "logChecker";
+        }), errorLogger.ids.end());
+        ASSERT_EQUALS(0, errorLogger.ids.size());
+    }
+
+    // TODO: hwo to actually get duplicated findings
+    void unique_errors() const
+    {
+        ScopedFile file("inc.h",
+                        "inline void f()\n"
+                        "{\n"
+                        "  (void)*((int*)0);\n"
+                        "}");
+        ScopedFile test_file_a("a.cpp",
+                               "#include \"inc.h\"");
+        ScopedFile test_file_b("b.cpp",
+                               "#include \"inc.h\"");
+
+        ErrorLogger2 errorLogger;
+        CppCheck cppcheck(errorLogger, false, {});
+        ASSERT_EQUALS(1, cppcheck.check(test_file_a.path()));
+        ASSERT_EQUALS(1, cppcheck.check(test_file_b.path()));
+        // TODO: how to properly disable these warnings?
+        errorLogger.errmsgs.erase(std::remove_if(errorLogger.errmsgs.begin(), errorLogger.errmsgs.end(), [](const ErrorMessage& errmsg) {
+            return errmsg.id == "logChecker";
+        }), errorLogger.errmsgs.end());
+        // the internal errorlist is cleared after each check() call
+        ASSERT_EQUALS(2, errorLogger.errmsgs.size());
+        auto it = errorLogger.errmsgs.cbegin();
+        ASSERT_EQUALS("nullPointer", it->id);
+        ++it;
+        ASSERT_EQUALS("nullPointer", it->id);
+    }
+
+    // TODO: test suppressions
+    // TODO: test all with FS
 };
 
 REGISTER_TEST(TestCppcheck)

test/testprocessexecutor.cpp

@@ -149,6 +149,7 @@ private:
         TEST_CASE(showtime_file);
         TEST_CASE(showtime_summary);
         TEST_CASE(showtime_file_total);
+        TEST_CASE(suppress_error_library);
 #endif // !WIN32
     }
 
@@ -329,6 +330,34 @@ private:
         TODO_ASSERT(output_s.find("Check time: " + fprefix() + "_2.cpp: ") != std::string::npos);
     }
 
+    void suppress_error_library() {
+        SUPPRESS;
+        const Settings settingsOld = settings;
+        const char xmldata[] = R"(<def format="2"><markup ext=".cpp" reporterrors="false"/></def>)";
+        settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
+        check(2, 1, 0,
+              "int main()\n"
+              "{\n"
+              "  int i = *((int*)0);\n"
+              "  return 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+        settings = settingsOld;
+    }
+
+    void unique_errors() {
+        SUPPRESS;
+        ScopedFile inc_h(fprefix() + ".h",
+                         "inline void f()\n"
+                         "{\n"
+                         "  (void)*((int*)0);\n"
+                         "}");
+        check(2, 2, 2,
+              "#include \"" + inc_h.name() +"\"");
+        // this is made unique by the executor
+        ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str());
+    }
+
     // TODO: test whole program analysis
 };
 

test/testsingleexecutor.cpp

@@ -153,6 +153,8 @@ private:
         TEST_CASE(showtime_file);
         TEST_CASE(showtime_summary);
         TEST_CASE(showtime_file_total);
+        TEST_CASE(suppress_error_library);
+        TEST_CASE(unique_errors);
     }
 
     void many_files() {
@@ -321,7 +323,36 @@ private:
         ASSERT(output_s.find("Check time: " + fprefix() + "_" + zpad3(2) + ".cpp: ") != std::string::npos);
     }
 
+    void suppress_error_library() {
+        SUPPRESS;
+        const Settings settingsOld = settings;
+        const char xmldata[] = R"(<def format="2"><markup ext=".cpp" reporterrors="false"/></def>)";
+        settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
+        check(1, 0,
+              "int main()\n"
+              "{\n"
+              "  int i = *((int*)0);\n"
+              "  return 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+        settings = settingsOld;
+    }
+
+    void unique_errors() {
+        SUPPRESS;
+        ScopedFile inc_h(fprefix() + ".h",
+                         "inline void f()\n"
+                         "{\n"
+                         "  (void)*((int*)0);\n"
+                         "}");
+        check(2, 2,
+              "#include \"" + inc_h.name() + "\"");
+        // these are not actually made unique by the implementation. That needs to be done by the given ErrorLogger
+        ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str());
+    }
+
     // TODO: test whole program analysis
+    // TODO: test unique errors
 };
 
 class TestSingleExecutorFiles : public TestSingleExecutorBase {

test/testthreadexecutor.cpp

@@ -149,6 +149,8 @@ private:
         TEST_CASE(showtime_file);
         TEST_CASE(showtime_summary);
         TEST_CASE(showtime_file_total);
+        TEST_CASE(suppress_error_library);
+        TEST_CASE(unique_errors);
     }
 
     void deadlock_with_many_errors() {
@@ -326,6 +328,34 @@ private:
         ASSERT(output_s.find("Check time: " + fprefix() + "_2.cpp: ") != std::string::npos);
     }
 
+    void suppress_error_library() {
+        SUPPRESS;
+        const Settings settingsOld = settings;
+        const char xmldata[] = R"(<def format="2"><markup ext=".cpp" reporterrors="false"/></def>)";
+        settings = settingsBuilder().libraryxml(xmldata, sizeof(xmldata)).build();
+        check(2, 1, 0,
+              "int main()\n"
+              "{\n"
+              "  int i = *((int*)0);\n"
+              "  return 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+        settings = settingsOld;
+    }
+
+    void unique_errors() {
+        SUPPRESS;
+        ScopedFile inc_h(fprefix() + ".h",
+                         "inline void f()\n"
+                         "{\n"
+                         "  (void)*((int*)0);\n"
+                         "}");
+        check(2, 2, 2,
+              "#include \"" + inc_h.name() +"\"");
+        // this is made unique by the executor
+        ASSERT_EQUALS("[" + inc_h.name() + ":3]: (error) Null pointer dereference: (int*)0\n", errout.str());
+    }
+
     // TODO: test whole program analysis
 };