Running a static code analyser on Godot's codebase

[ghost]

Have any of you tried to run a static code analyser on Godot's codebase? GCC offers extra analysis via the -Wextra and -Weffc++ flags, Clang comes with its own static analyzer clang-check and there are a few standalone programs that perform static analysis (like cppcheck).

I tried to use clang-check but it requires a compilation database, which can be generated automatically from CMake but not from scons.

It should be a good way to find memory leaks and poor loops automatically, if anything.

[akien-mga]

@tomreyn set up Coverity scans for Godot at some time: https://scan.coverity.com/projects/godot-engine It has some interesting results, but sadly they're not public, and the handful of devs with access to it don't necessarily have the time/motivation to go through.

If those results could be made public, it would be nice. Godot is not mission critical software so I don't see any issue with having defects exposed to the public, if that can help getting them fixed.

[tomreyn]

https://scan.coverity.com/policy lists some restrictions which may rule out publishing the results (e.g. "You will not publish any findings regarding or resulting from use of the Service or the Software"). Setting up a shared user account may or may not be an option, IANAL. I suggest to talk to them (send mail to: scan hyphon admin at coverity dot com) and find a solution which both you and they will be happy with. This is usually much more useful than trying to parse legalese.

See also https://scan.coverity.com/faq#who-can-have-access on their perspective on how the results should be handled and why.

(Finally, just to be clear, I do not consider whether or not to make those Coverity Scan results available to a wider audience something I should have a choice on, but the project lead - I think this would be reduz + punto? If they'll have a say on this I'll be happy to try to work into that direction.)

This said, cppcheck + clang-check, while equipped with (much) fewer and less trained + tested patterns, are nice picks, too (and, in contrary to Coverity Scan) under liberal licenses).

[Marqin]

Current state (ee37c2f4330e9bd247576b5834cd2660cb520e13). This ignores /drivers directory (excluding gles2 driver).

[ghost]

Although your kindness is appreciated, you should delete the image, since it seems like sharing the results is against their ToS.

[Marqin]

No. It's not findings, but screen of public project overview (that you can see here).

You will not publish any findings regarding or resulting from use of the Service or the Software

It says findings. I'm not publishing those.

[Marqin]

Clang comes with its own static analyzer clang-check

@paper-pauper I think you meant scan-build called ClangAnalyzer command. It doesn't need any DB and I'd ran it few times against Godot, but it hasn't found much critical bugs (btw. Godot compile logs are flooded with warnings so it's hard to find those interesting onces).

[ghost]

@Marqin Whoops, sorry! I just get over-cautious when I see ToSs, that's why I prefer free software.

Well, I suppose that since you have access to it, only you could potentially fix those "defects", which Coverity doesn't want the rest of us to know about. I hope they aren't serious, but I guess some of them could be discovered via Valgrind.

[Marqin]

you have access to it

@bojidar-bg also has access to my fork results, and @reduz or @punto- have access to @tomreyn's

I hope they aren't serious

As I said, I'm not proficent with Godot internals so I cannot say which of it's problems with Reference is false-positive and which is real problem.

[tomreyn]

@Marqin Just FWIW, here's the components which I think reduz (? sorry - memory is fading) configured on 'my' project:

Component name	Pattern	Ignore in analysis
Core	/(core|main)/.*	No
Platform specific code	/platform/.*	No
Scene code	/scene/.*	No
Servers	/servers/.*	No
Third party drivers	/drivers/(?!gles2)(.*)	Yes
GLES driver	/drivers/gles2/.*	No
Modules - GDScript	/modules/gdscript/.*	No
Modules - GridMap	/modules/gridmap/.*	No
Modules - other	/modules/(?!gridmap|gdscript)(.*)	No
Unit tests	/bin/tests/.*	No
Tools - Collada	/tools/collada/.*	No
Tools - Editor	/tools/editor/.*	No
Tools - other	/tools/(?!collada|editor)(.*)	No

[hubbyist]

As all "Ignore in analysis" checks are "No", Nothing is "Ignored in analysis" as it seems. In combination with defect density of 0.09 this seems a good result to me. :)

[vnen]

Actually, the "Third party drivers" component is ignored. It is also ignoring some of Godot code though, since some files there contain engine code (at least alsa, pulseaudio, unix and windows folders have only Godot code, and many of the others have a bit of engine code too).

[tomreyn]

If you ignore the "Third party drivers" component you end up with a defect density of 0.70 (edit: I updated this number since I just pushed a new build): https://scan.coverity.com/projects/godot-engine

Categorization of code into components serves three purposes:

• ignore parts of the code (such as third party code)
• allow for filtering on the 'defect browser'
• gain a better understanding of the more problematic parts of the code

This table shows (only) the numbers of 'defects' of 'high impact' across components of the latest (todays') build (on Ubuntu 16.04 amd64) analysis:

[Calinou]

Is there any progress on this issue lately? If there are no developers interested in fixing some of the issues found by a Coverity scan, this issue should be closed for now.

[akien-mga]

I'm interested in having Coverity scans and having contributors work on fix issues, but the current setup we have is based on @tomreyn's fork (and uses outdated patterns). I'm also unhappy with Coverity's "private by default" system, so only approved contributors can access the stats (so basically noone, and thus nothing gets fixed).

I'm now looking into setting up Coverity scans for godotengine/godot, and if possible public for everyone (if not, I'll add all GitHub org members and then some so that they have access).

I'll open new issues to keep track of build results and work done to address them though, so closing this one.

[tomreyn]

While this doesn't make results universally available, it can ensure you always have current results: https://scan.coverity.com/travis_ci https://docs.travis-ci.com/user/coverity-scan/ https://blogs.s-osg.org/using-travis-ci-submit-coverity-scan-service/ https://github.com/bjorn/tiled/commit/c0fac49b6f9e96716a5554cb7d9a990bddbe7c29

Making results publicly available should prove difficult. I tried to download a full result set but it's not easy since it's just different sets of (not well defined) JSON data wrapped around a somewhat convoluted web UI.

[akien-mga]

Thanks for the input - I've also used your components as a reference to set the new ones, so thanks for that (thinking back I could maybe have just tweaked the tomreyn/godot project to make it more "official" instead of creating a new one, but well.. it's done).

Here's the new "official" Coverity Scan: https://scan.coverity.com/projects/godotengine-godot/

I plan to add Travis integration indeed, it's more convenient than uploading local builds (especially when they're still 250 MB xz-compressed).

I set the "Project summary and defects are viewable in read-only mode by all users" option, but it still doesn't seem to allow viewing defects without being invited first (at least in private mode/not logged in, I can't access defects). I guess I'll just invite all core contributors.

[akien-mga]

I'm looking into the Travis / Coverity integration, and it seems simple enough (see diff below).

What I'm not sure about is how to configure what build of the build matrix should run collect and upload the Coverity Scan data. Our current build matrix has 6 jobs, and it seems overkill to setup and collect Coverity data on all 6 of them when we're limited to one build/day and 7 builds/week due to the size of the code base.

None of the links given above seem to address this, so I'm a bit puzzled. I checked tiled's last Coverity run, and it did run on all three of their build jobs: https://travis-ci.org/bjorn/tiled/builds/216107908

WIP Travis integration:

diff --git a/.travis.yml b/.travis.yml
index 73b1be69d..f1fb27049 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -8,7 +8,8 @@ env:
   global:
     - SCONS_CACHE=$HOME/.scons_cache
     - SCONS_CACHE_LIMIT=1024
-    - OPTIONS="verbose=yes progress=no openmp=no gdnative_wrapper=yes"
+    - OPTIONS="verbose=yes progress=no gdnative_wrapper=yes"
+    - secure: "n/H0YCWDhq3Pl2vOtv8Ai3IltGBHHh9VdL+SENitCBln1N7ygNf17KTEnmBZA+kVCsAVL3M2no8bXDYshlsU5AwxwOC8x3hn+rLmanNkyY3YQiHvCUeduVRbUj+JmirJZGyf/RDifk1mj43ksHlLxDgRnzKEFh5dnXW1neAywU7XKsOOKbvE1+chmCsGeif9/zrn77y+znq0ZTsoy+TDUEXvBSTJsBc6gvyosD7qCf5BHjsv7/g8WkC1hM8C8qwNw9loihDfurQtYLenOffK8duldyqiFiggbRY4RoPGPn6s92Zd30/NNjFsUGMooVj6If3aYHP4oNqToCW4okMIER94Vt4tExlCrkhqClfUM+H/miQYfPNlYH02cGXREwxyKwHBhb5IEjUcLNV9bnW9hQPaTAVkJLRbaTqDjK1wIADgfNPJZ5p8EDnJL9sd6zVCWDEB3Jnf1nxhUkmDOBYRQv/j22m6tgOhZPt00sjwrJbFq8CB5qCgS5KktSN2Tnq9HMkZFFTK7uwYuMm2DMcuwS9B+eQWC0Cdauc2Pf4j4SF0SC6pbKrDyrb3UHM8Zoq+9BFrBiriAhKfZilCmLk38Jc/fkV8DJ/pkz8CUHVbRsSuKjtpvO50kdNIx+2qDcwVWhkRnbQf+gbJ1bvwbT9yfSRGZuTYr86f3XTBfvDFK9c="

 cache:
   directories:
@@ -71,6 +72,18 @@ addons:
       # For style checks.
       - clang-format-5.0

+  coverity_scan:
+    project:
+      name: "godotengine/godot"
+      description: "Godot Engine Coverity scans"
+    notification_email: coverity@godotengine.org
+    build_command_prepend: ""
+    build_command: "scons p=x11 -j2 $OPTIONS"
+    branch_pattern: coverity_scan
+
+before_install:
+  - echo -n | openssl s_client -connect https://scan.coverity.com:443 | sed -ne '/-BEGIN CERTIFICATE-/,/-END CERTIFICATE-/p' | sudo tee -a /etc/ssl/certs/ca-
+
 install:
   - if [ "$TRAVIS_OS_NAME" = "linux" ] && [ "$GODOT_TARGET" = "android" ]; then
       misc/travis/android-tools-linux.sh;

[tomreyn]

Indeed the build submission frequency limits will often collide with an automated build (and submission) mechanism - I have no (fully automated) solution for this either. I guess a cron job triggering a (not otherwise modified) fork to be synced with its upstream, committing and pushing this stage (just into the fork) then triggering a build via Travis-CI and that triggering a submission to Coverity / Synopsis Scan could do. Quite a bit of overhead, I guess, but it should work since you can decide on the build / submission frequency this way, remaining below the limits.

In case you use Jenkins CI, you could use the Coverity Scan plugin and setup time based build triggers.

[tomreyn]

Revisiting the topic on how to extract analysis results, I stumbled upon https://stackoverflow.com/questions/44119285/does-coverity-have-rest-api

I did not succeed in using HTTP basic authentication to authenticate against one of the report servers using a URL like this: curl --verbose --user 'tomreyn%40megaglest.org:MYPASSWORD' 'https://scan7.coverity.com/api/viewContents/issues/v1/All%20Newly%20Detected?projectId=MegaGlest&rowCount=-1'

I was, however, able to access this URL directly from my web browser just fine, and got a somewhat usable JSON output. Apparently the "REST API" allowing this type of scripted access is a newer feature. Chances are that the Coverity Scan (for OSS scans) platform is just not updated to the latest Coverity Analysis platform, yet.

I also learned that there actually is an export feature on the web UI: