From a00b6e1f8a39f46a208154417b00db94ba40e021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Sat, 4 Mar 2023 12:05:17 +0100 Subject: [PATCH] simplified `ThreadExecutor` class by moving some code out of it / fixed some thread safety issues (#4849) --- Makefile | 2 +- cli/cppcheckexecutor.cpp | 2 + cli/executor.cpp | 20 ++++ cli/executor.h | 12 +++ cli/processexecutor.cpp | 15 +-- cli/threadexecutor.cpp | 200 +++++++++++++++++++---------------- cli/threadexecutor.h | 6 +- lib/timer.cpp | 3 +- test/fixture.cpp | 10 +- test/redirect.h | 31 +++++- test/testprocessexecutor.cpp | 21 +++- test/testthreadexecutor.cpp | 21 +++- 12 files changed, 221 insertions(+), 122 deletions(-) diff --git a/Makefile b/Makefile index 8d48555bb..fbdd57fdd 100644 --- a/Makefile +++ b/Makefile @@ -643,7 +643,7 @@ cli/cppcheckexecutorseh.o: cli/cppcheckexecutorseh.cpp cli/cppcheckexecutor.h cl cli/cppcheckexecutorsig.o: cli/cppcheckexecutorsig.cpp cli/cppcheckexecutor.h cli/cppcheckexecutorsig.h cli/stacktrace.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/suppressions.h $(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cppcheckexecutorsig.cpp -cli/executor.o: cli/executor.cpp cli/executor.h +cli/executor.o: cli/executor.cpp cli/executor.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/utils.h $(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/executor.cpp cli/filelister.o: cli/filelister.cpp cli/filelister.h lib/config.h lib/path.h lib/pathmatch.h lib/utils.h diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index 40375b39c..dab78d27c 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -345,6 +345,7 @@ int CppCheckExecutor::check_internal(CppCheck& cppcheck) } } + // TODO: not performed when multiple jobs are being used // second loop to parse all markup files which may not work until all // c/cpp files have been parsed and checked for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) { @@ -462,6 +463,7 @@ void CppCheckExecutor::reportStatus(std::size_t fileindex, std::size_t filecount oss << fileindex << '/' << filecount << " files checked " << percentDone << "% done"; + // TODO: do not unconditionally print in color std::cout << Color::FgBlue << oss.str() << Color::Reset << std::endl; } } diff --git a/cli/executor.cpp b/cli/executor.cpp index 32424f767..587578c95 100644 --- a/cli/executor.cpp +++ b/cli/executor.cpp @@ -18,6 +18,11 @@ #include "executor.h" +#include "errorlogger.h" +#include "settings.h" + +#include + Executor::Executor(const std::map &files, Settings &settings, ErrorLogger &errorLogger) : mFiles(files), mSettings(settings), mErrorLogger(errorLogger) {} @@ -25,3 +30,18 @@ Executor::Executor(const std::map &files, Settings &se Executor::~Executor() {} +bool Executor::hasToLog(const ErrorMessage &msg) +{ + if (!mSettings.nomsg.isSuppressed(msg)) + { + std::string errmsg = msg.toString(mSettings.verbose); + + std::lock_guard lg(mErrorListSync); + if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) == mErrorList.cend()) { + mErrorList.emplace_back(std::move(errmsg)); + return true; + } + } + return false; +} + diff --git a/cli/executor.h b/cli/executor.h index 3ce02a372..3a0a6ec62 100644 --- a/cli/executor.h +++ b/cli/executor.h @@ -22,10 +22,12 @@ #include #include #include +#include #include class Settings; class ErrorLogger; +class ErrorMessage; /// @addtogroup CLI /// @{ @@ -45,9 +47,19 @@ public: virtual unsigned int check() = 0; protected: + /** + * @brief Check if message is being suppressed and unique. + * @param msg the message to check + * @return true if message is not suppressed and unique + */ + bool hasToLog(const ErrorMessage &msg); + const std::map &mFiles; Settings &mSettings; ErrorLogger &mErrorLogger; + +private: + std::mutex mErrorListSync; std::list mErrorList; }; diff --git a/cli/processexecutor.cpp b/cli/processexecutor.cpp index b8bc55710..a07f81b8d 100644 --- a/cli/processexecutor.cpp +++ b/cli/processexecutor.cpp @@ -169,16 +169,11 @@ int ProcessExecutor::handleRead(int rpipe, unsigned int &result) std::exit(EXIT_FAILURE); } - if (!mSettings.nomsg.isSuppressed(msg)) { - // Alert only about unique errors - std::string errmsg = msg.toString(mSettings.verbose); - if (std::find(mErrorList.cbegin(), mErrorList.cend(), errmsg) == mErrorList.cend()) { - mErrorList.emplace_back(std::move(errmsg)); - if (type == PipeWriter::REPORT_ERROR) - mErrorLogger.reportErr(msg); - else - mErrorLogger.reportInfo(msg); - } + if (hasToLog(msg)) { + if (type == PipeWriter::REPORT_ERROR) + mErrorLogger.reportErr(msg); + else + mErrorLogger.reportInfo(msg); } } else if (type == PipeWriter::CHILD_END) { std::istringstream iss(buf); diff --git a/cli/threadexecutor.cpp b/cli/threadexecutor.cpp index f49110b4e..aa9f10826 100644 --- a/cli/threadexecutor.cpp +++ b/cli/threadexecutor.cpp @@ -19,6 +19,7 @@ #include "threadexecutor.h" #include "color.h" +#include "config.h" #include "cppcheck.h" #include "cppcheckexecutor.h" #include "errorlogger.h" @@ -45,27 +46,70 @@ ThreadExecutor::ThreadExecutor(const std::map &files, ThreadExecutor::~ThreadExecutor() {} -class ThreadExecutor::SyncLogForwarder : public ErrorLogger +class Data { public: - explicit SyncLogForwarder(ThreadExecutor &threadExecutor) - : mThreadExecutor(threadExecutor), mProcessedFiles(0), mTotalFiles(0), mProcessedSize(0) { + Data(const std::map &files, const std::list &fileSettings) + : mFiles(files), mFileSettings(fileSettings), mProcessedFiles(0), mProcessedSize(0) + { + mItNextFile = mFiles.begin(); + mItNextFileSettings = mFileSettings.begin(); - const std::map& files = mThreadExecutor.mFiles; - mItNextFile = files.begin(); - mItNextFileSettings = mThreadExecutor.mSettings.project.fileSettings.begin(); - - mTotalFiles = files.size() + mThreadExecutor.mSettings.project.fileSettings.size(); - mTotalFileSize = std::accumulate(files.cbegin(), files.cend(), std::size_t(0), [](std::size_t v, const std::pair& p) { + mTotalFiles = mFiles.size() + mFileSettings.size(); + mTotalFileSize = std::accumulate(mFiles.cbegin(), mFiles.cend(), std::size_t(0), [](std::size_t v, const std::pair& p) { return v + p.second; }); } + bool finished() { + std::lock_guard l(mFileSync); + return mItNextFile == mFiles.cend() && mItNextFileSettings == mFileSettings.cend(); + } + + bool next(const std::string *&file, const ImportProject::FileSettings *&fs, std::size_t &fileSize) { + std::lock_guard l(mFileSync); + if (mItNextFile != mFiles.end()) { + file = &mItNextFile->first; + fileSize = mItNextFile->second; + ++mItNextFile; + return true; + } + if (mItNextFileSettings != mFileSettings.end()) { + fs = &(*mItNextFileSettings); + fileSize = 0; + ++mItNextFileSettings; + return true; + } + + return false; + } + +private: + const std::map &mFiles; + std::map::const_iterator mItNextFile; + const std::list &mFileSettings; + std::list::const_iterator mItNextFileSettings; + +public: + std::size_t mProcessedFiles; + std::size_t mTotalFiles; + std::size_t mProcessedSize; + std::size_t mTotalFileSize; + + std::mutex mFileSync; +}; + +class SyncLogForwarder : public ErrorLogger +{ +public: + explicit SyncLogForwarder(ThreadExecutor &threadExecutor, ErrorLogger &errorLogger) + : mThreadExecutor(threadExecutor), mErrorLogger(errorLogger) {} + void reportOut(const std::string &outmsg, Color c) override { std::lock_guard lg(mReportSync); - mThreadExecutor.mErrorLogger.reportOut(outmsg, c); + mErrorLogger.reportOut(outmsg, c); } void reportErr(const ErrorMessage &msg) override { @@ -76,18 +120,6 @@ public: report(msg, MessageType::REPORT_INFO); } - ThreadExecutor &mThreadExecutor; - - std::map::const_iterator mItNextFile; - std::list::const_iterator mItNextFileSettings; - - std::size_t mProcessedFiles; - std::size_t mTotalFiles; - std::size_t mProcessedSize; - std::size_t mTotalFileSize; - - std::mutex mFileSync; - std::mutex mErrorSync; std::mutex mReportSync; private: @@ -95,47 +127,77 @@ private: void report(const ErrorMessage &msg, MessageType msgType) { - if (mThreadExecutor.mSettings.nomsg.isSuppressed(msg)) + if (!mThreadExecutor.hasToLog(msg)) return; - // Alert only about unique errors - bool reportError = false; + std::lock_guard lg(mReportSync); - { - std::string errmsg = msg.toString(mThreadExecutor.mSettings.verbose); + switch (msgType) { + case MessageType::REPORT_ERROR: + mErrorLogger.reportErr(msg); + break; + case MessageType::REPORT_INFO: + mErrorLogger.reportInfo(msg); + break; + } + } - std::lock_guard lg(mErrorSync); - if (std::find(mThreadExecutor.mErrorList.cbegin(), mThreadExecutor.mErrorList.cend(), errmsg) == mThreadExecutor.mErrorList.cend()) { - mThreadExecutor.mErrorList.emplace_back(std::move(errmsg)); - reportError = true; - } + ThreadExecutor &mThreadExecutor; + ErrorLogger &mErrorLogger; +}; + +static unsigned int STDCALL threadProc(Data *data, SyncLogForwarder* logForwarder, const Settings &settings) +{ + unsigned int result = 0; + + for (;;) { + if (data->finished()) { + break; } - if (reportError) { - std::lock_guard lg(mReportSync); + const std::string *file = nullptr; + const ImportProject::FileSettings *fs = nullptr; + std::size_t fileSize; + if (!data->next(file, fs, fileSize)) + break; - switch (msgType) { - case MessageType::REPORT_ERROR: - mThreadExecutor.mErrorLogger.reportErr(msg); - break; - case MessageType::REPORT_INFO: - mThreadExecutor.mErrorLogger.reportInfo(msg); - break; + CppCheck fileChecker(*logForwarder, false, CppCheckExecutor::executeCommand); + fileChecker.settings() = settings; + + if (fs) { + // file settings.. + result += fileChecker.check(*fs); + if (settings.clangTidy) + fileChecker.analyseClangTidy(*fs); + } else { + // Read file from a file + result += fileChecker.check(*file); + } + + { + std::lock_guard l(data->mFileSync); + data->mProcessedSize += fileSize; + data->mProcessedFiles++; + if (!settings.quiet) { + std::lock_guard lg(logForwarder->mReportSync); + CppCheckExecutor::reportStatus(data->mProcessedFiles, data->mTotalFiles, data->mProcessedSize, data->mTotalFileSize); } } } -}; + return result; +} unsigned int ThreadExecutor::check() { std::vector> threadFutures; threadFutures.reserve(mSettings.jobs); - SyncLogForwarder logforwarder(*this); + Data data(mFiles, mSettings.project.fileSettings); + SyncLogForwarder logforwarder(*this, mErrorLogger); for (unsigned int i = 0; i < mSettings.jobs; ++i) { try { - threadFutures.emplace_back(std::async(std::launch::async, threadProc, &logforwarder)); + threadFutures.emplace_back(std::async(std::launch::async, &threadProc, &data, &logforwarder, mSettings)); } catch (const std::system_error &e) { std::cerr << "#### ThreadExecutor::check exception :" << e.what() << std::endl; @@ -147,53 +209,3 @@ unsigned int ThreadExecutor::check() return v + f.get(); }); } - -unsigned int STDCALL ThreadExecutor::threadProc(SyncLogForwarder* logForwarder) -{ - unsigned int result = 0; - - std::map::const_iterator &itFile = logForwarder->mItNextFile; - std::list::const_iterator &itFileSettings = logForwarder->mItNextFileSettings; - - // guard static members of CppCheck against concurrent access - logForwarder->mFileSync.lock(); - - for (;;) { - if (itFile == logForwarder->mThreadExecutor.mFiles.cend() && itFileSettings == logForwarder->mThreadExecutor.mSettings.project.fileSettings.cend()) { - logForwarder->mFileSync.unlock(); - break; - } - - CppCheck fileChecker(*logForwarder, false, CppCheckExecutor::executeCommand); - fileChecker.settings() = logForwarder->mThreadExecutor.mSettings; - - std::size_t fileSize = 0; - if (itFile != logForwarder->mThreadExecutor.mFiles.end()) { - const std::string &file = itFile->first; - fileSize = itFile->second; - ++itFile; - - logForwarder->mFileSync.unlock(); - - // Read file from a file - result += fileChecker.check(file); - } else { // file settings.. - const ImportProject::FileSettings &fs = *itFileSettings; - ++itFileSettings; - logForwarder->mFileSync.unlock(); - result += fileChecker.check(fs); - if (logForwarder->mThreadExecutor.mSettings.clangTidy) - fileChecker.analyseClangTidy(fs); - } - - logForwarder->mFileSync.lock(); - - logForwarder->mProcessedSize += fileSize; - logForwarder->mProcessedFiles++; - if (!logForwarder->mThreadExecutor.mSettings.quiet) { - std::lock_guard lg(logForwarder->mReportSync); - CppCheckExecutor::reportStatus(logForwarder->mProcessedFiles, logForwarder->mTotalFiles, logForwarder->mProcessedSize, logForwarder->mTotalFileSize); - } - } - return result; -} diff --git a/cli/threadexecutor.h b/cli/threadexecutor.h index facf5e536..5c24dfe0c 100644 --- a/cli/threadexecutor.h +++ b/cli/threadexecutor.h @@ -19,8 +19,6 @@ #ifndef THREADEXECUTOR_H #define THREADEXECUTOR_H -#include "config.h" - #include "executor.h" #include @@ -46,9 +44,7 @@ public: unsigned int check() override; -private: - class SyncLogForwarder; - static unsigned int STDCALL threadProc(SyncLogForwarder *logForwarder); + friend class SyncLogForwarder; }; /// @} diff --git a/lib/timer.cpp b/lib/timer.cpp index 1a81dbe99..d1d655ac8 100644 --- a/lib/timer.cpp +++ b/lib/timer.cpp @@ -25,7 +25,6 @@ /* TODO: - rename "file" to "single" - - synchronise map access in multithreaded mode or disable timing - add unit tests - for --showtime (needs input file) - for Timer* classes @@ -48,9 +47,9 @@ void TimerResults::showResults(SHOWTIME_MODES mode) const TimerResultsData overallData; std::vector data; - data.reserve(mResults.size()); { std::lock_guard l(mResultsSync); + data.reserve(mResults.size()); data.insert(data.begin(), mResults.cbegin(), mResults.cend()); } std::sort(data.begin(), data.end(), more_second_sec); diff --git a/test/fixture.cpp b/test/fixture.cpp index b76c0777a..138239146 100644 --- a/test/fixture.cpp +++ b/test/fixture.cpp @@ -102,7 +102,7 @@ bool TestFixture::prepareTest(const char testname[]) std::putchar('.'); // Use putchar to write through redirection of std::cout/cerr std::fflush(stdout); } else { - std::cout << classname << "::" << testname << std::endl; + std::cout << classname << "::" << mTestname << std::endl; } return true; } @@ -316,7 +316,7 @@ void TestFixture::run(const std::string &str) try { if (quiet_tests) { std::cout << '\n' << classname << ':'; - REDIRECT; + SUPPRESS; run(); } else @@ -324,15 +324,15 @@ void TestFixture::run(const std::string &str) } catch (const InternalError& e) { ++fails_counter; - errmsg << "InternalError: " << e.errorMessage << std::endl; + errmsg << classname << "::" << mTestname << " - InternalError: " << e.errorMessage << std::endl; } catch (const std::exception& error) { ++fails_counter; - errmsg << "exception: " << error.what() << std::endl; + errmsg << classname << "::" << mTestname << " - Exception: " << error.what() << std::endl; } catch (...) { ++fails_counter; - errmsg << "Unknown exception" << std::endl; + errmsg << classname << "::" << mTestname << " - Unknown exception" << std::endl; } } diff --git a/test/redirect.h b/test/redirect.h index fe8c902da..092e2c9bd 100644 --- a/test/redirect.h +++ b/test/redirect.h @@ -29,6 +29,7 @@ extern std::ostringstream output; * @brief Utility class for capturing cout and cerr to ostringstream buffers * for later use. Uses RAII to stop redirection when the object goes out of * scope. + * NOTE: This is *not* thread-safe. */ class RedirectOutputError { public: @@ -83,10 +84,38 @@ private: std::streambuf *_oldCerr; }; -#define REDIRECT RedirectOutputError redir; do {} while (false) +class SuppressOutput { +public: + /** Set up suppression, flushing anything in the pipes. */ + SuppressOutput() { + // flush all old output + std::cout.flush(); + std::cerr.flush(); + + _oldCout = std::cout.rdbuf(); // back up cout's streambuf + _oldCerr = std::cerr.rdbuf(); // back up cerr's streambuf + + std::cout.rdbuf(nullptr); // disable cout + std::cerr.rdbuf(nullptr); // disable cerr + } + + /** Revert cout and cerr behaviour */ + ~SuppressOutput() { + std::cout.rdbuf(_oldCout); // restore cout's original streambuf + std::cerr.rdbuf(_oldCerr); // restore cerrs's original streambuf + } + +private: + std::streambuf *_oldCout; + std::streambuf *_oldCerr; +}; + +#define REDIRECT RedirectOutputError redir #define GET_REDIRECT_OUTPUT redir.getOutput() #define CLEAR_REDIRECT_OUTPUT redir.clearOutput() #define GET_REDIRECT_ERROUT redir.getErrout() #define CLEAR_REDIRECT_ERROUT redir.clearErrout() +#define SUPPRESS SuppressOutput supprout + #endif diff --git a/test/testprocessexecutor.cpp b/test/testprocessexecutor.cpp index c8d15ba90..de24a3aa0 100644 --- a/test/testprocessexecutor.cpp +++ b/test/testprocessexecutor.cpp @@ -43,7 +43,7 @@ private: * Execute check using n jobs for y files which are have * identical data, given within data. */ - void check(unsigned int jobs, int files, int result, const std::string &data, SHOWTIME_MODES showtime = SHOWTIME_MODES::SHOWTIME_NONE) { + void check(unsigned int jobs, int files, int result, const std::string &data, SHOWTIME_MODES showtime = SHOWTIME_MODES::SHOWTIME_NONE, const char* const plistOutput = nullptr) { errout.str(""); output.str(""); @@ -56,6 +56,9 @@ private: settings.jobs = jobs; settings.showtime = showtime; + if (plistOutput) + settings.plistOutput = plistOutput; + // TODO: test with settings.project.fileSettings; ProcessExecutor executor(filemap, settings, *this); std::vector> scopedfiles; scopedfiles.reserve(filemap.size()); @@ -72,6 +75,7 @@ private: TEST_CASE(deadlock_with_many_errors); TEST_CASE(many_threads); TEST_CASE(many_threads_showtime); + TEST_CASE(many_threads_plist); TEST_CASE(no_errors_more_files); TEST_CASE(no_errors_less_files); TEST_CASE(no_errors_equal_amount_files); @@ -103,7 +107,7 @@ private: // #11249 - reports TSAN errors void many_threads_showtime() { - REDIRECT; + SUPPRESS; check(16, 100, 100, "int main()\n" "{\n" @@ -112,6 +116,19 @@ private: "}", SHOWTIME_MODES::SHOWTIME_SUMMARY); } + void many_threads_plist() { + const char plistOutput[] = "plist"; + ScopedFile plistFile("dummy", plistOutput); + + SUPPRESS; + check(16, 100, 100, + "int main()\n" + "{\n" + " char *a = malloc(10);\n" + " return 0;\n" + "}", SHOWTIME_MODES::SHOWTIME_NONE, plistOutput); + } + void no_errors_more_files() { check(2, 3, 0, "int main()\n" diff --git a/test/testthreadexecutor.cpp b/test/testthreadexecutor.cpp index ca880427e..2e117181a 100644 --- a/test/testthreadexecutor.cpp +++ b/test/testthreadexecutor.cpp @@ -43,7 +43,7 @@ private: * Execute check using n jobs for y files which are have * identical data, given within data. */ - void check(unsigned int jobs, int files, int result, const std::string &data, SHOWTIME_MODES showtime = SHOWTIME_MODES::SHOWTIME_NONE) { + void check(unsigned int jobs, int files, int result, const std::string &data, SHOWTIME_MODES showtime = SHOWTIME_MODES::SHOWTIME_NONE, const char* const plistOutput = nullptr) { errout.str(""); output.str(""); @@ -56,6 +56,9 @@ private: settings.jobs = jobs; settings.showtime = showtime; + if (plistOutput) + settings.plistOutput = plistOutput; + // TODO: test with settings.project.fileSettings; ThreadExecutor executor(filemap, settings, *this); std::vector> scopedfiles; scopedfiles.reserve(filemap.size()); @@ -71,6 +74,7 @@ private: TEST_CASE(deadlock_with_many_errors); TEST_CASE(many_threads); TEST_CASE(many_threads_showtime); + TEST_CASE(many_threads_plist); TEST_CASE(no_errors_more_files); TEST_CASE(no_errors_less_files); TEST_CASE(no_errors_equal_amount_files); @@ -101,7 +105,7 @@ private: // #11249 - reports TSAN errors - only applies to threads not processes though void many_threads_showtime() { - REDIRECT; + SUPPRESS; check(16, 100, 100, "int main()\n" "{\n" @@ -110,6 +114,19 @@ private: "}", SHOWTIME_MODES::SHOWTIME_SUMMARY); } + void many_threads_plist() { + const char plistOutput[] = "plist"; + ScopedFile plistFile("dummy", plistOutput); + + SUPPRESS; + check(16, 100, 100, + "int main()\n" + "{\n" + " char *a = malloc(10);\n" + " return 0;\n" + "}", SHOWTIME_MODES::SHOWTIME_NONE, plistOutput); + } + void no_errors_more_files() { check(2, 3, 0, "int main()\n"