Thread
-
elog/ereport noreturn decoration
Peter Eisentraut <peter_e@gmx.net> — 2012-06-29T20:35:13Z
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. -
Re: elog/ereport noreturn decoration
Tom Lane <tgl@sss.pgh.pa.us> — 2012-06-29T21:35:34Z
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 -
Re: elog/ereport noreturn decoration
Peter Geoghegan <peter@2ndquadrant.com> — 2012-06-30T01:28:13Z
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
-
Re: elog/ereport noreturn decoration
Peter Eisentraut <peter_e@gmx.net> — 2012-06-30T10:44:04Z
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.
-
Re: elog/ereport noreturn decoration
Tom Lane <tgl@sss.pgh.pa.us> — 2012-06-30T14:52:29Z
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
-
Re: elog/ereport noreturn decoration
Peter Eisentraut <peter_e@gmx.net> — 2012-07-14T21:59:37Z
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.
-
Re: elog/ereport noreturn decoration
Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-14T22:16:12Z
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
-
Re: elog/ereport noreturn decoration
Robert Haas <robertmhaas@gmail.com> — 2012-07-15T19:36:47Z
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
-
Re: elog/ereport noreturn decoration
Tom Lane <tgl@sss.pgh.pa.us> — 2012-07-15T20:14:41Z
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 -
Re: elog/ereport noreturn decoration
Peter Eisentraut <peter_e@gmx.net> — 2012-08-19T02:37:27Z
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.