bool: symbol name collision

bryanh@giraffe-data.com (Bryan Henderson) writes:

The interface header files for Postgres server extensions define "bool",
but that name is commonly used by other parts of user code, including
by standards (C99, C++). That causes, at best, compile failures.

If Postgres has to define a boolean type in public header files, it should
use a name that won't collide, like postgres_bool.

Sorry, this isn't going to happen. It would break far too much existing
code, and we consider building server extensions with C++ to be
unsupported anyway.

regards, tom lane

On sön, 2010-05-09 at 11:35 -0400, Tom Lane wrote:

bryanh@giraffe-data.com (Bryan Henderson) writes:

The interface header files for Postgres server extensions define "bool",
but that name is commonly used by other parts of user code, including
by standards (C99, C++). That causes, at best, compile failures.

If Postgres has to define a boolean type in public header files, it should
use a name that won't collide, like postgres_bool.

Sorry, this isn't going to happen. It would break far too much existing
code, and we consider building server extensions with C++ to be
unsupported anyway.

Um, our code has

#ifndef __cplusplus

#ifndef bool
typedef char bool;
#endif

#ifndef true
#define true ((bool) 1)
#endif

etc.

so somehow it was once thought that it is worth supporting other
definitions of bool. Now to make this work in practice you probably
need to play some games with undefining and redefining and include file
order and so on, but I think it could work. In any case, it would be
better if Bryan could show us a concrete example that is causing
problems.

Peter Eisentraut <peter_e@gmx.net> writes:

On sön, 2010-05-09 at 11:35 -0400, Tom Lane wrote:

... we consider building server extensions with C++ to be
unsupported anyway.

Um, our code has

#ifndef __cplusplus

#ifndef bool
typedef char bool;
#endif

etc.

Yeah, I know those #if's are there, but whether they actually do
anything useful is highly questionable. There is no reason to assume
that a compiler's built-in version of bool will be bit-compatible with
ours. And changing the width of bool is guaranteed to Not Work.

regards, tom lane

On Sun, May 9, 2010 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I know those #if's are there, but whether they actually do
anything useful is highly questionable.  There is no reason to assume
that a compiler's built-in version of bool will be bit-compatible with
ours.  And changing the width of bool is guaranteed to Not Work.

Supporting C++ in the server would be a big task, but supporting C99,
it seems to me, would only require we rename our "bool" "true" and
"false" defines. The only other C99 keyword or typedef we use is
"inline" for which I don't understand the issues yet.

--
greg

Greg Stark <gsstark@mit.edu> writes:

On Sun, May 9, 2010 at 5:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I know those #if's are there, but whether they actually do
anything useful is highly questionable. �There is no reason to assume
that a compiler's built-in version of bool will be bit-compatible with
ours. �And changing the width of bool is guaranteed to Not Work.

Supporting C++ in the server would be a big task, but supporting C99,
it seems to me, would only require we rename our "bool" "true" and
"false" defines. The only other C99 keyword or typedef we use is
"inline" for which I don't understand the issues yet.

Huh? We build just fine on C99 compilers, AFAIK. Or are you saying
that we should try to adopt <stdbool.h>'s definition of bool? The
problem there is, again, that we don't know what width that will be.

regards, tom lane

Um, our code has

#ifndef __cplusplus

#ifndef bool
typedef char bool;
#endif

#ifndef true
#define true ((bool) 1)
#endif

etc.

so somehow it was once thought that it is worth supporting other
definitions of bool.

And the Postgres manual says you can use C++ (34.9 - C-Language Functions:
"User defined functions can be written in C (or a language that can be made
compatible with C, such as C++)." If in fact the code as it stands is what
the developers want, then C++ cannot be made compatible with C for Postgres
purposes because there's no way to make "bool" not a reserved word in C++. So
the manual should be changed to explicitly say you better not use C++ because
the fmgr.h interface will probably not work. And the same goes for any code
that happens to define a "bool" macro and doesn't define it to "char".

It's pretty clear looking at this code that the author never expected "bool"
to be part of an interface structure.

If renaming "bool" is not acceptable, you should at least change this code to
do a #error if it isn't the same size as "char".

If you look at a bunch of other systems that have C interfaces, I don't think
you'll find any that define "bool" in a header file that a user is supposed to
#include in his program. You will find some that use boolean variables, but
they define some "my_bool" type for it. I've noticed this since way before
C99 standardized "bool".

It would break far too much existing code,

There must be a dozen ways to avoid this problem that have no effect on
existing code and are superior to what I do now. What I do now is patch
fmgr.h to change all instances of "bool" to "char" and, after installing,
chop the "bool" section out of c.h.

Here's one that's one step better than local modifications to Postgres: define
both "bool" and "pgbool" to the same thing in c.h and use only pgbool in
fmgr.h. In c.h, put the "bool" definition under #ifdef
POSTGRES_DONT_DEFINE_BOOL .

it would be
better if Bryan could show us a concrete example that is causing
problems.

I don't know how concrete you want. A user defined function server extension
#includes a header file that #includes another that #includes the C99 standard
header file <stdbool.h>. The server extension also #includes <postgres.h>.
The compile fails with the multiple definition of type "bool".

Here's a worse one (but hypothetical; I haven't actually done this): the
server extension is compiled with a C++ compiler that uses 4 bytes for "bool".
Everything compiles, but the PG_GETARG_INT32() call gets the wrong argument
because the server thinks bool is one byte while the server extension thinks
it is 4.

--
Bryan Henderson San Jose, California

On Sun, May 9, 2010 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Huh?  We build just fine on C99 compilers, AFAIK.  Or are you saying
that we should try to adopt <stdbool.h>'s definition of bool?  The
problem there is, again, that we don't know what width that will be.

No, I'm saying we should use something like pgbool so that users can
compile code that uses stdbool.h in a c99 environment.

This would break any existing modules which use bool to refer to the
postgres bool. It wouldn't be hard to replace bool with pgbool in
those modules, and if they want to work with multiple versions of
postgres then they can add a #ifndef bool #define bool pgbool and be
done.

It's hardly our highest priority but it seems a reasonable course
given that c99 is becoming quite standard. It's hardly as invasive as
what would be needed to be c++ safe. I'm not sure whether our include
files have an non-c99 inline uses which would be harder to deal with.

I don't see any other conflicts offhand that would create problems
using a c99 compiler to build server modules. It's quite annoying and
sad that they added "bool" to c99 since otherwise it would just be a
drop-in replacement with extra functionality and very low risk of
conflicts. Instead they virtually guaranteed conflicts with any large
software over a single define.

--
greg

On sön, 2010-05-09 at 17:37 +0000, Bryan Henderson wrote:

it would be
better if Bryan could show us a concrete example that is causing
problems.

I don't know how concrete you want.

Something one can download and compile.

A user defined function server extension
#includes a header file that #includes another that #includes the C99
standard header file <stdbool.h>. The server extension also #includes
<postgres.h>. The compile fails with the multiple definition of type
"bool".

Well, let's say including stdbool.h is not supported then. ;-)

Here's a worse one (but hypothetical; I haven't actually done this):
the server extension is compiled with a C++ compiler that uses 4 bytes
for "bool". Everything compiles, but the PG_GETARG_INT32() call gets
the wrong argument because the server thinks bool is one byte while
the server extension thinks it is 4.

You should use PG_GETARG_BOOL(). I don't see why this necessarily
couldn't work.

We did actually include a sizable patch into 9.0 to remove conflicts
with C++ reserved words (typename and such). I don't recall any
complaints about bool at the time. There is a test script for this in
src/tools/pginclude/cpluspluscheck.

On mån, 2010-05-10 at 02:02 +0100, Greg Stark wrote:

I don't see any other conflicts offhand that would create problems
using a c99 compiler to build server modules. It's quite annoying and
sad that they added "bool" to c99 since otherwise it would just be a
drop-in replacement with extra functionality and very low risk of
conflicts. Instead they virtually guaranteed conflicts with any large
software over a single define.

For that reason they put it into a separate header file stdbool.h that
no one is required to include.

I don't know how concrete you want.

Something one can download and compile.

That wouldn't be worth anyone's effort, since the problem is esaily enough
elucidated with a few words of explanation. I.e. I'm sure you can imagine
writing a program that would demonstrate the problem of two header files
that both typedef "bool".

Here's a worse one (but hypothetical; I haven't actually done this):
the server extension is compiled with a C++ compiler that uses 4 bytes
for "bool". Everything compiles, but the PG_GETARG_INT32() call gets
the wrong argument because the server thinks bool is one byte while
the server extension thinks it is 4.

You should use PG_GETARG_BOOL(). I don't see why this necessarily
couldn't work.

Sorry, I should have been more explicit about how it screws up the interface
to have "bool" mean something different when you compile a server extension
from what it means when you compile Postgres. Postgres and the user function
pass back and forth a struct FunctionCallInfoData. It contains such things as
the array of argument values. It has one member, before the argument array,
that fmgr.h defines as type "bool". Now if the user program thinks "bool" is
4 bytes and Postgres thinks it is 1 byte, the user program and Postgres will
disagree about the offsets of every field after that. The way the alignment
works out, the effect is that the memory the user program looks to for
Argument 3 is the memory in which Postgres placed Argument 4. So
PG_GETARG_INT32(3) does not get Argument 3.

But this is just a practical example of the general principle that an
interface cannot use types that are specific to the environment on one end of
the interface. Which is how I know whoever wrote the c.h code that says,
"if the environment already has bool, just use it" didn't expect "bool" to
be used in an interface between Postgres and something else.

We did actually include a sizable patch into 9.0 to remove conflicts
with C++ reserved words (typename and such). I don't recall any
complaints about bool at the time.

Bear in mind that this isn't a syntax error - the compiler simply generates
the wrong code.

--
Bryan Henderson San Jose, California

It's quite annoying and
sad that they added "bool" to c99 since otherwise it would just be a
drop-in replacement with extra functionality and very low risk of
conflicts. Instead they virtually guaranteed conflicts with any large
software over a single define.

For that reason they put it into a separate header file stdbool.h that
no one is required to include.

Yes, and they didn't really create any conflicts that didn't already exist.
No one thinking broadly enough would define "bool" because of the high chance
that somebody else did too, because it's such an obvious name. The same is
true of names such as "int32".

For this reason, before C99, there were few software distributions that
defined "bool", with the chance of a distribution doing so inversely
proportional to how popular the distribution was.

Likewise, programmers who defined "bool" in their own private code were (and I
guess still are) largely the inexperienced ones -- who hadn't yet been bitten
by an interface header file that arrogantly claimed the name "bool."

--
Bryan Henderson San Jose, California

On mån, 2010-05-10 at 15:55 +0000, Bryan Henderson wrote:

I don't know how concrete you want.

Something one can download and compile.

That wouldn't be worth anyone's effort, since the problem is esaily
enough elucidated with a few words of explanation. I.e. I'm sure you
can imagine writing a program that would demonstrate the problem of
two header files that both typedef "bool".

Sure I can fabricate code that fails, but that wouldn't necessarily be
worth fixing, because it's fabricated and doesn't address anyone's real
problems.

We could try to make this work, but we're probably not going to rename
our bool type, so we'll have to work out some reasonable workaround.
But that's hard to do without having some practical example to work
with.

On Tue, May 11, 2010 at 1:08 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On mån, 2010-05-10 at 15:55 +0000, Bryan Henderson wrote:

I don't know how concrete you want.

Something one can download and compile.

That wouldn't be worth anyone's effort, since the problem is esaily
enough elucidated with a few words of explanation.  I.e. I'm sure you
can imagine writing a program that would demonstrate the problem of
two header files that both typedef "bool".

Sure I can fabricate code that fails, but that wouldn't necessarily be
worth fixing, because it's fabricated and doesn't address anyone's real
problems.

We could try to make this work, but we're probably not going to rename
our bool type, so we'll have to work out some reasonable workaround.
But that's hard to do without having some practical example to work
with.

Just to be clear, we wouldn't need to rename the type at the SQL level
- Catalog.pm can cope with mapping a C type name onto a different SQL
type name. You probably knew that already, but...

I guess the question that comes to mind for me is how many other
things fall into this category. We define a lot of symbols like int4
and int32 that other people could also have defined, and I don't
really want to s/^/pg/ all of them. If it's really only a question of
renaming bool I could see doing it.

On the flip side if the code that purports to cope with other
definitions of bool is useless, we should remove it so as to avoid
giving the impression that we have any ability to so cope.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Tue, May 11, 2010 at 12:42, Robert Haas <robertmhaas@gmail.com> wrote:

I guess the question that comes to mind for me is how many other
things fall into this category.  We define a lot of symbols like int4
and int32 that other people could also have defined, and I don't
really want to s/^/pg/ all of them.  If it's really only a question of
renaming bool I could see doing it.

You mean i'd get the pleasure of 'fixing' all my 3rd party C modules?
Not that that is a huge problem, we have broken calling conventions in
most releases...

Alex Hunsaker <badalex@gmail.com> writes:

On Tue, May 11, 2010 at 12:42, Robert Haas <robertmhaas@gmail.com> wrote:

I guess the question that comes to mind for me is how many other
things fall into this category.  We define a lot of symbols like int4
and int32 that other people could also have defined, and I don't
really want to s/^/pg/ all of them.  If it's really only a question of
renaming bool I could see doing it.

You mean i'd get the pleasure of 'fixing' all my 3rd party C modules?

Yeah, it's the implications for 3rd-party modules that make me not want
to do this. A search & replace on our own code base is one thing, but
when it's positively guaranteed to hit most add-on modules as well,
you need to show a pretty strong benefit from it. I think the argument
for changing this is too thin to support that.

regards, tom lane

On tis, 2010-05-11 at 14:42 -0400, Robert Haas wrote:

I guess the question that comes to mind for me is how many other
things fall into this category. We define a lot of symbols like int4
and int32 that other people could also have defined, and I don't
really want to s/^/pg/ all of them. If it's really only a question of
renaming bool I could see doing it.

Well, anything that you link into the backend is most likely either
your own code or a library that has reasonable namespace standards. You
can't expect to be able to link together *two* unclean namespaces under
any circumstances. :-) But you could probably even work around that
with linker scripts, for example.

The issue at hand, however, is that bool is a reserved word in C++ and
therefore cannot easily be complained away. I think you could work
around that though, for example, by doing #define bool char before
including a PostgreSQL server header. It would depend on what you want
to do in the code, which is why an example would help.

Then again, in 2 out of 2 cases I checked, sizeof(bool) is 1 in C++, so
there is no actual issue. I know it doesn't have to be, but at least
for some people it's working now.

On the flip side if the code that purports to cope with other
definitions of bool is useless, we should remove it so as to avoid
giving the impression that we have any ability to so cope.

Indeed, that code is what led me to believe I could work around my bool
conflict problem with a "#define bool bool" in my code. It appeared to work
at first -- the code compiled and even ran, but later when I added a Version 1
function (that's an interface that has a structure with a "bool" member),
things fell apart (my compiler's bool is 4 bytes) and it took hours to trace
it back to my workaround and try something else.

So it would be an improvement to remove the code that purports to cope with
other definitions of bool. And maybe add a comment emphasizing that "bool" is
part of the binary interface between server extensions and the base server.

--
Bryan Henderson San Jose, California

On Tue, May 11, 2010 at 3:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alex Hunsaker <badalex@gmail.com> writes:

On Tue, May 11, 2010 at 12:42, Robert Haas <robertmhaas@gmail.com> wrote:

I guess the question that comes to mind for me is how many other
things fall into this category.  We define a lot of symbols like int4
and int32 that other people could also have defined, and I don't
really want to s/^/pg/ all of them.  If it's really only a question of
renaming bool I could see doing it.

You mean i'd get the pleasure of 'fixing' all my 3rd party C modules?

Yeah, it's the implications for 3rd-party modules that make me not want
to do this.  A search & replace on our own code base is one thing, but
when it's positively guaranteed to hit most add-on modules as well,
you need to show a pretty strong benefit from it.  I think the argument
for changing this is too thin to support that.

Yeah, that may well be. I don't think we should have a policy of
folding our arms and shouting "no" whenever someone asks us to clean
up our namespace, but on the flip side one request (or even two) is
probably not enough reason to do anything drastic, and this would be
fairly drastic. Aside from breaking third-party modules, it would
also create merge problems for pending patches and companies with
private forks of the code base, and, if aesthetics count for anything,
it would be sort of ugly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Yeah, that may well be. I don't think we should have a policy of
folding our arms and shouting "no" whenever someone asks us to clean
up our namespace, but on the flip side one request (or even two) is
probably not enough reason to do anything drastic, and this would be
fairly drastic.

How about something less drastic? Could you at least eliminate "bool" from
interface structures that are intended to be compiled in multiple
environments? ("char" works fine, as does "pgbool"). Could you make c.h skip
the bool definition if it finds HAVE_BOOL defined? Then you could put in the
user guide where it talks about what header files and macros a server
extension needs if your program defines bool independently, define
HAVE_BOOL and if you want Postgres to define it, don't.

--
Bryan Henderson San Jose, California