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