Skip to content

Commit b6e7ae6

Browse files
committed
improved errorhandling of analyzer information loading [skip ci]
1 parent 6369e51 commit b6e7ae6

9 files changed

Lines changed: 204 additions & 33 deletions

File tree

cli/cmdlineparser.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,9 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
621621
mSettings.cppHeaderProbe = true;
622622
}
623623

624+
else if (std::strcmp(argv[i], "--debug-analyzerinfo") == 0)
625+
mSettings.debugainfo = true;
626+
624627
else if (std::strcmp(argv[i], "--debug-ast") == 0)
625628
mSettings.debugast = true;
626629

cli/cppcheckexecutor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ int CppCheckExecutor::check_internal(const Settings& settings, Suppressions& sup
427427

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

430-
if (settings.severity.isEnabled(Severity::information) || settings.checkConfiguration) {
430+
if ((settings.severity.isEnabled(Severity::information) || settings.checkConfiguration) && !supprs.nomsg.getSuppressions().empty()) {
431431
const bool err = reportUnmatchedSuppressions(settings, supprs.nomsg, mFiles, mFileSettings, stdLogger);
432432
if (err && returnValue == 0)
433433
returnValue = settings.exitCode;

lib/analyzerinfo.cpp

Lines changed: 85 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#include <cstring>
2727
#include <exception>
28+
#include <iostream>
2829
#include <map>
2930
#include <sstream>
3031
#include <utility>
@@ -84,34 +85,61 @@ void AnalyzerInformation::close()
8485
}
8586
}
8687

87-
bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors)
88+
bool AnalyzerInformation::skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors, bool debug)
8889
{
8990
const tinyxml2::XMLElement * const rootNode = analyzerInfoDoc.FirstChildElement();
90-
if (rootNode == nullptr)
91+
if (rootNode == nullptr) {
92+
if (debug)
93+
std::cout << "discarding cached result - no root node found" << std::endl;
9194
return false;
95+
}
9296

93-
const char *attr = rootNode->Attribute("hash");
94-
if (!attr || attr != std::to_string(hash))
97+
if (strcmp(rootNode->Name(), "analyzerinfo") != 0) {
98+
if (debug)
99+
std::cout << "discarding cached result - unexpected root node" << std::endl;
95100
return false;
101+
}
96102

97-
// Check for invalid license error or internal error, in which case we should retry analysis
98-
for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) {
99-
if (std::strcmp(e->Name(), "error") == 0 &&
100-
(e->Attribute("id", "premium-invalidLicense") ||
101-
e->Attribute("id", "premium-internalError") ||
102-
e->Attribute("id", "internalError")
103-
))
104-
return false;
103+
const char * const attr = rootNode->Attribute("hash");
104+
if (!attr) {
105+
if (debug)
106+
std::cout << "discarding cached result - no 'hash' attribute found" << std::endl;
107+
return false;
108+
}
109+
if (attr != std::to_string(hash)) {
110+
if (debug)
111+
std::cout << "discarding cached result - hash mismatch" << std::endl;
112+
return false;
105113
}
106114

107115
for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) {
108-
if (std::strcmp(e->Name(), "error") == 0)
109-
errors.emplace_back(e);
116+
if (std::strcmp(e->Name(), "error") != 0)
117+
continue;
118+
119+
// TODO: discarding results on internalError doesn't make sense since that won't fix itself
120+
// Check for invalid license error or internal error, in which case we should retry analysis
121+
static std::array<const char*, 3> s_ids{
122+
"premium-invalidLicense",
123+
"premium-internalError",
124+
"internalError"
125+
};
126+
for (const auto* id : s_ids)
127+
{
128+
if (e->Attribute("id", id)) {
129+
if (debug)
130+
std::cout << "discarding cached result - '" << id << "' encountered" << std::endl;
131+
errors.clear();
132+
return false;
133+
}
134+
}
135+
136+
errors.emplace_back(e);
110137
}
111138

112139
return true;
113140
}
114141

142+
// TODO: report errors
115143
std::string AnalyzerInformation::getAnalyzerInfoFileFromFilesTxt(std::istream& filesTxt, const std::string &sourcefile, const std::string &cfg, int fileIndex)
116144
{
117145
const std::string id = (fileIndex > 0) ? std::to_string(fileIndex) : "";
@@ -145,24 +173,39 @@ std::string AnalyzerInformation::getAnalyzerInfoFile(const std::string &buildDir
145173
return Path::join(buildDir, std::move(filename)) + ".analyzerinfo";
146174
}
147175

148-
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)
176+
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)
149177
{
178+
if (mOutputStream.is_open())
179+
throw std::runtime_error("analyzer information file is already open");
180+
150181
if (buildDir.empty() || sourcefile.empty())
151182
return true;
152-
close();
153183

154184
const std::string analyzerInfoFile = AnalyzerInformation::getAnalyzerInfoFile(buildDir,sourcefile,cfg,fileIndex);
155185

156-
tinyxml2::XMLDocument analyzerInfoDoc;
157-
const tinyxml2::XMLError xmlError = analyzerInfoDoc.LoadFile(analyzerInfoFile.c_str());
158-
if (xmlError == tinyxml2::XML_SUCCESS && skipAnalysis(analyzerInfoDoc, hash, errors))
159-
return false;
186+
{
187+
tinyxml2::XMLDocument analyzerInfoDoc;
188+
const tinyxml2::XMLError xmlError = analyzerInfoDoc.LoadFile(analyzerInfoFile.c_str());
189+
if (xmlError == tinyxml2::XML_SUCCESS) {
190+
if (skipAnalysis(analyzerInfoDoc, hash, errors, debug)) {
191+
if (debug)
192+
std::cout << "skipping analysis - loaded " << errors.size() << " cached finding(s) from '" << analyzerInfoFile << "'" << std::endl;
193+
return false;
194+
}
195+
}
196+
else if (xmlError != tinyxml2::XML_ERROR_FILE_NOT_FOUND) {
197+
if (debug)
198+
std::cout << "discarding cached result - failed to load '" << analyzerInfoFile << "' (" << tinyxml2::XMLDocument::ErrorIDToName(xmlError) << ")" << std::endl;
199+
}
200+
else if (debug)
201+
std::cout << "no cached result '" << analyzerInfoFile << "' found" << std::endl;
202+
}
160203

161204
mOutputStream.open(analyzerInfoFile);
162-
if (mOutputStream.is_open()) {
163-
mOutputStream << "<?xml version=\"1.0\"?>\n";
164-
mOutputStream << "<analyzerinfo hash=\"" << hash << "\">\n";
165-
}
205+
if (!mOutputStream.is_open())
206+
throw std::runtime_error("failed to open '" + analyzerInfoFile + "'");
207+
mOutputStream << "<?xml version=\"1.0\"?>\n";
208+
mOutputStream << "<analyzerinfo hash=\"" << hash << "\">\n";
166209

167210
return true;
168211
}
@@ -179,6 +222,7 @@ void AnalyzerInformation::setFileInfo(const std::string &check, const std::strin
179222
mOutputStream << " <FileInfo check=\"" << check << "\">\n" << fileInfo << " </FileInfo>\n";
180223
}
181224

225+
// TODO: report errors
182226
bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) {
183227
const std::string::size_type sep1 = filesTxtLine.find(sep);
184228
if (sep1 == std::string::npos)
@@ -206,6 +250,7 @@ bool AnalyzerInformation::Info::parse(const std::string& filesTxtLine) {
206250
return true;
207251
}
208252

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

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

223269
tinyxml2::XMLDocument doc;
224270
const tinyxml2::XMLError error = doc.LoadFile(xmlfile.c_str());
225-
if (error != tinyxml2::XML_SUCCESS)
271+
if (error == tinyxml2::XML_ERROR_FILE_NOT_FOUND)
226272
return;
227273

228-
const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement();
229-
if (rootNode == nullptr)
274+
if (error != tinyxml2::XML_SUCCESS) {
275+
std::cerr << "failed to load '" + xmlfile + "' from '" + filesTxt + "'";
230276
return;
277+
}
278+
279+
const tinyxml2::XMLElement * const rootNode = doc.FirstChildElement();
280+
if (rootNode == nullptr) {
281+
std::cerr << "no root node found in '" + xmlfile + "' from '" + filesTxt + "'";
282+
continue;
283+
}
231284

232285
for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) {
233286
if (std::strcmp(e->Name(), "FileInfo") != 0)
234287
continue;
235288
const char *checkattr = e->Attribute("check");
236-
if (checkattr == nullptr)
289+
if (checkattr == nullptr) {
290+
std::cerr << "'check' attribute missing in 'FileInfo' in '" + xmlfile + "' from '" + filesTxt + "'";
237291
continue;
292+
}
238293
handler(checkattr, e, filesTxtInfo);
239294
}
240295
}
296+
297+
// TODO: error on empty file?
241298
}
242299

lib/analyzerinfo.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,10 @@ class CPPCHECKLIB AnalyzerInformation {
6161

6262
/** Close current TU.analyzerinfo file */
6363
void close();
64-
bool analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex, std::size_t hash, std::list<ErrorMessage> &errors);
64+
/**
65+
* @throws std::runtime_error thrown if the output file is already open or the output file cannot be opened
66+
*/
67+
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);
6568
void reportErr(const ErrorMessage &msg);
6669
void setFileInfo(const std::string &check, const std::string &fileInfo);
6770
static std::string getAnalyzerInfoFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, int fileIndex);
@@ -84,7 +87,7 @@ class CPPCHECKLIB AnalyzerInformation {
8487

8588
static std::string getAnalyzerInfoFileFromFilesTxt(std::istream& filesTxt, const std::string &sourcefile, const std::string &cfg, int fileIndex);
8689

87-
static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors);
90+
static bool skipAnalysis(const tinyxml2::XMLDocument &analyzerInfoDoc, std::size_t hash, std::list<ErrorMessage> &errors, bool debug = false);
8891

8992
private:
9093
std::ofstream mOutputStream;

lib/cppcheck.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -974,7 +974,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str
974974
mLogger->setAnalyzerInfo(nullptr);
975975

976976
std::list<ErrorMessage> errors;
977-
analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors);
977+
analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors, mSettings.debugainfo);
978978
analyzerInformation->setFileInfo("CheckUnusedFunctions", mUnusedFunctionsCheck->analyzerInfo(tokenizer));
979979
analyzerInformation->close();
980980
}
@@ -1020,7 +1020,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str
10201020
// Calculate hash so it can be compared with old hash / future hashes
10211021
const std::size_t hash = calculateHash(preprocessor, file.spath());
10221022
std::list<ErrorMessage> errors;
1023-
if (!analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors)) {
1023+
if (!analyzerInformation->analyzeFile(mSettings.buildDir, file.spath(), cfgname, fileIndex, hash, errors, mSettings.debugainfo)) {
10241024
while (!errors.empty()) {
10251025
mErrorLogger.reportErr(errors.front());
10261026
errors.pop_front();

lib/errorlogger.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ ErrorMessage::ErrorMessage(ErrorPath errorPath, const TokenList *tokenList, Seve
162162
// hash = calculateWarningHash(tokenList, hashWarning.str());
163163
}
164164

165+
// TODO: improve errorhandling?
165166
ErrorMessage::ErrorMessage(const tinyxml2::XMLElement * const errmsg)
166167
: severity(Severity::none),
167168
cwe(0U),

lib/settings.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,9 @@ class CPPCHECKLIB WARN_UNUSED Settings {
189189
/** @brief Are we running from DACA script? */
190190
bool daca{};
191191

192+
/** @brief Is --debug-analyzerinfo given? */
193+
bool debugainfo{};
194+
192195
/** @brief Is --debug-ast given? */
193196
bool debugast{};
194197

test/cli/other_test.py

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4119,3 +4119,99 @@ def test_active_unusedfunction_only_misra_builddir(tmp_path):
41194119
'CheckUnusedFunctions::check'
41204120
]
41214121
__test_active_checkers(tmp_path, 1, 1175, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
4122+
4123+
4124+
def test_analyzerinfo(tmp_path):
4125+
test_file = tmp_path / 'test.c'
4126+
with open(test_file, "w") as f:
4127+
f.write(
4128+
"""void f()
4129+
{
4130+
(void)(*((int*)0));
4131+
}
4132+
""")
4133+
4134+
build_dir = tmp_path / 'b1'
4135+
os.makedirs(build_dir)
4136+
4137+
test_a1_file = build_dir / 'test.a1'
4138+
4139+
args = [
4140+
'-q',
4141+
'--debug-analyzerinfo',
4142+
'--template=simple',
4143+
'--cppcheck-build-dir={}'.format(build_dir),
4144+
str(test_file)
4145+
]
4146+
4147+
stderr_exp = [
4148+
'{}:3:14: error: Null pointer dereference: (int*)0 [nullPointer]'.format(test_file)
4149+
]
4150+
4151+
def run_and_assert_cppcheck(stdout_exp):
4152+
exitcode, stdout, stderr = cppcheck(args)
4153+
assert exitcode == 0, stdout
4154+
assert stdout.splitlines() == stdout_exp
4155+
assert stderr.splitlines() == stderr_exp
4156+
4157+
# no cached results
4158+
run_and_assert_cppcheck([
4159+
"no cached result '{}' found".format(test_a1_file)
4160+
])
4161+
4162+
# cached results
4163+
run_and_assert_cppcheck([
4164+
"skipping analysis - loaded 1 cached finding(s) from '{}'".format(test_a1_file)
4165+
])
4166+
4167+
# modified file
4168+
with open(test_file, 'a') as f:
4169+
f.write('\n#define DEF')
4170+
4171+
run_and_assert_cppcheck([
4172+
"discarding cached result - hash mismatch" # TODO: add filename
4173+
])
4174+
4175+
# invalid XML
4176+
with open(test_a1_file, 'a') as f:
4177+
f.write('.')
4178+
4179+
run_and_assert_cppcheck([
4180+
"discarding cached result - failed to load '{}' (XML_ERROR_PARSING_TEXT)".format(test_a1_file)
4181+
])
4182+
4183+
# missing root node
4184+
with open(test_a1_file, 'w') as f:
4185+
f.write('<?xml version="1.0"?>')
4186+
4187+
run_and_assert_cppcheck([
4188+
"discarding cached result - no root node found" # TODO: add filename
4189+
])
4190+
4191+
# mismatched root node
4192+
with open(test_a1_file, 'w') as f:
4193+
f.write('<?xml version="1.0"?><root/>')
4194+
4195+
run_and_assert_cppcheck([
4196+
"discarding cached result - unexpected root node" # TODO: add filename
4197+
])
4198+
4199+
# missing 'hash' attribute
4200+
with open(test_a1_file, 'w') as f:
4201+
f.write('<?xml version="1.0"?><analyzerinfo/>')
4202+
4203+
run_and_assert_cppcheck([
4204+
"discarding cached result - no 'hash' attribute found" # TODO: add filename
4205+
])
4206+
4207+
# invalid 'hash' attribute
4208+
with open(test_a1_file, 'w') as f:
4209+
f.write('<?xml version="1.0"?><analyzerinfo hash="hash"/>')
4210+
4211+
run_and_assert_cppcheck([
4212+
"discarding cached result - hash mismatch" # TODO: add filename
4213+
])
4214+
4215+
# TODO:
4216+
# - invalid error
4217+
# - internalError

test/testcmdlineparser.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -493,6 +493,7 @@ class TestCmdlineParser : public TestFixture {
493493
TEST_CASE(safetyOverride);
494494
TEST_CASE(noSafety);
495495
TEST_CASE(noSafetyOverride);
496+
TEST_CASE(debugAnalyzerinfo);
496497

497498
TEST_CASE(ignorepaths1);
498499
TEST_CASE(ignorepaths2);
@@ -3424,6 +3425,13 @@ class TestCmdlineParser : public TestFixture {
34243425
ASSERT_EQUALS(false, settings->safety);
34253426
}
34263427

3428+
void debugAnalyzerinfo() {
3429+
REDIRECT;
3430+
const char * const argv[] = {"cppcheck", "--debug-analyzerinfo", "file.cpp"};
3431+
ASSERT_EQUALS_ENUM(CmdLineParser::Result::Success, parseFromArgs(argv));
3432+
ASSERT_EQUALS(true, settings->debugainfo);
3433+
}
3434+
34273435
void ignorepaths1() {
34283436
REDIRECT;
34293437
const char * const argv[] = {"cppcheck", "-isrc", "file.cpp"};

0 commit comments

Comments
 (0)