From d5c3b7f0a4b43c3fc6e7a18265c26a95bf71c9ae Mon Sep 17 00:00:00 2001 From: firewave Date: Thu, 8 Jan 2026 19:20:31 +0100 Subject: [PATCH] improved errorhandling of analyzer information loading --- cli/cmdlineparser.cpp | 3 + lib/analyzerinfo.cpp | 132 ++++++++++++++++++++++--------- lib/analyzerinfo.h | 9 ++- lib/checkunusedfunctions.cpp | 7 +- lib/cppcheck.cpp | 21 +++-- lib/errorlogger.cpp | 1 + lib/settings.h | 3 + lib/utils.h | 5 ++ test/cli/clang-import_test.py | 1 + test/cli/other_test.py | 98 +++++++++++++++++++++++ test/testanalyzerinformation.cpp | 13 +-- test/testcmdlineparser.cpp | 8 ++ test/testutils.cpp | 12 +++ 13 files changed, 257 insertions(+), 56 deletions(-) diff --git a/cli/cmdlineparser.cpp b/cli/cmdlineparser.cpp index 92f4c6cef3b..f183b5c646e 100644 --- a/cli/cmdlineparser.cpp +++ b/cli/cmdlineparser.cpp @@ -621,6 +621,9 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a mSettings.cppHeaderProbe = true; } + else if (std::strcmp(argv[i], "--debug-analyzerinfo") == 0) + mSettings.debugainfo = true; + else if (std::strcmp(argv[i], "--debug-ast") == 0) mSettings.debugast = true; diff --git a/lib/analyzerinfo.cpp b/lib/analyzerinfo.cpp index c7045252f3d..55bd9127305 100644 --- a/lib/analyzerinfo.cpp +++ b/lib/analyzerinfo.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -84,29 +85,55 @@ void AnalyzerInformation::close() } } -bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors) +bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors, bool debug) { const tinyxml2::XMLElement * const rootNode = analyzerInfoDoc.FirstChildElement(); - if (rootNode == nullptr) + if (rootNode == nullptr) { + if (debug) + std::cout << "discarding cached result - no root node found" << std::endl; return false; + } - const char *attr = rootNode->Attribute("hash"); - if (!attr || attr != std::to_string(hash)) + if (strcmp(rootNode->Name(), "analyzerinfo") != 0) { + if (debug) + std::cout << "discarding cached result - unexpected root node" << std::endl; return false; + } - // Check for invalid license error or internal error, in which case we should retry analysis - for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { - if (std::strcmp(e->Name(), "error") == 0 && - (e->Attribute("id", "premium-invalidLicense") || - e->Attribute("id", "premium-internalError") || - e->Attribute("id", "internalError") - )) - return false; + const char * const attr = rootNode->Attribute("hash"); + if (!attr) { + if (debug) + std::cout << "discarding cached result - no 'hash' attribute found" << std::endl; + return false; + } + if (attr != std::to_string(hash)) { + if (debug) + std::cout << "discarding cached result - hash mismatch" << std::endl; + return false; } for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { - if (std::strcmp(e->Name(), "error") == 0) - errors.emplace_back(e); + if (std::strcmp(e->Name(), "error") != 0) + continue; + + // TODO: discarding results on internalError doesn't make sense since that won't fix itself + // Check for invalid license error or internal error, in which case we should retry analysis + static std::array s_ids{ + "premium-invalidLicense", + "premium-internalError", + "internalError" + }; + for (const auto* id : s_ids) + { + if (e->Attribute("id", id)) { + if (debug) + std::cout << "discarding cached result - '" << id << "' encountered" << std::endl; + errors.clear(); + return false; + } + } + + errors.emplace_back(e); } return true; @@ -114,15 +141,13 @@ bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfo std::string AnalyzerInformation::getAnalyzerInfoFileFromFilesTxt(std::istream& filesTxt, const std::string &sourcefile, const std::string &cfg, int fileIndex) { - const std::string id = (fileIndex > 0) ? std::to_string(fileIndex) : ""; std::string line; - const std::string end(sep + cfg + sep + id + sep + Path::simplifyPath(sourcefile)); while (std::getline(filesTxt,line)) { - if (line.size() <= end.size() + 2U) - continue; - if (!endsWith(line, end.c_str(), end.size())) - continue; - return line.substr(0,line.find(sep)); + AnalyzerInformation::Info filesTxtInfo; + if (!filesTxtInfo.parse(line)) + continue; // TODO: report error? + if (endsWith(sourcefile, filesTxtInfo.sourceFile) && filesTxtInfo.cfg == cfg && filesTxtInfo.fileIndex == fileIndex) + return filesTxtInfo.afile; } return ""; } @@ -145,24 +170,39 @@ std::string AnalyzerInformation::getAnalyzerInfoFile(const std::string &buildDir return Path::join(buildDir, std::move(filename)) + ".analyzerinfo"; } -bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list &errors) +bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list &errors, bool debug) { + if (mOutputStream.is_open()) + throw std::runtime_error("analyzer information file is already open"); + if (buildDir.empty() || sourcefile.empty()) return true; - close(); const std::string analyzerInfoFile = AnalyzerInformation::getAnalyzerInfoFile(buildDir,sourcefile,cfg,fileIndex); - tinyxml2::XMLDocument analyzerInfoDoc; - const tinyxml2::XMLError xmlError = analyzerInfoDoc.LoadFile(analyzerInfoFile.c_str()); - if (xmlError == tinyxml2::XML_SUCCESS && skipAnalysis(analyzerInfoDoc, hash, errors)) - return false; + { + tinyxml2::XMLDocument analyzerInfoDoc; + const tinyxml2::XMLError xmlError = analyzerInfoDoc.LoadFile(analyzerInfoFile.c_str()); + if (xmlError == tinyxml2::XML_SUCCESS) { + if (skipAnalysis(analyzerInfoDoc, hash, errors, debug)) { + if (debug) + std::cout << "skipping analysis - loaded " << errors.size() << " cached finding(s) from '" << analyzerInfoFile << "'" << std::endl; + return false; + } + } + else if (xmlError != tinyxml2::XML_ERROR_FILE_NOT_FOUND) { + if (debug) + std::cout << "discarding cached result - failed to load '" << analyzerInfoFile << "' (" << tinyxml2::XMLDocument::ErrorIDToName(xmlError) << ")" << std::endl; + } + else if (debug) + std::cout << "no cached result '" << analyzerInfoFile << "' found" << std::endl; + } mOutputStream.open(analyzerInfoFile); - if (mOutputStream.is_open()) { - mOutputStream << "\n"; - mOutputStream << "\n"; - } + if (!mOutputStream.is_open()) + throw std::runtime_error("failed to open '" + analyzerInfoFile + "'"); + mOutputStream << "\n"; + mOutputStream << "\n"; return true; } @@ -179,6 +219,7 @@ void AnalyzerInformation::setFileInfo(const std::string &check, const std::strin mOutputStream << " \n" << fileInfo << " \n"; } +// TODO: report detailed errors? bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) { const std::string::size_type sep1 = filesTxtLine.find(sep); if (sep1 == std::string::npos) @@ -206,37 +247,50 @@ bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) { return true; } -// TODO: bail out on unexpected data -void AnalyzerInformation::processFilesTxt(const std::string& buildDir, const std::function& handler) +std::string AnalyzerInformation::processFilesTxt(const std::string& buildDir, const std::function& handler, bool debug) { const std::string filesTxt(buildDir + "/files.txt"); std::ifstream fin(filesTxt.c_str()); std::string filesTxtLine; while (std::getline(fin, filesTxtLine)) { AnalyzerInformation::Info filesTxtInfo; - if (!filesTxtInfo.parse(filesTxtLine)) { - return; - } + if (!filesTxtInfo.parse(filesTxtLine)) + return "failed to parse '" + filesTxtLine + "' from '" + filesTxt + "'"; + + if (filesTxtInfo.afile.empty()) + return "empty afile from '" + filesTxt + "'"; const std::string xmlfile = buildDir + '/' + filesTxtInfo.afile; tinyxml2::XMLDocument doc; const tinyxml2::XMLError error = doc.LoadFile(xmlfile.c_str()); + if (error == tinyxml2::XML_ERROR_FILE_NOT_FOUND) + return "'" + xmlfile + "' from '" + filesTxt + "' not found"; + if (error != tinyxml2::XML_SUCCESS) - return; + return "failed to load '" + xmlfile + "' from '" + filesTxt + "'"; const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement(); if (rootNode == nullptr) - return; + return "no root node found in '" + xmlfile + "' from '" + filesTxt + "'"; + + if (strcmp(rootNode->Name(), "analyzerinfo") != 0) + return "unexpected root node in '" + xmlfile + "' from '" + filesTxt + "'"; for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) { if (std::strcmp(e->Name(), "FileInfo") != 0) continue; const char *checkattr = e->Attribute("check"); - if (checkattr == nullptr) + if (checkattr == nullptr) { + if (debug) + std::cout << "'check' attribute missing in 'FileInfo' in '" << xmlfile << "' from '" << filesTxt + "'"; continue; + } handler(checkattr, e, filesTxtInfo); } } + + // TODO: error on empty file? + return ""; } diff --git a/lib/analyzerinfo.h b/lib/analyzerinfo.h index 881076c09f4..82f79cc3f76 100644 --- a/lib/analyzerinfo.h +++ b/lib/analyzerinfo.h @@ -61,7 +61,10 @@ class CPPCHECKLIB AnalyzerInformation { /** Close current TU.analyzerinfo file */ void close(); - bool analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list &errors); + /** + * @throws std::runtime_error thrown if the output file is already open or the output file cannot be opened + */ + bool analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list &errors, bool debug = false); void reportErr(const ErrorMessage &msg); void setFileInfo(const std::string &check, const std::string &fileInfo); static std::string getAnalyzerInfoFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex); @@ -77,14 +80,14 @@ class CPPCHECKLIB AnalyzerInformation { std::string sourceFile; }; - static void processFilesTxt(const std::string& buildDir, const std::function& handler); + static std::string processFilesTxt(const std::string& buildDir, const std::function& handler, bool debug = false); protected: static std::string getFilesTxt(const std::list &sourcefiles, const std::string &userDefines, const std::list &fileSettings); static std::string getAnalyzerInfoFileFromFilesTxt(std::istream& filesTxt, const std::string &sourcefile, const std::string &cfg, int fileIndex); - static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors); + static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list &errors, bool debug = false); private: std::ofstream mOutputStream; diff --git a/lib/checkunusedfunctions.cpp b/lib/checkunusedfunctions.cpp index 7edaff6c241..eb06a4691f1 100644 --- a/lib/checkunusedfunctions.cpp +++ b/lib/checkunusedfunctions.cpp @@ -482,7 +482,12 @@ void CheckUnusedFunctions::analyseWholeProgram(const Settings &settings, ErrorLo } }; - AnalyzerInformation::processFilesTxt(buildDir, handler); + const std::string err = AnalyzerInformation::processFilesTxt(buildDir, handler, settings.debugainfo); + if (!err.empty()) { + const ErrorMessage errmsg({}, "", Severity::error, err, "internalError", Certainty::normal); + errorLogger.reportErr(errmsg); + return; + } for (auto decl = decls.cbegin(); decl != decls.cend(); ++decl) { const std::string &functionName = stripTemplateParameters(decl->first); diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 399eb6d3d6e..407b956bb88 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -974,7 +974,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str mLogger->setAnalyzerInfo(nullptr); std::list errors; - analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors); + analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors, mSettings.debugainfo); analyzerInformation->setFileInfo("CheckUnusedFunctions", mUnusedFunctionsCheck->analyzerInfo(tokenizer)); analyzerInformation->close(); } @@ -1020,7 +1020,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str // Calculate hash so it can be compared with old hash / future hashes const std::size_t hash = calculateHash(preprocessor, file.spath()); std::list errors; - if (!analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors)) { + if (!analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors, mSettings.debugainfo)) { while (!errors.empty()) { mErrorLogger.reportErr(errors.front()); errors.pop_front(); @@ -1861,12 +1861,17 @@ unsigned int CppCheck::analyseWholeProgram(const std::string &buildDir, const st } }; - AnalyzerInformation::processFilesTxt(buildDir, handler); - - // Analyse the tokens - // cppcheck-suppress shadowFunction - TODO: fix this - for (Check *check : Check::instances()) - check->analyseWholeProgram(ctuFileInfo, fileInfoList, mSettings, mErrorLogger); + const std::string err = AnalyzerInformation::processFilesTxt(buildDir, handler, mSettings.debugainfo); + if (!err.empty()) { + const ErrorMessage errmsg({}, "", Severity::error, err, "internalError", Certainty::normal); + mErrorLogger.reportErr(errmsg); + } + else { + // Analyse the tokens + // cppcheck-suppress shadowFunction - TODO: fix this + for (Check *check : Check::instances()) + check->analyseWholeProgram(ctuFileInfo, fileInfoList, mSettings, mErrorLogger); + } for (Check::FileInfo *fi : fileInfoList) delete fi; diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp index e62ed6b806d..13f66c13d1a 100644 --- a/lib/errorlogger.cpp +++ b/lib/errorlogger.cpp @@ -162,6 +162,7 @@ ErrorMessage::ErrorMessage(ErrorPath errorPath, const TokenList *tokenList, Seve // hash = calculateWarningHash(tokenList, hashWarning.str()); } +// TODO: improve errorhandling? ErrorMessage::ErrorMessage(const tinyxml2::XMLElement * const errmsg) : severity(Severity::none), cwe(0U), diff --git a/lib/settings.h b/lib/settings.h index 04541cda08c..6521ba4581d 100644 --- a/lib/settings.h +++ b/lib/settings.h @@ -189,6 +189,9 @@ class CPPCHECKLIB WARN_UNUSED Settings { /** @brief Are we running from DACA script? */ bool daca{}; + /** @brief Is --debug-analyzerinfo given? */ + bool debugainfo{}; + /** @brief Is --debug-ast given? */ bool debugast{}; diff --git a/lib/utils.h b/lib/utils.h index c59e4d3a32c..c84c72236c1 100644 --- a/lib/utils.h +++ b/lib/utils.h @@ -124,6 +124,11 @@ bool endsWith(const std::string& str, const char (&end)[N]) return endsWith(str, end, N - 1); } +inline bool endsWith(const std::string& str, const std::string& end) +{ + return endsWith(str, end.c_str(), end.length()); +} + inline static bool isPrefixStringCharLiteral(const std::string &str, char q, const std::string& p) { // str must be at least the prefix plus the start and end quote diff --git a/test/cli/clang-import_test.py b/test/cli/clang-import_test.py index e4ec19a5ada..23be48c91ca 100644 --- a/test/cli/clang-import_test.py +++ b/test/cli/clang-import_test.py @@ -217,6 +217,7 @@ def test_cmd_std_c(tmp_path): # #13129 # TODO: remove when we inject the build-dir into all tests +@pytest.mark.xfail(strict=True) # TODO: no analyzer information generated with --clang - see #14456 def test_cmd_std_c_builddir(tmp_path): # #13129 build_dir = tmp_path / 'b1' os.makedirs(build_dir) diff --git a/test/cli/other_test.py b/test/cli/other_test.py index c4ae2fab5cf..bf50f8504d5 100644 --- a/test/cli/other_test.py +++ b/test/cli/other_test.py @@ -4119,3 +4119,101 @@ def test_active_unusedfunction_only_misra_builddir(tmp_path): 'CheckUnusedFunctions::check' ] __test_active_checkers(tmp_path, 1, 1175, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp) + + +def test_analyzerinfo(tmp_path): + test_file = tmp_path / 'test.c' + with open(test_file, "w") as f: + f.write( +"""void f() +{ + (void)(*((int*)0)); +} +""") + + build_dir = tmp_path / 'b1' + os.makedirs(build_dir) + + test_a1_file = build_dir / 'test.a1' + + args = [ + '-q', + '--debug-analyzerinfo', + '--template=simple', + '--cppcheck-build-dir={}'.format(build_dir), + '--enable=all', + str(test_file) + ] + + stderr_exp = [ + '{}:3:14: error: Null pointer dereference: (int*)0 [nullPointer]'.format(test_file), + "{}:1:6: style: The function 'f' is never used. [unusedFunction]".format(test_file) + ] + + def run_and_assert_cppcheck(stdout_exp): + exitcode, stdout, stderr = cppcheck(args) + assert exitcode == 0, stdout + assert stdout.splitlines() == stdout_exp + assert stderr.splitlines() == stderr_exp + + # no cached results + run_and_assert_cppcheck([ + "no cached result '{}' found".format(test_a1_file) + ]) + + # cached results + run_and_assert_cppcheck([ + "skipping analysis - loaded 1 cached finding(s) from '{}'".format(test_a1_file) + ]) + + # modified file + with open(test_file, 'a') as f: + f.write('\n#define DEF') + + run_and_assert_cppcheck([ + "discarding cached result - hash mismatch" # TODO: add filename + ]) + + # invalid XML + with open(test_a1_file, 'a') as f: + f.write('.') + + run_and_assert_cppcheck([ + "discarding cached result - failed to load '{}' (XML_ERROR_PARSING_TEXT)".format(test_a1_file) + ]) + + # missing root node + with open(test_a1_file, 'w') as f: + f.write('') + + run_and_assert_cppcheck([ + "discarding cached result - no root node found" # TODO: add filename + ]) + + # mismatched root node + with open(test_a1_file, 'w') as f: + f.write('') + + run_and_assert_cppcheck([ + "discarding cached result - unexpected root node" # TODO: add filename + ]) + + # missing 'hash' attribute + with open(test_a1_file, 'w') as f: + f.write('') + + run_and_assert_cppcheck([ + "discarding cached result - no 'hash' attribute found" # TODO: add filename + ]) + + # invalid 'hash' attribute + with open(test_a1_file, 'w') as f: + f.write('') + + run_and_assert_cppcheck([ + "discarding cached result - hash mismatch" # TODO: add filename + ]) + + # TODO: + # - invalid error + # - internalError diff --git a/test/testanalyzerinformation.cpp b/test/testanalyzerinformation.cpp index 13dd721f0a4..861b27c58fe 100644 --- a/test/testanalyzerinformation.cpp +++ b/test/testanalyzerinformation.cpp @@ -39,6 +39,7 @@ class TestAnalyzerInformation : public TestFixture { }; void run() override { + TEST_CASE(getAnalyzerInfoFileFromFilesTxt); TEST_CASE(getAnalyzerInfoFile); TEST_CASE(duplicateFile); TEST_CASE(filesTextDuplicateFile); @@ -46,12 +47,14 @@ class TestAnalyzerInformation : public TestFixture { TEST_CASE(skipAnalysis); } - void getAnalyzerInfoFile() const { + void getAnalyzerInfoFileFromFilesTxt() const { constexpr char filesTxt[] = "file1.a4:::file1.c\n"; - std::istringstream f1(filesTxt); - ASSERT_EQUALS("file1.a4", AnalyzerInformationTest::getAnalyzerInfoFileFromFilesTxt(f1, "file1.c", "", 0)); - std::istringstream f2(filesTxt); - ASSERT_EQUALS("file1.a4", AnalyzerInformationTest::getAnalyzerInfoFileFromFilesTxt(f2, "./file1.c", "", 0)); + std::istringstream f(filesTxt); + //ASSERT_EQUALS("file1.a4", AnalyzerInformationTest::getAnalyzerInfoFileFromFilesTxt(f, "file1.c", "", 0)); + ASSERT_EQUALS("file1.a4", AnalyzerInformationTest::getAnalyzerInfoFileFromFilesTxt(f, "./file1.c", "", 0)); + } + + void getAnalyzerInfoFile() const { ASSERT_EQUALS("builddir/file1.c.analyzerinfo", AnalyzerInformation::getAnalyzerInfoFile("builddir", "file1.c", "", 0)); ASSERT_EQUALS("builddir/file1.c.analyzerinfo", AnalyzerInformation::getAnalyzerInfoFile("builddir", "some/path/file1.c", "", 0)); } diff --git a/test/testcmdlineparser.cpp b/test/testcmdlineparser.cpp index 3b343cbc92e..b6690f8d5c7 100644 --- a/test/testcmdlineparser.cpp +++ b/test/testcmdlineparser.cpp @@ -493,6 +493,7 @@ class TestCmdlineParser : public TestFixture { TEST_CASE(safetyOverride); TEST_CASE(noSafety); TEST_CASE(noSafetyOverride); + TEST_CASE(debugAnalyzerinfo); TEST_CASE(ignorepaths1); TEST_CASE(ignorepaths2); @@ -3424,6 +3425,13 @@ class TestCmdlineParser : public TestFixture { ASSERT_EQUALS(false, settings->safety); } + void debugAnalyzerinfo() { + REDIRECT; + const char * const argv[] = {"cppcheck", "--debug-analyzerinfo", "file.cpp"}; + ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parseFromArgs(argv)); + ASSERT_EQUALS(true, settings->debugainfo); + } + void ignorepaths1() { REDIRECT; const char * const argv[] = {"cppcheck", "-isrc", "file.cpp"}; diff --git a/test/testutils.cpp b/test/testutils.cpp index 37c1030000d..0aad2c447a1 100644 --- a/test/testutils.cpp +++ b/test/testutils.cpp @@ -44,6 +44,7 @@ class TestUtils : public TestFixture { TEST_CASE(splitString); TEST_CASE(as_const); TEST_CASE(memoize); + TEST_CASE(endsWith); } void isValidGlobPattern() const { @@ -563,6 +564,17 @@ class TestUtils : public TestFixture { ASSERT_EQUALS(1, callF()); ASSERT_EQUALS(1, count); } + + void endsWith() const + { + ASSERT(::endsWith("test", "test")); + ASSERT(::endsWith("test2", "2")); + ASSERT(::endsWith("test test", "test")); + ASSERT(::endsWith("test", "t")); + ASSERT(!::endsWith("", "test")); + ASSERT(!::endsWith("tes", "test")); + ASSERT(!::endsWith("2test", "2")); + } }; REGISTER_TEST(TestUtils)