Thread: bool: symbol name collision
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. Alternatively, it might just #include <stdbool.h>, if it can depend upon a C99 compiler. Incidentally, this collision is particularly heinous because structures that are part of the server extension interface have "bool" members, and if the server and user program are compiled with bools of different sizes, disaster occurs. Postgres's bool is one byte; often, bool is 4 bytes. I saw this in Postgres 8.4.3. -- Bryan Henderson San Jose, California
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. =A0There is no reason to assume > that a compiler's built-in version of bool will be bit-compatible with > ours. =A0And 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. --=20 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? =A0We build just fine on C99 compilers, AFAIK. =A0Or are you saying > that we should try to adopt <stdbool.h>'s definition of bool? =A0The > 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. --=20 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=E5n, 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. =A0I.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. --=20 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. =C2=A0We 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. =C2=A0If 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 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. =A0We 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. =A0If 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. =A0A 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. =A0I 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. --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
>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
>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
On Tue, May 11, 2010 at 8:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alex Hunsaker <badalex@gmail.com> writes: > >> 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. =A0A 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. =A0I think the argument > for changing this is too thin to support that. Well you could imagine doing this for all our types by doing: search and replace int4 -> pgint4 etc. add #ifndef int4 #define int4 pgint4 at the end of postgres.h That way third-party apps which define their own int4 would be required to use pgint4. But third-party apps which don't could continue to use int4. All struct interfaces etc would be defined using pgint4 directly so wouldn't be affected by any unusual definition of int4. The trick would be to ensure that nothing in the postgres interfaces depend on the bool or int4 definitions. That would be easy to ensure after the search/replace but hard to guarantee long-term. --=20 greg
On ons, 2010-05-12 at 12:44 +0100, Greg Stark wrote: > Well you could imagine doing this for all our types by doing: > > search and replace int4 -> pgint4 etc. > add #ifndef int4 #define int4 pgint4 at the end of postgres.h > > That way third-party apps which define their own int4 would be > required to use pgint4. But third-party apps which don't could > continue to use int4. All struct interfaces etc would be defined using > pgint4 directly so wouldn't be affected by any unusual definition of > int4. > > The trick would be to ensure that nothing in the postgres interfaces > depend on the bool or int4 definitions. That would be easy to ensure > after the search/replace but hard to guarantee long-term. The problem with the bool type is that it could have different sizes on different systems. Which will lead to problems. I doubt that that problem exists with int4. There is the somewhat related issue that we should be using int32 instead of int4 (and indeed most of our header files do, but not all), and if we wanted to ever change anything about that, we should probably go with int32_t. But in none of these cases is there any chance of a binary representation mismatch. (Except perhaps int8. :-) )
On Wed, May 12, 2010 at 1:01 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > The problem with the bool type is that it could have different sizes on > different systems. =A0Which will lead to problems. =A0I doubt that that > problem exists with int4. > I could imagine macros that do the wrong thing if the types they use inside them have the wrong signedness... I tihnk the reasons bool is particularly eggregious are a) we have these misleading #ifdefs that don't do the right thing and b) there's a stdbool.h making it hard for c99 programmers to avoid doing the wrong thing. The other types are part of the postgres server interface and module writers should just avoid redefining them -- if they do they'll get errors. I think it would be nice to make that better but at least they won't be silently redefining the postgres interfaces to potentially incorrect definitions. --=20 greg
bryanh@giraffe-data.com (Bryan Henderson) writes:
> How about something less drastic? Could you at least eliminate "bool" from
> interface structures that are intended to be compiled in multiple
> environments?
No. That's not distinguishably different from eliminating it altogether.
regards, tom lane