Clang static analyzer is now available for Linux and Mac builds

[Kevin Marshall]

Hi Chromium developers!

I'm happy to announce that I have landed Chromium toolchain support for the Clang static analyzer on Mac and Linux. (Windows coming soon!) Enabling the analyzer for your build is as easy as setting "use_clang_static_analyzer = true" in your GN build args. You should start to see analysis errors reported as warnings to stderr.

The analyzer tool generates quite a few warnings while building all of Chrome, so it would be a great help if you could spend some time building your code and seeing if there are any interesting warnings that jump out at you. I have two bugs open for tracking cases: 

• If Clang analyzer was useful and caught a legitimate issue: link the bug and/or fix CL to this bug: crbug.com/686838
  
• If Clang analyzer's output was wrong or not helpful: link to this bug: crbug.com/686829

Documentation and TODOs can be found here:

https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_analyzer.md

Note that analysis builds are a bit slow, as they don't work on Goma at present. You might want to try this in between major work items. I'll be working on that as well as trying to get static analysis integrated into the trybots and build waterfall.

Happy linting!

Kevin Marshall

[Kevin Marshall]

Here is the tracking bug for the static analyzer tool effort: crbug.com/648210

And thank-you to sdefresne@ for getting the ball rolling with this.

[Alexander Yashkin]

Unfortunately the link to last bug is not very useful for non-googlers.

"You do not have permission to view the requested page.
Reason: User is not allowed to view this issue"

Otherwise, thanks for efforts.

31.01.17 2:25, 'Kevin Marshall' via Chromium-dev пишет:

> Here is the tracking bug for the static analyzer tool effort:

> crbug.com/648210 <http://crbug.com/648210>

>
> And thank-you to sdefresne@ for getting the ball rolling with this.
>
> On Mon, Jan 30, 2017 at 2:39 PM, Kevin Marshall <mars...@google.com
> <mailto:mars...@google.com>> wrote:
>
> Hi Chromium developers!
>
> I'm happy to announce that I have landed Chromium toolchain support
> for the Clang static analyzer on Mac and Linux. (Windows coming
> soon!) Enabling the analyzer for your build is as easy as setting
> "use_clang_static_analyzer = true" in your GN build args. You should
> start to see analysis errors reported as warnings to stderr.
>
> The analyzer tool generates quite a few warnings while building all
> of Chrome, so it would be a great help if you could spend some time
> building your code and seeing if there are any interesting warnings
> that jump out at you. I have two bugs open for tracking cases:
>

> * *If Clang analyzer was useful* and caught a legitimate issue:

> link the bug and/or fix CL to this bug: crbug.com/686838

> <http://crbug.com/686838>
> * *If Clang analyzer's output was wrong or not helpful*: link to
> this bug: crbug.com/686829 <http://crbug.com/686829>

>
>
> Documentation and TODOs can be found here:
>
> https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_analyzer.md
> <https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_analyzer.md>
>
>
> Note that analysis builds are a bit slow, as they don't work on Goma
> at present. You might want to try this in between major work items.
> I'll be working on that as well as trying to get static analysis
> integrated into the trybots and build waterfall.
>
>
> Happy linting!
> Kevin Marshall
>
>

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
> ---
> You received this message because you are subscribed to the Google
> Groups "Chromium-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to chromium-dev...@chromium.org
> <mailto:chromium-dev...@chromium.org>.

--
С уважением,
Александр Яшкин,
разработчик Нашего Браузера
http://staff.yandex-team.ru/a-v-y/

[Éric Noyau]

\o/

Awesome. Thanks Kevin. Can't wait for the bot!

-- Éric

[Jeremy Roman]

I'll note that in addition to not working, trying will cause it to run a ton of jobs locally. This caused my workstation to hang.

[Jeremy Roman]

On Tue, Jan 31, 2017 at 10:48 AM, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jan 30, 2017 at 5:39 PM, 'Kevin Marshall' via Chromium-dev <chromi...@chromium.org> wrote:

Hi Chromium developers!

I'm happy to announce that I have landed Chromium toolchain support for the Clang static analyzer on Mac and Linux. (Windows coming soon!) Enabling the analyzer for your build is as easy as setting "use_clang_static_analyzer = true" in your GN build args. You should start to see analysis errors reported as warnings to stderr.

The analyzer tool generates quite a few warnings while building all of Chrome, so it would be a great help if you could spend some time building your code and seeing if there are any interesting warnings that jump out at you. I have two bugs open for tracking cases: 

• If Clang analyzer was useful and caught a legitimate issue: link the bug and/or fix CL to this bug: crbug.com/686838
  
• If Clang analyzer's output was wrong or not helpful: link to this bug: crbug.com/686829

Documentation and TODOs can be found here:

https://chromium.googlesource.com/chromium/src/+/master/docs/clang_static_analyzer.md

Note that analysis builds are a bit slow, as they don't work on Goma at present. You might want to try this in between major work items. I'll be working on that as well as trying to get static analysis integrated into the trybots and build waterfall.

I'll note that in addition to not working, trying will cause it to run a ton of jobs locally. This caused my workstation to hang.

Err, to be clear, I meant in addition to not working [on Goma], trying [a lot of jobs, even when use_goma=true] caused my workstation to hang.

[Wez]

FWIW, in case you're not aware of it (I wasn't), you can run ninja with not only -j N (N-wise parallel build) but also -l M (limit system load to M[*]).

[*] I have no idea whether ninja has a notion of load under non Unix systems. ;)

[Kevin Marshall]

Oh, right - that's because Goma is falling back to local compilation. I should add a GN assert preventing this from happening until Goma is supported. (Which... I'm working on now.)

[Wez]

The fallback behaviour seems a more general issue with GOMA; fallback isn't visible the build system (except via load, for example) so it can't adapt its parallelism to account for that; I thought GOMA was supposed to limit the number of fallback processes to the number of available cores on the client system?

[Mike Bjorge]

Is there a way to tell the analyzer to only give me warnings that come from chromium/src/module/I/care/about/* and suppress all the jillion other warnings?

[Kevin Marshall]

nyquist@ pinged me with a clarifying question about support for Android builds.

This tool works for Mac and Linux hosts, not just Mac and Linux targets. So, go ahead, analyze your iOS and your Android builds. :)

[Kevin Marshall]

Would it be more useful to filter at a file level (module/I/care/about/*.cc) , or the GN label level (//module/I/care/about)? I agree that scoping down the areas would improve the SNR a lot, and spent some time investigating the latter, but ran into some roadblocks WRT variable bindings in the toolchain templates. I can resume work on that front later.

Alternatively, we could just spend some time whittling down the warnings to zero and then just upgrade all analysis warnings to errors on the CQ trybots. Then any analysis errors you might see would definitely be of your own doing. ;)

[Nico Weber]

Kevin, have you done a rough evaluation of false positive rate (say, by running it on a small target such as base/ and classifying what it finds)?

I think clang's analyzer has a list of checks one can enable and disable, does this just use the default set?

[Max Bogue]

Kevin, I think either would be useful. I think the suggestion was because it's sort of hard to whittle away at the warnings if you can't find the ones that you want to fix (e.g. in your team's code).

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Mike Bjorge]

I basically figured the plan was to get the warnings down to zero and then upgrade warnings to errors. It's just not clear to me how much time "some time" will be to get to that point :). But definitely +1 for this when we get to that point.

In practice, if filtering on GN label == filtering on the sources in the GN target, then the two are probably mostly equivalent and I'd be delighted with either and would go with whichever is easier/cleaner on the backend. If filtering on the GN label level would also result in warnings from the deps of the target, then I think that would be less useful (since I don't want to be getting all the warnings from //base and //net and such).

[Nico Weber]

Let's please evaluate false positive rate before applying fixes for things this finds. I mean, fix things that are obviously wrong, but if the tool is noisy we shouldn't change working, non-confusing code just because it confuses this tool.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Mike Bjorge]

Yes, exactly this!

[Nico Weber]

On Tue, Jan 31, 2017 at 10:33 AM, 'Mike Bjorge' via Chromium-dev <chromi...@chromium.org> wrote:

I basically figured the plan was to get the warnings down to zero and then upgrade warnings to errors.

That's what we've do with compiler warnings. Static analyzers usually have some noise almost by design. I don't think that should be the plan here.

[Mike Bjorge]

I guess it would depend on both how noisy the warnings are and if there is a simple way to suppress errors (like what is done with the sanitizers, for example). In my head, I was thinking zero errors meaning all the good errors cleaned up and all the false-positives suppressed.

[Kevin Marshall]

Yeah, I've gone through some relatively hermetic targets and evaluated the warnings against the code.

Nullability checks seem to be the worst culprits WRT false positives. These can be avoided through explicit annotations, but those would probably clutter the codebase. I wonder if we can make our DCHECK(ptr) convention do double duty in this regard. Dead store detection seemed to be quite reliable in most cases. All bets appear to be off when working with low level memory - the analyzer doesn't know what's going on.

[Wez]

To Nico's point, it seems likely that some form of "exclusions" list, ideally to warning granularity but perhaps just to file-level granularity, will be necessary even once we have fixes and annotations in place for the genuine issues.  The Linux system headers, for example, seemed to trigger a slew of warnings that we might need to just tolerate via exclusion.

[Bruce]

I've been running a parallel static analysis effort using Microsoft's /analyze compile option for two years. It initially found a few dozen bugs and since then has found new bugs every now and then.

The limiting factor with /analyze is the noisiness - it has a lot of false positives. After deduping warnings from header files the last run had 4,444 distinct warnings, from 51 different types. Unfortunately its high-value and its noisy heuristics often share warning numbers, making better filtering challenging.

2141 of those warnings were for variable shadowing and 313 of those were for "Return value ignored: 'snprintf'.". I should probably update my analysis scripts to ignore those completely. But, even with those filtered out I find that more than 90% of the new warnings are false positives. This means that I've taken to only looking at the results once a week, because I don't want to spend too much time on a low-value activity.

Unfortunately, looking at the results once a week means that most bugs that /analyze could find have already been noticed, and sometimes even fixed, before I look at the results.

So...

• A low false-positive rate is crucial
  
• Generating a report which only shows new warnings is extremely valuable
  
• Reporting new warnings promptly increases their value greatly, but makes a low false-positive rate even more crucial
  

The crude scripts that I used to retrieve the latest /analyze warnings and create new-warning reports are checked in here:

https://cs.chromium.org/chromium/src/tools/win/new_analyze_warnings/retrieve_latest_warnings.bat?type=cs&q=retrieve_latest&l=1

I had high hopes that the variable shadowing warnings would be useful, and occasionally they are, but not enough to justify their noise level. I think I'll suppress them.