Re: [HACKERS] clang's static checker report.

2009-10-03 Thread Grzegorz Jaskiewicz

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.

2009-09-15 Thread Grzegorz Jaskiewicz



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.

2009-09-15 Thread Michael Meskes
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-09-15 Thread Nicolas Barbier
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.

2009-09-14 Thread Grzegorz Jaskiewicz


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.

2009-09-13 Thread Tom Lane
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.

2009-09-12 Thread Grzegorz Jaskiewicz

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.

2009-09-04 Thread Grzegorz Jaskiewicz

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.

2009-09-02 Thread Grzegorz Jaskiewicz

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.

2009-09-02 Thread Alvaro Herrera
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.

2009-09-02 Thread Grzegorz Jaskiewicz


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.

2009-08-30 Thread Grzegorz Jaskiewicz


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.

2009-08-30 Thread Grzegorz Jaskiewicz
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.

2009-08-30 Thread Peter Eisentraut
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.

2009-08-30 Thread Greg Stark
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.

2009-08-30 Thread Greg Stark
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.

2009-08-30 Thread Grzegorz Jaskiewicz


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.

2009-08-30 Thread Greg Stark
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.

2009-08-30 Thread Tom Lane
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.

2009-08-30 Thread Tom Lane
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.

2009-08-30 Thread Grzegorz Jaskiewicz

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.

2009-08-30 Thread Greg Stark
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.

2009-08-30 Thread Grzegorz Jaskiewicz


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.

2009-08-30 Thread Grzegorz Jaskiewicz

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.

2009-08-30 Thread Grzegorz Jaskiewicz
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.

2009-08-30 Thread Grzegorz Jaskiewicz


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.

2009-08-30 Thread Tom Lane
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.

2009-08-30 Thread Tom Lane
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.

2009-08-29 Thread Grzegorz Jaskiewicz


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.

2009-08-29 Thread Greg Stark
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.

2009-08-29 Thread Grzegorz Jaskiewicz


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.

2009-08-29 Thread Greg Stark
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.

2009-08-27 Thread Grzegorz Jaskiewicz


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.

2009-08-27 Thread Grzegorz Jaskiewicz

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.

2009-08-27 Thread Alex Hunsaker
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.

2009-08-27 Thread Tom Lane
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.

2009-08-24 Thread Peter Eisentraut
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.

2009-08-23 Thread Grzegorz Jaskiewicz

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.

2009-08-23 Thread Greg Stark
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.

2009-08-23 Thread Grzegorz Jaskiewicz


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.

2009-08-23 Thread Martijn van Oosterhout
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.

2009-08-23 Thread Grzegorz Jaskiewicz


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.

2009-08-23 Thread Grzegorz Jaskiewicz

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.

2009-08-23 Thread Greg Stark
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.

2009-08-23 Thread Grzegorz Jaskiewicz
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.

2009-08-23 Thread Tom Lane
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.

2009-08-23 Thread Grzegorz Jaskiewicz

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.

2009-08-23 Thread Grzegorz Jaskiewicz


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.

2009-08-23 Thread Tom Lane
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.

2009-08-23 Thread Grzegorz Jaskiewicz


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.

2009-08-23 Thread Grzegorz Jaskiewicz

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.

2009-08-23 Thread Greg Stark
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.

2009-08-23 Thread Grzegorz Jaskiewicz


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