Add feature: Cache check results for future runs: by nirbar · Pull Request #799 · cppcheck-opensource/cppcheck

nirbar · 2016-05-24T10:51:40Z

Add command line flag: --cache=path/to/cache.xml
Each check is cached along side the file's pre-processed size, code hash (SHA-512), configuration, check results
In subsequent runs, if a file with the same path, size, hash is found in cache- report the cached results rather than running the full check again.
File hash and size are calculated on preprocessed code. This ensures that the file hasn't changed as well as the included files.
Results are cached disregarding uniquity. This allows subsequent runs with different configuration-sets to have all the results available. Report output is still unique- just the cache isn't.
File path must be exact across runs. (either use same relative paths or same full path)

- Add command line flag: --cache=path/to/cache.xml - Each check is cached along side the check file's size, preprocessed code hash (SHA-512), configuration, check results - In subsequent runs, if a file with the same path, size, hash is found in cache- report the cached results rather than running the full check again. - File hash and size are calculated on preprocessed code. This ensures that the file hasn't changed as well as the included files. - Results are cached disregarding uniquity. This allows subsequent runs with different configuration-sets to have all the results available. Report output is still unique- just the cache isn't. - File path must be exact across runs. (either use same relative paths or same full path)

- Each check is cached along side the file's pre-processed size, code hash (SHA-512), configuration, check results - In subsequent runs, if a file with the same path, size, hash is found in cache- report the cached results rather than running the full check again. - File hash and size are calculated on preprocessed code. This ensures that the file hasn't changed as well as the included files. - Results are cached disregarding uniquity. This allows subsequent runs with different configuration-sets to have all the results available. Report output is still unique- just the cache isn't. - File path must be exact across runs. (either use same relative paths or same full path)

danmar · 2016-05-24T15:03:14Z

I don't like that cryptopp dependency is added.. please remove that.

We don't need to put this in cppcheck as far as I see. It is possible to use make to only check changed source files, isn't it?

danmar · 2016-05-24T15:04:29Z

- * That is very sensitive to the operating system, hardware, compiler and runtime!
- * The code is not meant for production environment, it's using functions not whitelisted for usage in a signal handler function.
- */
+    /*


It is hard to read the diff when you are making lots of redundant indentation changes like this one. Please remove all redundant indentation changes.

nirbar · 2016-05-24T15:10:30Z

The first commit had many whitespace changes, so in the 2nd commit I reverted the whitespaces and kept only the functional change.
Cryptopp is used to produce a SHA-512 hash of the code (after preprocessing). Testing the modification time doens't behave well on build environments (all code may be clean-pulled on every build). Do you have another hashing algorithm that is unique enough without use of an external library?

danmar · 2016-05-24T15:37:19Z

cryptopp is a quite large dependency just to calculate a checksum. We just need a function as far as I see. We have the TokenList::calculateChecksum btw it is very simplistic so feel free to update that.

danmar · 2016-05-24T16:59:20Z

Testing the modification time doens't behave well on build environments (all code may be clean-pulled on every build)

so you have the same problem with compilation. But maybe that is much faster anyway.

I don't like to put this in Cppcheck. It's like a workaround for your build environment. If some external tool can be used I'd prefer that.

But at the same time .. maybe this could be used for distributed analysis also? I think the cache file should contain FileInfo data also if possible so we can get whole-program-analysis.

danmar · 2016-05-24T17:02:09Z

            }

+            // Cache file (--cache=)
+            else if (std::strncmp(argv[i], "--cache=", strlen("--cache=")) == 0) {


strlen=>sizeof

nirbar · 2016-05-25T10:51:50Z

Checking changes by midification time is VERY error prune in this case. It should be checked for the source file and for every file it recursively includes, and then for every file that the included' files include... Calculating a unique-enough hash plus the code size is much safer.

seb777 · 2016-05-25T13:47:44Z

@nirbar
Maybe, cryptopp dependency can be replaced easily with the C++11 std::hash.
Also, I'm not sure you need a base64 encoder.

danmar · 2016-05-27T13:20:50Z

Checking changes by midification time is VERY error prune in this case. It should be checked for the source file and for every file it recursively includes, and then for every file that the included' files include... Calculating a unique-enough hash plus the code size is much safer.

That is how make works. I would not say that make works badly. For users that can use modification time, we could create a script in cppcheck/tools that generates a makefile. And to run incremental analysis, the normal make is used.

But I guess that you need a hash-based solution. I don't think it's optimal for performance reasons to calculate the hash after the cppcheck-preprocessing. At least it's possible, as far as I see, to create a script in tools/cppcheck that calculate such hashes and run Cppcheck on modified files and then creates a full report for all files. Do you think this is a bad choice for some reason?

Maybe, cryptopp dependency can be replaced easily with the C++11 std::hash.

that would be better. we don't fully require C++11 yet so I am not sure if that is available yet though. But if it isn't then maybe we can temporarily use something like djb2 here http://www.cse.yorku.ca/~oz/hash.html

nirbar · 2016-05-27T13:47:46Z

Calculating hash is indeed time consuming but much less so comparing to a full scan. On tests I ran on a pretty large code base, average full scan took 40 minuets and an average incremental run with no changes to code took 3 minutes.

I am not too particular about which hash to use. I chose SHA-512 because it is widely used and known to be quite unique. If the hash you're referring to can be trusted with that than lets go for it.

danmar · 2016-05-28T18:55:30Z

basically.. I am very careful about adding new command line flags.

yes we need to allow incremental analysis.

I don't want to add a special flag that is only intended for hash-based incremental analysis. I would like to see how "make" like incremental analysis are done using this flag also. And I think this flag should allow distributed analysis using standard tools (for instance dispy/rsh/etc).

nirbar · 2016-05-29T05:18:37Z

Distributes analysis is much different- you need to start processes on
remote machines and open a communication channel with them (message queue,
TCP/IP or sockets to name a few mechanisms).
As a starter I would work on multi-process (multi-core?) support which
means starting processes on the same machine. This would also require
implementing the communication mechanism.
Once achieved, you can add a mechanism to start processes on remote
machines and use the communication mechanism with the remote processes.

During builds, an object file is generated for each source file- this part
is what you don't currently have in CppCheck. With the cache, you get an
XML element per file, which you can communicate to the host process over
the aforementioned communication line.
Incremental compilation usually compares the modification time of the
object file to the pre-processed modification time of the source file and
it's included headers. I oppose to time stamp comparison because in many
scenarios it forces full build even though an incremental builds could have
been used. For many of my clients, incremental builds is not an option
simply because of that.

About the cache feature- personally, I don't see a reason not to add it;
It's a value adding feature that- correct me if I'm wrong- adds no risk of
missing reports or of false alarms, and does that much faster than full
scan. Of course, this is your decision.

Nir Bar
Panel::Software Architecture and Development
Mobile: +972-54-2490321
Mail: nir.bar@panel-sw.co.il nir.bar@panel-sw.co.il
Web_: www.panel-sw.co.il http://www.panel-sw.co.il_

- C++ On Windows, Linux and Embedded Platforms*
- WiX & InstallShield*
- Build & Continuous Integration*

2016-05-28 21:55 GMT+03:00 Daniel Marjamäki notifications@github.com:

basically.. I am very careful about adding new command line flags.

yes we need to allow incremental analysis.

I don't want to add a special flag that is only intended for hash-based
incremental analysis. I would like to see how "make" like incremental
analysis are done using this flag also. And I think this flag should allow
distributed analysis using standard tools.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#799 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AD3IrAJWs01PRpbeNo6TGNpUrwqyFzUbks5qGI-pgaJpZM4IlWuc
.

danmar · 2016-05-29T06:50:54Z

I am not opposed to your hash-based incremental analysis. I want full flexibility so it will work for more use cases.

There are already tools for distributed computing and timestamp-based incremental builds.. I do not want to put that functionality into Cppcheck also.

Distributed analysis:
User has many computers. By using some tool (rsh,dispy,etc) those run Cppcheck on different files and "object files" are created for all files. Then all the "object files" are transferred to a single computer.
Now the user has a directory full of "object files" and wants to "link" those into a report. Cppcheck should have a flag for linking those.

timestamp-based incremental builds:
The user creates a Makefile. Using --cache, the user can create an "object file" for each source file.
Cppcheck should have a flag for linking those. Then the user can use "make" to get a report.

This "linking" should not read any sourcefiles, it should just read the xml files and create the report.

danmar · 2016-05-29T06:59:27Z

Distributes analysis is much different- you need to start processes on remote machines and open a communication channel with them (message queue, TCP/IP or sockets to name a few mechanisms). As a starter I would work on multi-process (multi-core?) support which means starting processes on the same machine. This would also require implementing the communication mechanism. Once achieved, you can add a mechanism to start processes on remote machines and use the communication mechanism with the remote processes.

I do no want to implement these mechanisms into Cppcheck.

During builds, an object file is generated for each source file- this part is what you don't currently have in CppCheck. With the cache, you get an XML element per file

Exactly. So I hope we can pretend that the cache file is an object file.

Incremental compilation usually compares the modification time of the object file to the pre-processed modification time of the source file and it's included headers. I oppose to time stamp comparison because in many scenarios it forces full build even though an incremental builds could have been used.

There is no technical reason to disallow this for those who wants it.

For many of my clients, incremental builds is not an option simply because of that.

ok I am fine with that.

About the cache feature- personally, I don't see a reason not to add it; It's a value adding feature that- correct me if I'm wrong- adds no risk of missing reports or of false alarms, and does that much faster than full scan.

I am especially careful about adding command line flags.

Imagine that we add hash-based object files now. And you start distributing this to your clients.

And then later we want to have distributed analysis and we must change the object files.

It will be a pain for you if I change the command line flags and object files later.

danmar · 2016-05-30T07:52:32Z

have you considered to include configuration also in the hash;

If a different Cppcheck version is used the cache file should probably be rewritten.
If the command line flags are changed (--std, --library, --enable, etc) it might be a good idea to rewrite the cache file.
If some cfg file that is used is changed it's probably a good idea to rewrite the cache file.

I don't think we must implement that to start with. but it should be considered.

nirbar · 2016-05-30T08:05:58Z

I think it is risky to let the user perform all the distribution work- the
user will have to ensure all files (including system included headers) are
the same, and that they have equal time stamps. That probably changes with
the installation time of the system and latest code check out. In that
case, it is even more valuable to have a hash-based caching.

About configuration management of cache- these indeed should be added.

Nir Bar
Panel::Software Architecture and Development
Mobile: +972-54-2490321
Mail: nir.bar@panel-sw.co.il nir.bar@panel-sw.co.il
Web_: www.panel-sw.co.il http://www.panel-sw.co.il_

- C++ On Windows, Linux and Embedded Platforms*
- WiX & InstallShield*
- Build & Continuous Integration*

2016-05-30 10:52 GMT+03:00 Daniel Marjamäki notifications@github.com:

have you considered to include configuration also in the hash;

If a different Cppcheck version is used the cache file should
probably be rewritten.

If the command line flags are changed (--std, --library, --enable,
etc) it might be a good idea to rewrite the cache file.

If some cfg file that is used is changed it's probably a good idea
to rewrite the cache file.

I don't think we must implement that to start with. but it should be
considered.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#799 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AD3IrGiGLNij8NJgWkasQHFl5fBXinPXks5qGpdIgaJpZM4IlWuc
.

thomasjfox · 2016-05-30T08:39:00Z

On Monday, 30. May 2016 01:06:01 Nir Bar wrote:

I think it is risky to let the user perform all the distribution work- the
user will have to ensure all files (including system included headers)
are the same, and that they have equal time stamps. That probably changes
with the installation time of the system and latest code check out. In
that case, it is even more valuable to have a hash-based caching.

distcc, a compile job distribution system, solved the header expansion
problem in the early days by doing the C preprocessor stage on the "master"
machine before sending the file to the client machine.

Homepage:
https://github.com/distcc/distcc

There's also a "pump" mode in distcc that sends the headers
files to the clients, more details about the workings here:
https://github.com/distcc/distcc/blob/master/README.pump

Interesting is the last paragraph in README.pump:

The pump mode was developed to be used with large clusters of distcc
servers, providing hundreds of CPUs. With versions of gcc >= 4.1.1,
pump mode will probably not show major performance gains using
clusters of less than ten CPUs. The preprocessor running on the

workstation is fast enough to keep that many machines busy.

Thomas

nirbar · 2016-05-30T09:03:00Z

That's what I had in mind too.
However, if the user is responsible to do the job distribution rather than
cppcheck managing remote process startup and communication, then that's not
an option.

Nir Bar
Panel::Software Architecture and Development
Mobile: +972-54-2490321
Mail: nir.bar@panel-sw.co.il nir.bar@panel-sw.co.il
Web_: www.panel-sw.co.il http://www.panel-sw.co.il_

- C++ On Windows, Linux and Embedded Platforms*
- WiX & InstallShield*
- Build & Continuous Integration*

2016-05-30 11:39 GMT+03:00 Thomas Jarosch notifications@github.com:

On Monday, 30. May 2016 01:06:01 Nir Bar wrote:

I think it is risky to let the user perform all the distribution work-
the
user will have to ensure all files (including system included headers)
are the same, and that they have equal time stamps. That probably changes
with the installation time of the system and latest code check out. In
that case, it is even more valuable to have a hash-based caching.

distcc, a compile job distribution system, solved the header expansion
problem in the early days by doing the C preprocessor stage on the
"master"
machine before sending the file to the client machine.

Homepage:
https://github.com/distcc/distcc

There's also a "pump" mode in distcc that sends the headers
files to the clients, more details about the workings here:
https://github.com/distcc/distcc/blob/master/README.pump

Interesting is the last paragraph in README.pump:

The pump mode was developed to be used with large clusters of distcc
servers, providing hundreds of CPUs. With versions of gcc >= 4.1.1,
pump mode will probably not show major performance gains using
clusters of less than ten CPUs. The preprocessor running on the

workstation is fast enough to keep that many machines busy.

Thomas

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#799 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AD3IrAfuj-Ob273zS5UH54ITY0boL-Inks5qGqIrgaJpZM4IlWuc
.

…ple source files. This somewhat reduces unique repoting, but follows full-scan convention.

danmar · 2016-06-02T08:20:40Z

yes I agree the source code to analyse should be sent from the host to the client machine. cfg files and rule files should also be passed from the host to the client.

How we do that is a future problem imho. I do not think it's impossible.

Then as I see it, when the client has all source files, headers, cfg, etc then it should be possible to do the analysis on the client machine using this cache functionality.

then the cache file can be returned to the host.

when all cache files are returned to host then it should not be hard for the host to generate a full analysis report.

danmar · 2016-06-02T08:26:40Z

If we forget about cfg files and rule files for a moment.. and since I don't recommend that system headers are included let's forget those too.. it should then be possible to use distcc in pump mode right?

nirbar · 2016-06-02T09:13:02Z

I'm not that familiar with distcc...

Nir Bar
Panel::Software Architecture and Development
Mobile: +972-54-2490321
Mail: nir.bar@panel-sw.co.il nir.bar@panel-sw.co.il
Web_: www.panel-sw.co.il http://www.panel-sw.co.il_

- C++ On Windows, Linux and Embedded Platforms*
- WiX & InstallShield*
- Build & Continuous Integration*

2016-06-02 11:26 GMT+03:00 Daniel Marjamäki notifications@github.com:

If we forget about cfg files and rule files for a moment.. and since I
don't recommend that system headers are included let's forget those too..
it should then be possible to use distcc in pump mode right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#799 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AD3IrORwulxHTqWnJNAQ3nBXjFVg1JPcks5qHpPHgaJpZM4IlWuc
.

danmar · 2016-08-07T16:12:00Z

I am starting to think that we can add this.

Can you create a commit that does not break appveyor and travis?

See https://travis-ci.org/danmar/cppcheck/builds/134163827

danmar · 2016-10-29T20:05:39Z

I am working on this. See --cppcheck-build-dir in git head. It is unfinished and under work but it has the caching of error messages functionality.

nirbar added 2 commits May 24, 2016 13:40

danmar reviewed May 24, 2016
View reviewed changes

Report cached errors in headers multiple times when included in multi…

f172bc1

…ple source files. This somewhat reduces unique repoting, but follows full-scan convention.

danmar closed this Oct 29, 2016

Uh oh!

Conversation

nirbar commented May 24, 2016

Uh oh!

danmar commented May 24, 2016

Uh oh!

danmar May 24, 2016

Choose a reason for hiding this comment

Uh oh!

nirbar commented May 24, 2016

Uh oh!

danmar commented May 24, 2016

Uh oh!

danmar commented May 24, 2016

Uh oh!

danmar May 24, 2016

Choose a reason for hiding this comment

Uh oh!

nirbar commented May 25, 2016

Uh oh!

seb777 commented May 25, 2016

Uh oh!

danmar commented May 27, 2016

Uh oh!

nirbar commented May 27, 2016

Uh oh!

danmar commented May 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirbar commented May 29, 2016

Uh oh!

danmar commented May 29, 2016

Uh oh!

danmar commented May 29, 2016

Uh oh!

danmar commented May 30, 2016

Uh oh!

nirbar commented May 30, 2016

Uh oh!

thomasjfox commented May 30, 2016

Interesting is the last paragraph in README.pump:

workstation is fast enough to keep that many machines busy.

Uh oh!

nirbar commented May 30, 2016

Interesting is the last paragraph in README.pump:

workstation is fast enough to keep that many machines busy.

Uh oh!

danmar commented Jun 2, 2016

Uh oh!

danmar commented Jun 2, 2016

Uh oh!

nirbar commented Jun 2, 2016

Uh oh!

danmar commented Aug 7, 2016

Uh oh!

danmar commented Oct 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danmar commented May 28, 2016 •

edited

Loading