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

From Tom Lane
Subject Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date
Msg-id 13628.1358102496@sss.pgh.pa.us
Whole thread Raw
In response to Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)  (Andres Freund <andres@2ndquadrant.com>)
List pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-01-12 15:39:16 -0500, Tom Lane wrote:
>> This is actually a disadvantage of the proposal to replace the abort()
>> calls with __builtin_unreachable(), too.  The gcc boys interpret the
>> semantics of that as "if control reaches here, the behavior is
>> undefined"

> We could make add an Assert() additionally?

I see little value in that.  In a non-assert build it will do nothing at
all, and in an assert build it'd just add code bloat.

I think there might be a good argument for defining pg_unreachable() as
abort() in assert-enabled builds, even if the compiler has
__builtin_unreachable(): that way, if the impossible happens, we'll find
out about it in a predictable and debuggable way.  And by definition,
we're not so concerned about either performance or code bloat in assert
builds.

> Where my variant is:
> #define ereport_domain(elevel, domain, rest)                                \
>     do {                                                                    \
>         const int elevel_ = elevel;                                         \
>         if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain))\
>         {                                                                   \
>             errfinish rest;                                                 \
>         }                                                                   \
>         if (elevel_>= ERROR)                                                \
>             pg_unreachable();                                               \
>     } while(0)

This is the same implementation I'd arrived at after looking at
Heikki's.  I think we should probably use this for non-gcc builds.
However, for recent gcc (those with __builtin_constant_p) I think that
my original suggestion is better, ie don't use a local variable but
instead use __builtin_constant_p(elevel) && (elevel) >= ERROR in the
second test.  That way we don't have the problem with unreachability
tests failing at -O0.  We may still have some issues with bogus warnings
in other compilers, but I think most PG developers are using recent gcc.
        regards, tom lane



pgsql-hackers by date:

Previous
From: Andrew Dunstan
Date:
Subject: psql binary output
Next
From: "Mark Hellegers"
Date:
Subject: Re: Porting to Haiku