git and the Clang Static Analyzer

git and the Clang Static Analyzer

[Дилян Палаузов]

Hello,

I compiled git/master using the clang 4.0 static analyzer with

scan-build ./configure --with-libpcre --with-openssl
scan-build make

and here are the results:
   https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/

Please note, that the information is only about what gets actually 
compiled, code disabled by #if .. #endif is not considered (e.g. when 
determining whether a variable assignment is useless).  There are 
probably false-positives.  However in case of e.g. builtin/notes.c:1018, 
builtin/reset.c:294 or fast-import.c:2057 I consider the hints as justified.

This is for your information,  I wouldn't have a problem if you ignore 
the analysis.  When you are worried about javascript, use lynx.

Regards
   Дилян

Re: git and the Clang Static Analyzer

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4043 bytes --]

Hi Dilyan,


On Sun, 7 May 2017, Дилян Палаузов wrote:

> I compiled git/master

... which advances from time to time, so you definitely want to include a
more informative data point here, e.g. 4fa66c85f11 (Git 2.13-rc2,
2017-05-04) ...

> using the clang 4.0 static analyzer with
> 
> scan-build ./configure --with-libpcre --with-openssl
> scan-build make
> 
> and here are the results:
>   https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/
> 
> Please note, that the information is only about what gets actually compiled,
> code disabled by #if .. #endif is not considered (e.g. when determining
> whether a variable assignment is useless).

So you already know that the report is specific to your setup. It may make
a lot of sense to actually state what your setup is, i.e. Operating
System, installed libraries (and their respective versions), CPU, etc.

> There are probably false-positives.

Probably. So why don't you give it a try and look through the report? Then
summarize your findings here. That would definitely find a warm welcome, I
would expect.

> However in case of e.g. builtin/notes.c:1018, builtin/reset.c:294 or
> fast-import.c:2057 I consider the hints as justified.

Okay. And those hint are...?

Remember: the easier you make it for your readers to follow your thoughts,
the more readers will try to do that.

> This is for your information,  I wouldn't have a problem if you ignore
> the analysis.

Thank you for running the scan.

However, I would like to encourage you to do better than just call Clang
and slap the report verbatim, without much in the way of a manual
analysis, onto some website.

In fact, I fear that doing it this way is rather counter-productive.

Instead, you could try to organize the report a lot better (see the many
mails about Coverity in the past):

- we already *know* that static analyzers have tons of problems with the
  way we specify "FLEX_ARRAYS": we define structs to have a 0-item or
  1-item array as their last field, then use a variable-length malloc() to
  tailor the allocated data to the needs of a variable-length array (most
  often: string). To have these issues in one category would be very
  helpful, as we could ignore those parts of the report easier.

- we also already know quite well that Git's source code often abuses
  exit() for a garbage collector on short-running processes: In many
  cmd_*() functions, there are "memory leaks" that are intentional (read:
  lazy). Again, it would be helpful to be able to fade out those reports.

- Likewise, our test helpers abide by a lot less strict rules, as we only
  use them in the test suite (and therefore it is not *really* necessary
  to, say, validate the input passed as command-line parameters). Here
  again, it would be very helpful to put those into a separate category
  that can be selectively hidden.

- there have been a number of patches floating about to fix one or more of
  the issues reported by Coverity. These patches may not yet have made it
  into `master`, but at least some of them are in `pu`. A careful analysis
  which issues reported by Clang would be addressed by `pu` would also be
  quite welcome.

Also: to make it *really* useful for other developers, it would be a good
idea for you to try your hand at patching the .travis.yml file in such a
way that the static analysis is performed as part of the Continuous
Testing, and to contribute said patch. That way, other contributors could
not only see how it is done (and copy-paste the commands, including `apt
install` calls insofar necessary), but also to see the reports on a
trusted website (Travis').

I really would like to believe that you meant to be helpful in sending
this mail. But the style does come over as "throwing a couple of scraps to
the dogs". I do not believe that is what you wanted, and I think you can
turn that impression around by at least some of the things I mentioned
above.

Ciao,
Johannes

Re: git and the Clang Static Analyzer

[Lars Schneider]

> On 08 May 2017, at 12:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> Hi Dilyan,
> 
> 
> On Sun, 7 May 2017, Дилян Палаузов wrote:
> 
>> ...
> 
> Also: to make it *really* useful for other developers, it would be a good
> idea for you to try your hand at patching the .travis.yml file in such a
> way that the static analysis is performed as part of the Continuous
> Testing, and to contribute said patch. That way, other contributors could
> not only see how it is done (and copy-paste the commands, including `apt
> install` calls insofar necessary), but also to see the reports on a
> trusted website (Travis').

I did that here:
https://github.com/larsxschneider/git/commit/faf4ecfdca1a732459c1f93c334928ee2826d490

Unfortunately, there are still too many issues to activate this job:
https://travis-ci.org/larsxschneider/git/jobs/205507241

See this thread for more info:
http://public-inbox.org/git/BAB1EE0E-B258-4108-AE24-110172086DE4@gmail.com/

- Lars

Re: git and the Clang Static Analyzer

[Дилян Палаузов]

Hello Johannes,

>> I compiled git/master
>
> ... which advances from time to time, so you definitely want to include a
> more informative data point here, e.g. 4fa66c85f11 (Git 2.13-rc2,
> 2017-05-04) ...
>

The 4fa66c85f11 you mentioned is part of the URL I sent.

>> Please note, that the information is only about what gets actually compiled,
>> code disabled by #if .. #endif is not considered (e.g. when determining
>> whether a variable assignment is useless).
>
> So you already know that the report is specific to your setup. It may make
> a lot of sense to actually state what your setup is, i.e. Operating
> System, installed libraries (and their respective versions), CPU, etc.

I don't think this is of much relevance.  The hints provided encourage one to look at the code and to evaluate mentally the lines.  By tweaking the preprocessor directives, you could get less warnings (a previously unused variable now appears within an asser()), or more warnings (as more code gets compiled).  Getting more warnings makes sense, after the current ones are processed.  Getting less warnings means (again) compiling more code.  I use already pcre and openssl, what else can I enable?
  
>> There are probably false-positives.
>
> Probably. So why don't you give it a try and look through the report? Then
> summarize your findings here. That would definitely find a warm welcome, I
> would expect.
>
>> However in case of e.g. builtin/notes.c:1018, builtin/reset.c:294 or
>> fast-import.c:2057 I consider the hints as justified.
>
> Okay. And those hint are...?

Click on  https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/ and then on "fast-import.c: line 2057 -> View Report" and you will see pointless assignment.

I cannot organize the report much better, as filtering out the false positives requires usually too deep understanding of the code organization of git, which I do not have.

This is the analysis done on the pu-branch:
   https://mail.aegee.org/dpa/scan-build-git-7dd243c75

Both reports do not list files in the same order, as I did parallel builds, but I do not see on the spot any difference.

Learning Travis is not on my priority list, I sent the commands I called to get the report.  I also compiled clang by myself.  For those who mistrust sites, there are no-javascipt, no-css browsers like lynx.

Greetings
   Dilyan

Re: git and the Clang Static Analyzer

[Jeff King]

On Mon, May 08, 2017 at 09:26:11PM +0200, Дилян Палаузов wrote:

> Click on  https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/ and
> then on "fast-import.c: line 2057 -> View Report" and you will see
> pointless assignment.
> 
> I cannot organize the report much better, as filtering out the false
> positives requires usually too deep understanding of the code
> organization of git, which I do not have.

Right, but I think Johannes's point is that we already know about
scan-build. It's been discussed on the list multiple times. The next
step is somebody actually looking at each reported issue, fixing any
problematic ones, and suppressing (or at least cataloging) the false
positives.

Here's a link to the last discussion on this topic, from a few months
ago:

  http://public-inbox.org/git/CAJZjrdXP3n5fOLx4rEEkbJT7JBMPUqk4Qdutm6KpvMVUMwCSPQ@mail.gmail.com/T/#u

It even involves a Travis build. :)

-Peff