[LU-6372] cppcheck to statically verify change in Gerrit

• Status: Resolved
• Project: Lustre
• Type: Task
• Priority: Minor
• Reporter: Denis Kondratenko (Inactive)
• Assignee: WC Triage
• Resolution: Done

Description

As discussed on LWG meeting in Livermore we want to integrate cppcheck to check patches for community.

At Seagate we found this very useful. Feedback to get that for community at LWG was positive.

So here our first try:
http://review.whamcloud.com/#/c/14087/
http://54.164.238.188/job/lustre_release_cppcheck/16/CppCheck_Report/
http://54.164.238.188/job/lustre_release_cppcheck/

So we would debug it and check that reports are clear.
Meanwhile please review and provide any feedback.
Also please consider new user to get it more friendly.

Issue Links

• is related to: LU-2753 Tracking bug for static code analysis...

Comments

[Roman (Inactive)]

we need "Stream Events" permission for deep integration. may be it is good idea to use non-interactive user for it.

[Andreas Dilger]

Hi Denis, this is definitely an interesting project. We've been doing static code analysis with a different tool, but only manually so far because it doesn't allow the output to be generated easily. I suspect there are different bugs to be found by cppcheck or clang. Our current process for fixing these minor issues is to file a separate bug for each release (e.g. one for 2.8.0 so that it can be marked with "Fix Version" for that release and closed as part of the release) and then push multiple patches for the various issues found by static analysis.

If you want to take it a step further, it would be possible to add static analysis for all new patches submitted to Gerrit in a similar manner to HPDD Checkpatch, which is using the Gerrit REST API from a remote system to add comments for individual lines in patches. The script to implement the parsing of the checkpatch.pl output and posting as per-line comments to Gerrit is in contrib/scripts/gerrit_checkpatch.py. If this of interest to you, we can create a separate Gerrit account for this kind of automated analysis (e.g. "Seagate CPPCheck" or whatever you want). I think that would be more useful than referencing a remote website, since it puts the comments into context on the patch itself, and makes it easier to verify that the issues have been fixed when the patch is refreshed.

[Denis Kondratenko (Inactive)]

Hi Andreas,

we need some Static Analyzer Gerrit account for this activity to get events from Gerrit.

>Our current process for fixing these minor issues
OK. That is good for planned work - run - file and fix.
Goal of this one - find issues that were introduced by patch.

But problem is that cppcheck is not accurate. The scope of static analyzer could be wider than changes. Like you remove variable from the expression and static analyzer would fire up error in the beginning of the file.
So maybe commenting in each line is not good idea as it would comment code that wasn't touched by patch itself ...

What probably could be done if needed is to store issues that cppcheck found for the file and report only issues that are different with a patch.

So we need find out right process for this tool. Because looking to number of comments inside Gerrit - it is really easy to miss that report.

Also it might be a good process to fix all issues from static analyzer even if they aren't related to patch itself - that would allow to progress faster with smaller portions without opening new ticket.

How we could get a user?
What your suggestion about the process ?

[Denis Kondratenko (Inactive)]

morrone can you also suggest something?

[Andreas Dilger]

Denis, I think you should be able to create the account for this yourself, just by registering a new email address and naming it what you want (you probably don't want to use your regular email address, since it could be added to hundreds of patches). I'd recommend something more specific than "Static Analyzer" because there may be multiple tools like this in the future. Instead, I'd recommend "Seagate CPPCheck" or "CPPCheck Static Analyzer" or similar. If you have problems with this, please let me know.

I agree that cppcheck is not completely accurate, which is why this tool should NOT automatically mark a patch with +/-1, but only add comments to the patch, as is done with HPDD Checkpatch. Before the tool is run automatically on every new patch, the majority of existing defects should be fixed and/or excluded via separate series of patches, so that only issues related to each new patch are flagged. Otherwise, it could cause the scope of a patch to continue to grow as more issues are fixed. One option is to run cppcheck on the whole file, but only include errors within the scope (e.g. @@ -3979,6 +3979,10 @@ ) of each patch hunk. That would allow finding and fixing existing defects within code that is being modified without including all the defects in the whole file.

As for Stream Events permission - we don't even use this for HPDD Checkpatch, because events can be missed if the reviewing machine can't communicate with Gerrit for some reason or if the script is stopped, crashes, whatever. Instead, the script polls Gerrit for new changes that have not been reviewed, which doesn't impose any significant overhead. The contrib/scripts/gerrit-checkpatch.py script is fairly well written and easily understood, so I'd recommend using it as a starting point for integration once you get that far. It is already designed to parse the output from other tools, so it may be a trivial matter to adapt it to run against the cppcheck output.

[John Hammond]

> Denis, I think you should be able to create the account for this yourself, just by registering a new email address and naming it what you want (you probably don't want to use your regular email address, since it could be added to hundreds of patches). I'd recommend something more specific than "Static Analyzer" because there may be multiple tools like this in the future. Instead, I'd recommend "Seagate CPPCheck" or "CPPCheck Static Analyzer" or similar. If you have problems with this, please let me know.

The last time I looked at this I couldn't figure out how to authenticate with gerrit (to use the REST API) except to use password authentication. Setting up a password for an account will require the help of a gerrit administrator. You need to create the account first.

> I agree that cppcheck is not completely accurate, which is why this tool should NOT automatically mark a patch with +/-1, but only add comments to the patch, as is done with HPDD Checkpatch. Before the tool is run automatically on every new patch, the majority of existing defects should be fixed and/or excluded via separate series of patches, so that only issues related to each new patch are flagged. Otherwise, it could cause the scope of a patch to continue to grow as more issues are fixed. One option is to run cppcheck on the whole file, but only include errors within the scope (e.g. @@ -3979,6 +3979,10 @@ ) of each patch hunk. That would allow finding and fixing existing defects within code that is being modified without including all the defects in the whole file.

Any approach that requires us to first fix all known issues then start adding warnings will never catch up. Perhaps just filter the warning to those within 15 lines of a changed line. Or to the same function if that can be done easily. It's not perfect but it's something.

[Roman (Inactive)]

Few comments about current implementation:

1. cppcheck is oriented to work with full files. current steps are:
   • checkout gerrit revision
   • select touched files
   • run cppcheck on selected files and store results
   • checkout previous commit
   • do cppcheck on same file set, save results
   • get only new results for report generation
     Decreasing context for code analyzers is not so good and I prefer to keep current schema.
2. Current algorithm decreases number of warnings and often developer see 0-5 warnings, so there is no issue "first fix all known issue"
3. Next improving is smart calculation of changes include code line(or code line hash) for avoid including warnings after code insertion/removing

Stream Events permission - we don't even use this for HPDD Checkpatch, because events can be missed if the reviewing machine can't communicate with Gerrit for some reason or if the script is stopped, crashes, whatever.

I agree, that outages could be issue. But it is not very important service and may be we could start integration step-by-step and start REST usage when we observe how outage disturb us? Now we use from jenkins not only notification about event but also cloned repo with checkout-ed revision.

[Denis Kondratenko (Inactive)]

Who could grant "Stream Events" capability to this user:

Username    cppcheck
Full Name    CPPCheck
Account ID    489

?

[Christopher Morrone]

I like that your script is running CPPCheck on the before and after versions of the file, and only reporting the new issues that were caused by the patch.

[Denis Kondratenko (Inactive)]

jhammond who could help us with Gerrit permissions ?

[Peter Jones]

Denis

I have made the request to out sysadmin team. This request will need to be balanced against other requests.

Peter

[Denis Kondratenko (Inactive)]

Thanks Peter!

[Denis Kondratenko (Inactive)]

Hi @pjones , sorry for disturbing , is there any progress to get permissions? we have Roman to work on this now so we could get it done before he is gone...

[Peter Jones]

Hi Denis

I am afraid that I do not have an update yet but I will certainly update here as soon as I hear something. If Roman is anxious to start work on this then perhaps it makes sense to use an interim solution like periodically polling the repository (hourly, say).

Peter

[Denis Kondratenko (Inactive)]

Hi Peter,

any news on getting permissions?

Meanwhile Roman did full, regular report from cppcheck:
http://54.164.238.188/job/lustre_master_regular_cppcheck/

Thanks,
Denis

[Andreas Dilger]

I'm looking into enabling the stream-events permission for your account. There was some uncertainty about what events would be visible with this permission, because we have both public and private projects hosted in Gerrit.

I looked at the reports available at http://54.164.238.188/job/lustre_master_regular_cppcheck/ and they are definitely interesting. It seems you are connecting a full Jenkins instance for the cppcheck? It looks like you are polling on 10min intervals already? It seems that one useful view for each build would be available from the URL:

http://54.164.238.188/job/lustre_master_regular_cppcheck/61/cppcheckResult/source.all/?before=5&after=5&states=new

since we want to focus on new problems, but it would be nice to also see existing problems in the area of code being modified. Unfortunately, it looks like the "unchanged" state shows all of the issues in the tree and not just those related to the patch.

How do you plan to post the output from cppcheck back into Gerrit for each patch? Using per-line annotations like contrib/scripts/gerrit_checkpatch.py (which is also sent to patch watchers via email like all Gerrit comments) is definitely preferable to just having a URL to the test output. I suspect many developers won't actually go to the cppcheck site each time if they don't have to, but any issues will be clearly visible to both the patch submitter and the reviewers if they are inline comments in the patch.

[Roman (Inactive)]

Hi Andreas,

full Jenkins instance for the cppcheck

yes, it comparable simple and allows to just copy our internal setup for community needs

it looks like you are polling on 10min intervals already

yes, current setup checks new commit in master branch every 10 minutes. gerrit pre-commit changes are not observing

one useful view for each build would be available from the URL:
http://54.164.238.188/job/lustre_master_regular_cppcheck/61/cppcheckResult/source.all/?before=5&after=5&states=new
since we want to focus on new problems, but it would be nice to also see existing problems in the area of code being modified
Unfortunately, it looks like the "unchanged" state shows all of the issues in the tree and not just those related to the patch.

It's master branch check report. We agreed with Denis that master check just shows high level picture about full project, and warning diffs are not goal there( from my vision diff is especially interesting on pre-commit stage). I could add diff view also for master branches.

Using per-line annotations like contrib/scripts/gerrit_checkpatch.py (which is also sent to patch watchers via email like all Gerrit comments) ... visible to both the patch submitter and the reviewers if they are inline comments in the patch.

I'm planing to implement this workflow in near time.

[Denis Kondratenko (Inactive)]

Hi Andreas,

from your comment about events I didn't understand - could it be done or should we seek for other approaches?

Thanks,
Denis

[Christopher Morrone]

They do not yet know if events will be enabled. Andreas was just explaining why gerrit events need to be investigated before they can tell us if it is possible. More time is needed to do that investigation.

[Minh Diep]

Hi Denis,

I have granted username 'cppcheck' streaming permission. Please check and let me know how it goes.

[Andreas Dilger]

The required streaming permission was granted to the "cppcheck" user, but I don't think anything was ever done with this. At least there has not been anything shared publicly.

For future reference, the page http://wiki.lustre.org/Simple_Gerrit_Builder_Howto describes a simple polling mechanism to track patches in Gerrit that can be used by any user without special permissions, and is currently in use for the HPDD Checkpatch and Misc Code Checks Robot static analysis tools.