Improved addon execution errorhandling

addons/README.md

@@ -10,6 +10,35 @@ Addons are scripts that analyses Cppcheck dump files to check compatibility with
  Checks Linux system for [year 2038 problem](https://en.wikipedia.org/wiki/Year_2038_problem) safety. This required [modified environment](https://github.com/3adev/y2038). See complete description [here](https://github.com/danmar/cppcheck/blob/main/addons/doc/y2038.txt).
 + [threadsafety.py](https://github.com/danmar/cppcheck/blob/main/addons/threadsafety.py) 
  Analyse Cppcheck dump files to locate threadsafety issues like static local objects used by multiple threads.
++ [naming.py](https://github.com/danmar/cppcheck/blob/main/addons/naming.py)
+ Enforces naming conventions across the code.
++ [namingng.py](https://github.com/danmar/cppcheck/blob/main/addons/namingng.py)
+ Enforces naming conventions across the code. Enhanced version with support for type prefixes in variable and function names.
++ [findcasts.py](https://github.com/danmar/cppcheck/blob/main/addons/findcasts.py)
+ Locates casts in the code.
++ [misc.py](https://github.com/danmar/cppcheck/blob/main/addons/misc.py)
+ Performs miscellaneous checks.
+
+### Other files
+
+- doc
+ Additional files for documentation generation.
+- tests
+ Contains various unit tests for the addons.
+- cppcheck.py
+ Internal helper used by Cppcheck binary to run the addons.
+- cppcheckdata.doxyfile
+ Configuration file for documentation generation.
+- cppcheckdata.py
+ Helper class for reading Cppcheck dump files within an addon.
+- misra_9.py
+ Implementation of the MISRA 9.x rules used by `misra` addon. 
+- naming.json
+ Example configuration for `namingng` addon. 
+- ROS_naming.json
+ Example configuration for the `namingng` addon enforcing the [ROS naming convention for C++ ](http://wiki.ros.org/CppStyleGuide#Files).
+- runaddon.py
+ Internal helper used by Cppcheck binary to run the addons.
 
 ## Usage
 

cli/cppcheckexecutor.cpp

@@ -540,7 +540,7 @@ bool CppCheckExecutor::tryLoadLibrary(Library& destination, const std::string& b
  */
 // cppcheck-suppress passedByValue - used as callback so we need to preserve the signature
 // NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
-bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_)
+int CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_)
 {
  output_.clear();
 
@@ -567,9 +567,12 @@ bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string>
 #else
  FILE *p = popen(cmd.c_str(), "r");
 #endif
+ //std::cout << "invoking command '" << cmd << "'" << std::endl;
  if (!p) {
- // TODO: read errno
- return false;
+ // TODO: how to provide to caller?
+ //const int err = errno;
+ //std::cout << "popen() errno " << std::to_string(err) << std::endl;
+ return -1;
  }
  char buffer[1024];
  while (fgets(buffer, sizeof(buffer), p) != nullptr)
@@ -581,13 +584,19 @@ bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string>
  const int res = pclose(p);
 #endif
  if (res == -1) { // error occured
- // TODO: read errno
- return false;
+ // TODO: how to provide to caller?
+ //const int err = errno;
+ //std::cout << "pclose() errno " << std::to_string(err) << std::endl;
+ return res;
  }
-if (res != 0) { // process failed
- // TODO: need to get error details
- return false;
+#if !defined(WIN32) && !defined(__MINGW32__)
+if (WIFEXITED(res)) {
+ return WEXITSTATUS(res);
  }
- return true;
+ if (WIFSIGNALED(res)) {
+ return WTERMSIG(res);
+ }
+#endif
+ return res;
 }
 

cli/cppcheckexecutor.h

@@ -99,9 +99,9 @@ class CppCheckExecutor : public ErrorLogger {
  static bool tryLoadLibrary(Library& destination, const std::string& basepath, const char* filename);
 
  /**
- * Execute a shell command and read the output from it. Returns true if command terminated successfully.
+ * Execute a shell command and read the output from it. Returns exitcode of the executed command,.
  */
- static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_);
+ static int executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_);
 
 protected:
 

gui/checkthread.cpp

@@ -57,7 +57,7 @@
 #endif
 
 // NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
-static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output) // cppcheck-suppress passedByValue
+static int executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output) // cppcheck-suppress passedByValue
 {
  output.clear();
 
@@ -80,7 +80,7 @@ static bool executeCommand(std::string exe, std::vector<std::string> args, std::
  std::ofstream fout(redirect.substr(3));
  fout << process.readAllStandardError().toStdString();
  }
- return process.exitCode() == 0;
+ return process.exitCode();
 }
 
 

lib/cppcheck.cpp

@@ -180,16 +180,16 @@ namespace {
 
  std::string getAddonInfo(const std::string &fileName, const std::string &exename) {
  if (fileName[0] == '{') {
- std::istringstream in(fileName);
  picojson::value json;
- in >> json;
+ const std::string err = picojson::parse(json, fileName);
+ (void)err; // TODO: report
  return parseAddonInfo(json, fileName, exename);
  }
  if (fileName.find('.') == std::string::npos)
  return getAddonInfo(fileName + ".py", exename);
 
  if (endsWith(fileName, ".py")) {
- scriptFile = getFullPath(fileName, exename);
+ scriptFile = Path::fromNativeSeparators(getFullPath(fileName, exename));
  if (scriptFile.empty())
  return "Did not find addon " + fileName;
 
@@ -328,11 +328,35 @@ static void createDumpFile(const Settings& settings,
  << "/>" << '\n';
 }
 
-static std::string executeAddon(const AddonInfo &addonInfo,
- const std::string &defaultPythonExe,
- const std::string &file,
- const std::string &premiumArgs,
- const std::function<bool(std::string,std::vector<std::string>,std::string,std::string&)> &executeCommand)
+static std::string detectPython(const CppCheck::ExecuteCmdFn &executeCommand)
+{
+#ifdef _WIN32
+ const char *py_exes[] = { "python3.exe", "python.exe" };
+#else
+ const char *py_exes[] = { "python3", "python" };
+#endif
+ for (const char* py_exe : py_exes) {
+ std::string out;
+#ifdef _MSC_VER
+ // FIXME: hack to avoid debug assertion with _popen() in executeCommand() for non-existing commands
+ const std::string cmd = std::string(py_exe) + " --version >NUL";
+ if (system(cmd.c_str()) != 0) {
+ // TODO: get more detailed error?
+ break;
+ }
+#endif
+ if (executeCommand(py_exe, split("--version"), "2>&1", out) == EXIT_SUCCESS && startsWith(out, "Python ") && std::isdigit(out[7])) {
+ return py_exe;
+ }
+ }
+ return "";
+}
+
+static std::vector<picojson::value> executeAddon(const AddonInfo &addonInfo,
+ const std::string &defaultPythonExe,
+ const std::string &file,
+ const std::string &premiumArgs,
+ const CppCheck::ExecuteCmdFn &executeCommand)
 {
  const std::string redirect = "2>&1";
 
@@ -345,28 +369,11 @@ static std::string executeAddon(const AddonInfo &addonInfo,
  else if (!defaultPythonExe.empty())
  pythonExe = cmdFileName(defaultPythonExe);
  else {
-#ifdef _WIN32
- const char *py_exes[] = { "python3.exe", "python.exe" };
-#else
- const char *py_exes[] = { "python3", "python" };
-#endif
- for (const char* py_exe : py_exes) {
- std::string out;
-#ifdef _MSC_VER
- // FIXME: hack to avoid debug assertion with _popen() in executeCommand() for non-existing commands
- const std::string cmd = std::string(py_exe) + " --version >NUL";
- if (system(cmd.c_str()) != 0) {
- // TODO: get more detailed error?
- break;
- }
-#endif
- if (executeCommand(py_exe, split("--version"), redirect, out) && startsWith(out, "Python ") && std::isdigit(out[7])) {
- pythonExe = py_exe;
- break;
- }
- }
- if (pythonExe.empty())
+ // store in static variable so we only look this up once
+ static const std::string detectedPythonExe = detectPython(executeCommand);
+ if (detectedPythonExe.empty())
  throw InternalError(nullptr, "Failed to auto detect python");
+ pythonExe = detectedPythonExe;
  }
 
  std::string args;
@@ -380,27 +387,62 @@ static std::string executeAddon(const AddonInfo &addonInfo,
  args += fileArg;
 
  std::string result;
- if (!executeCommand(pythonExe, split(args), redirect, result)) {
- std::string message("Failed to execute addon '" + addonInfo.name + "' (command: '" + pythonExe + " " + args + "'). Exitcode is nonzero.");
+ if (const int exitcode = executeCommand(pythonExe, split(args), redirect, result)) {
+ std::string message("Failed to execute addon '" + addonInfo.name + "' - exitcode is " + std::to_string(exitcode));
+ std::string details = pythonExe + " " + args;
  if (result.size() > 2) {
- message = message + "\n" + message + "\nOutput:\n" + result;
- message.resize(message.find_last_not_of("\n\r"));
+ details += "\nOutput:\n";
+ details += result;
+ const auto pos = details.find_last_not_of("\n\r");
+ if (pos != std::string::npos)
+ details.resize(pos + 1);
  }
- throw InternalError(nullptr, message);
+ throw InternalError(nullptr, message, details);
  }
 
+ std::vector<picojson::value> addonResult;
+
  // Validate output..
  std::istringstream istr(result);
  std::string line;
  while (std::getline(istr, line)) {
- if (!startsWith(line,"Checking ") && !line.empty() && line[0] != '{') {
+ // TODO: also bail out?
+ if (line.empty()) {
+ //std::cout << "addon '" << addonInfo.name << "' result contains empty line" << std::endl;
+ continue;
+ }
+
+ // TODO: get rid of this
+ if (startsWith(line,"Checking ")) {
+ //std::cout << "addon '" << addonInfo.name << "' result contains 'Checking ' line" << std::endl;
+ continue;
+ }
+
+ if (line[0] != '{') {
+ //std::cout << "addon '" << addonInfo.name << "' result is not a JSON" << std::endl;
+
  result.erase(result.find_last_not_of('\n') + 1, std::string::npos); // Remove trailing newlines
  throw InternalError(nullptr, "Failed to execute '" + pythonExe + " " + args + "'. " + result);
  }
+
+ //std::cout << "addon '" << addonInfo.name << "' result is " << line << std::endl;
+
+ // TODO: make these failures?
+ picojson::value res;
+ const std::string err = picojson::parse(res, line);
+ if (!err.empty()) {
+ //std::cout << "addon '" << addonInfo.name << "' result is not a valid JSON (" << err << ")" << std::endl;
+ continue;
+ }
+ if (!res.is<picojson::object>()) {
+ //std::cout << "addon '" << addonInfo.name << "' result is not a JSON object" << std::endl;
+ continue;
+ }
+ addonResult.emplace_back(std::move(res));
  }
 
  // Valid results
- return result;
+ return addonResult;
 }
 
 static std::string getDefinesFlags(const std::string &semicolonSeparatedString)
@@ -413,7 +455,7 @@ static std::string getDefinesFlags(const std::string &semicolonSeparatedString)
 
 CppCheck::CppCheck(ErrorLogger &errorLogger,
  bool useGlobalSuppressions,
-std::function<bool(std::string,std::vector<std::string>,std::string,std::string&)> executeCommand)
+ExecuteCmdFn executeCommand)
  : mErrorLogger(errorLogger)
  , mUseGlobalSuppressions(useGlobalSuppressions)
  , mExecuteCommand(std::move(executeCommand))
@@ -532,7 +574,7 @@ unsigned int CppCheck::check(const std::string &path)
  }
 
  std::string output2;
- if (!mExecuteCommand(exe,split(args2),redirect2,output2) || output2.find("TranslationUnitDecl") == std::string::npos) {
+ if (mExecuteCommand(exe,split(args2),redirect2,output2) != EXIT_SUCCESS || output2.find("TranslationUnitDecl") == std::string::npos) {
  std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "'" << std::endl;
  return 0;
  }
@@ -596,7 +638,8 @@ unsigned int CppCheck::check(const std::string &path)
  executeAddons(dumpFile);
 
  } catch (const InternalError &e) {
- internalError(path, "Processing Clang AST dump failed: " + e.errorMessage);
+ const ErrorMessage errmsg = ErrorMessage::fromInternalError(e, nullptr, path, "Bailing out from analysis: Processing Clang AST dump failed");
+ reportErr(errmsg);
  } catch (const TerminateException &) {
  // Analysis is terminated
  return mExitCode;
@@ -1022,7 +1065,8 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string
  } catch (const std::bad_alloc &) {
  internalError(filename, "Checking file failed: out of memory");
  } catch (const InternalError &e) {
- internalError(filename, "Checking file failed: " + e.errorMessage);
+ const ErrorMessage errmsg = ErrorMessage::fromInternalError(e, nullptr, filename, "Bailing out from analysis: Checking file failed");
+ reportErr(errmsg);
  }
 
  if (!mSettings.buildDir.empty()) {
@@ -1448,23 +1492,14 @@ void CppCheck::executeAddons(const std::vector<std::string>& files)
  if (addonInfo.name != "misra" && !addonInfo.ctu && endsWith(files.back(), ".ctu-info"))
  continue;
 
- const std::string results =
+ const std::vector<picojson::value> results =
  executeAddon(addonInfo, mSettings.addonPython, fileList.empty() ? files[0] : fileList, mSettings.premiumArgs, mExecuteCommand);
- std::istringstream istr(results);
- std::string line;
 
  const bool misraC2023 = mSettings.premiumArgs.find("--misra-c-2023") != std::string::npos;
 
- while (std::getline(istr, line)) {
- if (!startsWith(line,"{"))
- continue;
-
- picojson::value res;
- std::istringstream istr2(line);
- istr2 >> res;
- if (!res.is<picojson::object>())
- continue;
-
+ for (const picojson::value& res : results) {
+ // TODO: get rid of copy?
+ // this is a copy so we can access missing fields and get a default value
  picojson::object obj = res.get<picojson::object>();
 
  ErrorMessage errmsg;
@@ -1519,7 +1554,8 @@ void CppCheck::executeAddonsWholeProgram(const std::map<std::string, std::size_t
  try {
  executeAddons(ctuInfoFiles);
  } catch (const InternalError& e) {
- internalError("", "Whole program analysis failed: " + e.errorMessage);
+ const ErrorMessage errmsg = ErrorMessage::fromInternalError(e, nullptr, "", "Bailing out from analysis: Whole program analysis failed");
+ reportErr(errmsg);
  }
 
  if (mSettings.buildDir.empty()) {
@@ -1687,8 +1723,8 @@ void CppCheck::analyseClangTidy(const ImportProject::FileSettings &fileSettings)
 
  const std::string args = "-quiet -checks=*,-clang-analyzer-*,-llvm* \"" + fileSettings.filename + "\" -- " + allIncludes + allDefines;
  std::string output;
- if (!mExecuteCommand(exe, split(args), emptyString, output)) {
- std::cerr << "Failed to execute '" << exe << "'" << std::endl;
+ if (const int exitcode = mExecuteCommand(exe, split(args), emptyString, output)) {
+ std::cerr << "Failed to execute '" << exe << "' (exitcode: " << std::to_string(exitcode) << ")" << std::endl;
  return;
  }
 

lib/cppcheck.h

@@ -50,12 +50,14 @@ class Tokenizer;
  */
 class CPPCHECKLIB CppCheck : ErrorLogger {
 public:
+ using ExecuteCmdFn = std::function<int (std::string,std::vector<std::string>,std::string,std::string&)>;
+
  /**
  * @brief Constructor.
  */
  CppCheck(ErrorLogger &errorLogger,
  bool useGlobalSuppressions,
-std::function<bool(std::string,std::vector<std::string>,std::string,std::string&)> executeCommand);
+ExecuteCmdFn executeCommand);
 
  /**
  * @brief Destructor.
@@ -230,7 +232,7 @@ class CPPCHECKLIB CppCheck : ErrorLogger {
  AnalyzerInformation mAnalyzerInformation;
 
  /** Callback for executing a shell command (exe, args, output) */
-std::function<bool(std::string,std::vector<std::string>,std::string,std::string&)> mExecuteCommand;
+ExecuteCmdFn mExecuteCommand;
 
  std::ofstream mPlistFile;
 };