Static analysis

[Jeremy Apthorp]

Just read this article[1] by John Carmack on various static analysis tools on Windows, including PC-Lint, Coverity and MSVS's /analyze.

I know we run Coverity on Chrome -- do we run any other static error analysers? Carmack praises /analyze highly.

[1]: http://altdevblogaday.com/2011/12/24/static-code-analysis/

[Rachel Blum]

/analyze is a great tool - if you happen to have an X360 compiler.
AFAIK, on the PC you need to buy the high-end version of Visual
Studio, which ends up costing several thousand dollars per seat....

Besides Coverity, we run a clang plugin for style violations, and you
can run the clang analyzer yourself:
http://code.google.com/p/chromium/wiki/ClangStaticAnalyzer (I don't
know why we don't run it regularly, but I'd assume it's the false
positive management. Also, on the Mac, I haven't had luck getting it
to run yet. Fails to build a local clang, for some reason. If you know
why or can figure out how to make it go, please update the wiki page
;)

The guys who make PVS Studio occasionally run it over Chromium and
share some of their findings, but that's it in terms of analysis I'm
aware of.

Rachel

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Jeremy Apthorp]

On Thu, Dec 29, 2011 at 10:22 AM, Rachel Blum <gr...@chromium.org> wrote:

/analyze is a great tool - if you happen to have an X360 compiler.
AFAIK, on the PC you need to buy the high-end version of Visual
Studio, which ends up costing several thousand dollars per seat....

Surely we could afford a few licenses to run bots? If not, we might be able to compile parts of Chrome (WebKit, perhaps?) in an X360 environment...

Besides Coverity, we run a clang plugin for style violations, and you
can run the clang analyzer yourself:
http://code.google.com/p/chromium/wiki/ClangStaticAnalyzer (I don't
know why we don't run it regularly, but I'd assume it's the false
positive management. Also, on the Mac, I haven't had luck getting it
to run yet. Fails to build a local clang, for some reason. If you know
why or can figure out how to make it go, please update the wiki page
;)

That's not really in the same league as the other tools discussed :P

[Carlos Pizano]

I have the ultra-premier-platinum edition of vs2010, for kicks I ran /
analyze on the base project.

Here is the first error:

1>d:\src\r0\src\base\time.cc(104): warning C6011: Dereferencing NULL
pointer 'time_string': Lines: 102, 104
1>d:\src\r0\src\base\time.cc(114): warning C6011: Dereferencing NULL
pointer 'parsed_time': Lines: 102, 104, 107, 108, 110, 113, 114


bool Time::FromString(const char* time_string, Time* parsed_time) {
DCHECK((time_string != NULL) && (parsed_time !=
NULL)); <--line 102

if (time_string[0] ==
'\0')
<--line 104
return false;

PRTime result_time = 0;
PRStatus result = PR_ParseTimeString(time_string, PR_FALSE,
&result_time);
if (PR_SUCCESS != result)
return false;

result_time += kTimeTToMicrosecondsOffset;
*parsed_time = Time(result_time);
return true;
}

Now how do we silence them?

[Rachel Blum]

You can do that via #pragma warning, I think - which is nasty work,
since you'll get a lot of warnings for things we DCHECK'ed. In
Coverity, we have "models" for things like DCHECK that tell Coverity
that after line 102, time_string can reasonably be expected to be not
NULL.

In VS, you'd have to annotate the function. I.e.

#include <CodeAnalysis\SourceAnnotations.h>
using namespace vc_attributes;
bool Time::FromString([Pre(Null=No)] const char* time_string,
[Pre(Null=No)] Time* parsed_time) {
...

Ideally, it would be nice if we could annotate functions in a portable
way, so that we don't litter the code with annotations for all the
analysis systems we might be using :) I assume we could come up with a
NOT_NULL macro that guarantees a post condition of not null - that
would work portably.

Rachel

[Carlos Pizano]

So the signature would change to

bool Time::FromString(NOT_NULL(const char* time_string),
NOT_NULL(Time* parsed_time))

Yuck!

Or is there a practical way to just tell VS in line 102 to do the
same?

[Antoine Labour]

Ideally, we could annotate DCHECK itself by saying that if the assertion triggers, it doesn't return, letting the static analysis magic do its work. I have no clue whether or not it has any chance of working with that tool.

Antoine

[Rachel Blum]

> Ideally, we could annotate DCHECK itself by saying that if the assertion
> triggers, it doesn't return, letting the static analysis magic do its work.

We have done that for Coverity. It might be possible for analyze, too.
Somewhere in DCHECK, you need to add an __analysis_assume.

void DCHECK(cond) {
if(!cond)
LogAndExplode();
__analysis_assume(cond);
}

I have no idea if doing that deep down in the guts of DCHECK will
actually bubble up to the actual function. For Coverity runs, we're
using a simplified DCHECK macro that does nothing but say "assume this
is true from here"

Rachel

[Alexei Svitkine]

We could also run /analyze in a mode where DCHECK(x) expands to
nothing, assuming that will make /analyze ignore those.

-Alexei

[Timur Iskhodzhanov]

FTR,
We also have a bunch of tools that do dynamic analysis.
It's not a silver bullet too but does find quite a number of bugs:
http://build.chromium.org/p/chromium.memory/console
http://build.chromium.org/p/chromium.memory.fyi/console

http://code.google.com/p/chromium/issues/list?can=1&q=Stability%3DValgrind+OR+Stability%3DAddressSanitizer+OR+Stability%3DThreadSanitizer+OR+Stability%3DDrMemory+OR+Stability%3DHeapChecker
-> gives 2k bugs out of ~100k bugs total :)
And that doesn't count the bugs filed before the Stability-X labels
have been introduced...

Timur Iskhodzhanov,
Google Russia

[Marc-Antoine Ruel]

/analyze is free in the Windows SDK.

http://randomascii.wordpress.com/2011/10/15/try-analyze-for-free/

First step: get rid of ATL.

I have a patch pending to enable /Wall, which has many of the /analyze reports.

M-A

[Rick Byers]

/analyze is also shipping in VS 2011 Express (free) editions.  See http://blogs.msdn.com/b/sdl/archive/2011/10/19/code-analysis-for-all.aspx.

In addition to __analysis_assume, we can also annotate functions that terminate the process on error conditions with declspec(noreturn), which can help the analysis figure out that the condition can't be true past that point.  Of course just because we DCHECK something doesn't mean it's actually true in all cases, and we'd probably want to run the analysis on retail builds anyway to test what we're actually shipping.  If you're using /analyze as defense against security bugs (as Microsoft does) then you probably want to know about such warnings and consider replacing DCHECK by CHECK (or some other handling of the condition in release builds).

This DCHECK issue is just one example of the larger trade-off between annotations and analysis precision.  To get the full benefit out of /analyze we'd probably have to add a lot of SAL annotations.

Rick

[Rachel Blum]

> point.  Of course just because we DCHECK something doesn't mean it's
> actually true in all cases,

That is the assumption. The docs actually _specifically_ say to not
handle conditions spelled out by DCHECK.

What we do want to do is probably specify a pre-condition for DCHECK,
saying the argument MUST be true. And possibly a post condition saying
that even if it wasn't true before, it WILL be true now. (To avoid a
slew of dependent errors if the pre-condition is violated. Not sure if
/analyze is smart enough to do that on its own)

> This DCHECK issue is just one example of the larger trade-off between
> annotations and analysis precision.  To get the full benefit out of /analyze
> we'd probably have to add a lot of SAL annotations.

Again, if we head down that route, we should figure out how to best
annotate in both a cross-platform and cross-analysis tool way. I
_really_ would prefer not spelling out things for every single tool
separately :)

Rachel

[Carlos Pizano]

Using Rachel's suggestion I was able to reduce the /analyze output
diarrhea to the point that I was able to spot a real bug:

http://codereview.chromium.org/9004052/

Question for the people who look at the coverity output: did coverity
catch that bug?

[Ryan Sleevi]

This was found by PVS Studio in their first scan

http://crbug.com/83873 , then split into an individual bug http://crbug.com/83234

See the discussion on the bug to know why it wasn't landed then.

[Rachel Blum]

Since I'm part of the folk who look at Coverity - no, that wasn't
found. (Off to file more Coverity bugs. They must hate me by now :)

Rachel.

[Chris Palmer]

On Thu, Dec 29, 2011 at 4:49 PM, Rachel Blum <gr...@chromium.org> wrote:

> Since I'm part of the folk who look at Coverity - no, that wasn't
> found. (Off to file more Coverity bugs. They must hate me by now :)

I'm surprised Coverity didn't find it, because GCC can find it with -Wall:

$ c++ -o t -Wall -Werror t.cc
cc1plus: warnings being treated as errors
t.cc: In function ‘int main()’:
t.cc:5: error: suggest parentheses around comparison in operand of ‘&’
$ cat t.cc
int main(void) {
int x = 0;
int y = 1;

return x & y != 0;
}

Don't we use -Werror on GCC(-like) platforms? How did this ever build?

[Ryan Sleevi]

On Dec 29, 7:57 pm, Chris Palmer <pal...@google.com> wrote:
> On Thu, Dec 29, 2011 at 4:49 PM, Rachel Blum <gr...@chromium.org> wrote:
>
> Don't we use -Werror on GCC(-like) platforms? How did this ever build?

src/base/platform_file_win.cc

Meaning no GCC or Clang (which also warns about this)

[Rachel Blum]

It builds by not using gcc :) It's Windows code, so the compiler is
VS.NET. And I assume we don't crank the warnings up all the way
because we're using ATL/Windows headers - Win folks, can you confirm?

Rachel

[Chris Palmer]

On Thu, Dec 29, 2011 at 4:59 PM, Rachel Blum <gr...@chromium.org> wrote:

> It builds by not using gcc :) It's Windows code, so the compiler is
> VS.NET. And I assume we don't crank the warnings up all the way
> because we're using ATL/Windows headers - Win folks, can you confirm?

I had assumed that since GCC can find it, a commercial compiler could,
and that since a commercial compiler could, surely an expensive and
specialized static analysis tool could too. But I didn't know that we
have to turn warnings down on VS — do ATL and/or Windows headers
really not cleanly pass VS' own warnings? That is sad...

My general feeling about static analysis is: I read the Cousots as
cautioning us not to hope for too much, not as cheerleading for SA.
Although I'm a firm believer in compiler warnings and I turn as many
of them on as I can, that's kind of the best we can expect on a
day-to-day, practical basis.

Since we care about quality at runtime, runtime checks, dynamic
analysis (ASAN!), and heavy unit testing is more effective and a
better use of time IME. I'd much rather write unit tests to assert the
method's contract (as in Carlos' CL) than sift through 5,000 pages of
SA output. (A couple jobs ago, we used SA output to haze interns but
for no other purpose. Although one time the source code revealed
crashers in the SA product...)

[Rachel Blum]

> have to turn warnings down on VS — do ATL and/or Windows headers
> really not cleanly pass VS' own warnings? That is sad...

Based on old experience. That's why I asked the folk who work on the
Win build to confirm. So take with a grain of salt.

> My general feeling about static analysis is: I read the Cousots as
> cautioning us not to hope for too much, not as cheerleading for SA.

Have you read Carmack's article? He found _significant_ bugs via SA
that escaped their analysis and testing before.

> Although I'm a firm believer in compiler warnings and I turn as many
> of them on as I can, that's kind of the best we can expect on a
> day-to-day, practical basis.

Why? What's wrong with fixing SA bugs as they come up? We're currently
getting about ~15 or so a day, over the entire Chromium code base
including boatloads of third-party stuff. If folk could fix them the
moment they came up, it wouldn't be that hard to at least stay on our
current level. Most are trivial fixes. But about 1 in 75 is a serious
bug - resource leak, memory overwrite, NULL deref.

> Since we care about quality at runtime, runtime checks, dynamic
> analysis (ASAN!), and heavy unit testing is more effective and a
> better use of time IME.

"Quality at runtime" is obviously determined by a static product, the
source. So running SA doesn't seem like a bad plan to me :) (Note - I
agree that we should keep runtime checks, ASAN, unit testing, etc.
too. Obviously. They find plenty of issues)

> I'd much rather write unit tests to assert the
> method's contract (as in Carlos' CL)

Unit tests can only assert post conditions, not pre conditions (for
the function under test)

> than sift through 5,000 pages of
> SA output.

You're exaggerating :) There are significantly less than 5,000 issues
being detected by SA right now.

> (A couple jobs ago, we used SA output to haze interns but
> for no other purpose. Although one time the source code revealed
> crashers in the SA product...)

SA has improved a bit since then, I think :)

Rachel