Re: clang's static checker report.

So,

after successful, and helpful Saturday with llvm's clang static
checker, I decided to run it against postgresql's source code.
Result can be seen at: http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/
.
One directory below is the tar file, with the report.

I am sure there's plenty of false positives, but I am also quite sure
there's many real errors on that list. As I have rather bad
experiences with any patches sent here - I hope that's least I can
help with.

To run clang-check I had to change one of the makefiles slightly, as
the postgresql's build system seems to ignore $CC variable
completely , always sticking to the one chosen by the configure script.

The changed file is ./src/Makefile.global. Around line 210 I included
ifdef CC, like that:

ifndef CC
CC = gcc -no-cpp-precomp
GCC = yes
endif

Which later allowed me to run "scan-build make" without issues.

hope that's helpfull.

thanks.

On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> I am sure there's plenty of false positives, but I am also quite sure
> there's many real errors on that list.

Do you know how to teach clang about functions which never return?
That seems to be causing most of the false positives because it
doesn't recognize that our error checks stop execution and avoid the
use of the unitialized variables afterwards.

--
greg
http://mit.edu/~gsstark/resume.pdf

On 23 Aug 2009, at 17:41, Greg Stark wrote:

> On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl
> > wrote:
>> I am sure there's plenty of false positives, but I am also quite sure
>> there's many real errors on that list.
>
> Do you know how to teach clang about functions which never return?
> That seems to be causing most of the false positives because it
> doesn't recognize that our error checks stop execution and avoid the
> use of the unitialized variables afterwards.

I am not the clang developer, so I honestly have no idea how to do it.
But as far as I checked report myself, there's couple 'division by
zero', and 'null reference' errors that looked plausible to someone
as unfamiliar with the postgresql's source as myself.
Like with all static checkers, this one will generate a lot of false
positives, and it is the inevitable cost of using such a tool having
to go through all errors and sieve out positives yourself.

You probably refer to the functions that never return. Sadly, even tho
llvm clang is capable of doing so (one of its strengths is linking
optimization) - the checker is unable to cross reference files, or so
it seems.

Well, like I said - l hope at least part of that report is useful.

On Sun, Aug 23, 2009 at 05:41:24PM +0100, Greg Stark wrote:
> On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> > I am sure there's plenty of false positives, but I am also quite sure
> > there's many real errors on that list.
>
> Do you know how to teach clang about functions which never return?
> That seems to be causing most of the false positives because it
> doesn't recognize that our error checks stop execution and avoid the
> use of the unitialized variables afterwards.

This caused many of the false positives in Coverity's tool as well.

The way you work around it is by altering the definition of
elog/ereport slightly so that after the usual expansion you add:

if( level >= ERROR) exit(1)

If it were just a matter of a function that didn't return it'd be easy.
What we have is a function that doesn't return depending on the
arguments, this is much trickier.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

On 23 Aug 2009, at 17:41, Greg Stark wrote:
>
> Do you know how to teach clang about functions which never return?

http://clang-analyzer.llvm.org/annotations.html#attr_noreturn

http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/report-MAVb5D.html#EndPath
for a very positive one - at least from strict language point of view.

consider: float f = 100000000; f++; printf("%f\n", f);

On Sun, Aug 23, 2009 at 6:11 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/report-MAVb5D.html#EndPath
> for a very positive one - at least from strict language point of view.
>
> consider: float f = 100000000; f++; printf("%f\n", f);

I believe the maximum value of the numbers involved here is the sample
size which is currently capped at 10,000. But I'm not exactly sure.

--
greg
http://mit.edu/~gsstark/resume.pdf

about the false positives around 'null reference'. I'll try sticking
exit(1)'s at the end of each macro - and see if that makes most of
them go away.

Greg Stark <gsstark(at)mit(dot)edu> writes:
> On Sun, Aug 23, 2009 at 6:11 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
>> http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/report-MAVb5D.html#EndPath
>> for a very positive one - at least from strict language point of view.
>>
>> consider: float f = 100000000; f++; printf("%f\n", f);

> I believe the maximum value of the numbers involved here is the sample
> size which is currently capped at 10,000. But I'm not exactly sure.

No, the maximum value is somewhere around the maximum number of rows in
a table, which is on the rough order of 4e12, which is several orders of
magnitude below the threshold at which counting in a double becomes
inaccurate. It is, however, above the point at which counting in an
int32 will overflow. So the alternative would be to assume that we have
a working int64 data type, which doesn't strike me as an improvement
in the portability of the code.

regards, tom lane

memory leak:
http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/report-46wcmJ.html#EndPath

this one should contain substantialy less false positives, because
error functions were marked as the 'never exit' points:

http://zlew.org/postgresql_static_check/scan-build-2009-08-23-9/

for the record, here's patch that marks elog, etc as dead ends:

Index: src/include/utils/elog.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/elog.h,v
retrieving revision 1.101
diff -u -b -r1.101 elog.h
--- src/include/utils/elog.h 11 Jun 2009 14:49:13 -0000 1.101
+++ src/include/utils/elog.h 23 Aug 2009 19:20:55 -0000
@@ -112,8 +112,8 @@
#define TEXTDOMAIN NULL

extern bool errstart(int elevel, const char *filename, int lineno,
- const char *funcname, const char *domain);
-extern void errfinish(int dummy,...);
+ const char *funcname, const char *domain)
__attribute__((analyzer_noreturn));
+extern void errfinish(int dummy,...)
__attribute__((analyzer_noreturn));

extern int errcode(int sqlerrcode);

@@ -197,7 +197,7 @@
elog_finish(int elevel, const char *fmt,...)
/* This extension allows gcc to check the format string for
consistency with
the supplied arguments. */
-__attribute__((format(printf, 2, 3)));
+__attribute__((format(printf, 2, 3)))
__attribute__((analyzer_noreturn));

/* Support for attaching context information to error reports */

Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
> for the record, here's patch that marks elog, etc as dead ends:

That does not look like the right thing at all, since now the checker
will believe that elog(NOTICE) and such don't return. I think you
need to use Martijn's suggestion instead.

regards, tom lane

On 23 Aug 2009, at 20:31, Tom Lane wrote:

> Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
>> for the record, here's patch that marks elog, etc as dead ends:
>
> That does not look like the right thing at all, since now the checker
> will believe that elog(NOTICE) and such don't return. I think you
> need to use Martijn's suggestion instead.
>

Still, there are few worrying finds on that list as it is anyway.
I hope you guys will find it useful.

I'll modify macro according to Martijn's suggesion, and rerun it again.
My laptop is pretty slow, so it will be probably another 1-1.5h before
I'll get it.

ok folks, here's the last one for Today:

http://zlew.org/postgresql_static_check/scan-build-2009-08-23-29/

tar ball with report can be downloaded from here: http://zlew.org/postgresql_static_check/postgresql_static_check_23rdAugust2009.tar.xz
(compressed with lzma's xz tool).

here's the patch for elog stuff:

Index: src/include/utils/elog.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/elog.h,v
retrieving revision 1.101
diff -u -b -r1.101 elog.h
--- src/include/utils/elog.h 11 Jun 2009 14:49:13 -0000 1.101
+++ src/include/utils/elog.h 23 Aug 2009 22:16:05 -0000
@@ -104,7 +104,7 @@
*/
#define ereport_domain(elevel, domain, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
- (errfinish rest) : (void) 0)
+ (errfinish rest) : (void) 0), (elevel >= ERROR) ? exit(1) : 0

#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -190,7 +190,7 @@
* elog(ERROR, "portal \"%s\" not found", stmt->portalname);
*----------
*/
-#define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO),
elog_finish
+#define elog(A, ...) elog_start(__FILE__, __LINE__,
PG_FUNCNAME_MACRO), elog_finish(A, __VA_ARGS__), (A >= ERROR) ?
exit(1) : 0

extern void elog_start(const char *filename, int lineno, const char
*funcname);
extern void

hopefully satisfying everyone.

Hope to see few fixes out of that ;)

thanks.

On Sun, Aug 23, 2009 at 11:16 PM, Grzegorz
Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> ok folks, here's the last one for Today:
>
> http://zlew.org/postgresql_static_check/scan-build-2009-08-23-29/

This does look better. The first one I looked at looks like a
legitimate bug. The nice thing is that this seems to be picking up a
lot of error handling cases that we don't bother to have regression
tests for.

One more request though. Can you configure with --enable-assertions so
that it doesn't pick up failures where we have already documented that
the case it's claiming can happen can't happen. Those could possibly
be bugs but they're more likely to be cases where we know that a given
data structure's rep invariant prohibits the combination of states
that it's assuming.

--
greg
http://mit.edu/~gsstark/resume.pdf

On 24 Aug 2009, at 00:15, Greg Stark wrote:

> On Sun, Aug 23, 2009 at 11:16 PM, Grzegorz
> Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
>> ok folks, here's the last one for Today:
>>
>> http://zlew.org/postgresql_static_check/scan-build-2009-08-23-29/
>
> This does look better. The first one I looked at looks like a
> legitimate bug. The nice thing is that this seems to be picking up a
> lot of error handling cases that we don't bother to have regression
> tests for.
true
>
> One more request though. Can you configure with --enable-assertions so

--enable-cassert, enabled, and also added exit_* in pg_dump to list
of functions that never return.

new report's at: http://zlew.org/postgresql_static_check/scan-build-2009-08-24-2/

the archive is at : http://zlew.org/postgresql_static_check/postgresql_static_check_24thAugust2009.tar.xz

So that the overall 'static check' patch now looks like this:

Index: src/bin/pg_dump/pg_backup.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup.h,v
retrieving revision 1.52
diff -u -b -r1.52 pg_backup.h
--- src/bin/pg_dump/pg_backup.h 11 Jun 2009 14:49:07 -0000 1.52
+++ src/bin/pg_dump/pg_backup.h 23 Aug 2009 23:31:43 -0000
@@ -150,7 +150,7 @@

extern void
exit_horribly(Archive *AH, const char *modulename, const char
*fmt,...)
-__attribute__((format(printf, 3, 4)));
+__attribute__((format(printf, 3, 4)))
__attribute__((analyzer_noreturn));

/* Lets the archive know we have a DB connection to shutdown if it
dies */
Index: src/bin/pg_dump/pg_dump.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dump.h,v
retrieving revision 1.156
diff -u -b -r1.156 pg_dump.h
--- src/bin/pg_dump/pg_dump.h 2 Aug 2009 22:14:52 -0000 1.156
+++ src/bin/pg_dump/pg_dump.h 23 Aug 2009 23:31:43 -0000
@@ -481,7 +481,7 @@
extern void *pg_realloc(void *ptr, size_t size);

extern void check_conn_and_db(void);
-extern void exit_nicely(void);
+extern void exit_nicely(void) __attribute__((analyzer_noreturn));

extern void parseOidArray(const char *str, Oid *array, int arraysize);

Index: src/include/postgres.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/postgres.h,v
retrieving revision 1.92
diff -u -b -r1.92 postgres.h
--- src/include/postgres.h 1 Jan 2009 17:23:55 -0000 1.92
+++ src/include/postgres.h 23 Aug 2009 23:31:43 -0000
@@ -691,6 +691,6 @@

extern int ExceptionalCondition(const char *conditionName,
const char *errorType,
- const char *fileName, int lineNumber);
+ const char *fileName, int lineNumber)
__attribute__((analyzer_noreturn));

#endif /* POSTGRES_H */
Index: src/include/utils/elog.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/elog.h,v
retrieving revision 1.101
diff -u -b -r1.101 elog.h
--- src/include/utils/elog.h 11 Jun 2009 14:49:13 -0000 1.101
+++ src/include/utils/elog.h 23 Aug 2009 23:31:43 -0000
@@ -104,7 +104,7 @@
*/
#define ereport_domain(elevel, domain, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
- (errfinish rest) : (void) 0)
+ (errfinish rest) : (void) 0), (elevel >= ERROR) ? exit(1) : 0

#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -190,7 +190,7 @@
* elog(ERROR, "portal \"%s\" not found", stmt->portalname);
*----------
*/
-#define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO),
elog_finish
+#define elog(A, ...) elog_start(__FILE__, __LINE__,
PG_FUNCNAME_MACRO), elog_finish(A, __VA_ARGS__), (A >= ERROR) ?
exit(1) : 0

extern void elog_start(const char *filename, int lineno, const char
*funcname);

That's it folks for Today, gotta go to sleep.
Have fun...

On mån, 2009-08-24 at 00:42 +0100, Grzegorz Jaskiewicz wrote:
> --enable-cassert, enabled, and also added exit_* in pg_dump to list
> of functions that never return.

A few more functions to mark noreturn: DateTimeParseError(), and
die_horribly() in pg_dump

On 24 Aug 2009, at 14:40, Peter Eisentraut wrote:

> On mån, 2009-08-24 at 00:42 +0100, Grzegorz Jaskiewicz wrote:
>> --enable-cassert, enabled, and also added exit_* in pg_dump to list
>> of functions that never return.
>
> A few more functions to mark noreturn: DateTimeParseError(), and
> die_horribly() in pg_dump

done.

new scan at: http://zlew.org/postgresql_static_check/scan-build-2009-08-27-2/
archive at: http://zlew.org/postgresql_static_check/postgresql_static_check_27thAugust2009.tar.xz

If people find it useful (altho, I've only seen single commit as
result of that checker, and nothing fancy either) - I can write a
script that would update it on daily basis.

what you people say ?

New Patch :

Index: src/Makefile.global.in
===================================================================
RCS file: /projects/cvsroot/pgsql/src/Makefile.global.in,v
retrieving revision 1.258
diff -u -b -r1.258 Makefile.global.in
--- src/Makefile.global.in 26 Aug 2009 22:24:42 -0000 1.258
+++ src/Makefile.global.in 27 Aug 2009 11:54:36 -0000
@@ -205,7 +205,10 @@
endif
endif # not PGXS

+ifndef CC
CC = @CC@
+endif
+
GCC = @GCC@
SUN_STUDIO_CC = @SUN_STUDIO_CC@
CFLAGS = @CFLAGS@
Index: src/bin/pg_dump/pg_backup.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup.h,v
retrieving revision 1.52
diff -u -b -r1.52 pg_backup.h
--- src/bin/pg_dump/pg_backup.h 11 Jun 2009 14:49:07 -0000 1.52
+++ src/bin/pg_dump/pg_backup.h 27 Aug 2009 11:54:37 -0000
@@ -150,7 +150,7 @@

extern void
exit_horribly(Archive *AH, const char *modulename, const char
*fmt,...)
-__attribute__((format(printf, 3, 4)));
+__attribute__((format(printf, 3, 4))) __attribute__
((analyzer_noreturn));

/* Lets the archive know we have a DB connection to shutdown if it
dies */
Index: src/bin/pg_dump/pg_backup_archiver.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_backup_archiver.h,v
retrieving revision 1.82
diff -u -b -r1.82 pg_backup_archiver.h
--- src/bin/pg_dump/pg_backup_archiver.h 7 Aug 2009 22:48:34 -0000 1.82
+++ src/bin/pg_dump/pg_backup_archiver.h 27 Aug 2009 11:54:37 -0000
@@ -325,7 +325,7 @@
/* Used everywhere */
extern const char *progname;

-extern void die_horribly(ArchiveHandle *AH, const char *modulename,
const char *fmt,...) __attribute__((format(printf, 3, 4)));
+extern void die_horribly(ArchiveHandle *AH, const char *modulename,
const char *fmt,...) __attribute__((format(printf, 3, 4)))
__attribute__((analyzer_noreturn));
extern void warn_or_die_horribly(ArchiveHandle *AH, const char
*modulename, const char *fmt,...) __attribute__((format(printf, 3, 4)));
extern void write_msg(const char *modulename, const char *fmt,...)
__attribute__((format(printf, 2, 3)));

Index: src/bin/pg_dump/pg_dump.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dump.h,v
retrieving revision 1.156
diff -u -b -r1.156 pg_dump.h
--- src/bin/pg_dump/pg_dump.h 2 Aug 2009 22:14:52 -0000 1.156
+++ src/bin/pg_dump/pg_dump.h 27 Aug 2009 11:54:37 -0000
@@ -481,7 +481,7 @@
extern void *pg_realloc(void *ptr, size_t size);

extern void check_conn_and_db(void);
-extern void exit_nicely(void);
+extern void exit_nicely(void) __attribute__((analyzer_noreturn));

extern void parseOidArray(const char *str, Oid *array, int arraysize);

Index: src/include/postgres.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/postgres.h,v
retrieving revision 1.92
diff -u -b -r1.92 postgres.h
--- src/include/postgres.h 1 Jan 2009 17:23:55 -0000 1.92
+++ src/include/postgres.h 27 Aug 2009 11:54:37 -0000
@@ -691,6 +691,6 @@

extern int ExceptionalCondition(const char *conditionName,
const char *errorType,
- const char *fileName, int lineNumber);
+ const char *fileName, int lineNumber) __attribute__
((analyzer_noreturn));

#endif /* POSTGRES_H */
Index: src/include/utils/datetime.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/datetime.h,v
retrieving revision 1.75
diff -u -b -r1.75 datetime.h
--- src/include/utils/datetime.h 11 Jun 2009 14:49:13 -0000 1.75
+++ src/include/utils/datetime.h 27 Aug 2009 11:54:37 -0000
@@ -300,7 +300,7 @@
int *dtype, struct pg_tm * tm, fsec_t *fsec);

extern void DateTimeParseError(int dterr, const char *str,
- const char *datatype);
+ const char *datatype) __attribute__((__noreturn__));

extern int DetermineTimeZoneOffset(struct pg_tm * tm, pg_tz *tzp);

Index: src/include/utils/elog.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/elog.h,v
retrieving revision 1.101
diff -u -b -r1.101 elog.h
--- src/include/utils/elog.h 11 Jun 2009 14:49:13 -0000 1.101
+++ src/include/utils/elog.h 27 Aug 2009 11:54:37 -0000
@@ -104,7 +104,7 @@
*/
#define ereport_domain(elevel, domain, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
- (errfinish rest) : (void) 0)
+ (errfinish rest) : (void) 0), (elevel >= ERROR) ? exit(1) : 0

#define ereport(elevel, rest) \
ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -190,7 +190,7 @@
* elog(ERROR, "portal \"%s\" not found", stmt->portalname);
*----------
*/
-#define elog elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO),
elog_finish
+#define elog(A, ...) elog_start(__FILE__, __LINE__,
PG_FUNCNAME_MACRO), elog_finish(A, __VA_ARGS__), (A >= ERROR) ? exit
(1) : 0

extern void elog_start(const char *filename, int lineno, const char
*funcname);
extern void

heh, sorry folks for the noise again :/

There was a fair amount of false positives there - due to nature of
Assert() macro. Mainly, since assert_enabled is a runtime variable,
not a macro (which I sadly overlooked).

So, hardcoding it to (1) (using CPP) removed quite few false positives.

New results at:

http://zlew.org/postgresql_static_check/scan-build-2009-08-27-4/
archive at: http://zlew.org/postgresql_static_check/postgresql_static_check_27thAugust2009_2.tar.xz

Please tell me, if you think that it can be improved more.

On Thu, Aug 27, 2009 at 06:39, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> heh, sorry folks for the noise again :/
>
> There was a fair amount of false positives there - due to nature of Assert()
> macro. Mainly, since assert_enabled is a runtime variable, not a macro
> (which I sadly overlooked).
>
> So, hardcoding it to (1) (using CPP) removed quite few false positives.
>
> New results at:
>
> http://zlew.org/postgresql_static_check/scan-build-2009-08-27-4/
> archive at:
> http://zlew.org/postgresql_static_check/postgresql_static_check_27thAugust2009_2.tar.xz
>
> Please tell me, if you think that it can be improved more.

Looks like your still missing ExitPostmaster(1) see
http://zlew.org/postgresql_static_check/scan-build-2009-08-27-4/report-iqR9gz.html#EndPath.

and maybe ereport(ERROR) ?

see http://zlew.org/postgresql_static_check/scan-build-2009-08-27-4/report-gkkK9S.html#EndPath
it calls report_untranslatable_char() which in turn calls ereport(ERROR)
(do you have to mark every function that calls ereport(ERROR) as one
that exits?)

:)

Alex Hunsaker <badalex(at)gmail(dot)com> writes:
> (do you have to mark every function that calls ereport(ERROR) as one
> that exits?)

That would be an open-ended project, and probably in many cases wouldn't
change the clang report anyway. I think it's only worth worrying about
the ones that we find will suppress some false positives.

regards, tom lane

new one at http://zlew.org/postgresql_static_check/scan-build-2009-08-29-3/
archive at : http://zlew.org/postgresql_static_check/postgresql_static_check_29thAugust2009.tar.xz

as always, comments are welcomed. And constructive explanation of any
wrong-results will be relayed to clang-checker developer(s).

hth.

We still have things like this showing "division by zero":

Assert(activeTapes > 0);
1913 slotsPerTape = (state->memtupsize - state->mergefirstfree) / activeTapes;

It looks like if you marked ExceptionalCondition() as never returning
then it should hide this.

--
greg
http://mit.edu/~gsstark/resume.pdf

On 29 Aug 2009, at 17:35, Greg Stark wrote:

> We still have things like this showing "division by zero":
>
> Assert(activeTapes > 0);
> 1913 slotsPerTape = (state->memtupsize - state->mergefirstfree) /
> activeTapes;
>
>
> It looks like if you marked ExceptionalCondition() as never returning
> then it should hide this.
well, it is marked as such , here's excerpt from differences to head:

extern int ExceptionalCondition(const char *conditionName,
const char *errorType,
- const char *fileName, int
lineNumber);
+ const char *fileName, int
lineNumber) __attribute__((analyzer_noreturn));

Oh, I think I see what's happening. Our assertions can still be turned
off at run-time with the variable assert_enabled.

Hm, you would have to replace assert_enabled with a #define in
postgres.h and then do something about the guc.c code which assigns to
it.

On another note is there any way to marke MemoryContextAlloc,
MemoryContextAllocZero, palloc, repalloc, and friends as never
returning NULL? I think that's causing most of the "null dereferenced"
errors.

On 29 Aug 2009, at 18:05, Greg Stark wrote:

> Oh, I think I see what's happening. Our assertions can still be turned
> off at run-time with the variable assert_enabled.

Index: src/include/postgres.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/postgres.h,v
retrieving revision 1.92
diff -b -u -r1.92 postgres.h
--- src/include/postgres.h 1 Jan 2009 17:23:55 -0000 1.92
+++ src/include/postgres.h 30 Aug 2009 11:17:50 -0000
@@ -639,6 +639,7 @@
*/

extern PGDLLIMPORT bool assert_enabled;
+#define assert_enabled (1)

/*
* USE_ASSERT_CHECKING, if defined, turns on all the assertions.
@@ -666,7 +667,7 @@
* Isn't CPP fun?
*/
#define TrapMacro(condition, errorType) \
- ((bool) ((! assert_enabled) || ! (condition) || \
+ ((bool) ( ! (condition) || \
(ExceptionalCondition(CppAsString(condition), (errorType), \
__FILE__, __LINE__))))

@@ -689,8 +690,10 @@
Trap(!(condition), "BadState")
#endif /* USE_ASSERT_CHECKING */

+#undef assert_enabled
+
extern int ExceptionalCondition(const char *conditionName,
const char *errorType,
- const char *fileName, int lineNumber);
+ const char *fileName, int lineNumber) __attribute__
((analyzer_noreturn));

like that ?
This is another excerpt from my local mods, that I use before running
clang-checker over it.

but looking at Assert() macros in code (it expands macros if you
hoover mouse pointer over one) - it still keeps 'assert_enabled'
literal there. damn...

okay, I think I nailed the assert part right. (3rd iteration, about
time...).
as usual: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-2/

archive one dir up.

the current patch attached.

On lör, 2009-08-29 at 17:35 +0100, Greg Stark wrote:
> We still have things like this showing "division by zero":
>
> Assert(activeTapes > 0);
> 1913 slotsPerTape = (state->memtupsize - state->mergefirstfree) / activeTapes;
>
>
> It looks like if you marked ExceptionalCondition() as never returning
> then it should hide this.

Well, if you can disable the assertion, then there is still a possible
bug here, no?

On Sun, Aug 30, 2009 at 3:26 PM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
> On lör, 2009-08-29 at 17:35 +0100, Greg Stark wrote:
>> We still have things like this showing "division by zero":
>>
>> Assert(activeTapes > 0);
>> 1913          slotsPerTape = (state->memtupsize - state->mergefirstfree) / activeTapes;
>>
>>
>> It looks like if you marked ExceptionalCondition() as never returning
>> then it should hide this.
>
> Well, if you can disable the assertion, then there is still a possible
> bug here, no?

Yeah, the problem is that clang doesn't know our rep invariants and
inter-procedure call signature guarantees. Ie, activeTapes here is
initialized to the count of tapes in the tuplesort state when a merge
begins. Clang doesn't know that we never call beginmerge on a
tuplesort that has no active tapes.

So going on the assumption that our Asserts indicate somebody actually
thought about the case they cover and checked that it's a reasonable
assumption... then we don't need clang to tell us about them.

I think most of the remaining false positives are cases where palloc,
palloc0, repalloc, MemoryContextAlloc, or MemoryContextAllocZero
return values are deferenced. Clang doesn't know that these functions
never return NULL so it's marking every case as a possible NULL
dereference.

--
greg
http://mit.edu/~gsstark/resume.pdf

So three of the four dead initialization warnings are legitimate -- if
minor -- errors. Attached is a patch to remove the redundant
initializations.

--
greg
http://mit.edu/~gsstark/resume.pdf

On 30 Aug 2009, at 15:46, Greg Stark wrote:

> So three of the four dead initialization warnings are legitimate -- if
> minor -- errors. Attached is a patch to remove the redundant
> initializations.

well, all I can do is this: http://llvm.org/bugs/show_bug.cgi?id=4832

I find it hard to belive tho, that it only found 4 possible positives ;)

On Sun, Aug 30, 2009 at 3:57 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
>
> On 30 Aug 2009, at 15:46, Greg Stark wrote:
>
>> So three of the four dead initialization warnings are legitimate -- if
>> minor -- errors. Attached is a patch to remove the redundant
>> initializations.
>
>
> well, all I can do is this: http://llvm.org/bugs/show_bug.cgi?id=4832
>
> I find it hard to belive tho, that it only found 4 possible positives ;)

You might try something like the attached.

--
greg
http://mit.edu/~gsstark/resume.pdf

Greg Stark <gsstark(at)mit(dot)edu> writes:
> I think most of the remaining false positives are cases where palloc,
> palloc0, repalloc, MemoryContextAlloc, or MemoryContextAllocZero
> return values are deferenced. Clang doesn't know that these functions
> never return NULL so it's marking every case as a possible NULL
> dereference.

If clang assumes that every function that returns a pointer could return
NULL, then we are going to have many many many many false positives
at levels far removed from palloc. I'd almost suggest that we look for
a way to reverse its default assumption about that. Failing that,
I fear we shall simply have to ignore that particular message as
uselessly noisy.

regards, tom lane

Greg Stark <gsstark(at)mit(dot)edu> writes:
> So three of the four dead initialization warnings are legitimate -- if
> minor -- errors. Attached is a patch to remove the redundant
> initializations.

Applied, thanks.

regards, tom lane

with Greg's suggested palloc and friends patch: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-3

On Sun, Aug 30, 2009 at 5:58 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl> wrote:
> with Greg's suggested palloc and friends patch:
> http://zlew.org/postgresql_static_check/scan-build-2009-08-30-3

Argh. That didn't help at all. hm, I suppose instead of (exit(1),NULL)
we could just put ((void*)1) there?

But I think Tom's right. Worse, I think until it can do
inter-procedural analysis these messages will always be nearly all
false positives. Many if not most of our functions take pointers or
data structures which contain pointers as arguments or return values.
Most of the time those arguments and return values cannot contain NULL
pointers and the code doesn't bother to check that every single time.

--
greg
http://mit.edu/~gsstark/resume.pdf

On 30 Aug 2009, at 18:07, Greg Stark wrote:

> On Sun, Aug 30, 2009 at 5:58 PM, Grzegorz Jaskiewicz<gj(at)pointblue(dot)com(dot)pl
> > wrote:
>> with Greg's suggested palloc and friends patch:
>> http://zlew.org/postgresql_static_check/scan-build-2009-08-30-3
>
> Argh. That didn't help at all. hm, I suppose instead of (exit(1),NULL)
> we could just put ((void*)1) there?
>
> But I think Tom's right. Worse, I think until it can do
> inter-procedural analysis these messages will always be nearly all
> false positives. Many if not most of our functions take pointers or
> data structures which contain pointers as arguments or return values.
> Most of the time those arguments and return values cannot contain NULL
> pointers and the code doesn't bother to check that every single time.

sure, I can try.
Btw, I got response to my bug from llvm devs, and they fully agree on
all that.

http://zlew.org/postgresql_static_check/scan-build-2009-08-30-4/

please don't tell me that this is bogus: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-4/report-7JaICX.html#EndPath
?
How come ?

Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
> please don't tell me that this is bogus: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-4/report-7JaICX.html#EndPath

Yes, it's bogus. ctid is never passed as NULL. It might point at
an "invalid" itempointer (one with ip_posid == 0). Look at the only
call site.

This seems to indicate that clang is stupider than I would have hoped.
Isn't it capable of doing the same types of analysis that gcc does
when inlining?

regards, tom lane

On 30 Aug 2009, at 19:14, Tom Lane wrote:

> Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
>> please don't tell me that this is bogus: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-4/report-7JaICX.html#EndPath
>
> Yes, it's bogus. ctid is never passed as NULL. It might point at
> an "invalid" itempointer (one with ip_posid == 0). Look at the only
> call site.

so why do we check if the pointer is valid ?

>
> This seems to indicate that clang is stupider than I would have hoped.
> Isn't it capable of doing the same types of analysis that gcc does
> when inlining?

well, it is usable, but that doesn't mean complete. That's why I am
trying to work both ways to provide some info to clang-checker devs,
and you guys - with the reports it generates.
on the side note, xcode in snow leopard uses it under the hood to do
'build & analyze', and it helped me to locate few potential issues in
my iphone code. Now, of course my code is times less complicated than
postgresql's, but still - it is potentially useful.

Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
> On 30 Aug 2009, at 19:14, Tom Lane wrote:
>> Yes, it's bogus. ctid is never passed as NULL. It might point at
>> an "invalid" itempointer (one with ip_posid == 0). Look at the only
>> call site.

> so why do we check if the pointer is valid ?

[ shrug... ] The macro is more general than is necessary in this
specific context. In an actual build I'd expect the compiler to figure
out that the null-pointer test is redundant and optimize it away, since
after inlining it would see that ctid is the address of a local variable.

regards, tom lane

new report: http://zlew.org/postgresql_static_check/scan-build-2009-09-02-1/

archive one dir up, as usual (with index of all previous reports).

Grzegorz Jaskiewicz escribió:
> new report: http://zlew.org/postgresql_static_check/scan-build-2009-09-02-1/
>
> archive one dir up, as usual (with index of all previous reports).

What's with the "analyzer failures"? Did you submit bug reports about
them?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 2 Sep 2009, at 21:38, Alvaro Herrera wrote:

> Grzegorz Jaskiewicz escribió:
>> new report: http://zlew.org/postgresql_static_check/scan-build-2009-09-02-1/
>>
>> archive one dir up, as usual (with index of all previous reports).
>
> What's with the "analyzer failures"? Did you submit bug reports about
> them?

honestly, why would you even ask me such a question....

My currently open bugs:

http://llvm.org/bugs/show_bug.cgi?id=4867
http://llvm.org/bugs/show_bug.cgi?id=4832

clang developers were quick to iron out their bugs, here's Today report:

http://zlew.org/postgresql_static_check/scan-build-2009-09-04-1/

usual round of updates to the scan report.
Today's report available at:

http://zlew.org/postgresql_static_check/scan-build-2009-09-12-1/

Grzegorz Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> writes:
> usual round of updates to the scan report.
> Today's report available at:

> http://zlew.org/postgresql_static_check/scan-build-2009-09-12-1/

Looks like the clang guys still have some work to do. The
null-dereference reports, in particular, seem to be willing to make
self-contradictory assumptions in order to claim there is a possibility
of a null dereference. The clearest example I found was this one:
http://zlew.org/postgresql_static_check/scan-build-2009-09-12-1/report-Ybdv3J.html#EndPath
where to conclude that lp might be null, clang first assumes
PageGetMaxOffsetNumber(page) < offnum (at line 4251); but it then
must assume that that is *false* in order to suppose that control
can arrive at the dereference inside ItemIdIsNormal at line 4254.

regards, tom lane

On 14 Sep 2009, at 06:04, Tom Lane wrote:
> Looks like the clang guys still have some work to do.

Thanks Tom, reported to clang dev's .

meanwhile, since quite a lot stuff went in over weekend, and since
Yesterday, new report at:

http://zlew.org/postgresql_static_check/scan-build-2009-09-14-1/

>
http://llvm.org/bugs/show_bug.cgi?id=4979

will see, one issue is already fixed. I'll retry when the second one
is too.
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Mon, Sep 14, 2009 at 06:39:11PM +0100, Grzegorz Jaskiewicz wrote:
> meanwhile, since quite a lot stuff went in over weekend, and since
> Yesterday, new report at:
>
> http://zlew.org/postgresql_static_check/scan-build-2009-09-14-1/

Looking at
http://zlew.org/postgresql_static_check/scan-build-2009-09-14-1/report-3LPmKK.html#EndPath
it tells me that the value stored to 'counter' is never used. However, the
"counter++" is called inside a loop and thus will be read the next time the
loop is run.

Looks to me like a bug, or did I miss something?

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

2009/9/15 Michael Meskes <meskes(at)postgresql(dot)org>:

> Looking at
> http://zlew.org/postgresql_static_check/scan-build-2009-09-14-1/report-3LPmKK.html#EndPath
> it tells me that the value stored to 'counter' is never used. However, the
> "counter++" is called inside a loop and thus will be read the next time the
> loop is run.
>
> Looks to me like a bug, or did I miss something?

I guess that the problem is that the variable "counter" is declared
inside that loop itself.

Nicolas

new version under:
http://zlew.org/postgresql_static_check/scan-build-2009-10-03-1/

What's strange, is the increase of 48.2 percent in reports, that
happened about two weeks before (weekend before the previous one).

enjoy.