Skip to content

improved errorhandling of analyzer information loading #8159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
firewave wants to merge 1 commit into
base: main
Choose a base branch
from
+163 −34
Draft

improved errorhandling of analyzer information loading #8159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cli/cmdlineparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion cli/cppcheckexecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ int CppCheckExecutor::check_internal(const Settings& settings, Suppressions& sup

returnValue |= cppcheck.analyseWholeProgram(settings.buildDir, mFiles, mFileSettings, stdLogger.getCtuInfo());

if (settings.severity.isEnabled(Severity::information) || settings.checkConfiguration) {
if ((settings.severity.isEnabled(Severity::information) || settings.checkConfiguration) && !supprs.nomsg.getSuppressions().empty()) {
const bool err = reportUnmatchedSuppressions(settings, supprs.nomsg, mFiles, mFileSettings, stdLogger);
if (err && returnValue == 0)
returnValue = settings.exitCode;
Expand Down
107 changes: 78 additions & 29 deletions lib/analyzerinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include <cstring>
#include <exception>
#include <iostream>
#include <map>
#include <sstream>
#include <utility>
Expand Down Expand Up @@ -84,29 +85,48 @@ void AnalyzerInformation::close()
}
}

bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors)
bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &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))
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;

// 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;
}

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;

// Check for invalid license error or internal error, in which case we should retry analysis
static std::array<const char*, 3> 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;
Expand Down Expand Up @@ -145,24 +165,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<ErrorMessage> &errors)
bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list<ErrorMessage> &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 << "<?xml version=\"1.0\"?>\n";
mOutputStream << "<analyzerinfo hash=\"" << hash << "\">\n";
}
if (!mOutputStream.is_open())
throw std::runtime_error("failed to open '" + analyzerInfoFile + "'");
mOutputStream << "<?xml version=\"1.0\"?>\n";
mOutputStream << "<analyzerinfo hash=\"" << hash << "\">\n";

return true;
}
Expand All @@ -179,6 +214,7 @@ void AnalyzerInformation::setFileInfo(const std::string &check, const std::strin
mOutputStream << " <FileInfo check=\"" << check << "\">\n" << fileInfo << " </FileInfo>\n";
}

// TODO: report errors
bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) {
const std::string::size_type sep1 = filesTxtLine.find(sep);
if (sep1 == std::string::npos)
Expand Down Expand Up @@ -206,6 +242,7 @@ bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) {
return true;
}

// TODO: how to report errors properly?
// TODO: bail out on unexpected data
void AnalyzerInformation::processFilesTxt(const std::string& buildDir, const std::function<void(const char* checkattr, const tinyxml2::XMLElement* e, const Info& filesTxtInfo)>& handler)
{
Expand All @@ -215,28 +252,40 @@ void AnalyzerInformation::processFilesTxt(const std::string& buildDir, const std
while (std::getline(fin, filesTxtLine)) {
AnalyzerInformation::Info filesTxtInfo;
if (!filesTxtInfo.parse(filesTxtLine)) {
std::cerr << "failed to parse '" + filesTxtLine + "' from '" + filesTxt + "'";
return;
}

const std::string xmlfile = buildDir + '/' + filesTxtInfo.afile;

tinyxml2::XMLDocument doc;
const tinyxml2::XMLError error = doc.LoadFile(xmlfile.c_str());
if (error != tinyxml2::XML_SUCCESS)
if (error == tinyxml2::XML_ERROR_FILE_NOT_FOUND)
return;

const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement();
if (rootNode == nullptr)
if (error != tinyxml2::XML_SUCCESS) {
std::cerr << "failed to load '" + xmlfile + "' from '" + filesTxt + "'";
return;
}

const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement();
if (rootNode == nullptr) {
std::cerr << "no root node found in '" + xmlfile + "' from '" + filesTxt + "'";
continue;
}

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) {
std::cerr << "'check' attribute missing in 'FileInfo' in '" + xmlfile + "' from '" + filesTxt + "'";
continue;
}
handler(checkattr, e, filesTxtInfo);
}
}

// TODO: error on empty file?
}

7 changes: 5 additions & 2 deletions lib/analyzerinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ErrorMessage> &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<ErrorMessage> &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);
Expand All @@ -84,7 +87,7 @@ class CPPCHECKLIB AnalyzerInformation {

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<ErrorMessage> &errors);
static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors, bool debug = false);

private:
std::ofstream mOutputStream;
Expand Down
4 changes: 2 additions & 2 deletions lib/cppcheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str
mLogger->setAnalyzerInfo(nullptr);

std::list<ErrorMessage> 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();
}
Expand Down Expand Up @@ -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<ErrorMessage> 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();
Expand Down
3 changes: 3 additions & 0 deletions lib/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{};

Expand Down
63 changes: 63 additions & 0 deletions test/cli/other_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4119,3 +4119,66 @@ 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_builddir_(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),
str(test_file)
]

stderr_exp = [
'{}:3:14: error: Null pointer dereference: (int*)0 [nullPointer]'.format(test_file)
]

exitcode, stdout, stderr = cppcheck(args)
assert exitcode == 0, stdout
assert stdout.splitlines() == [
"no cached result '{}' found".format(test_a1_file)
]
assert stderr.splitlines() == stderr_exp

exitcode, stdout, stderr = cppcheck(args)
assert exitcode == 0, stdout
assert stdout.splitlines() == [
"skipping analysis - loaded 1 cached finding(s) from '{}'".format(test_a1_file)
]
assert stderr.splitlines() == stderr_exp

with open(test_file, 'a') as f:
f.write('\n#define DEF')

exitcode, stdout, stderr = cppcheck(args)
assert exitcode == 0, stdout
assert stdout.splitlines() == [
"discarding cached result - hash mismatch"
]
assert stderr.splitlines() == stderr_exp

a_a1_file = build_dir / 'test.a1'
with open(a_a1_file, 'a') as f:
f.write('.')

exitcode, stdout, stderr = cppcheck(args)
assert exitcode == 0, stdout
assert stdout.splitlines() == [
"discarding cached result - failed to load '{}' (XML_ERROR_PARSING_TEXT)".format(test_a1_file)
]
assert stderr.splitlines() == stderr_exp
8 changes: 8 additions & 0 deletions test/testcmdlineparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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"};
Expand Down