elog/ereport noreturn decoration

Peter Eisentraut <peter_e@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

On 29 June 2012 22:35, Tom Lane <tgl@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

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.

Peter Eisentraut <peter_e@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

On lör, 2012-06-30 at 10:52 -0400, Tom Lane wrote:

Peter Eisentraut <peter_e@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.

Peter Eisentraut <peter_e@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

On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter_e@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

Robert Haas <robertmhaas@gmail.com> writes:

On Sat, Jul 14, 2012 at 6:16 PM, Tom Lane <tgl@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

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.