sparse (static analyzer) report

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:
http://www.osdl.org/plm-cgi/plm?module=patch_info&patch_id=4065

On Thu, Jan 13, 2005 at 01:31:36PM -0800, Mark Wong wrote:

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:
http://www.osdl.org/plm-cgi/plm?module=patch_info&patch_id=4065

I took a peek at the first sparse report you posted, and it's too noisy
to be useful. The parser seems confused in several ways in thousands of
places.

Maybe there's something useful to be extracted, but we'd need to adapt
sparse.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
www.google.com: interfaz de l�nea de comando para la web.

On Thu, Jan 13, 2005 at 08:06:23PM -0300, Alvaro Herrera wrote:

On Thu, Jan 13, 2005 at 01:31:36PM -0800, Mark Wong wrote:

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:
http://www.osdl.org/plm-cgi/plm?module=patch_info&amp;patch_id=4065

I took a peek at the first sparse report you posted, and it's too noisy
to be useful. The parser seems confused in several ways in thousands of
places.

Maybe there's something useful to be extracted, but we'd need to adapt
sparse.

It might not have helped to dump every source file's report into a
single file. Would it have helped to split out the results per file?

Mark

On Thu, Jan 13, 2005 at 03:09:19PM -0800, Mark Wong wrote:

On Thu, Jan 13, 2005 at 08:06:23PM -0300, Alvaro Herrera wrote:

On Thu, Jan 13, 2005 at 01:31:36PM -0800, Mark Wong wrote:

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:
http://www.osdl.org/plm-cgi/plm?module=patch_info&amp;patch_id=4065

I took a peek at the first sparse report you posted, and it's too noisy
to be useful. The parser seems confused in several ways in thousands of
places.

Maybe there's something useful to be extracted, but we'd need to adapt
sparse.

It might not have helped to dump every source file's report into a
single file. Would it have helped to split out the results per file?

Something like this:
http://developer.osdl.org/markw/pgsql/sparse/pg-8.0rc5.html

Mark

On Fri, Jan 14, 2005 at 03:53:09PM -0800, Mark Wong wrote:

On Thu, Jan 13, 2005 at 03:09:19PM -0800, Mark Wong wrote:

On Thu, Jan 13, 2005 at 08:06:23PM -0300, Alvaro Herrera wrote:

On Thu, Jan 13, 2005 at 01:31:36PM -0800, Mark Wong wrote:

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:

http://developer.osdl.org/markw/pgsql/sparse/pg-8.0rc5.html

Hmm. Well, it showed the multiple incorrect uses of 0 as NULL in
dllist.c and other places, but there's still lots of spurious entries.
See backend/transam/xlog.c: it's strange that the parser is so confused
about XLogCtrl, for example.

It's complaining in several places about function as variables in
function declarations (the multiple walkers and mutators for example);
not sure how correct that is.

It is also analyzing flex and bison output files ... is it capable of
analyzing .y and .l files instead?

It's strange that DatumGetInt32 shows as a undefined identifier ...
there's some problem with postgres.h apparently. And fmgroids.h is
missing; not sure what's the minimal make target to install it, because
make -C src/backend/utils fmgroids.h
generates it, but the symbolic link to src/include/utils is still
needed.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"La gente vulgar solo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

... And fmgroids.h is
missing; not sure what's the minimal make target to install it, because
make -C src/backend/utils fmgroids.h
generates it, but the symbolic link to src/include/utils is still
needed.

Looks like the symlinks are made in src/backend/Makefile.

regards, tom lane

On Fri, Jan 14, 2005 at 09:54:24PM -0300, Alvaro Herrera wrote:

On Fri, Jan 14, 2005 at 03:53:09PM -0800, Mark Wong wrote:

On Thu, Jan 13, 2005 at 03:09:19PM -0800, Mark Wong wrote:

On Thu, Jan 13, 2005 at 08:06:23PM -0300, Alvaro Herrera wrote:

On Thu, Jan 13, 2005 at 01:31:36PM -0800, Mark Wong wrote:

We've also started automating sparse analyses in our PLM tool, which
will show an error and warning count. Here's an example:

http://developer.osdl.org/markw/pgsql/sparse/pg-8.0rc5.html

Hmm. Well, it showed the multiple incorrect uses of 0 as NULL in
dllist.c and other places, but there's still lots of spurious entries.
See backend/transam/xlog.c: it's strange that the parser is so confused
about XLogCtrl, for example.

I'm sure there are a number of false positives, if that's what you're
getting at.

It's complaining in several places about function as variables in
function declarations (the multiple walkers and mutators for example);
not sure how correct that is.

It is also analyzing flex and bison output files ... is it capable of
analyzing .y and .l files instead?

Yeah, I generate the file list to run sparse against with a
"find . -name '*.c'". So that's simple enough. But flex and bison
files don't end in .c, do they?

It's strange that DatumGetInt32 shows as a undefined identifier ...
there's some problem with postgres.h apparently. And fmgroids.h is
missing; not sure what's the minimal make target to install it, because
make -C src/backend/utils fmgroids.h
generates it, but the symbolic link to src/include/utils is still
needed.

Perhaps part of this has to do with my rather blind selection of files
to run sparse against. For example, you still have to run configure
first and it probably doesn't make sense to run on some of the files
in ports. I did notice one of the files complained about missing
Windows.h. :)

Mark

BTW, perhaps one reason for the relatively small number of legitimate
issues picked up by sparse is that I ran sparse on the tree a month or
two ago and fixed some of the stylistic issues it reported. Most of the
stuff I didn't bother to fix looked like either a sparse bug, or a
marginal style improvement I didn't bother applying (like fixing 0 =>
NULL in dllist.c).

I've been meaning to investigate whether sparse can be used as something
more than just a fussy syntax checker (i.e. whether it can do any
meaningful static analysis for interesting properties), but I haven't
had a chance yet.

Alvaro Herrera wrote:

It's complaining in several places about function as variables in
function declarations (the multiple walkers and mutators for example);
not sure how correct that is.

I believe the conclusion of prior discussions about making the
walker/mutator prototypes more precise is that it's not worth the cost.

-Neil

P.S. Hope everyone had a good holiday. I'm back at work on Monday.

Ah, so you beat me to it Neil. ;) Out of curiosity, how much worse
was it before you started fixing things?

Mark

On Fri, Jan 14, 2005 at 05:50:30PM -0800, Mark Wong wrote:

Yeah, I generate the file list to run sparse against with a
"find . -name '*.c'". So that's simple enough. But flex and bison
files don't end in .c, do they?

Generated files do. I have a list of generated files here:

pl_gram.c
pl_scan.c
psqlscan.c
preproc.c
pgc.c
guc-file.c
fmgrtab.c
gram.c
scan.c
bootparse.c
bootscanner.c

Not sure how to skip them with find ... I think you can do that with
-regex but it's cumbersome. Re: port, I think you can -prune it.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
Jude: I wish humans laid eggs
Ringlord: Why would you want humans to lay eggs?
Jude: So I can eat them

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

Hmm. Well, it showed the multiple incorrect uses of 0 as NULL in
dllist.c and other places,

Incidentally, while it may not be conformant to your style guidelines, use of
the constant 0 compared to or assigned to a pointer is a perfectly valid ANSI
spelling for NULL. (The same is not true for an expression that happens to
evaluate to 0.)

--
greg

Greg Stark <gsstark@mit.edu> writes:

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

Hmm. Well, it showed the multiple incorrect uses of 0 as NULL in
dllist.c and other places,

Incidentally, while it may not be conformant to your style guidelines, use of
the constant 0 compared to or assigned to a pointer is a perfectly valid ANSI
spelling for NULL.

Absolutely. But I agree that it is more readable to use NULL when you
mean a null pointer, and 0 when you mean an integer zero. The C
standard may not distinguish these concepts, but I do ;-)

Something that I don't have a real strong feeling about is
if (ptr != NULL)
versus
if (ptr)
I've been known to write both. Can anyone mount a good readability
argument for one over the other?

How about the inverse case,
if (ptr == NULL)
versus
if (!ptr)
Applying a boolean ! to a pointer seems a bit shaky to me, though
it's certainly a common locution.

regards, tom lane

Tom Lane wrote:

Greg Stark <gsstark@mit.edu> writes:

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

Hmm. Well, it showed the multiple incorrect uses of 0 as NULL in
dllist.c and other places,

Incidentally, while it may not be conformant to your style guidelines, use of
the constant 0 compared to or assigned to a pointer is a perfectly valid ANSI
spelling for NULL.

Absolutely. But I agree that it is more readable to use NULL when you
mean a null pointer, and 0 when you mean an integer zero. The C
standard may not distinguish these concepts, but I do ;-)

Something that I don't have a real strong feeling about is
if (ptr != NULL)
versus
if (ptr)
I've been known to write both. Can anyone mount a good readability
argument for one over the other?

How about the inverse case,
if (ptr == NULL)
versus
if (!ptr)
Applying a boolean ! to a pointer seems a bit shaky to me, though
it's certainly a common locution.

If we allow "if (ptr)" then allowing the inverse to be "if (! ptr)"
seems logical enough. As you say, it's a very common idiom, and allowing
one without the other would be rather non-orthogonal.

cheers

andrew

On Sat, Jan 15, 2005 at 08:31:42AM -0500, Andrew Dunstan wrote:

Tom Lane wrote:

Something that I don't have a real strong feeling about is
if (ptr != NULL)
versus
if (ptr)
I've been known to write both. Can anyone mount a good readability
argument for one over the other?

I assume people don't like the PointerIsValid() macro defined in c.h?

Just for consistency we could use that everywhere or get rid of it ...
but then, maybe it is used by external code so we shouldn't do the
latter.

How about the inverse case,
if (ptr == NULL)
versus
if (!ptr)
Applying a boolean ! to a pointer seems a bit shaky to me, though
it's certainly a common locution.

If we allow "if (ptr)" then allowing the inverse to be "if (! ptr)"
seems logical enough. As you say, it's a very common idiom, and allowing
one without the other would be rather non-orthogonal.

I'd rather have legibility over orthogonality on this issue. I prefer
ptr == NULL myself, though not too strongly.

--
Alvaro Herrera (<alvherre[@]dcc.uchile.cl>)
"No necesitamos banderas
No reconocemos fronteras" (Jorge Gonz���lez)

Tom Lane <tgl@sss.pgh.pa.us> writes:

if (ptr)

Can anyone mount a good readability argument for one over the other?

My argument in favour of "if (ptr)" as well as "if (integer)" and "if (!ptr)"
is simply that all else being equal the shorter expression is clearer. Forcing
the reader to slog through additional syntax to read the code has a cost.

Obviously that argument is subject to abuse and it's very dependent on whether
you consider all else equal.

For a reduction argument, pretending there's a boolean type with no casts
leads to the excessively verbose java-ish syntax like:

if (foo != 0 && bar != null && baz != 0)

which you can't tell me is easier to read than

if (foo && bar && baz)

What I miss most in both C and Java is the lispish ability to write
expressions like:

foo = bar() || baz() || qux();

instead you have to write out things like this (except worse since you have to
start storing things in temporary variables):

if (bar() != null) {
foo = bar();
} else if (baz() != 0) {
foo = baz()
} else {
foo = qux();
}

I've seen code that consists of a dozen or so of these things repeated. When
the code starts getting so verbose that what could be 12 very simple and clear
lines no longer fits on the screen because you're trying to make it easier to
understand, well, I think you've lost the battle.

--
greg

On Sat, Jan 15, 2005 at 10:44:48 -0500,
Greg Stark <gsstark@mit.edu> wrote:

What I miss most in both C and Java is the lispish ability to write
expressions like:

foo = bar() || baz() || qux();

Are you sure that C doesn't guarenty short circuit evaluation?
I don't have my C reference handy, but my memory is that evaluation
will stop after the first function call that returns true in the
above expression.

Bruno Wolff III <bruno@wolff.to> writes:

Greg Stark <gsstark@mit.edu> wrote:

What I miss most in both C and Java is the lispish ability to write
expressions like:

foo = bar() || baz() || qux();

Are you sure that C doesn't guarenty short circuit evaluation?
I don't have my C reference handy, but my memory is that evaluation
will stop after the first function call that returns true in the
above expression.

Yeah, but you can only find out the boolean result, not the actually
returned value --- that is, foo will get 1 or 0.

regards, tom lane

Am Samstag, 15. Januar 2005 21:38 schrieb Bruno Wolff III:

On Sat, Jan 15, 2005 at 10:44:48 -0500,

Greg Stark <gsstark@mit.edu> wrote:

What I miss most in both C and Java is the lispish ability to write
expressions like:

foo = bar() || baz() || qux();

Are you sure that C doesn't guarenty short circuit evaluation?
I don't have my C reference handy, but my memory is that evaluation
will stop after the first function call that returns true in the
above expression.

C do guaranty short circuit evaluation.

You can also write:

(foo = bar()) || (foo = baz()) || (foo = qux())

this is a valid shortcut, where bar(), baz() and qux() are not evaluated
twice, like in the if-cascade. But it is a ugly style every stylechecker
should have no problems complaining about. Even a compiler would warn about
'=' and '=='-confusion. But you can fix it:

(foo = bar()) != NULL || (foo = baz()) != NULL || foo = qux()) != NULL;

It's short, but not quite that readable.

Tommi

Mark Wong wrote:

Ah, so you beat me to it Neil. ;) Out of curiosity, how much worse
was it before you started fixing things?

As I recall, not too different than things are today -- sparse flagged a
bunch of stylistic issues that I fixed, like:

void some_func() { ... } => void some_func(void) { ... }

extern void some_func(void) { ... } => void some_func(void) { ... }

etc. But given that there are 3000 odd warnings (mostly bogus), I doubt
I made a significant dent on the total warning count.

Also, I don't recall sparse picking up any significant errors or
problems. As I said, I've been meaning to explore if that's all that
sparse can do -- I just looked at the lowest of the low-hanging fruit.

-Neil