Re: [HACKERS] elog/ereport noreturn decoration

2012-08-18 Thread Peter Eisentraut 
On Fri, 2012-06-29 at 23:35 +0300, Peter Eisentraut wrote:
 My proposal with ereport would be to do this:
 
 diff --git i/src/include/utils/elog.h w/src/include/utils/elog.h
 --- i/src/include/utils/elog.h
 +++ w/src/include/utils/elog.h
 @@ -104,7 +104,8 @@
   */
  #define ereport_domain(elevel, domain, rest)   \
 (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
 -(errfinish rest) : (void) 0)
 +(errfinish rest) : (void) 0), \
 +   (elevel = ERROR ? abort() : (void) 0)
 
 There are no portability problems with that.

While the discussion on what to do about elog() was ongoing, I think
there should be no problems with this ereport() change, so I intend to
go ahead with 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] elog/ereport noreturn decoration

2012-07-15 Thread Robert Haas 
On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Peter Eisentraut pete...@gmx.net writes:
 A small sidetrack here.  I've managed to set up the Solaris Studio
 compiler on Linux and tried this out.  It turns out these statement not
 reached warnings have nothing to do with knowledge about library
 functions such as abort() or exit() not returning.  The warnings come
 mostly from loops that never end (except by returning from the function)
 and some other more silly cases where the supposed fallback return
 statement is clearly unnecessary.  I think these should be fixed,
 because the code is wrong and could mask real errors if someone ever
 wanted to rearrange those loops or something.

 Patch attached.  I tried this out with old and new versions of gcc,
 clang, and the Solaris compiler, and everyone was happy about.  I didn't
 touch the regex code.  And there's some archeological knowledge about
 Perl in there.

 Hm.  I seem to recall that at least some of these lines were themselves
 put in to suppress compiler warnings.  So we may just be moving the
 warnings from one set of non-mainstream compilers to another set.
 Still, we might as well try it and see if there's a net reduction.
 (In some places we might need to tweak the code a bit harder than this,
 to make it clearer that the unreachable spot is unreachable.)

You mean things like this?

-
-   /* keep compiler happy */
-   return NULL;

I admit that compiler warnings are horribly annoying and we probably
ought to favor new compilers over old ones, at least in master, but
I'm kinda skeptical about the contention that no compiler anywhere
will complain if we remove hunks like that one.  The comment seems
pretty clear.  I feel like we're missing something here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] elog/ereport noreturn decoration

2012-07-15 Thread Tom Lane 
Robert Haas robertmh...@gmail.com writes:
 On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Hm.  I seem to recall that at least some of these lines were themselves
 put in to suppress compiler warnings.

 You mean things like this?

 -
 - /* keep compiler happy */
 - return NULL;

That particular case appears to have been in the aboriginal commit of
the GIN code.  It's possible that Teodor's compiler complained without
it, but it seems at least as likely that (a) he was just guessing that
some compilers would complain, or (b) it was leftover from an earlier
version of the function in which the loop wasn't so obviously
non-exitable.

 I admit that compiler warnings are horribly annoying and we probably
 ought to favor new compilers over old ones, at least in master, but
 I'm kinda skeptical about the contention that no compiler anywhere
 will complain if we remove hunks like that one.  The comment seems
 pretty clear.  I feel like we're missing something here.

I don't have a whole lot of sympathy for compilers that think a
for (;;) { ... } loop with no break statement might terminate.
If you're going to emit warnings on the basis of reachability
analysis, I think you should be doing reachability analysis that's
not completely brain-dead.  Or else not be surprised when people
ignore your warnings.

Now, I'm prepared to believe that some of these functions might
contain cases that are not quite as black-and-white as that one.
That's why I suggested we might end up doing further code tweaking.
But at least for this example, I don't have a problem with applying
the patch and seeing what happens.

The bigger picture here is that we're threading a line between
getting warnings from compilers that are too smart, versus warnings
from compilers that are not smart enough (which could actually be
the same compiler with different settings :-().  We're not going
to be able to satisfy all cases --- but I think it's probably wise
to tilt towards satisfying the smarter compilers, when we have to
compromise.

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] elog/ereport noreturn decoration

2012-07-14 Thread Peter Eisentraut 
On lör, 2012-06-30 at 10:52 -0400, Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
  But my point was, there aren't any unused code warnings.  None of the
  commonly used compilers issue any.  I'd be interested to know if there
  is any recent C compiler supported by PostgreSQL that issues some kind
  of unused code warning under any circumstances, and see an example of
  that.  Then we can determine whether there is an issue here.
 
 Well, the Solaris Studio compiler produces warning: statement not
 reached messages, as seen for example on buildfarm member castoroides.
 I don't have one available to experiment with, so I'm not sure whether
 it knows that abort() doesn't return; but I think it rather foolish to
 assume that such a combination doesn't exist in the wild.

A small sidetrack here.  I've managed to set up the Solaris Studio
compiler on Linux and tried this out.  It turns out these statement not
reached warnings have nothing to do with knowledge about library
functions such as abort() or exit() not returning.  The warnings come
mostly from loops that never end (except by returning from the function)
and some other more silly cases where the supposed fallback return
statement is clearly unnecessary.  I think these should be fixed,
because the code is wrong and could mask real errors if someone ever
wanted to rearrange those loops or something.

Patch attached.  I tried this out with old and new versions of gcc,
clang, and the Solaris compiler, and everyone was happy about.  I didn't
touch the regex code.  And there's some archeological knowledge about
Perl in there.

The Solaris compiler does not, by the way, complain about the elog patch
I had proposed.

diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index aadf050..7bdac3d 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -163,8 +163,6 @@
 
 		state-ptr++;
 	}
-
-	return false;
 }
 
 #define WKEY	0
diff --git a/contrib/intarray/_int_bool.c b/contrib/intarray/_int_bool.c
index dfb113a..d0572af 100644
--- a/contrib/intarray/_int_bool.c
+++ b/contrib/intarray/_int_bool.c
@@ -136,7 +136,6 @@
 		}
 		(state-buf)++;
 	}
-	return END;
 }
 
 /*
@@ -301,7 +300,6 @@
 		else
 			return execute(curitem - 1, checkval, calcnot, chkcond);
 	}
-	return false;
 }
 
 /*
@@ -404,7 +402,6 @@
 		else
 			return false;
 	}
-	return false;
 }
 
 bool
diff --git a/contrib/intarray/_int_gist.c b/contrib/intarray/_int_gist.c
index e429c8b..60de393 100644
--- a/contrib/intarray/_int_gist.c
+++ b/contrib/intarray/_int_gist.c
@@ -217,8 +217,6 @@
 	}
 	else
 		PG_RETURN_POINTER(entry);
-
-	PG_RETURN_POINTER(entry);
 }
 
 Datum
diff --git a/contrib/ltree/ltxtquery_io.c b/contrib/ltree/ltxtquery_io.c
index c2e532c..583ff2a 100644
--- a/contrib/ltree/ltxtquery_io.c
+++ b/contrib/ltree/ltxtquery_io.c
@@ -139,7 +139,6 @@
 
 		state-buf += charlen;
 	}
-	return END;
 }
 
 /*
diff --git a/contrib/ltree/ltxtquery_op.c b/contrib/ltree/ltxtquery_op.c
index bedbe24..64f9d21 100644
--- a/contrib/ltree/ltxtquery_op.c
+++ b/contrib/ltree/ltxtquery_op.c
@@ -40,7 +40,6 @@
 		else
 			return ltree_execute(curitem + 1, checkval, calcnot, chkcond);
 	}
-	return false;
 }
 
 typedef struct
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 82ac53e..3efdedd 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -146,9 +146,6 @@
 			stack-predictNumber = 1;
 		}
 	}
-
-	/* keep compiler happy */
-	return NULL;
 }
 
 void
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 022bd27..5702259 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -354,8 +354,6 @@
 		 */
 		stack-off++;
 	}
-
-	return true;
 }
 
 /*
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index c790ad6..2253e7c 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -535,8 +535,6 @@
 			} while (so-nPageData == 0);
 		}
 	}
-
-	PG_RETURN_BOOL(false);		/* keep compiler quiet */
 }
 
 /*
diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c
index 80e282b..a8a1fe6 100644
--- a/src/backend/executor/nodeGroup.c
+++ b/src/backend/executor/nodeGroup.c
@@ -184,9 +184,6 @@
 		else
 			InstrCountFiltered1(node, 1);
 	}
-
-	/* NOTREACHED */
-	return NULL;
 }
 
 /* -
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index e0ab599..0d66dab 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -201,9 +201,9 @@
 {
 #ifdef USE_SSL
 	return ssl_loaded_verify_locations;
-#endif
-
+#else
 	return false;
+#endif
 }
 
 /*
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index c927747..d96b7a7 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -233,9 +233,6 @@ static void

Re: [HACKERS] elog/ereport noreturn decoration

2012-07-14 Thread Tom Lane 
Peter Eisentraut pete...@gmx.net writes:
 A small sidetrack here.  I've managed to set up the Solaris Studio
 compiler on Linux and tried this out.  It turns out these statement not
 reached warnings have nothing to do with knowledge about library
 functions such as abort() or exit() not returning.  The warnings come
 mostly from loops that never end (except by returning from the function)
 and some other more silly cases where the supposed fallback return
 statement is clearly unnecessary.  I think these should be fixed,
 because the code is wrong and could mask real errors if someone ever
 wanted to rearrange those loops or something.

 Patch attached.  I tried this out with old and new versions of gcc,
 clang, and the Solaris compiler, and everyone was happy about.  I didn't
 touch the regex code.  And there's some archeological knowledge about
 Perl in there.

Hm.  I seem to recall that at least some of these lines were themselves
put in to suppress compiler warnings.  So we may just be moving the
warnings from one set of non-mainstream compilers to another set.
Still, we might as well try it and see if there's a net reduction.
(In some places we might need to tweak the code a bit harder than this,
to make it clearer that the unreachable spot is unreachable.)

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] elog/ereport noreturn decoration

2012-06-30 Thread Peter Eisentraut 
On fre, 2012-06-29 at 17:35 -0400, Tom Lane wrote:
 Yes.  The problem with trying to change that is that it's damned if
 you do and damned if you don't: compilers that are aware that abort()
 doesn't return will complain about unreachable code if we keep those
 extra variable initializations, while those that are not so aware will
 complain about uninitialized variables if we don't.

But my point was, there aren't any unused code warnings.  None of the
commonly used compilers issue any.  I'd be interested to know if there
is any recent C compiler supported by PostgreSQL that issues some kind
of unused code warning under any circumstances, and see an example of
that.  Then we can determine whether there is an issue here.


-- 
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] elog/ereport noreturn decoration

2012-06-30 Thread Tom Lane 
Peter Eisentraut pete...@gmx.net writes:
 But my point was, there aren't any unused code warnings.  None of the
 commonly used compilers issue any.  I'd be interested to know if there
 is any recent C compiler supported by PostgreSQL that issues some kind
 of unused code warning under any circumstances, and see an example of
 that.  Then we can determine whether there is an issue here.

Well, the Solaris Studio compiler produces warning: statement not
reached messages, as seen for example on buildfarm member castoroides.
I don't have one available to experiment with, so I'm not sure whether
it knows that abort() doesn't return; but I think it rather foolish to
assume that such a combination doesn't exist in the wild.

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

[HACKERS] elog/ereport noreturn decoration

2012-06-29 Thread Peter Eisentraut 
There is continued interest in static analyzers (clang, coverity), and
the main problem with those is that they don't know that elog and
ereport with level = ERROR don't return, leading to lots of false
positives.

I looked in the archive; there were some attempts to fix this some time
ago.  One was putting an __attribute__((analyzer_noreturn)) on these
functions, but that was decided to be incorrect (and it would only work
for clang anyway).  Another went along the lines of what I'm proposing
here (but before ereport was introduced, if I read it correctly), but
didn't go anywhere.

My proposal with ereport would be to do this:

diff --git i/src/include/utils/elog.h w/src/include/utils/elog.h
--- i/src/include/utils/elog.h
+++ w/src/include/utils/elog.h
@@ -104,7 +104,8 @@
  */
 #define ereport_domain(elevel, domain, rest)   \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
-(errfinish rest) : (void) 0)
+(errfinish rest) : (void) 0), \
+   (elevel = ERROR ? abort() : (void) 0)

There are no portability problems with that.

With elog, I would do this:

diff --git i/src/include/utils/elog.h w/src/include/utils/elog.h
@@ -196,7 +197,11 @@
  * elog(ERROR, portal \%s\ not found, stmt-portalname);
  *--
  */
+#if defined(__STDC_VERSION__)  __STDC_VERSION__ = 199901L
+#define elog(elevel, ...)  elog_start(__FILE__, __LINE__, 
PG_FUNCNAME_MACRO), elog_finish(elevel, __VA_ARGS__), (elevel = ERROR ? 
abort() : (void) 0)
+#else
 #define elog   elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish
+#endif

I think the issue here was that if we support two separate code paths,
we still need to do the actually unreachable /* keep compiler happy */
bits, and that compilers that know about elog not returning would
complain about unreachable code.  But I have never seen warnings like
that, so maybe it's a nonissue.  But it could be tested if there are
concerns.

Complete patch attached for easier applying and testing.

For clang aficionados: This reduces scan-build warnings from 1222 to 553
for me.
diff --git i/src/include/utils/elog.h w/src/include/utils/elog.h
index 93b141d..b32737e 100644
--- i/src/include/utils/elog.h
+++ w/src/include/utils/elog.h
@@ -104,7 +104,8 @@
  */
 #define ereport_domain(elevel, domain, rest)	\
 	(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) ? \
-	 (errfinish rest) : (void) 0)
+	 (errfinish rest) : (void) 0),	   \
+		(elevel = ERROR ? abort() : (void) 0)
 
 #define ereport(elevel, rest)	\
 	ereport_domain(elevel, TEXTDOMAIN, rest)
@@ -196,7 +197,11 @@ extern int	getinternalerrposition(void);
  *		elog(ERROR, portal \%s\ not found, stmt-portalname);
  *--
  */
+#if defined(__STDC_VERSION__)  __STDC_VERSION__ = 199901L
+#define elog(elevel, ...)	elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish(elevel, __VA_ARGS__), (elevel = ERROR ? abort() : (void) 0)
+#else
 #define elog	elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO), elog_finish
+#endif
 
 extern void elog_start(const char *filename, int lineno, const char *funcname);
 extern void

-- 
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] elog/ereport noreturn decoration

2012-06-29 Thread Tom Lane 
Peter Eisentraut pete...@gmx.net writes:
 I think the issue here was that if we support two separate code paths,
 we still need to do the actually unreachable /* keep compiler happy */
 bits, and that compilers that know about elog not returning would
 complain about unreachable code.

Yes.  The problem with trying to change that is that it's damned if you
do and damned if you don't: compilers that are aware that abort()
doesn't return will complain about unreachable code if we keep those
extra variable initializations, while those that are not so aware will
complain about uninitialized variables if we don't.  I don't think
that's going to be a step forward.  IOW I am not on board with reducing
the number of warnings in clang by increasing the number everywhere
else.

Perhaps we could do something like

default:
elog(ERROR, unrecognized drop object type: %d,
 (int) drop-removeType);
-   relkind = 0; /* keep compiler quiet */
+   UNREACHABLE(relkind = 0);/* keep compiler quiet */
break;

where UNREACHABLE(stuff) expands to the given statements (possibly
empty) or to abort() depending on the compiler's properties.  If we
did something like that we'd not need to monkey with the definition
of either elog or ereport, but instead we'd have to run around and
fix everyplace that was emitting warnings of this sort.

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] elog/ereport noreturn decoration

2012-06-29 Thread Peter Geoghegan 
On 29 June 2012 22:35, Tom Lane t...@sss.pgh.pa.us wrote:
 IOW I am not on board with reducing
 the number of warnings in clang by increasing the number everywhere
 else.

I successfully lobbied the Clang developers to remove some warnings
that came from certain sites where we use a single element array at
the end of a struct as pseudo flexible arrays (Clang tried to do
compile-time bounds checking, with predictable results). Now, I had to
make a nuisance of myself in order to get that result, but you might
consider trying that.

I have been using the Clang static analyser some. I've seen all those
false positives. The greater problem is that the analyser apparently
won't work across translation units.

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers