On 2013-01-12 19:47:18 -0500, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> > When I compile with gcc -O0, I get one warning with this:
>
> > datetime.c: In function �DateTimeParseError�:
> > datetime.c:3575:1: warning: �noreturn� function does return [enabled by default]
>
> > That suggests that the compiler didn't correctly deduce that
> > ereport(ERROR, ...) doesn't return. With -O1, the warning goes away.
>
> Yeah, I am seeing this too. It appears to be endemic to the
> local-variable approach, ie if we have
>
> const int elevel_ = (elevel);
> ...
> (elevel_ >= ERROR) ? pg_unreachable() : (void) 0
>
> then we do not get the desired results at -O0, which is not terribly
> surprising --- I'd not really expect the compiler to propagate the
> value of elevel_ when not optimizing.
>
> If we don't use a local variable, we don't get the warning, which
> I take to mean that gcc will fold "ERROR >= ERROR" to true even at -O0,
> and that it does this early enough to conclude that unreachability
> holds.
>
> I experimented with some variant ways of phrasing the macro, but the
> only thing that worked at -O0 required __builtin_constant_p, which
> rather defeats the purpose of making this accessible to non-gcc
> compilers.
Yea, its rather sad. The only thing I can see is actually doing both
variants like:
#define ereport_domain(elevel, domain, rest) \ do {
\ const int elevel_ = elevel; \ if
(errstart(elevel_,__FILE__,__LINE__, PG_FUNCNAME_MACRO, domain))\ {
\ errfinish rest; \ }
\ if (pg_is_known_constant(elevel) && (elevel) >= ERROR)
\ { \ pg_unreachable();
\ Assert(0); \
} \ else if (elevel_>= ERROR)
\ { \
pg_unreachable(); \ Assert(0);
\ } \ } while(0)
but that obviously still relies on __builtin_constant_p giving
reasonable answers.
We should probably put that Assert(0) into pg_unreachable()...
> If we go with the local-variable approach, we could probably suppress
> this warning by putting an abort() call at the bottom of
> DateTimeParseError. It seems a tad ugly though, and what's a bigger
> deal is that if the compiler is unable to deduce unreachability at -O0
> then we are probably going to be fighting such bogus warnings for all
> time to come. Note also that an abort() (much less a pg_unreachable())
> would not do anything positive like give us a compile warning if we
> mistakenly added a case that could fall through.
>
> On the other hand, if there's only one such case in our tree today,
> maybe I'm worrying too much.
I think the only reason there's only one such case (two here actually,
second in renametrig) is that all others already have been silenced
because the abort() didn't use to be there. And I think the danger youre
alluding to above is a real one.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
