Thread: 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@pointblue.com.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 Sun, Aug 23, 2009 at 05:41:24PM +0100, Greg Stark wrote: > On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewicz<gj@pointblue.com.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@svana.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: > On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewicz<gj@pointblue.com.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 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@pointblue.com.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@mit.edu> writes: > On Sun, Aug 23, 2009 at 6:11 PM, Grzegorz Jaskiewicz<gj@pointblue.com.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
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 stringfor 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@pointblue.com.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@pointblue.com.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@pointblue.com.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@pointblue.com.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, constchar *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@pointblue.com.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@gmail.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.
Attachment
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@gmx.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
Attachment
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@pointblue.com.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
Attachment
Greg Stark <gsstark@mit.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@mit.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@pointblue.com.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@pointblue.com.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.
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@pointblue.com.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@pointblue.com.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@pointblue.com.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@pointblue.com.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@postgresql.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@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
2009/9/15 Michael Meskes <meskes@postgresql.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.