Skip to content

Fix #11750 (refactor ctu-info to generate fewer artifacts on disk) #6778

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

Merged
danmar merged 5 commits into from
Oct 7, 2024
Merged

Fix #11750 (refactor ctu-info to generate fewer artifacts on disk) #6778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
071efaf
Fix #11750 (refactor ctu-info to generate fewer artifacts on disk)
danmar Sep 7, 2024
f694c93
use std::string for ctuinfo in GUI
danmar Sep 27, 2024
6de65af
2
danmar Sep 27, 2024
d91b2ea
tmp_path
danmar Sep 27, 2024
980aeac
rename
danmar Sep 27, 2024
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
13 changes: 8 additions & 5 deletions addons/cppcheckdata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1688,11 +1688,14 @@ def reportError(location, severity, message, addon, errorId, extra='', columnOve
EXIT_CODE = 1

def reportSummary(dumpfile, summary_type, summary_data):
# dumpfile ends with ".dump"
ctu_info_file = dumpfile[:-4] + "ctu-info"
with open(ctu_info_file, 'at') as f:
msg = {'summary': summary_type, 'data': summary_data}
f.write(json.dumps(msg) + '\n')
msg = {'summary': summary_type, 'data': summary_data}
if '--cli' in sys.argv:
sys.stdout.write(json.dumps(msg) + '\n')
else:
# dumpfile ends with ".dump"
ctu_info_file = dumpfile[:-4] + "ctu-info"
with open(ctu_info_file, 'at') as f:
f.write(json.dumps(msg) + '\n')


def get_path_premium_addon():
Expand Down
2 changes: 2 additions & 0 deletions addons/test/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def convert_json_output(raw_json_strings):
"""Convert raw stdout/stderr cppcheck JSON output to python dict."""
json_output = {}
for line in raw_json_strings:
if line.startswith('{"summary":'):
continue
try:
json_line = json.loads(line)
# json_output[json_line['errorId']] = json_line
Expand Down
18 changes: 16 additions & 2 deletions cli/cppcheckexecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ namespace {
return !mCriticalErrors.empty();
}

const std::string& getCtuInfo() const {
return mCtuInfo;
Copy link
Collaborator

@firewave firewave Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit strange that this lives in the just remotely related logger. I wonder if all the logic should live in its own object and cleanup should be RAII-based and such.

That would be similar to what I tried in #6634 which comes up short in making it a local object because it needs to intercept the logged error at some point (which also applies to the code here). So adding some kind of hook into the error logging which std, XML and CTU (and even SARIF) could use would make a lot of sense.

Copy link
Owner Author

@danmar danmar Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I understand these comments fully.

I do feel the ctu info belongs in cli and gui rather than lib. it's aggregated info from all threads. For me it's reasonable that CppcheckExecutor owns the data (directly or indirectly) however I can agree that the functionality does not technically belong in StdLogger actually.

We also have the mActiveCheckers and mCriticalErrors. Those do not really fit very well in the StdLogger neither.

I do not see a very elegant way to insert hooks. We could add one more ErrorLogger that owns the StdLogger?

 class AllLogger : public ErrorLogger {
 private:
     void reportErr(const ErrorMessage& errmsg) {
          ... handle active checkers, critical errors, ctu, ...

          // pass remaining errors to stdLogger
          stdLogger.reportErr(errmsg);
     }
     StdLogger stdLogger;
};

Do you have better ideas..?

Copy link
Owner Author

@danmar danmar Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I still suggest to put the condition in StdLogger that ensures the output is formatted with sarif/text/xml so it will take a ErrorMessage input and output the proper string..

Copy link
Collaborator

@firewave firewave Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, again something in this file turns into a grab-bag after things got sorted out at some point.

This is not something we need to solve in this PR. I will see if I am put together a PR with the proposed hook based on the previously linked cleanup.

}

private:
/**
* Information about progress is directed here. This should be
Expand Down Expand Up @@ -170,9 +174,14 @@ namespace {
std::set<std::string> mActiveCheckers;

/**
* True if there are critical errors
* List of critical errors
*/
std::string mCriticalErrors;

/**
* CTU information
*/
std::string mCtuInfo;
Copy link
Collaborator

@firewave firewave Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safe in terms of memory usage? I have not really understood what this is doing yet (maybe add some explanation to the PR) but appears to accumulate the the data in the memory and this could be megabytes in size or even much, much more.

Copy link
Owner Author

@danmar danmar Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should check it but my hypothesis is that the ctu info will not be huge.

Copy link
Owner Author

@danmar danmar Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about memory usage, here is one small test case: cppcheck/test/cli/whole-program

  • the files whole1.c and whole2.c are 94 and 64 bytes.
  • the ctu-info that is generated for those are 220 bytes and 227 bytes.

in this test case ~160 bytes source code generates ~450 bytes ctu-info

I don't think that memory usage will be a large issue. If we pretend that scanning a large project with 160MB source code would require 450MB memory for ctu info.

Copy link
Collaborator

@firewave firewave Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

But that has to be constructed as a string and is required to be continuous memory.

And we also need to profile that. That sounds like it might slow down things quite a bit. Maybe we might need both modes?

};
}

Expand Down Expand Up @@ -292,7 +301,7 @@ int CppCheckExecutor::check_internal(const Settings& settings) const
#endif
}

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

if (settings.severity.isEnabled(Severity::information) || settings.checkConfiguration) {
const bool err = reportSuppressions(settings, suppressions, settings.checks.isEnabled(Checks::unusedFunction), mFiles, mFileSettings, stdLogger);
Expand Down Expand Up @@ -430,6 +439,11 @@ void StdLogger::reportErr(const ErrorMessage &msg)
return;
}

if (msg.severity == Severity::internal && msg.id == "ctuinfo") {
mCtuInfo += msg.shortMessage() + "\n";
return;
}

if (ErrorLogger::isCriticalErrorId(msg.id) && mCriticalErrors.find(msg.id) == std::string::npos) {
if (!mCriticalErrors.empty())
mCriticalErrors += ", ";
Expand Down
7 changes: 5 additions & 2 deletions gui/checkthread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,11 @@ void CheckThread::check(const Settings &settings)
start();
}

void CheckThread::analyseWholeProgram(const QStringList &files)
void CheckThread::analyseWholeProgram(const QStringList &files, const std::string& ctuInfo)
{
mFiles = files;
mAnalyseWholeProgram = true;
mCtuInfo = ctuInfo;
start();
}

Expand All @@ -131,12 +132,14 @@ void CheckThread::run()

if (!mFiles.isEmpty() || mAnalyseWholeProgram) {
mAnalyseWholeProgram = false;
std::string ctuInfo;
ctuInfo.swap(mCtuInfo);
qDebug() << "Whole program analysis";
std::list<FileWithDetails> files2;
std::transform(mFiles.cbegin(), mFiles.cend(), std::back_inserter(files2), [&](const QString& file) {
return FileWithDetails{file.toStdString(), 0};
});
mCppcheck.analyseWholeProgram(mCppcheck.settings().buildDir, files2, {});
mCppcheck.analyseWholeProgram(mCppcheck.settings().buildDir, files2, {}, ctuInfo);
mFiles.clear();
emit done();
return;
Expand Down
4 changes: 3 additions & 1 deletion gui/checkthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ class CheckThread : public QThread {
/**
* @brief Run whole program analysis
* @param files All files
* @param ctuInfo Ctu info for addons
*/
void analyseWholeProgram(const QStringList &files);
void analyseWholeProgram(const QStringList &files, const std::string& ctuInfo);

void setAddonsAndTools(const QStringList &addonsAndTools) {
mAddonsAndTools = addonsAndTools;
Expand Down Expand Up @@ -143,6 +144,7 @@ class CheckThread : public QThread {

QStringList mFiles;
bool mAnalyseWholeProgram{};
std::string mCtuInfo;
QStringList mAddonsAndTools;
QStringList mClangIncludePaths;
QList<SuppressionList::Suppression> mSuppressions;
Expand Down
7 changes: 6 additions & 1 deletion gui/threadhandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ void ThreadHandler::clearFiles()
mLastFiles.clear();
mResults.clearFiles();
mAnalyseWholeProgram = false;
mCtuInfo.clear();
mAddonsAndTools.clear();
mSuppressions.clear();
}
Expand Down Expand Up @@ -102,6 +103,8 @@ void ThreadHandler::check(const Settings &settings)
addonsAndTools << s;
}

mCtuInfo.clear();

for (int i = 0; i < mRunningThreadCount; i++) {
mThreads[i]->setAddonsAndTools(addonsAndTools);
mThreads[i]->setSuppressions(mSuppressions);
Expand Down Expand Up @@ -164,8 +167,9 @@ void ThreadHandler::removeThreads()
void ThreadHandler::threadDone()
{
if (mRunningThreadCount == 1 && mAnalyseWholeProgram) {
mThreads[0]->analyseWholeProgram(mLastFiles);
mThreads[0]->analyseWholeProgram(mLastFiles, mCtuInfo);
mAnalyseWholeProgram = false;
mCtuInfo.clear();
return;
}

Expand All @@ -187,6 +191,7 @@ void ThreadHandler::stop()
{
mCheckStartTime = QDateTime();
mAnalyseWholeProgram = false;
mCtuInfo.clear();
for (CheckThread* thread : mThreads) {
thread->stop();
}
Expand Down
1 change: 1 addition & 0 deletions gui/threadhandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ protected slots:
int mRunningThreadCount{};

bool mAnalyseWholeProgram{};
std::string mCtuInfo;

QStringList mAddonsAndTools;
QList<SuppressionList::Suppression> mSuppressions;
Expand Down
69 changes: 37 additions & 32 deletions lib/cppcheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ static std::string getDumpFileName(const Settings& settings, const std::string&
return settings.dumpFile;

std::string extension;
if (settings.dump)
if (settings.dump || !settings.buildDir.empty())
extension = ".dump";
else
extension = "." + std::to_string(settings.pid) + ".dump";
Expand Down Expand Up @@ -167,7 +167,7 @@ static void createDumpFile(const Settings& settings,
if (!fdump.is_open())
return;

{
if (!settings.buildDir.empty()) {
std::ofstream fout(getCtuInfoFileName(dumpFile));
}

Expand Down Expand Up @@ -1418,8 +1418,7 @@ void CppCheck::executeAddons(const std::vector<std::string>& files, const std::s
std::string fileList;

if (files.size() >= 2 || endsWith(files[0], ".ctu-info")) {
fileList = Path::getPathFromFilename(files[0]) + FILELIST + std::to_string(mSettings.pid);
Copy link
Collaborator

@firewave firewave Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the unique identifier (pid) from the filename re-introduces the issue this was fixing - parallel executions writing the same file. This is exposed by running the Python tests with python-xdist - see #6696.

Copy link
Owner Author

@danmar danmar Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no!! :-(

Copy link
Contributor

@amacgregor-sym amacgregor-sym Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am seeing the same bug, but not totally sure.

This repo helps reproduce a concurrency problem on my Linux machine
https://github.com/andymacg/cppcheck-addon-concurrency-bug

Copy link
Collaborator

@firewave firewave Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick fix is simply adding back the pid to the filename. Will take a look later - I totally forgot about it.

Copy link
Owner Author

@danmar danmar Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick fix is simply adding back the pid to the filename. Will take a look later - I totally forgot about it.

I have done that with #7082. Feel free to review it..

filesDeleter.addFile(fileList);
fileList = Path::getPathFromFilename(files[0]) + FILELIST;
std::ofstream fout(fileList);
for (const std::string& f: files)
fout << f << std::endl;
Expand All @@ -1428,6 +1427,8 @@ void CppCheck::executeAddons(const std::vector<std::string>& files, const std::s
// ensure all addons have already been resolved - TODO: remove when settings are const after creation
assert(mSettings.addonInfos.size() == mSettings.addons.size());

std::string ctuInfo;

for (const AddonInfo &addonInfo : mSettings.addonInfos) {
if (addonInfo.name != "misra" && !addonInfo.ctu && endsWith(files.back(), ".ctu-info"))
continue;
Expand All @@ -1444,6 +1445,18 @@ void CppCheck::executeAddons(const std::vector<std::string>& files, const std::s

ErrorMessage errmsg;

if (obj.count("summary") > 0) {
if (!mSettings.buildDir.empty()) {
ctuInfo += res.serialize() + "\n";
} else {
errmsg.severity = Severity::internal;
errmsg.id = "ctuinfo";
errmsg.setmsg(res.serialize());
reportErr(errmsg);
}
continue;
}

if (obj.count("file") > 0) {
std::string fileName = obj["file"].get<std::string>();
const int64_t lineNumber = obj["linenr"].get<int64_t>();
Expand Down Expand Up @@ -1482,13 +1495,30 @@ void CppCheck::executeAddons(const std::vector<std::string>& files, const std::s
reportErr(errmsg);
}
}

if (!mSettings.buildDir.empty() && fileList.empty()) {
const std::string& ctuInfoFile = getCtuInfoFileName(files[0]);
std::ofstream fout(ctuInfoFile);
fout << ctuInfo;
}
}

void CppCheck::executeAddonsWholeProgram(const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings)
void CppCheck::executeAddonsWholeProgram(const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings, const std::string& ctuInfo)
{
if (mSettings.addons.empty())
return;

if (mSettings.buildDir.empty()) {
const std::string fileName = std::to_string(mSettings.pid) + ".ctu-info";
FilesDeleter filesDeleter;
filesDeleter.addFile(fileName);
std::ofstream fout(fileName);
fout << ctuInfo;
fout.close();
executeAddons({fileName}, "");
return;
}

std::vector<std::string> ctuInfoFiles;
for (const auto &f: files) {
const std::string &dumpFileName = getDumpFileName(mSettings, f.path());
Expand All @@ -1506,11 +1536,6 @@ void CppCheck::executeAddonsWholeProgram(const std::list<FileWithDetails> &files
const ErrorMessage errmsg = ErrorMessage::fromInternalError(e, nullptr, "", "Bailing out from analysis: Whole program analysis failed");
reportErr(errmsg);
}

if (mSettings.buildDir.empty()) {
for (const std::string &f: ctuInfoFiles)
std::remove(f.c_str());
}
}

Settings &CppCheck::settings()
Expand Down Expand Up @@ -1806,13 +1831,9 @@ bool CppCheck::analyseWholeProgram()
return errors && (mExitCode > 0);
}

unsigned int CppCheck::analyseWholeProgram(const std::string &buildDir, const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings)
unsigned int CppCheck::analyseWholeProgram(const std::string &buildDir, const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings, const std::string& ctuInfo)
{
executeAddonsWholeProgram(files, fileSettings);
if (buildDir.empty()) {
removeCtuInfoFiles(files, fileSettings);
return mExitCode;
}
executeAddonsWholeProgram(files, fileSettings, ctuInfo);
if (mSettings.checks.isEnabled(Checks::unusedFunction))
CheckUnusedFunctions::analyseWholeProgram(mSettings, *this, buildDir);
std::list<Check::FileInfo*> fileInfoList;
Expand Down Expand Up @@ -1876,22 +1897,6 @@ unsigned int CppCheck::analyseWholeProgram(const std::string &buildDir, const st
return mExitCode;
}

void CppCheck::removeCtuInfoFiles(const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings)
{
if (mSettings.buildDir.empty()) {
for (const auto& f: files) {
const std::string &dumpFileName = getDumpFileName(mSettings, f.path());
const std::string &ctuInfoFileName = getCtuInfoFileName(dumpFileName);
std::remove(ctuInfoFileName.c_str());
}
for (const auto& fs: fileSettings) {
const std::string &dumpFileName = getDumpFileName(mSettings, fs.filename());
const std::string &ctuInfoFileName = getCtuInfoFileName(dumpFileName);
std::remove(ctuInfoFileName.c_str());
}
}
}

// cppcheck-suppress unusedFunction - only used in tests
void CppCheck::resetTimerResults()
{
Expand Down
9 changes: 3 additions & 6 deletions lib/cppcheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,8 @@ class CPPCHECKLIB CppCheck : ErrorLogger {
/** Analyze all files using clang-tidy */
void analyseClangTidy(const FileSettings &fileSettings);

/** analyse whole program use .analyzeinfo files */
unsigned int analyseWholeProgram(const std::string &buildDir, const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings);

/** Remove *.ctu-info files */
void removeCtuInfoFiles(const std::list<FileWithDetails>& files, const std::list<FileSettings>& fileSettings); // cppcheck-suppress functionConst // has side effects
/** analyse whole program use .analyzeinfo files or ctuinfo string */
unsigned int analyseWholeProgram(const std::string &buildDir, const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings, const std::string& ctuInfo);

static void resetTimerResults();
static void printTimerResults(SHOWTIME_MODES mode);
Expand Down Expand Up @@ -196,7 +193,7 @@ class CPPCHECKLIB CppCheck : ErrorLogger {
/**
* Execute addons
*/
void executeAddonsWholeProgram(const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings);
void executeAddonsWholeProgram(const std::list<FileWithDetails> &files, const std::list<FileSettings>& fileSettings, const std::string& ctuInfo);

#ifdef HAVE_RULES
/**
Expand Down
Loading