define bool in pgtypeslib_extern.h

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

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

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

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

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

"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.

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

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

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

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

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

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.

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

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