Horton

Thread

Commits

  1. Fix ecpglib.h to declare bool consistently with c.h.

  2. Move declaration of ecpg_gettext() to a saner place.

  3. Sync our DTrace infrastructure with c.h's definition of type bool.

  4. Get rid of useless/dangerous redefinition of bool in ECPG.

  1. define bool in pgtypeslib_extern.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-25T09:48:37Z

    Today, I committed a patch (dddf4cdc) to reorder some of the file
header inclusions and buildfarm members prairiedog and locust failed
as a result of that.  The reason turns out to be that we have defined
a bool in pgtypeslib_extern.h and that definition is different from
what we define in c.h.

c.h defines it as:
#ifndef bool
typedef unsigned char bool;
#endif

pgtypeslib_extern.h defines it as:
#ifndef bool
#define bool char
#endif

Prior to dddf4cdc,  pgtypeslib_extern.h was included as a first header
before any usage of bool, but commit moves it after dt.h in file
dt_common.c.  Now, it seems like dt.h was using a version of bool as
defined in c.h and dt_common.c uses as defined by pgtypeslib_extern.h
which leads to some compilation errors as below:

dt_common.c:672: error: conflicting types for 'EncodeDateOnly'
dt.h:321: error: previous declaration of 'EncodeDateOnly' was here
dt_common.c:756: error: conflicting types for 'EncodeDateTime'
dt.h:316: error: previous declaration of 'EncodeDateTime' was here
dt_common.c:1783: error: conflicting types for 'DecodeDateTime'
dt.h:324: error: previous declaration of 'DecodeDateTime' was here
make[4]: *** [dt_common.o] Error 1

As suggested by Andrew Gierth [1], I think we can remove the define in
pgtypeslib_extern.h as it doesn't seem to be exposed.

Thoughts?

Note - For the time being, I have changed the order of file inclusions
(c114229ca2) in dt_common.c as it was before so that the buildfarm
becomes green again.

[1] - https://www.postgresql.org/message-id/87h83xmg4m.fsf%40news-spur.riddles.org.uk

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  2. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-25T13:41:53Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> As suggested by Andrew Gierth [1], I think we can remove the define in
> pgtypeslib_extern.h as it doesn't seem to be exposed.

Yeah, it's not good that that results in a header ordering dependency,
and it doesn't seem like a good idea for pgtypeslib_extern.h to be
messing with the issue at all.

If you like, I can experiment with that locally on prairiedog's host
before we make the buildfarm jump through hoops.

			regards, tom lane

  3. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-25T16:25:55Z

    I wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> As suggested by Andrew Gierth [1], I think we can remove the define in
>> pgtypeslib_extern.h as it doesn't seem to be exposed.

> Yeah, it's not good that that results in a header ordering dependency,
> and it doesn't seem like a good idea for pgtypeslib_extern.h to be
> messing with the issue at all.
> If you like, I can experiment with that locally on prairiedog's host
> before we make the buildfarm jump through hoops.

I checked that that works and fixes the immediate problem, so I pushed
it.  However, we're not out of the woods, because lookee here in
ecpglib.h:

#ifndef __cplusplus
#ifndef bool
#define bool char
#endif                          /* ndef bool */

#ifndef true
#define true    ((bool) 1)
#endif                          /* ndef true */
#ifndef false
#define false   ((bool) 0)
#endif                          /* ndef false */
#endif                          /* not C++ */

#ifndef TRUE
#define TRUE    1
#endif                          /* TRUE */

#ifndef FALSE
#define FALSE   0
#endif                          /* FALSE */

This stuff *is* exposed to client programs, so it's not clear how
painless it'd be to monkey around with it.  And it is used, further
down in the same file, so we can't fix it just by deleting it.
Nor can we import c.h to get the "real" definition from that.

I'm more than slightly surprised that we haven't already seen
problems due to this conflicting with d26a810eb.  I've not bothered
to run to ground exactly why not, though.

Thoughts?

			regards, tom lane

  4. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-25T16:52:20Z

    I wrote:
> I checked that that works and fixes the immediate problem, so I pushed
> it.  However, we're not out of the woods, because lookee here in
> ecpglib.h:
> ...

Oh, and for extra fun, take a look in src/backend/utils/probes.d :-(

			regards, tom lane

  5. Re: define bool in pgtypeslib_extern.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-26T03:21:14Z

    On Fri, Oct 25, 2019 at 9:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> >> As suggested by Andrew Gierth [1], I think we can remove the define in
> >> pgtypeslib_extern.h as it doesn't seem to be exposed.
>
> > Yeah, it's not good that that results in a header ordering dependency,
> > and it doesn't seem like a good idea for pgtypeslib_extern.h to be
> > messing with the issue at all.
> > If you like, I can experiment with that locally on prairiedog's host
> > before we make the buildfarm jump through hoops.
>
> I checked that that works and fixes the immediate problem, so I pushed
> it.
>

Thank you.

>  However, we're not out of the woods, because lookee here in
> ecpglib.h:
>
> #ifndef __cplusplus
> #ifndef bool
> #define bool char
> #endif                          /* ndef bool */
>
> #ifndef true
> #define true    ((bool) 1)
> #endif                          /* ndef true */
> #ifndef false
> #define false   ((bool) 0)
> #endif                          /* ndef false */
> #endif                          /* not C++ */
>
> #ifndef TRUE
> #define TRUE    1
> #endif                          /* TRUE */
>
> #ifndef FALSE
> #define FALSE   0
> #endif                          /* FALSE */
>
> This stuff *is* exposed to client programs, so it's not clear how
> painless it'd be to monkey around with it.  And it is used, further
> down in the same file, so we can't fix it just by deleting it.
> Nor can we import c.h to get the "real" definition from that.
>
> I'm more than slightly surprised that we haven't already seen
> problems due to this conflicting with d26a810eb.
>

I think it is because it never gets any imports from c.h.  It instead
uses postgres_ext.h.  If we want to fix this, the simplest thing that
comes to mind is to change the definition of bool in ecpglib.h and
probes.d to match with c.h.  These files contain exposed interfaces,
so the change can impact clients, but not sure what else we can do
here.  I have also tried to think about moving bool definition to
postgres_ext.h, but I think that won't be straightforward.   OTOH, if
you think that might be worth investigating, I can spend some more
time to see if we can do that way.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  6. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-26T17:19:24Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Oct 25, 2019 at 9:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, we're not out of the woods, because lookee here in
>> ecpglib.h:
>> #ifndef bool
>> #define bool char
>> #endif                          /* ndef bool */
>> I'm more than slightly surprised that we haven't already seen
>> problems due to this conflicting with d26a810eb.

> I think it is because it never gets any imports from c.h.

On closer inspection, it seems to be just blind luck.  For example,
if I rearrange the inclusion order in a file using ecpglib.h:

diff --git a/src/interfaces/ecpg/ecpglib/data.c b/src/interfaces/ecpg/ecpglib/data.c
index 7d2a78a..09944ff 100644
--- a/src/interfaces/ecpg/ecpglib/data.c
+++ b/src/interfaces/ecpg/ecpglib/data.c
@@ -6,8 +6,8 @@
 #include <math.h>
 
 #include "ecpgerrno.h"
-#include "ecpglib.h"
 #include "ecpglib_extern.h"
+#include "ecpglib.h"
 #include "ecpgtype.h"
 #include "pgtypes_date.h"
 #include "pgtypes_interval.h"

then on a PPC Mac I get

data.c:210: error: conflicting types for 'ecpg_get_data'
ecpglib_extern.h:167: error: previous declaration of 'ecpg_get_data' was here

It's exactly the same problem as we saw with pgtypeslib_extern.h:
header ordering changes affect the meaning of uses of bool, and that's
just not acceptable.

In this case it's even worse because we're mucking with type definitions
in a user-visible header.  I'm surprised we've not gotten bug reports
about that.  Maybe all ECPG users include <stdbool.h> before they
include ecpglib.h, but that doesn't exactly make things worry-free either,
because code doing that will think that these functions return _Bool,
when the compiled library possibly thinks differently.  Probably the
only thing saving us is that sizeof(_Bool) is 1 on just about every
platform in common use nowadays.

I'm inclined to think that we need to make ecpglib.h's bool-related
definitions exactly match c.h, which will mean that it has to pull in
<stdbool.h> on most platforms, which will mean adding a control symbol
for that to ecpg_config.h.  I do not think we should export
HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
configure make the choice and export something named like PG_USE_STDBOOL.

			regards, tom lane

  7. Re: define bool in pgtypeslib_extern.h

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-10-26T19:29:03Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> On closer inspection, it seems to be just blind luck. For example,
 Tom> if I rearrange the inclusion order in a file using ecpglib.h:

Ugh.

 Tom> I'm inclined to think that we need to make ecpglib.h's
 Tom> bool-related definitions exactly match c.h,

I'm wondering whether we should actually go the opposite way and say
that c.h's "bool" definition should be backend only, and that in
frontend code we should define a PG_bool type or something of that ilk
for when we want "PG's 1-byte bool" and otherwise let the platform
define "bool" however it wants.

And we certainly shouldn't be defining "bool" in something that's going
to be included in the user's code the way that ecpglib.h is.

-- 
Andrew.

  8. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-26T20:12:20Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> I'm inclined to think that we need to make ecpglib.h's
>  Tom> bool-related definitions exactly match c.h,

> I'm wondering whether we should actually go the opposite way and say
> that c.h's "bool" definition should be backend only, and that in
> frontend code we should define a PG_bool type or something of that ilk
> for when we want "PG's 1-byte bool" and otherwise let the platform
> define "bool" however it wants.

> And we certainly shouldn't be defining "bool" in something that's going
> to be included in the user's code the way that ecpglib.h is.

The trouble here is the hazard of creating an ABI break, if we modify
ecpglib.h in a way that causes its "bool" references to be interpreted
differently than they were before.  I don't think we want that (although
I suspect we have inadvertently caused ABI breaks already on platforms
where this matters).

In practice, since v11 on every modern platform, the exported ecpglib
functions have supposed that "bool" is _Bool, because they were compiled
in files that included c.h before ecpglib.h.  I assert furthermore that
clients might well have included <stdbool.h> before ecpglib.h and thereby
been fully compatible with that.  If we start having ecpglib.h include
<stdbool.h> itself, we'll just be eliminating a minor header inclusion
order hazard.  It's also rather hard to argue that including <stdbool.h>
automatically is really likely to break anything that was including
ecpglib.h already, since that file was already usurping those symbols.
Except on platforms where sizeof(_Bool) isn't 1, but things are already
pretty darn broken there.

I think it's possible to construct a counterexample that will fail
for *anything* we can do here.  I'm not inclined to uglify things like
mad to reduce the problem space from 0.1% to 0.01% of use-cases, or
whatever the numbers would be in practice.

			regards, tom lane

  9. Re: define bool in pgtypeslib_extern.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-10-28T06:00:14Z

    On Sat, Oct 26, 2019 at 10:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I'm inclined to think that we need to make ecpglib.h's bool-related
> definitions exactly match c.h, which will mean that it has to pull in
> <stdbool.h> on most platforms, which will mean adding a control symbol
> for that to ecpg_config.h.  I do not think we should export
> HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
> configure make the choice and export something named like PG_USE_STDBOOL.
>

This sounds reasonable to me, but we also might want to do something
for probes.d.  To be clear, I am not immediately planning to write a
patch for this.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  10. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-10-28T17:57:19Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sat, Oct 26, 2019 at 10:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm inclined to think that we need to make ecpglib.h's bool-related
>> definitions exactly match c.h, which will mean that it has to pull in
>> <stdbool.h> on most platforms, which will mean adding a control symbol
>> for that to ecpg_config.h.  I do not think we should export
>> HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
>> configure make the choice and export something named like PG_USE_STDBOOL.

> This sounds reasonable to me, but we also might want to do something
> for probes.d.  To be clear, I am not immediately planning to write a
> patch for this.

As far as probes.d goes, it seems to work to do

@@ -20,7 +20,7 @@
 #define BlockNumber unsigned int
 #define Oid unsigned int
 #define ForkNumber int
-#define bool char
+#define bool _Bool
 
 provider postgresql {
 
although removing the macro altogether leads to compilation failures.
I surmise that dtrace is trying to compile the generated code without
any #include's, so that only compiler built-in types will do.

(I tried this on macOS, FreeBSD, and NetBSD, to the extent of seeing
whether a build with --enable-dtrace goes through.  I don't know
enough about dtrace to test the results easily, but I suppose that
if it compiles then this is OK.)

This would, of course, not work on any platform where we're not
using <stdbool.h>, but I doubt that the set of platforms where
dtrace works includes any such.

A plausible alternative is to do

-#define bool char
+#define bool unsigned char

which is correct on platforms where we don't use <stdbool.h>,
and is at least no worse than now on those where we do.  In
practice, since we know sizeof(_Bool) == 1 on platforms where
we use it, this is probably just fine for dtrace's purposes.

Anyone have a preference?

			regards, tom lane

  11. Re: define bool in pgtypeslib_extern.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-06T09:34:20Z

    On Mon, Oct 28, 2019 at 11:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Sat, Oct 26, 2019 at 10:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'm inclined to think that we need to make ecpglib.h's bool-related
> >> definitions exactly match c.h, which will mean that it has to pull in
> >> <stdbool.h> on most platforms, which will mean adding a control symbol
> >> for that to ecpg_config.h.  I do not think we should export
> >> HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
> >> configure make the choice and export something named like PG_USE_STDBOOL.
>
> > This sounds reasonable to me, but we also might want to do something
> > for probes.d.  To be clear, I am not immediately planning to write a
> > patch for this.
>
> As far as probes.d goes, it seems to work to do
>
> @@ -20,7 +20,7 @@
>  #define BlockNumber unsigned int
>  #define Oid unsigned int
>  #define ForkNumber int
> -#define bool char
> +#define bool _Bool
>
>  provider postgresql {
>
> although removing the macro altogether leads to compilation failures.
> I surmise that dtrace is trying to compile the generated code without
> any #include's, so that only compiler built-in types will do.
>
> (I tried this on macOS, FreeBSD, and NetBSD, to the extent of seeing
> whether a build with --enable-dtrace goes through.  I don't know
> enough about dtrace to test the results easily, but I suppose that
> if it compiles then this is OK.)
>
> This would, of course, not work on any platform where we're not
> using <stdbool.h>, but I doubt that the set of platforms where
> dtrace works includes any such.
>
> A plausible alternative is to do
>
> -#define bool char
> +#define bool unsigned char
>
> which is correct on platforms where we don't use <stdbool.h>,
> and is at least no worse than now on those where we do.  In
> practice, since we know sizeof(_Bool) == 1 on platforms where
> we use it, this is probably just fine for dtrace's purposes.
>
> Anyone have a preference?
>

+1 for the second alternative as it will make it similar to c.h.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  12. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-06T14:54:25Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Oct 28, 2019 at 11:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A plausible alternative is to do
>> 
>> -#define bool char
>> +#define bool unsigned char
>> 
>> which is correct on platforms where we don't use <stdbool.h>,
>> and is at least no worse than now on those where we do.  In
>> practice, since we know sizeof(_Bool) == 1 on platforms where
>> we use it, this is probably just fine for dtrace's purposes.

> +1 for the second alternative as it will make it similar to c.h.

Yeah, that's the minimum-risk alternative.  I'll go make it so.

			regards, tom lane

  13. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-06T19:48:29Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I'm wondering whether we should actually go the opposite way and say
> that c.h's "bool" definition should be backend only, and that in
> frontend code we should define a PG_bool type or something of that ilk
> for when we want "PG's 1-byte bool" and otherwise let the platform
> define "bool" however it wants.
> And we certainly shouldn't be defining "bool" in something that's going
> to be included in the user's code the way that ecpglib.h is.

I experimented with doing things that way, and ended up with the attached
draft patch.  It basically gets ecpglib.h out of the business of declaring
any bool-related stuff at all, instead insisting that client code include
<stdbool.h> or otherwise declare bool for itself.  The function
declarations that were previously relying on "bool" now use the "pqbool"
typedef that libpq-fe.h was already exporting.  Per discussion, that's
not an ABI break, even on platforms where sizeof(_Bool) > 1, because
the actual underlying library functions are certainly expecting to take
or return a value of size 1.

While this seems like a generally cleaner place to be, I'm a bit concerned
about a number of aspects:

* This will of course be an API break for clients, which might not've
included <stdbool.h> before.

* On platforms where sizeof(_Bool) > 1, it's far from clear to me that
ECPG will interface correctly with client code that is treating bool
as _Bool.  There are some places that seem to be prepared for bool
client variables to be either sizeof(char) or sizeof(int), for example
ecpg_store_input(), but there are a fair number of other places that
seem to assume that sizeof(bool) is relevant, which it won't be.
The ECPG regression tests do pass for me on a PPC Mac, but I wonder
how much that proves.

* The "sql/dyntest.pgc" test declares BOOLVAR as "char" and then does

  exec sql var BOOLVAR is bool;

It's not clear to me what the implications of that statement are
(and our manual is no help), but looking at the generated code,
it seems like this causes ecpg to believe that the size of the
variable is sizeof(bool).  So that looks like buffer overrun
trouble waiting to happen.  I changed the variable declaration to
"bool" in the attached, but I wonder what's supposed to be getting
tested there.

On the whole I'm not finding this an attractive way to proceed
compared to the other approach I sketched.  It will certainly
cause some clients to have compile failures, and I'm at best
queasy about whether it will really work on platforms where
sizeof(_Bool) > 1.  I think we're better off to go with the
other approach of making ecpglib.h export what we think the
correct definition of bool is.  For most people that will
end up being <stdbool.h>, which I think will be unsurprising.

			regards, tom lane

PS: another issue this fixes, which I think we ought to fix and back-patch
regardless of what we decide about bool, is it moves the declaration for
ecpg_gettext() out of ecpglib.h and into the private header
ecpglib_extern.h.  That function isn't meant for client access, the
declaration is wrong where it is because it is not inside extern "C",
and the declaration wouldn't even compile for clients because they
will not know what pg_attribute_format_arg() is.  The only reason we've
not had complaints, I imagine, is that nobody's tried to compile client
code with ENABLE_NLS defined ... but that's already an intrusion on
client namespace.

  14. Re: define bool in pgtypeslib_extern.h

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-07T20:47:52Z

    I wrote:
> I'm inclined to think that we need to make ecpglib.h's bool-related
> definitions exactly match c.h, which will mean that it has to pull in
> <stdbool.h> on most platforms, which will mean adding a control symbol
> for that to ecpg_config.h.  I do not think we should export
> HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
> configure make the choice and export something named like PG_USE_STDBOOL.

Here's a proposed patch that does it like that.

I'm of two minds about whether to back-patch or not.  This shouldn't
really change anything except on platforms where sizeof(_Bool) isn't
one.  We have some reason to think that nobody is actually using
ecpg on such platforms :-(, because if they were, they'd likely have
complained about breakage.  So maybe we should just put this in HEAD
and be done.

			regards, tom lane

  15. Re: define bool in pgtypeslib_extern.h

    Amit Kapila <amit.kapila16@gmail.com> — 2019-11-08T02:36:50Z

    On Fri, Nov 8, 2019 at 2:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > I'm inclined to think that we need to make ecpglib.h's bool-related
> > definitions exactly match c.h, which will mean that it has to pull in
> > <stdbool.h> on most platforms, which will mean adding a control symbol
> > for that to ecpg_config.h.  I do not think we should export
> > HAVE_STDBOOL_H and SIZEOF_BOOL there though; probably better to have
> > configure make the choice and export something named like PG_USE_STDBOOL.
>
> Here's a proposed patch that does it like that.
>
> I'm of two minds about whether to back-patch or not.  This shouldn't
> really change anything except on platforms where sizeof(_Bool) isn't
> one.  We have some reason to think that nobody is actually using
> ecpg on such platforms :-(, because if they were, they'd likely have
> complained about breakage.
>

Yeah, this is a valid point, but I think this would have caused
breakage only after d26a810eb which is a recent change.  If that is
right, then I am not sure such an assumption is safe.  Also, we have
already backpatched the probes.d change, so it seems reasonable to
make this change and keep the bool definition consistent in code.
OTOH, I think there is no harm in making this change for HEAD and if
later we face any complaint, we can backpatch it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com