Skip site navigation (1) Skip section navigation (2)

Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>,Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date: 2013-01-13 18:01:07
Message-ID: 20130113180107.GB26173@awork2.anarazel.de (view raw or flat)
Thread:
Lists: pgsql-hackers
On 2013-01-12 19:47:18 -0500, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)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


In response to

pgsql-hackers by date

Next:From: Markus WannerDate: 2013-01-13 18:02:46
Subject: Re: Re: logical changeset generation v3 - comparison to Postgres-R change set format
Previous:From: Andres FreundDate: 2013-01-13 17:51:26
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group