Re: [HACKERS] clang's static checker report.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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: mes...@jabber.org Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
2009/9/15 Michael Meskes mes...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Grzegorz Jaskiewicz g...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 - 1.92 +++ src/include/postgres.h 30 Aug 2009 11:17:50 - @@ -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... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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. postgres_checker_patch.patch.bz2 Description: BZip2 compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 30, 2009 at 3:26 PM, Peter Eisentrautpete...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 diff --git a/src/backend/tsearch/regis.c b/src/backend/tsearch/regis.c index 8980c2a..e492562 100644 --- a/src/backend/tsearch/regis.c +++ b/src/backend/tsearch/regis.c @@ -182,7 +182,7 @@ RS_free(Regis *r) static bool mb_strchr(char *str, char *c) { - int clen = pg_mblen(c), + int clen, plen, i; char *ptr = str; diff --git a/src/backend/tsearch/ts_parse.c b/src/backend/tsearch/ts_parse.c index f2440c4..c07488e 100644 --- a/src/backend/tsearch/ts_parse.c +++ b/src/backend/tsearch/ts_parse.c @@ -101,7 +101,6 @@ LexizeAddLemm(LexizeData *ld, int type, char *lemm, int lenlemm) { ParsedLex *newpl = (ParsedLex *) palloc(sizeof(ParsedLex)); - newpl = (ParsedLex *) palloc(sizeof(ParsedLex)); newpl-type = type; newpl-lemm = lemm; newpl-lenlemm = lenlemm; diff --git a/src/backend/utils/adt/encode.c b/src/backend/utils/adt/encode.c index b2042e7..1e144d9 100644 --- a/src/backend/utils/adt/encode.c +++ b/src/backend/utils/adt/encode.c @@ -159,7 +159,7 @@ hex_decode(const char *src, unsigned len, char *dst) *srcend; char v1, v2, - *p = dst; + *p; srcend = src + len; s = src; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 ;) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 30, 2009 at 3:57 PM, Grzegorz Jaskiewiczg...@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 diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 4939046..de011fe 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -23,6 +23,11 @@ #include utils/memutils.h +#undef MemoryContextAlloc +#undef MemoryContextAllocZero +#undef MemoryContextAllocZeroAligned +#undef repalloc + /* * GLOBAL MEMORY * diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h index e504ffa..d6ac006 100644 --- a/src/include/utils/palloc.h +++ b/src/include/utils/palloc.h @@ -49,6 +49,10 @@ extern void *MemoryContextAlloc(MemoryContext context, Size size); extern void *MemoryContextAllocZero(MemoryContext context, Size size); extern void *MemoryContextAllocZeroAligned(MemoryContext context, Size size); +#define MemoryContextAlloc(c,s) (MemoryContextAlloc((c),(s))?:(exit(1),NULL)) +#define MemoryContextAllocZero(c,s) (MemoryContextAlloc((c),(s))?:(exit(1),NULL)) +#define MemoryContextAllocZeroAligned(c,s) (MemoryContextAlloc((c),(s))?:(exit(1),NULL)) + #define palloc(sz) MemoryContextAlloc(CurrentMemoryContext, (sz)) #define palloc0(sz) MemoryContextAllocZero(CurrentMemoryContext, (sz)) @@ -70,6 +74,8 @@ extern void pfree(void *pointer); extern void *repalloc(void *pointer, Size size); +#define repalloc(p,s) (repalloc((p),(s))?:(exit(1),NULL)) + /* * MemoryContextSwitchTo can't be a macro in standard C compilers. * But we can make it an inline function when using GCC. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Greg Stark gsst...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Greg Stark gsst...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
with Greg's suggested palloc and friends patch: http://zlew.org/postgresql_static_check/scan-build-2009-08-30-3 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 30, 2009 at 5:58 PM, Grzegorz Jaskiewiczg...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On 30 Aug 2009, at 18:07, Greg Stark wrote: On Sun, Aug 30, 2009 at 5:58 PM, Grzegorz Jaskiewiczg...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
http://zlew.org/postgresql_static_check/scan-build-2009-08-30-4/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 ? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On 30 Aug 2009, at 19:14, Tom Lane wrote: Grzegorz Jaskiewicz g...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Grzegorz Jaskiewicz g...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Grzegorz Jaskiewicz g...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
We still have things like this showing division by zero: Assert(activeTapes 0); 1913slotsPerTape = (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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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)); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 - 1.258 +++ src/Makefile.global.in 27 Aug 2009 11:54:36 - @@ -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 - 1.52 +++ src/bin/pg_dump/pg_backup.h 27 Aug 2009 11:54:37 - @@ -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.h7 Aug 2009 22:48:34 - 1.82 +++ src/bin/pg_dump/pg_backup_archiver.h27 Aug 2009 11:54:37 - @@ -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 - 1.156 +++ src/bin/pg_dump/pg_dump.h 27 Aug 2009 11:54:37 - @@ -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 - 1.92 +++ src/include/postgres.h 27 Aug 2009 11:54:37 - @@ -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.h11 Jun 2009 14:49:13 - 1.75 +++ src/include/utils/datetime.h27 Aug 2009 11:54:37 - @@ -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
Re: [HACKERS] clang's static checker report.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Thu, Aug 27, 2009 at 06:39, Grzegorz Jaskiewiczg...@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?) :) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Alex Hunsaker bada...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewiczg...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On 23 Aug 2009, at 17:41, Greg Stark wrote: On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewiczg...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 23, 2009 at 05:41:24PM +0100, Greg Stark wrote: On Sun, Aug 23, 2009 at 4:57 PM, Grzegorz Jaskiewiczg...@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 klep...@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. signature.asc Description: Digital signature
Re: [HACKERS] clang's static checker report.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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 = 1; f++; printf(%f\n, f); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 23, 2009 at 6:11 PM, Grzegorz Jaskiewiczg...@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 = 1; 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Greg Stark gsst...@mit.edu writes: On Sun, Aug 23, 2009 at 6:11 PM, Grzegorz Jaskiewiczg...@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 = 1; 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
memory leak: http://zlew.org/postgresql_static_check/scan-build-2009-08-23-5/report-46wcmJ.html#EndPath -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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.h11 Jun 2009 14:49:13 - 1.101 +++ src/include/utils/elog.h23 Aug 2009 19:20:55 - @@ -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 */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
Grzegorz Jaskiewicz g...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On 23 Aug 2009, at 20:31, Tom Lane wrote: Grzegorz Jaskiewicz g...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
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.h11 Jun 2009 14:49:13 - 1.101 +++ src/include/utils/elog.h23 Aug 2009 22:16:05 - @@ -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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On Sun, Aug 23, 2009 at 11:16 PM, Grzegorz Jaskiewiczg...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] clang's static checker report.
On 24 Aug 2009, at 00:15, Greg Stark wrote: On Sun, Aug 23, 2009 at 11:16 PM, Grzegorz Jaskiewiczg...@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 - 1.52 +++ src/bin/pg_dump/pg_backup.h 23 Aug 2009 23:31:43 - @@ -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 - 1.156 +++ src/bin/pg_dump/pg_dump.h 23 Aug 2009 23:31:43 - @@ -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 - 1.92 +++ src/include/postgres.h 23 Aug 2009 23:31:43 - @@ -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.h11 Jun 2009 14:49:13 - 1.101 +++ src/include/utils/elog.h23 Aug 2009 23:31:43 - @@ -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... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers