Thread: elog/ereport noreturn decoration
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.
Attachment
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.
Attachment
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.